validation, fix: Use wtxid instead of txid in CheckEphemeralSpends #32025

pull marcofleon wants to merge 2 commits into bitcoin:master from marcofleon:2025/03/fix-txid-to-wtxid changing 7 files +80 −79
  1. marcofleon commented at 3:17 pm on March 10, 2025: contributor

    This PR addresses a small bug in AcceptMultipleTransactions where a txid was being inserted into a map that should only hold wtxids. CheckEphemeralSpends has an out parameter on failure that records that the child transaction did not spend the parent’s dust. Instead of using the txid of this child, use its wtxid.

    The second commit in this PR is a refactor of the PackageMempoolAcceptResult struct to use the Wtxid type instead of uint256. This helps to prevent errors like this in the future.

  2. DrahtBot commented at 3:17 pm on March 10, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32025.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK instagibbs, glozow, dergoegge

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. marcofleon commented at 3:21 pm on March 10, 2025: contributor
    note: I found this while working on the uint256 to txid/wtxid full refactor. I figured I would include a tiny part of that in this PR because it relates to the bug. If it’s preferred to only have the fix be merged for branch off, then I can remove the second commit and include it as part of the txid type safety project later on.
  4. marcofleon force-pushed on Mar 10, 2025
  5. DrahtBot commented at 3:31 pm on March 10, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38500023046

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  6. DrahtBot added the label CI failed on Mar 10, 2025
  7. dergoegge commented at 3:50 pm on March 10, 2025: member

    Concept ACK

    Awesome to see the txid types paying off!

  8. marcofleon force-pushed on Mar 10, 2025
  9. DrahtBot removed the label CI failed on Mar 10, 2025
  10. laanwj added the label Mempool on Mar 11, 2025
  11. glozow requested review from instagibbs on Mar 11, 2025
  12. in src/policy/ephemeral_policy.cpp:88 in 795cfcfa10 outdated
    82@@ -83,9 +83,9 @@ bool CheckEphemeralSpends(const Package& package, CFeeRate dust_relay_rate, cons
    83         }
    84 
    85         if (!unspent_parent_dust.empty()) {
    86-            out_child_txid = tx->GetHash();
    87+            out_child_wtxid = tx->GetWitnessHash();
    88             out_child_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "missing-ephemeral-spends",
    89-                                strprintf("tx %s did not spend parent's ephemeral dust", out_child_txid.ToString()));
    90+                                strprintf("tx %s did not spend parent's ephemeral dust", out_child_wtxid.ToString()));
    


    glozow commented at 3:06 pm on March 11, 2025:

    callers typically use txid, so it’s best to include both

    0                                strprintf("tx %s  (wtxid=%s) did not spend parent's ephemeral dust", out_child_txid.ToString(), out_child_wtxid.ToString()));
    

    marcofleon commented at 4:37 pm on March 11, 2025:
    Makes sense, thanks
  13. glozow commented at 3:08 pm on March 11, 2025: member
    nice catch, concept ACK
  14. instagibbs commented at 3:14 pm on March 11, 2025: member
    concept ACK, agree that txid should also be reported to user since that’s used quite often by callers
  15. in src/validation.cpp:1598 in 7ac6ab4d1a outdated
    1596-        if (!CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool, child_state, child_txid)) {
    1597+        Wtxid child_wtxid;
    1598+        if (!CheckEphemeralSpends(txns, m_pool.m_opts.dust_relay_feerate, m_pool, child_state, child_wtxid)) {
    1599             package_state.Invalid(PackageValidationResult::PCKG_TX, "unspent-dust");
    1600-            results.emplace(child_txid, MempoolAcceptResult::Failure(child_state));
    1601+            results.emplace(child_wtxid, MempoolAcceptResult::Failure(child_state));
    


    glozow commented at 3:40 pm on March 11, 2025:

    Interestingly, the effect of inserting by the child’s txid is that you get “missing inputs” instead of the slightly more correct “did not spend parent’s ephemeral dust” from the submitpackage results. There is a result for both wtxid and txid in the map: we put “missing inputs” when we tried the child initially, and then failed to overwrite it (because we’re using the wrong key here) the second time. The RPC code copies the result from a query by wtxid.

    Here is the diff for mempool_ephemeral_dust.py to see this bug. You don’t get a KeyError, but a string mismatch:

     0diff --git a/test/functional/mempool_ephemeral_dust.py b/test/functional/mempool_ephemeral_dust.py
     1index 1e55a6079fa..0ea9c585ed5 100755
     2--- a/test/functional/mempool_ephemeral_dust.py
     3+++ b/test/functional/mempool_ephemeral_dust.py
     4@@ -226,14 +226,17 @@ class EphemeralDustTest(BitcoinTestFramework):
     5         dusty_tx, sweep_tx = self.create_ephemeral_dust_package(tx_version=3, dust_value=329)
     6 
     7         # Valid sweep we will RBF incorrectly by not spending dust as well
     8-        self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
     9-        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    10+        # self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
    11+        # assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    12 
    13         # Doesn't spend in-mempool dust output from parent
    14         unspent_sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3)
    15+        unspent_sweep_tx["tx"].wit.vtxinwit[0].scriptWitness.stack = [b'a']
    16+        assert unspent_sweep_tx["txid"] != unspent_sweep_tx["wtxid"]
    17         assert_greater_than(unspent_sweep_tx["fee"], sweep_tx["fee"])
    18         res = self.nodes[0].submitpackage([dusty_tx["hex"], unspent_sweep_tx["hex"]])
    19-        assert_equal(res["tx-results"][unspent_sweep_tx["wtxid"]]["error"], f"missing-ephemeral-spends, tx {unspent_sweep_tx['wtxid']} did not spend parent's ephemeral dust")
    20+        print(res)
    21+        assert_equal(res["tx-results"][unspent_sweep_tx["wtxid"]]["error"], f"missing-ephemeral-spends, tx {unspent_sweep_tx['txid']} did not spend parent's ephemeral dust")
    22         assert_raises_rpc_error(-26, f"missing-ephemeral-spends, tx {unspent_sweep_tx['wtxid']} did not spend parent's ephemeral dust", self.nodes[0].sendrawtransaction, unspent_sweep_tx["hex"])
    23         assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    
     02025-03-11T15:27:59.777000Z TestFramework (INFO): Test that spending from a tx with ephemeral outputs is only allowed if dust is spent as well
     1{'package_msg': 'unspent-dust', 'tx-results': {'4e04718b0923cda667236ab6aab84731cb75f23c08124b0f589b4afa20fd798c': {'txid': '521317a6d852bad16c4b30e9b50be61247e7f47616163e6deb6cc446f77ec818', 'error': 'min relay fee not met, 0 < 147'}, '566d682ea9dbe55317436363bb76b5e4930c420dd126a4f29c03ffde82f7bb9f': {'txid': '9564fdd635de730de72bcdc807b494e7065dc8a01d32f14124d70b6497572f9f', 'error': 'bad-txns-inputs-missingorspent'}}, 'replaced-transactions': []}
     22025-03-11T15:27:59.781000Z TestFramework (ERROR): Assertion failed
     3Traceback (most recent call last):
     4  File "/Users/gloria/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
     5    self.run_test()
     6    ~~~~~~~~~~~~~^^
     7  File "/Users/gloria/bitcoin/build_debug/test/functional/mempool_ephemeral_dust.py", line 78, in run_test
     8    self.test_unspent_ephemeral()
     9    ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
    10  File "/Users/gloria/bitcoin/build_debug/test/functional/mempool_ephemeral_dust.py", line 239, in test_unspent_ephemeral
    11    assert_equal(res["tx-results"][unspent_sweep_tx["wtxid"]]["error"], f"missing-ephemeral-spends, tx {unspent_sweep_tx['txid']} did not spend parent's ephemeral dust")
    12    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    13  File "/Users/gloria/bitcoin/test/functional/test_framework/util.py", line 77, in assert_equal
    14    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    15AssertionError: not(bad-txns-inputs-missingorspent == missing-ephemeral-spends, tx 9564fdd635de730de72bcdc807b494e7065dc8a01d32f14124d70b6497572f9f did not spend parent's ephemeral dust)
    

    glozow commented at 3:42 pm on March 11, 2025:
    Btw I’m not suggesting you add this exact diff to the PR (I had to comment out a part of the test). But it can be adapted into a regression test later.
  16. glozow added this to the milestone 29.0 on Mar 11, 2025
  17. marcofleon force-pushed on Mar 11, 2025
  18. glozow added the label Bug on Mar 11, 2025
  19. validation: use wtxid instead of txid in CheckEphemeralSpends a3baead7cb
  20. refactor: Replace uint256 type with Wtxid in PackageMempoolAcceptResult struct e637dc2c01
  21. marcofleon force-pushed on Mar 11, 2025
  22. DrahtBot added the label CI failed on Mar 11, 2025
  23. instagibbs commented at 4:46 pm on March 11, 2025: member

    ACK https://github.com/bitcoin/bitcoin/pull/32025/commits/e637dc2c01c3b566e6c51c911c5881a8d206c924

    Places the result in the map under txid, improving the reported error in certain cases, and typing future proofs against regressions automagically.

  24. DrahtBot requested review from glozow on Mar 11, 2025
  25. DrahtBot requested review from dergoegge on Mar 11, 2025
  26. DrahtBot removed the label CI failed on Mar 11, 2025
  27. glozow commented at 8:43 pm on March 11, 2025: member
    ACK e637dc2c01c, hooray for type safety
  28. in src/validation.cpp:1442 in e637dc2c01
    1437@@ -1438,8 +1438,8 @@ MempoolAcceptResult MemPoolAccept::AcceptSingleTransaction(const CTransactionRef
    1438     }
    1439 
    1440     if (m_pool.m_opts.require_standard) {
    1441-        Txid dummy_txid;
    1442-        if (!CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool, ws.m_state, dummy_txid)) {
    1443+        Wtxid dummy_wtxid;
    1444+        if (!CheckEphemeralSpends(/*package=*/{ptx}, m_pool.m_opts.dust_relay_feerate, m_pool, ws.m_state, dummy_wtxid)) {
    


    aiswaryasankar commented at 6:31 am on March 12, 2025:
    The change from Txid to Wtxid in CheckEphemeralSpends calls could cause issues with transaction identification. The function now stores witness transaction IDs instead of regular transaction IDs, which might affect how transactions are tracked and identified in error cases.
  29. dergoegge approved
  30. dergoegge commented at 8:51 am on March 12, 2025: member
    Code review ACK e637dc2c01c3b566e6c51c911c5881a8d206c924
  31. fanquake merged this on Mar 12, 2025
  32. fanquake closed this on Mar 12, 2025

  33. theStack commented at 1:08 pm on March 12, 2025: contributor
    post-merge code-review ACK e637dc2c01c3b566e6c51c911c5881a8d206c924 good find 👌

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-03-31 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me