Use Txid in COutpoint #28922

pull dergoegge wants to merge 1 commits into bitcoin:master from dergoegge:2023-11-txid-oupoint changing 48 files +124 −114
  1. dergoegge commented at 12:05 PM on November 21, 2023: member

    This PR changes the type of the hash of a transaction outpoint from uint256 to Txid.

  2. DrahtBot commented at 12:05 PM on November 21, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

    For detailed information about the code coverage, see the test coverage report.

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK Sjors, stickies-v, TheCharlatan
    Concept ACK ismaelsadeeq, pablomartin4btc

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #28912 (refactor: VectorWriter and SpanReader without nVersion by maflcko)
    • #28886 (refactor: Replace sets of txiter with CTxMemPoolEntryRefs by TheCharlatan)
    • #28335 (RFC: Remove boost usage from kernel api / headers by TheCharlatan)
    • #27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
    • #27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. in src/primitives/transaction.h:46 in 9aeba9eac4 outdated 
      42 | @@ -43,8 +43,7 @@ class COutPoint
  43 |  
  44 |      friend bool operator<(const COutPoint& a, const COutPoint& b)
  45 |      {
  46 | -        int cmp = a.hash.Compare(b.hash);
  47 | -        return cmp < 0 || (cmp == 0 && a.n < b.n);
  48 | +        return a.hash < b.hash || (a.hash == b.hash && a.n < b.n);
    maflcko commented at 12:53 PM on November 21, 2023: 
            return std::tie(a.hash, a.n) < std::tie(b.hash, b.n);

    nit: This should compile, is shorter, and is recommended in https://en.cppreference.com/w/cpp/language/operators#Comparison_operators

    dergoegge commented at 1:16 PM on November 21, 2023:

    This is great, thanks!

  4. in src/rpc/blockchain.cpp:2017 in 9aeba9eac4 outdated 
    2013 | @@ -2014,7 +2014,7 @@ bool FindScriptPubKey(std::atomic<int>& scan_progress, const std::atomic<bool>&
2014 |          }
2015 |          if (count % 256 == 0) {
2016 |              // update progress reference every 256 item
2017 | -            uint32_t high = 0x100 * *key.hash.begin() + *(key.hash.begin() + 1);
2018 | +            uint32_t high = 0x100 * static_cast<const uint8_t>(*key.hash.begin()) + *(reinterpret_cast<const uint8_t*>(key.hash.begin()) + 1);
    maflcko commented at 12:56 PM on November 21, 2023:

    Not sure about using those casts here. Why not use the safe UCharCast wrapper?

    dergoegge commented at 1:16 PM on November 21, 2023:

    Done

  5. in src/test/fuzz/coinscache_sim.cpp:48 in 9aeba9eac4 outdated 
      42 | @@ -43,7 +43,9 @@ struct PrecomputedData
  43 |          for (uint32_t i = 0; i < NUM_OUTPOINTS; ++i) {
  44 |              uint32_t idx = (i * 1200U) >> 12; /* Map 3 or 4 entries to same txid. */
  45 |              const uint8_t ser[4] = {uint8_t(idx), uint8_t(idx >> 8), uint8_t(idx >> 16), uint8_t(idx >> 24)};
  46 | -            CSHA256().Write(PREFIX_O, 1).Write(ser, sizeof(ser)).Finalize(outpoints[i].hash.begin());
  47 | +            uint256 txid;
  48 | +            CSHA256().Write(PREFIX_O, 1).Write(ser, sizeof(ser)).Finalize(txid.begin());
  49 | +            outpoints[i].hash = Txid::FromUint256(txid);
    maflcko commented at 12:59 PM on November 21, 2023:

    Could just use Finalize(UCharCast(outpoints[i].hash.data())?

    dergoegge commented at 1:07 PM on November 21, 2023:

    I think I'd prefer to keep data const. I don't think there is any good reason to support mutating a txid.

    maflcko commented at 1:09 PM on November 21, 2023:

    Ah yes, I missed that it was const.

  6. in src/util/transaction_identifier.h:68 in 9aeba9eac4 outdated 
      64 | @@ -65,4 +65,9 @@ using Txid = transaction_identifier<false>;
  65 |  /** Wtxid commits to all transaction fields including the witness. */
  66 |  using Wtxid = transaction_identifier<true>;
  67 |  
  68 | +inline Txid TxidFromString(const std::string& str)
    maflcko commented at 1:01 PM on November 21, 2023:

    Why not string_view?

    dergoegge commented at 1:16 PM on November 21, 2023:

    Done

    maflcko commented at 2:53 PM on November 24, 2023:

    Actually, string_view::data in this context may be UB, according to the standard, because a NUL char is not guaranteed to come at the string_view::end position, but the constructor requires a NUL at the end char to function.

    I'll try to fix this in a follow-up.

  7. Use Txid in COutpoint 9e58c5bcd9
  8. in src/rest.cpp:799 in 9aeba9eac4 outdated 
     797 |              if (!ParseInt32(strOutput, &nOutput) || !IsHex(strTxid))
 798 |                  return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");
 799 |  
 800 | -            txid.SetHex(strTxid);
 801 | -            vOutPoints.emplace_back(txid, (uint32_t)nOutput);
 802 | +            vOutPoints.emplace_back(TxidFromString(strTxid), (uint32_t)nOutput);
    Sjors commented at 1:09 PM on November 21, 2023:

    (Followup) should check that the hex string is of the correct length

  9. in src/rpc/mempool.cpp:636 in 9aeba9eac4 outdated 
     632 | @@ -633,7 +633,7 @@ static RPCHelpMan gettxspendingprevout()
 633 |                                      {"vout", UniValueType(UniValue::VNUM)},
 634 |                                  }, /*fAllowNull=*/false, /*fStrict=*/true);
 635 |  
 636 | -                const uint256 txid(ParseHashO(o, "txid"));
 637 | +                const Txid txid = Txid::FromUint256(ParseHashO(o, "txid"));
    Sjors commented at 1:15 PM on November 21, 2023:

    (Followup) should check length

    dergoegge commented at 1:39 PM on November 21, 2023:

    Looks like ParseHashO has a length check internally?

    Sjors commented at 1:51 PM on November 21, 2023:

    Ah, it calls ParseHashV which indeed checks that it's 64 characters. So then only the rest.cpp line above doesn't check the length (and it's terribly documented).

  10. dergoegge force-pushed on Nov 21, 2023
  11. in src/rpc/blockchain.cpp:2017 in 9e58c5bcd9 
    2013 | @@ -2014,7 +2014,7 @@ bool FindScriptPubKey(std::atomic<int>& scan_progress, const std::atomic<bool>&
2014 |          }
2015 |          if (count % 256 == 0) {
2016 |              // update progress reference every 256 item
2017 | -            uint32_t high = 0x100 * *key.hash.begin() + *(key.hash.begin() + 1);
2018 | +            uint32_t high = 0x100 * *UCharCast(key.hash.begin()) + *(UCharCast(key.hash.begin()) + 1);
    Sjors commented at 1:19 PM on November 21, 2023:

    This seems unrelated?

    dergoegge commented at 1:37 PM on November 21, 2023:

    Txid::begin etc. return std::byte* which can't be used for arithmetic operations.

    See https://en.cppreference.com/w/cpp/types/byte.

    Sjors commented at 1:43 PM on November 21, 2023:

    I was confused where Txid comes to play here. But I see key is a COutPoint.

  12. in src/rpc/rawtransaction_util.cpp:37 in 9e58c5bcd9 
      33 | @@ -34,7 +34,7 @@ void AddInputs(CMutableTransaction& rawTx, const UniValue& inputs_in, std::optio
  34 |          const UniValue& input = inputs[idx];
  35 |          const UniValue& o = input.get_obj();
  36 |  
  37 | -        uint256 txid = ParseHashO(o, "txid");
  38 | +        Txid txid = Txid::FromUint256(ParseHashO(o, "txid"));
    Sjors commented at 1:19 PM on November 21, 2023:

    (Followup) should check length

    Sjors commented at 1:52 PM on November 21, 2023:

    Update: it does, via ParseHashV - deleting the other comments below

  13. in src/wallet/spend.cpp:324 in 9e58c5bcd9 
     320 | @@ -321,7 +321,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
 321 |      std::set<uint256> trusted_parents;
 322 |      for (const auto& entry : wallet.mapWallet)
 323 |      {
 324 | -        const uint256& wtxid = entry.first;
 325 | +        const uint256& txid = entry.first;
    Sjors commented at 1:25 PM on November 21, 2023:

    (Followup) should check length

    dergoegge commented at 1:40 PM on November 21, 2023:

    Not sure what you mean here? entry.first is a uint256.

    Sjors commented at 1:46 PM on November 21, 2023:

    I think I copy-pasted this comment in the wrong file. Can be ignored

    stickies-v commented at 8:36 PM on November 22, 2023:

    nit: Would simplify this to

    for (const auto& [txid, wtx] : wallet.mapWallet)

    <details> <summary>git diff on 9e58c5bcd9</summary>

    diff --git a/src/wallet/spend.cpp b/src/wallet/spend.cpp
index 35583642a5..0e72efe0ba 100644
--- a/src/wallet/spend.cpp
+++ b/src/wallet/spend.cpp
@@ -319,11 +319,8 @@ CoinsResult AvailableCoins(const CWallet& wallet,
     std::vector<COutPoint> outpoints;
 
     std::set<uint256> trusted_parents;
-    for (const auto& entry : wallet.mapWallet)
+    for (const auto& [txid, wtx] : wallet.mapWallet)
     {
-        const uint256& txid = entry.first;
-        const CWalletTx& wtx = entry.second;
-
         if (wallet.IsTxImmatureCoinBase(wtx) && !params.include_immature_coinbase)
             continue;

    </details>

    TheCharlatan commented at 9:30 PM on November 22, 2023:

    Nit: Maybe remove this line altogether?

  14. in src/rpc/blockchain.cpp:1060 in 9e58c5bcd9 
    1056 | @@ -1057,7 +1057,7 @@ static RPCHelpMan gettxout()
1057 |  
1058 |      UniValue ret(UniValue::VOBJ);
1059 |  
1060 | -    uint256 hash(ParseHashV(request.params[0], "txid"));
1061 | +    auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};
    Sjors commented at 1:27 PM on November 21, 2023:

    Txid hash ?

    stickies-v commented at 5:41 PM on November 22, 2023:

    nit

        const auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};
    TheCharlatan commented at 8:49 PM on November 22, 2023:

    NIt: Since you are switching to bracket style initialization here, do it consistently in all the initialization lines you touch?

  15. Sjors approved
  16. Sjors commented at 1:34 PM on November 21, 2023: member

    ACK 9e58c5bcd96e7ff2062274868814ccae0626589e

  17. Sjors commented at 1:36 PM on November 21, 2023: member

    @maflcko perhaps we should add another RPCArg type that's ~32~ 64 characters STR_HEX?

    Update: it already happens in practice, though might still make sense to be more explicit, since there are other hex things users can pass to the RPC like a script or public key.

  18. ismaelsadeeq commented at 4:02 PM on November 22, 2023: member

    Concept ACK

  19. in src/util/transaction_identifier.h:1 in 9e58c5bcd9
    pablomartin4btc commented at 5:26 PM on November 22, 2023:

    Are you keeping this and removing it in a follow-up?

    https://github.com/bitcoin/bitcoin/blob/9e58c5bcd96e7ff2062274868814ccae0626589e/src/util/transaction_identifier.h#L53-L60

    Same for the comparisons above those lines (#L18 // TODO).

    dergoegge commented at 11:50 AM on November 23, 2023:

    Yes the plan is to remove it eventually, but it should still be needed at the moment.

  20. pablomartin4btc commented at 5:27 PM on November 22, 2023: member

    Concept ACK

    Light CR.

  21. in src/test/fuzz/mini_miner.cpp:28 in 9e58c5bcd9 
      24 | @@ -25,7 +25,7 @@ void initialize_miner()
  25 |      static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
  26 |      g_setup = testing_setup.get();
  27 |      for (uint32_t i = 0; i < uint32_t{100}; ++i) {
  28 | -        g_available_coins.emplace_back(uint256::ZERO, i);
  29 | +        g_available_coins.emplace_back(Txid::FromUint256(uint256::ZERO), i);
    stickies-v commented at 5:42 PM on November 22, 2023:

    nit

            g_available_coins.emplace_back(Txid{}, i);
    TheCharlatan commented at 10:27 PM on November 22, 2023:

    Nit: Just do Txid{}, like you did in bench/disconnected_transactions?

  22. in src/test/fuzz/tx_pool.cpp:378 in 9e58c5bcd9 
     375 |      }
 376 |      for (int i{0}; i <= 3; ++i) {
 377 |          // Add some immature and non-existent outpoints
 378 |          txids.push_back(g_outpoints_coinbase_init_immature.at(i).hash);
 379 | -        txids.push_back(ConsumeUInt256(fuzzed_data_provider));
 380 | +        txids.push_back(Txid::FromUint256(ConsumeUInt256(fuzzed_data_provider)));
    stickies-v commented at 8:29 PM on November 22, 2023:

    nit

            txids.emplace_back(Txid::FromUint256(ConsumeUInt256(fuzzed_data_provider)));
    stickies-v commented at 8:56 AM on November 23, 2023:

    nvm, I don't think this is actually an improvement.

  23. in src/test/miniminer_tests.cpp:603 in 9e58c5bcd9 
     599 | +    };
 600 | +
 601 |      // Add chain of size 500
 602 |      TestMemPoolEntryHelper entry;
 603 | -    std::vector<uint256> chain_txids;
 604 | +    std::vector<Txid> chain_txids;
    stickies-v commented at 8:35 PM on November 22, 2023:

    Doesn't it make more sense to keep this as std::vector<uint256> chain_txids; for now?

  24. stickies-v approved
  25. stickies-v commented at 8:37 PM on November 22, 2023: contributor

    ACK 9e58c5bcd96e7ff2062274868814ccae0626589e. A sizeable diff, but very straightforward changes. Didn't see anything controversial. Left a few nits, but nothing blocking, only if you have to retouch.

  26. DrahtBot requested review from ismaelsadeeq on Nov 22, 2023
  27. DrahtBot requested review from pablomartin4btc on Nov 22, 2023
  28. TheCharlatan approved
  29. TheCharlatan commented at 10:41 PM on November 22, 2023: contributor

    ACK 9e58c5bcd96e7ff2062274868814ccae0626589e

  30. fanquake merged this on Nov 24, 2023
  31. fanquake closed this on Nov 24, 2023
  32. in src/rpc/txoutproof.cpp:72 in 9e58c5bcd9 
      68 | @@ -69,7 +69,7 @@ static RPCHelpMan gettxoutproof()
  69 |  
  70 |                  // Loop through txids and try to find which block they're in. Exit loop once a block is found.
  71 |                  for (const auto& tx : setTxids) {
  72 | -                    const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), tx);
  73 | +                    const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
    achow101 commented at 2:31 PM on November 28, 2023:

    New warning (fails with -werror)

    rpc/txoutproof.cpp: In lambda function:
rpc/txoutproof.cpp:72:33: error: possibly dangling reference to a temporary [-Werror=dangling-reference]
   72 |                     const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
      |                                 ^~~~
rpc/txoutproof.cpp:72:52: note: the temporary was destroyed at the end of the full expression ‘AccessByTxid((*(const CCoinsViewCache*)(&(& active_chainstate)->Chainstate::CoinsTip())), transaction_identifier<false>::FromUint256((* & tx)))’
   72 |                     const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));
      |                                        ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors
    dergoegge commented at 2:45 PM on November 28, 2023:

    What gcc version are you using? I don't get this warning (gcc 12).

    achow101 commented at 2:58 PM on November 28, 2023:

    gcc (GCC) 13.2.1 20230801

    maflcko commented at 3:20 PM on November 28, 2023:

    Can be fixed by turning setTxids into std::set<Txid>?

    maflcko commented at 4:06 PM on November 28, 2023:

    https://github.com/bitcoin/bitcoin/pull/28958

  33. fanquake referenced this in commit 8cf2137dbe on Nov 29, 2023
  34. hodlinator referenced this in commit 733222c183 on Jul 12, 2024
  35. hodlinator referenced this in commit a0ae0c6ab4 on Jul 12, 2024
  36. hodlinator referenced this in commit a4ead4e51c on Jul 12, 2024
  37. hodlinator referenced this in commit b9579cd020 on Jul 12, 2024
  38. hodlinator referenced this in commit 9bf527068c on Jul 12, 2024
  39. hodlinator referenced this in commit fed91bb7cb on Jul 12, 2024
  40. hodlinator referenced this in commit d20fc9cdff on Jul 16, 2024
  41. hodlinator referenced this in commit c3a9c71c70 on Jul 16, 2024
  42. hodlinator referenced this in commit 4923d90835 on Jul 17, 2024
  43. hodlinator referenced this in commit 649c88d4df on Jul 19, 2024
  44. hodlinator referenced this in commit 788fe9cc9a on Jul 19, 2024
  45. hodlinator referenced this in commit cdbfee6f2d on Jul 23, 2024
  46. hodlinator referenced this in commit 1b7bfbab04 on Jul 23, 2024
  47. hodlinator referenced this in commit bfcc016825 on Jul 23, 2024
  48. hodlinator referenced this in commit 09ce3501fa on Jul 23, 2024
  49. ryanofsky referenced this in commit 7cc00bfc86 on Jul 23, 2024
  50. davidgumberg referenced this in commit 8119ccde16 on Jul 29, 2024
  51. luke-jr referenced this in commit fffa0ff1f6 on Aug 1, 2024
  52. bitcoin locked this on Nov 27, 2024
Contributors
dergoeggeDrahtBotmaflckoSjorsstickies-vTheCharlatanismaelsadeeqpablomartin4btcachow101


ismaelsadeeqpablomartin4btc

Linked (view graph)
#28958 refactor: Use Txid in CMerkleBlock#30436 fix: Make TxidFromString() respect string_view length
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-05-02 18:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me