Use Txid in COutpoint #28922

dergoegge · 2023-11-21T12:05:04Z

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

DrahtBot · 2023-11-21T12:05:07Z

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.

src/primitives/transaction.h

src/rpc/blockchain.cpp

src/test/fuzz/coinscache_sim.cpp

src/util/transaction_identifier.h

Sjors

ACK 9e58c5b

Sjors · 2023-11-21T13:09:23Z

src/rest.cpp

            int32_t nOutput;
            std::string strTxid = uriParts[i].substr(0, uriParts[i].find('-'));
            std::string strOutput = uriParts[i].substr(uriParts[i].find('-')+1);

            if (!ParseInt32(strOutput, &nOutput) || !IsHex(strTxid))
                return RESTERR(req, HTTP_BAD_REQUEST, "Parse error");

-            txid.SetHex(strTxid);
-            vOutPoints.emplace_back(txid, (uint32_t)nOutput);
+            vOutPoints.emplace_back(TxidFromString(strTxid), (uint32_t)nOutput);


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

src/rpc/mempool.cpp

src/rpc/blockchain.cpp

src/rpc/rawtransaction_util.cpp

src/wallet/spend.cpp

Sjors · 2023-11-21T13:27:24Z

src/rpc/blockchain.cpp

@@ -1057,7 +1057,7 @@ static RPCHelpMan gettxout()

    UniValue ret(UniValue::VOBJ);

-    uint256 hash(ParseHashV(request.params[0], "txid"));
+    auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};


Txid hash ?

Sjors · 2023-11-21T13:36:21Z

@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 · 2023-11-22T16:02:01Z

Concept ACK

pablomartin4btc

Concept ACK

Light CR.

src/util/transaction_identifier.h

stickies-v

ACK 9e58c5b. 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.

stickies-v · 2023-11-22T17:41:47Z

src/rpc/blockchain.cpp

@@ -1057,7 +1057,7 @@ static RPCHelpMan gettxout()

    UniValue ret(UniValue::VOBJ);

-    uint256 hash(ParseHashV(request.params[0], "txid"));
+    auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};


nit

Suggested change

auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};

const auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};

stickies-v · 2023-11-22T17:42:25Z

src/test/fuzz/mini_miner.cpp

@@ -25,7 +25,7 @@ void initialize_miner()
    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
    g_setup = testing_setup.get();
    for (uint32_t i = 0; i < uint32_t{100}; ++i) {
-        g_available_coins.emplace_back(uint256::ZERO, i);
+        g_available_coins.emplace_back(Txid::FromUint256(uint256::ZERO), i);


nit

Suggested change

g_available_coins.emplace_back(Txid::FromUint256(uint256::ZERO), i);

g_available_coins.emplace_back(Txid{}, i);

src/test/fuzz/tx_pool.cpp

stickies-v · 2023-11-22T20:35:23Z

src/test/miniminer_tests.cpp

    // Add chain of size 500
    TestMemPoolEntryHelper entry;
-    std::vector<uint256> chain_txids;
+    std::vector<Txid> chain_txids;


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

stickies-v · 2023-11-22T20:36:54Z

src/wallet/spend.cpp

    for (const auto& entry : wallet.mapWallet)
    {
-        const uint256& wtxid = entry.first;
+        const uint256& txid = entry.first;


nit: Would simplify this to

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

git diff on 9e58c5b

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;

TheCharlatan

ACK 9e58c5b

TheCharlatan · 2023-11-22T20:49:54Z

src/rpc/blockchain.cpp

@@ -1057,7 +1057,7 @@ static RPCHelpMan gettxout()

    UniValue ret(UniValue::VOBJ);

-    uint256 hash(ParseHashV(request.params[0], "txid"));
+    auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};


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

TheCharlatan · 2023-11-22T21:30:59Z

src/wallet/spend.cpp

@@ -321,7 +321,7 @@ CoinsResult AvailableCoins(const CWallet& wallet,
    std::set<uint256> trusted_parents;
    for (const auto& entry : wallet.mapWallet)
    {
-        const uint256& wtxid = entry.first;
+        const uint256& txid = entry.first;


Nit: Maybe remove this line altogether?

TheCharlatan · 2023-11-22T22:27:12Z

src/test/fuzz/mini_miner.cpp

@@ -25,7 +25,7 @@ void initialize_miner()
    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
    g_setup = testing_setup.get();
    for (uint32_t i = 0; i < uint32_t{100}; ++i) {
-        g_available_coins.emplace_back(uint256::ZERO, i);
+        g_available_coins.emplace_back(Txid::FromUint256(uint256::ZERO), i);


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

achow101 · 2023-11-28T14:31:29Z

src/rpc/txoutproof.cpp

@@ -69,7 +69,7 @@ static RPCHelpMan gettxoutproof()

                // Loop through txids and try to find which block they're in. Exit loop once a block is found.
                for (const auto& tx : setTxids) {
-                    const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), tx);
+                    const Coin& coin = AccessByTxid(active_chainstate.CoinsTip(), Txid::FromUint256(tx));


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

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

gcc (GCC) 13.2.1 20230801

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

fa02c08 refactor: Use Txid in CMerkleBlock (MarcoFalke) Pull request description: This should also fix a gcc-13 compiler warning, see bitcoin/bitcoin#28922 (comment) ``` 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 ACKs for top commit: TheCharlatan: Re-ACK fa02c08 dergoegge: reACK fa02c08 Tree-SHA512: 2e6837b9d0c90bd6e9d766330e7086d68c6ec80bb27fe2cfc4702b251b00d91a79f8bfbc76d998cbcd90bee5317402cf617f61099eee96d94e7ac8f37ba7a642

Appears to have been a dormant bug. Introduced since inception of TxidFromString() in bitcoin#28922.

Appears to have been a fully dormant bug. Introduced since inception of TxidFromString(), discussed in bitcoin#28922 (comment).

Prior to this, passing in string_view::data() meant uint256S() would only receive the a naked char* pointer and scan past the string_view::length() until it found a null terminator. Appears to have been a fully dormant bug as callers where either passing a string literal or std::string directly to TxidFromFromString(), meaning null terminator always existed at pointer[length()]. Bug existed since original merge of TxidFromString(), discussed in bitcoin#28922 (comment).

Prior to this, passing in string_view::data() meant uint256S() would only receive the a naked char* pointer and scan past the string_view::length() until it found a null terminator. Appears to have been a fully dormant bug as callers were either passing a string literal or std::string directly to TxidFromFromString(), meaning null terminator always existed at pointer[length()]. Bug existed since original merge of TxidFromString(), discussed in bitcoin#28922 (comment).

Prior to this, passing string_view::data() into uint256S() meant the latter would only receive the a naked char* pointer and potentially scan past the string_view::length() until it found a null terminator (or some other non-hex character). Appears to have been a fully dormant bug as callers were either passing a string literal or std::string directly to TxidFromFromString(), meaning null terminator always existed at pointer[length()]. Bug existed since original merge of TxidFromString(), discussed in bitcoin#28922 (comment).

09ce350 fix: Make TxidFromString() respect string_view length (Hodlinator) 01e314c refactor: Change base_blob::SetHex() to take std::string_view (Hodlinator) 2f5577d test: uint256 - Garbage suffixes and zero padding (Hodlinator) f11f816 refactor: Make uint256_tests no longer use deprecated BOOST_CHECK() (Hodlinator) f0eeee2 test: Add test for TxidFromString() behavior (Ryan Ofsky) Pull request description: ### Problem Prior to this, `TxidFromString()` was passing `string_view::data()` into `uint256S()` which meant it would only receive the a naked `char*` pointer and potentially scan past the `string_view::length()` until it found a null terminator (or some other non-hex character). Appears to have been a fully dormant bug as callers were either passing a string literal or `std::string` directly to `TxidFromFromString()`, meaning a null terminator always existed at `pointer[length()]`. Bug existed since original merge of `TxidFromString()`. ### Solution Make `uint256S()` (and `base_blob::SetHex()`) take and operate on `std::string_view` instead of `const char*` and have `TxidFromString()` pass that in. (PR was prompted by comment in #30377 (comment) (referring to #28922 (comment))). ACKs for top commit: maflcko: re-ACK 09ce350 🕓 paplorinc: ACK 09ce350 ryanofsky: Code review ACK 09ce350. I think the current code changes are about as small as you could make to fix the bug without introducing a string copy, and the surrounding test improvements are all very nice and welcome. Tree-SHA512: c2c10551785fb6688d1e2492ba42a8eee4c19abbe8461bb0774d56a70c23cd6b0718d2641632890bee880c06202dee148126447dd2264eaed4f5fee7e1bcb581

Prior to this, passing string_view::data() into uint256S() meant the latter would only receive the a naked char* pointer and potentially scan past the string_view::length() until it found a null terminator (or some other non-hex character). Appears to have been a fully dormant bug as callers were either passing a string literal or std::string directly to TxidFromFromString(), meaning null terminator always existed at pointer[length()]. Bug existed since original merge of TxidFromString(), discussed in bitcoin#28922 (comment).

Prior to this, passing string_view::data() into uint256S() meant the latter would only receive the a naked char* pointer and potentially scan past the string_view::length() until it found a null terminator (or some other non-hex character). Appears to have been a fully dormant bug as callers were either passing a string literal or std::string directly to TxidFromFromString(), meaning null terminator always existed at pointer[length()]. Bug existed since original merge of TxidFromString(), discussed in bitcoin#28922 (comment). Github-Pull: bitcoin#30436 Rebased-From: 09ce350

DrahtBot mentioned this pull request Nov 21, 2023

refactor: VectorWriter and SpanReader without nVersion #28912

Merged

maflcko reviewed Nov 21, 2023

View reviewed changes

src/primitives/transaction.h Outdated Show resolved Hide resolved

src/rpc/blockchain.cpp Outdated Show resolved Hide resolved

src/test/fuzz/coinscache_sim.cpp Show resolved Hide resolved

src/util/transaction_identifier.h Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Nov 21, 2023

refactor: Replace sets of txiter with CTxMemPoolEntryRefs #28886

Closed

Use Txid in COutpoint

9e58c5b

dergoegge force-pushed the 2023-11-txid-oupoint branch from 9aeba9e to 9e58c5b Compare November 21, 2023 13:16

Sjors approved these changes Nov 21, 2023

View reviewed changes

This was referenced Nov 21, 2023

RFC: Remove boost usage from kernel api / headers #28335

Closed

wallet: Track no-longer-spendable TXOs separately #27865

Open

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Merged

pablomartin4btc reviewed Nov 22, 2023

View reviewed changes

src/util/transaction_identifier.h Show resolved Hide resolved

stickies-v approved these changes Nov 22, 2023

View reviewed changes

DrahtBot requested review from ismaelsadeeq and pablomartin4btc November 22, 2023 20:37

TheCharlatan approved these changes Nov 22, 2023

View reviewed changes

fanquake merged commit b5a2713 into bitcoin:master Nov 24, 2023

achow101 reviewed Nov 28, 2023

View reviewed changes

maflcko mentioned this pull request Nov 28, 2023

refactor: Use Txid in CMerkleBlock #28958

Merged

ryanofsky mentioned this pull request Nov 28, 2023

multiprocess: Add basic type conversion hooks #28921

Merged

theStack mentioned this pull request Nov 29, 2023

script/sign: avoid duplicated signature verification after signing (+introduce signing benchmarks) #28923

Merged

maflcko mentioned this pull request Jul 4, 2024

refactor: Replace ParseHex with consteval ""_hex literals #30377

Merged

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 12, 2024

fix: TxidFromString() now respects end of string

733222c

Appears to have been a dormant bug. Introduced since inception of TxidFromString() in bitcoin#28922.

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 12, 2024

fix: Make TxidFromString() respect string length

a0ae0c6

Appears to have been a dormant bug. Introduced since inception of TxidFromString() in bitcoin#28922.

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 12, 2024

fix: Make TxidFromString() respect string length

a4ead4e

Appears to have been a dormant bug. Introduced since inception of TxidFromString() in bitcoin#28922.

hodlinator mentioned this pull request Jul 12, 2024

fix: Make TxidFromString() respect string_view length #30436

Merged

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 12, 2024

fix: Make TxidFromString() respect string length

b9579cd

Appears to have been a fully dormant bug. Introduced since inception of TxidFromString(), discussed in bitcoin#28922 (comment).

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 12, 2024

fix: Make TxidFromString() respect string length

9bf5270

Appears to have been a fully dormant bug. Introduced since inception of TxidFromString(), discussed in bitcoin#28922 (comment).

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 12, 2024

fix: Make TxidFromString() respect string length

fed91bb

Appears to have been a fully dormant bug. Introduced since inception of TxidFromString(), discussed in bitcoin#28922 (comment).

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 16, 2024

fix: Make TxidFromString() respect string length

d20fc9c

Appears to have been a fully dormant bug. Introduced since inception of TxidFromString(), discussed in bitcoin#28922 (comment).

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 16, 2024

fix: Make TxidFromString() respect string length

c3a9c71

Appears to have been a fully dormant bug. Introduced since inception of TxidFromString(), discussed in bitcoin#28922 (comment).

hodlinator added a commit to hodlinator/bitcoin that referenced this pull request Jul 17, 2024

fix: Make TxidFromString() respect string length

4923d90

Appears to have been a fully dormant bug. Introduced since inception of TxidFromString(), discussed in bitcoin#28922 (comment).

bitcoin locked and limited conversation to collaborators Nov 27, 2024

	auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};
	const auto hash{Txid::FromUint256(ParseHashV(request.params[0], "txid"))};

	g_available_coins.emplace_back(Txid::FromUint256(uint256::ZERO), i);
	g_available_coins.emplace_back(Txid{}, i);

Use Txid in COutpoint #28922

Use Txid in COutpoint #28922

Uh oh!

Conversation

dergoegge commented Nov 21, 2023

Uh oh!

DrahtBot commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sjors commented Nov 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ismaelsadeeq commented Nov 22, 2023

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Nov 21, 2023 •

edited

Loading

Sjors commented Nov 21, 2023 •

edited

Loading