Use Txid in COutpoint

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.

DrahtBot commented at 12:05 pm on November 21, 2023: contributor

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

Code Coverage

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

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.

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.

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:

0        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!

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

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.

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.

Use Txid in COutpoint 9e58c5bcd9

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

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).

dergoegge force-pushed on Nov 21, 2023

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.

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

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

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

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

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

Nit: Maybe remove this line altogether?

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

0    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?

Sjors approved

Sjors commented at 1:34 pm on November 21, 2023: member

ACK 9e58c5bcd96e7ff2062274868814ccae0626589e

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.

ismaelsadeeq commented at 4:02 pm on November 22, 2023: member

Concept ACK

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.

pablomartin4btc commented at 5:27 pm on November 22, 2023: member

Concept ACK

Light CR.

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

0        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?

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

0        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.

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?

stickies-v approved

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.

DrahtBot requested review from ismaelsadeeq on Nov 22, 2023

DrahtBot requested review from pablomartin4btc on Nov 22, 2023

TheCharlatan approved

TheCharlatan commented at 10:41 pm on November 22, 2023: contributor

ACK 9e58c5bcd96e7ff2062274868814ccae0626589e

fanquake merged this on Nov 24, 2023

fanquake closed this on Nov 24, 2023

Use Txid in COutpoint #28922

Code Coverage

Reviews

Conflicts