Skip to content

rpc, refactor: Avoid duplicate set lookup in gettxoutproof #19847

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Dec 7, 2020
Merged

rpc, refactor: Avoid duplicate set lookup in gettxoutproof #19847

merged 2 commits into from
Dec 7, 2020

Conversation

promag
Copy link
Contributor

@promag promag commented Aug 31, 2020

No description provided.

@promag promag force-pushed the 2020-08-gettxoutproof branch from ac2bb14 to 9542e05 Compare August 31, 2020 15:09
@instagibbs
Copy link
Member

ACK 9542e05

@maflcko
Copy link
Member

maflcko commented Sep 3, 2020

could also test empty array?

@promag
Copy link
Contributor Author

promag commented Sep 4, 2020 •
edited
Loading

@MarcoFalke you mean to change from

bitcoin-cli -regtest gettxoutproof '[]'
error code: -5
error message:
Transaction not yet in block

to

bitcoin-cli -regtest gettxoutproof '[]'
error code: -8
error message:
Parameter 'txids' cannot be empty

@maflcko maflcko mentioned this pull request Sep 9, 2020
Merged
@maflcko
Copy link
Member

maflcko commented Sep 9, 2020

I've taken your test and my test suggestion and added them to #19922 (another pull reworking the test a bit)

laanwj added a commit that referenced this pull request Sep 11, 2020
@laanwj
Merge #19922: test: Run rpc_txoutproof.py even with wallet disabled
f2d9934 
faf251d test: gettxoutproof duplicate txid (João Barbosa)
faf5eb4 test: Test empty array in gettxoutproof (MarcoFalke)
fa56e86 test: Run rpc_txoutproof.py even with wallet disabled (MarcoFalke)
faba790 test: MiniWallet: Default fee_rate in send_self_transfer, Pass in utxo_to_spend (MarcoFalke)
fa65a11 test: bugfix: Actually pick largest utxo (MarcoFalke)

Pull request description:

  Run the consensus test even when the wallet was not compiled. Also:

  * Minor bugfix in MiniWallet
  * Two new test cases (one cherry-picked from #19847)

ACKs for top commit:
  jnewbery:
    utACK faf251d. Thanks Marco!
  kristapsk:
    ACK faf251d

Tree-SHA512: a5ab33695c88cfb3c369021d4506069c08ce298e24e891db55159130693ed3817444c72f6aad3f472235aa4597b2c601010af714411c2ec8ad9c2d2e0b00ecbc
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 11, 2020
@laanwj
Merge bitcoin#19922: test: Run rpc_txoutproof.py even with wallet dis…
316baa6 
…abled

faf251d test: gettxoutproof duplicate txid (João Barbosa)
faf5eb4 test: Test empty array in gettxoutproof (MarcoFalke)
fa56e86 test: Run rpc_txoutproof.py even with wallet disabled (MarcoFalke)
faba790 test: MiniWallet: Default fee_rate in send_self_transfer, Pass in utxo_to_spend (MarcoFalke)
fa65a11 test: bugfix: Actually pick largest utxo (MarcoFalke)

Pull request description:

  Run the consensus test even when the wallet was not compiled. Also:

  * Minor bugfix in MiniWallet
  * Two new test cases (one cherry-picked from bitcoin#19847)

ACKs for top commit:
  jnewbery:
    utACK faf251d. Thanks Marco!
  kristapsk:
    ACK faf251d

Tree-SHA512: a5ab33695c88cfb3c369021d4506069c08ce298e24e891db55159130693ed3817444c72f6aad3f472235aa4597b2c601010af714411c2ec8ad9c2d2e0b00ecbc
@promag promag force-pushed the 2020-08-gettxoutproof branch from 9542e05 to c4d8377 Compare October 5, 2020 17:29
@promag promag changed the title test: gettxoutproof duplicate txid rpc, refactor: Avoid duplicate set lookup in gettxoutproof Oct 5, 2020
theStack
theStack approved these changes Oct 11, 2020
View reviewed changes
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK c4d8377 🧾

@maflcko
Copy link
Member

maflcko commented Oct 11, 2020

Not 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
Copy link
Contributor Author

promag commented Oct 12, 2020

@MarcoFalke done.

@promag promag force-pushed the 2020-08-gettxoutproof branch from 1af04b5 to 63ed27a Compare October 12, 2020 08:04
luke-jr
luke-jr approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@luke-jr luke-jr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

maflcko
maflcko reviewed Oct 21, 2020
View reviewed changes
@promag promag force-pushed the 2020-08-gettxoutproof branch from 63ed27a to 52fc399 Compare December 4, 2020 18:39
@jonasschnelli
Copy link
Contributor

nice small optimization!
code review ACK 52fc399

@jonasschnelli jonasschnelli merged commit eab63b9 into bitcoin:master Dec 7, 2020
maflcko
maflcko reviewed Dec 7, 2020
View reviewed changes
src/rpc/rawtransaction.cpp
@@ -287,7 +283,7 @@ static RPCHelpMan gettxoutproof()
LOCK(cs_main);

if (pblockindex == nullptr) {
const CTransactionRef tx = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, oneTxid, Params().GetConsensus(), hashBlock);
const CTransactionRef tx = GetTransaction(/* block_index */ nullptr, /* mempool */ nullptr, *setTxids.begin(), Params().GetConsensus(), hashBlock);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in commit 73dc19a:

Just for future reference, this is reading uninitialized memory

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Care to explain?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

std::set::begin will return the end interator if the set is empty.

Dereferencing the end pointer is accessing uninitialized memory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, would you check !setTxids.empty() again?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the bug only exists in commit 73dc19a , which can't be modified because it is already merged

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for spotting @MarcoFalke.
I guess this needs for a follow up PR.

Copy link
Contributor Author

@promag promag Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The bug is fixed in the 2nd commit 52fc399. It should have been the 1st commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now I got it. An intermediate commit had this uninitialized memory access. No need for action.

@promag promag deleted the 2020-08-gettxoutproof branch December 7, 2020 10:45
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Dec 7, 2020
@jonasschnelli
Merge bitcoin#19847: rpc, refactor: Avoid duplicate set lookup in get…
c8ac34f 
…txoutproof

52fc399 rpc: Reject empty txids in gettxoutproof (João Barbosa)
73dc19a rpc, refactor: Avoid duplicate set lookup in gettxoutproof (João Barbosa)

Pull request description:

ACKs for top commit:
  jonasschnelli:
    code review ACK 52fc399

Tree-SHA512: 76b18e5235e8b2d394685515a4a60335666eeb0f6b31c1d397f7db2fbe681bc817b8cd3e8f6708b9dacd6113e4e1d94837072cae27834b8a1a22d2717db8191e
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
4 more reviewers

@jonasschnelli jonasschnelli jonasschnelli left review comments

@maflcko maflcko maflcko left review comments

@theStack theStack theStack approved these changes

@luke-jr luke-jr luke-jr approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
RPC/REST/ZMQ
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@promag @instagibbs @maflcko @jonasschnelli @theStack @luke-jr @DrahtBot