Skip to content

Instantly share code, notes, and snippets.

@delta1
Created April 4, 2023 08:14
Show Gist options
  • Save delta1/32a8fc5e0e33c0aab657d2970e233a75 to your computer and use it in GitHub Desktop.
Save delta1/32a8fc5e0e33c0aab657d2970e233a75 to your computer and use it in GitHub Desktop.
elements merging notes

merge notes for elements 24

please comment with

  • bitcoin PR / merged-master commit hash
  • notes about conflicts/resolution or things to keep track of
@jamesdorfman
Copy link

jamesdorfman commented Jul 14, 2023

delta1/elements@7d759da

for Bitcoin PR bitcoin/bitcoin#23201

The following assert was commented out without any notes. Let's add a note that this was just for Elements, and note it in this gist as well.

diff --git a/test/functional/rpc_fundrawtransaction.py b/test/functional/rpc_fundrawtransaction.py

-        assert_fee_amount(funded_tx4["fee"], tx4_vsize, Decimal(0.0001))
+        # assert_fee_amount(funded_tx4["fee"], tx4_vsize, Decimal(0.0001))

Same with the following assert in this file:

diff --git a/test/functional/wallet_send.py b/test/functional/wallet_send.py

-        assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001))
+        # assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001))

Update: I understand why these are commented out -- fees are very different between bitcoin and elements. I added comments to the ends of both of these assertion lines, in jamesdorfman/elements@dca9352.

@jamesdorfman
Copy link

jamesdorfman commented Jul 14, 2023

delta1/elements@c53eb39

for Bitcoin PR bitcoin/bitcoin#24035

Upstream removed the extra fee but we didn't. Should we?

diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
index 6113deae1c..fdc23b507f 100755
--- a/test/functional/mempool_accept.py
+++ b/test/functional/mempool_accept.py

         tx = tx_from_hex(raw_tx_0)
-<<<<<<< HEAD
         tx.vout[0].nValue.setToAmount(tx.vout[0].nValue.getAmount() - int(4 * fee * COIN))  # Set more fee
         tx.vout[1].nValue.setToAmount(tx.vout[1].nValue.getAmount() + int(4 * fee * COIN))
-        # skip re-signing the tx
-||||||| 767ee2e3a1
-        tx.vout[0].nValue -= int(4 * fee * COIN)  # Set more fee
-        # skip re-signing the tx
-=======
-        tx.vout[0].nValue -= int(4 * fee * COIN)  # Set more fee
->>>>>>> 807169e10b4a18324356ed6ee4d69587b96a7c70

Update: no we can't. The test fails without this extra fee, with the following error:

AssertionError: not([{'txid': 'e2b2d9feda958b9a0d74e0f13069c4bb5beacf69f0a82ca69eedaf13fb89416a', 'allowed': False, 'reject-reason': 'txn-mempool-conflict'}] == [{'txid': 'e2b2d9feda958b9a0d74e0f13069c4bb5beacf69f0a82ca69eedaf13fb89416a', 'allowed': False, 'reject-reason': 'txn-already-in-mempool'}])

@jamesdorfman
Copy link

jamesdorfman commented Jul 17, 2023

delta1/elements@5931d7d

for Bitcoin PR bitcoin/bitcoin#23508

Check this out -- this PR removed the following lines:

diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py
index 3ef94e1f25..19c2387b66 100755
--- a/test/functional/rpc_blockchain.py
+++ b/test/functional/rpc_blockchain.py
@@ -160,25 +160,6 @@ class BlockchainTest(BitcoinTestFramework):
         self.start_node(0, extra_args=[
             '-stopatheight=207',
             '-prune=550',
-<<<<<<< HEAD
-            '-testactivationheight=bip34@2',
-            '-testactivationheight=dersig@3',
-            '-testactivationheight=cltv@4',
-            '-testactivationheight=csv@5',
-            '-testactivationheight=segwit@6',
-            # ELEMENTS
-            "-con_bip34height=2",
-            "-con_bip66height=3",
-            "-con_bip65height=4",
-            "-con_csv_deploy_start=5",
-||||||| a4f7c41271
-            '-testactivationheight=bip34@2',
-            '-testactivationheight=dersig@3',
-            '-testactivationheight=cltv@4',
-            '-testactivationheight=csv@5',
-            '-testactivationheight=segwit@6',
-=======
->>>>>>> d4e92d843650b0480b86d15ce46ed02b6fd9b5be

We treated it like a deletion. However, they were actually moved (I found this in the original PR), lower in that same file test/functional/rpc_blockchain.py:

 def _test_getdeploymentinfo(self):
        # Note: continues past -stopatheight height, so must be invoked
        # after _test_stopatheight

        self.log.info("Test getdeploymentinfo")
        self.stop_node(0)
        self.start_node(0, extra_args=[
            '-testactivationheight=bip34@2',
            '-testactivationheight=dersig@3',
            '-testactivationheight=cltv@4',
            '-testactivationheight=csv@5',
            '-testactivationheight=segwit@6',
        ])

So, we need to add

# ELEMENTS
"-con_bip34height=2",
"-con_bip66height=3",
"-con_bip65height=4",
"-con_csv_deploy_start=5",

there as well.

Update: upon further review of the commit, these arguments were already correctly added during the commit. I think the 4-way diff Wass just being strange. This is not actually an issue.

@jamesdorfman
Copy link

jamesdorfman commented Jul 20, 2023

delta1/elements@eef2269

for Bitcoin PR bitcoin/bitcoin#24343

TODO: revisit failing test (I will do it when I'm done going through everything)

Update: fixed in jamesdorfman/elements@2ab19eb

@jamesdorfman
Copy link

jamesdorfman commented Jul 20, 2023

delta1/elements@51fbed1

for Bitcoin PR bitcoin/bitcoin#24409

Not specifically related to this commit, but we need to update the PACKAGE_NAME variable in file build_msvc/bitcoin_config.h.ini.
It's currently set to

/* Define to the full name of this package. */
#define PACKAGE_NAME "Bitcoin Core"

Update: fixed in jamesdorfman/elements@c37b88c

@jamesdorfman
Copy link

delta1/elements@055d634

for Bitcoin PR bitcoin/bitcoin#24451

Confirm with @delta1 -- your commit message says All conflicts in the locale translation files were resolved by taking the upstream version of the files.. I want to confirm that by upstream, you mean bitcoin. If that is the case, then I agree with this solution.

@delta1
Copy link
Author

delta1 commented Jul 20, 2023

delta1/elements@055d634

for Bitcoin PR bitcoin/bitcoin#24451

Confirm with @delta1 -- your commit message says All conflicts in the locale translation files were resolved by taking the upstream version of the files.. I want to confirm that by upstream, you mean bitcoin. If that is the case, then I agree with this solution.

@jamesdorfman yes exactly

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment