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

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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

    <!--85328a0da195eb286784d51f73fa0af9-->

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

    <details><summary>Hints</summary>

    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.

    </details>

  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

                                    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:

    diff --git a/test/functional/mempool_ephemeral_dust.py b/test/functional/mempool_ephemeral_dust.py
    index 1e55a6079fa..0ea9c585ed5 100755
    --- a/test/functional/mempool_ephemeral_dust.py
    +++ b/test/functional/mempool_ephemeral_dust.py
    @@ -226,14 +226,17 @@ class EphemeralDustTest(BitcoinTestFramework):
             dusty_tx, sweep_tx = self.create_ephemeral_dust_package(tx_version=3, dust_value=329)
     
             # Valid sweep we will RBF incorrectly by not spending dust as well
    -        self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
    -        assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    +        # self.nodes[0].submitpackage([dusty_tx["hex"], sweep_tx["hex"]])
    +        # assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
     
             # Doesn't spend in-mempool dust output from parent
             unspent_sweep_tx = self.wallet.create_self_transfer_multi(fee_per_output=2000, utxos_to_spend=[dusty_tx["new_utxos"][0]], version=3)
    +        unspent_sweep_tx["tx"].wit.vtxinwit[0].scriptWitness.stack = [b'a']
    +        assert unspent_sweep_tx["txid"] != unspent_sweep_tx["wtxid"]
             assert_greater_than(unspent_sweep_tx["fee"], sweep_tx["fee"])
             res = self.nodes[0].submitpackage([dusty_tx["hex"], unspent_sweep_tx["hex"]])
    -        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")
    +        print(res)
    +        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")
             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"])
             assert_mempool_contents(self, self.nodes[0], expected=[dusty_tx["tx"], sweep_tx["tx"]])
    
    
    2025-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
    {'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': []}
    2025-03-11T15:27:59.781000Z TestFramework (ERROR): Assertion failed
    Traceback (most recent call last):
      File "/Users/gloria/bitcoin/test/functional/test_framework/test_framework.py", line 135, in main
        self.run_test()
        ~~~~~~~~~~~~~^^
      File "/Users/gloria/bitcoin/build_debug/test/functional/mempool_ephemeral_dust.py", line 78, in run_test
        self.test_unspent_ephemeral()
        ~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
      File "/Users/gloria/bitcoin/build_debug/test/functional/mempool_ephemeral_dust.py", line 239, in test_unspent_ephemeral
        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")
        ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
      File "/Users/gloria/bitcoin/test/functional/test_framework/util.py", line 77, in assert_equal
        raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
    AssertionError: 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. dergoegge approved
  29. dergoegge commented at 8:51 AM on March 12, 2025: member

    Code review ACK e637dc2c01c3b566e6c51c911c5881a8d206c924

  30. fanquake merged this on Mar 12, 2025
  31. fanquake closed this on Mar 12, 2025

  32. theStack commented at 1:08 PM on March 12, 2025: contributor

    post-merge code-review ACK e637dc2c01c3b566e6c51c911c5881a8d206c924 good find 👌

  33. sedited referenced this in commit 2cab7a09ea on Mar 16, 2025
  34. stickies-v referenced this in commit d760fd3dda on Mar 17, 2025
  35. stickies-v referenced this in commit cc83553352 on Mar 17, 2025
  36. stickies-v referenced this in commit 2614933f06 on Mar 17, 2025
  37. stickies-v referenced this in commit b70418c5fc on Mar 17, 2025
  38. stickies-v referenced this in commit 69f8a1fe50 on Mar 17, 2025
  39. bug-castercv502 referenced this in commit a6aeab1eeb on Sep 28, 2025
  40. ivanlele referenced this in commit c1820ce4b9 on Apr 16, 2026
  41. Kino1994 referenced this in commit 1bdc32ed9a on Jun 28, 2026
  42. BigcoinBGC referenced this in commit 44774304be on Jun 30, 2026
  43. bitcoin locked this on Jul 30, 2026


instagibbs

Milestone
29.0


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: 2026-08-11 10:51 UTC

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