rpc, refactor: Avoid duplicate set lookup in gettxoutproof #19847
pull promag wants to merge 2 commits into bitcoin:master from promag:2020-08-gettxoutproof changing 2 files +8 −9-
promag commented at 2:49 pm on August 31, 2020: member
-
promag force-pushed on Aug 31, 2020
-
DrahtBot added the label RPC/REST/ZMQ on Aug 31, 2020
-
instagibbs commented at 12:08 pm on September 3, 2020: member
-
MarcoFalke commented at 1:42 pm on September 3, 2020: membercould also test empty array?
-
promag commented at 9:13 am on September 4, 2020: member
@MarcoFalke you mean to change from
0bitcoin-cli -regtest gettxoutproof '[]' 1error code: -5 2error message: 3Transaction not yet in block
to
0bitcoin-cli -regtest gettxoutproof '[]' 1error code: -8 2error message: 3Parameter 'txids' cannot be empty
-
MarcoFalke commented at 8:46 am on September 9, 2020: memberI’ve taken your test and my test suggestion and added them to #19922 (another pull reworking the test a bit)
-
laanwj referenced this in commit f2d9934381 on Sep 11, 2020
-
sidhujag referenced this in commit 316baa657c on Sep 11, 2020
-
DrahtBot added the label Needs rebase on Sep 11, 2020
-
promag force-pushed on Oct 5, 2020
-
promag renamed this:
test: gettxoutproof duplicate txid
rpc, refactor: Avoid duplicate set lookup in gettxoutproof
on Oct 5, 2020 -
DrahtBot removed the label Needs rebase on Oct 5, 2020
-
theStack approved
-
theStack commented at 3:58 pm on October 11, 2020: memberCode Review ACK c4d8377fbfa8b7b0f92552b0f66a100626ee178e 🧾
-
MarcoFalke commented at 5:24 pm on October 11, 2020: memberNot sure if this refactor is worth it. Unless you fix the error string for #19847 (comment) I don’t think this code needs to be touched.
-
promag commented at 0:46 am on October 12, 2020: member@MarcoFalke done.
-
promag force-pushed on Oct 12, 2020
-
luke-jr approved
-
luke-jr commented at 3:53 pm on October 20, 2020: memberutACK
-
rpc, refactor: Avoid duplicate set lookup in gettxoutproof 73dc19a330
-
rpc: Reject empty txids in gettxoutproof 52fc39917f
-
in src/rpc/rawtransaction.cpp:248 in 63ed27a3b3 outdated
245@@ -246,14 +246,15 @@ static RPCHelpMan gettxoutproof() 246 std::set<uint256> setTxids; 247 uint256 oneTxid; 248 UniValue txids = request.params[0].get_array(); 249+ if (txids.empty()) {
MarcoFalke commented at 9:21 am on October 21, 2020:you can move this to after the for loop and run the check on setTxids, then remove oneTxid and replace it with setTxids.front()
promag commented at 5:29 pm on December 4, 2020:ATMoneTxid
is the firsttxid
in the array, with you suggestion that wouldn’t be the case sinceset::set
orders, right?
MarcoFalke commented at 5:52 pm on December 4, 2020:oneTxid can be any txid
promag commented at 6:04 pm on December 4, 2020:Right, all must be in same block.
promag commented at 6:40 pm on December 4, 2020:Done, thanks! -
promag force-pushed on Dec 4, 2020
-
jonasschnelli commented at 8:16 am on December 7, 2020: contributornice small optimization! code review ACK 52fc39917fc52c2ff279fe434431e18900b347bd
-
jonasschnelli merged this on Dec 7, 2020
-
jonasschnelli closed this on Dec 7, 2020
-
in src/rpc/rawtransaction.cpp:286 in 73dc19a330 outdated
282@@ -287,7 +283,7 @@ static RPCHelpMan gettxoutproof() 283 LOCK(cs_main); 284 285 if (pblockindex == nullptr) { 286- const CTransactionRef tx = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, oneTxid, Params().GetConsensus(), hashBlock); 287+ const CTransactionRef tx = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, *setTxids.begin(), Params().GetConsensus(), hashBlock);
MarcoFalke commented at 8:52 am on December 7, 2020:in commit 73dc19a330f8cb063af46e6c4246f2e64a04bdc1:
Just for future reference, this is reading uninitialized memory
promag commented at 10:19 am on December 7, 2020:Care to explain?
MarcoFalke commented at 10:26 am on December 7, 2020:std::set::begin will return the end interator if the set is empty.
Dereferencing the end pointer is accessing uninitialized memory.
promag commented at 10:35 am on December 7, 2020:Right, would you check!setTxids.empty()
again?
MarcoFalke commented at 10:38 am on December 7, 2020:the bug only exists in commit 73dc19a , which can’t be modified because it is already merged
jonasschnelli commented at 10:41 am on December 7, 2020:Thanks for spotting @MarcoFalke. I guess this needs for a follow up PR.
promag commented at 10:43 am on December 7, 2020:The bug is fixed in the 2nd commit 52fc39917fc52c2ff279fe434431e18900b347bd. It should have been the 1st commit.
jonasschnelli commented at 11:00 am on December 7, 2020:Now I got it. An intermediate commit had this uninitialized memory access. No need for action. -
promag deleted the branch on Dec 7, 2020
-
sidhujag referenced this in commit c8ac34f1d4 on Dec 7, 2020
-
DrahtBot locked this on Feb 15, 2022