-
Notifications
You must be signed in to change notification settings - Fork 107
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.
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
change(consensus) verify that mempool transaction UTXOs are in the best chain #5616
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, this looks good!
But I think we need to check for unspent outputs, not just any available output. See my detailed note on the request comment.
We might also want to add a TODO to rename AwaitUtxo
to AwaitTransactionOutput
(the U
means "Unspent"). There is already a TODO to combine the Output
, Utxo
, and OrderedUtxo
types, maybe we can add a rename to it as well. But I don't want to do a mass rename during an audit freeze.
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #5616 +/- ##
==========================================
- Coverage 78.75% 78.74% -0.01%
==========================================
Files 305 305
Lines 38320 38407 +87
==========================================
+ Hits 30177 30245 +68
- Misses 8143 8162 +19 |
Co-authored-by: teor <[email protected]>
cf84654
to
ece1be5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
I particularly like how you re-used the utxo() function code.
…st chain (#5616) - manual merge to skip getblocktemplate-rpcs * Uses BestChainUtxo to find utxos for mempool * adds missing input test * Apply suggestions from code review Co-authored-by: teor <[email protected]> * update other instances of the renamed InputNotFound error * adds read::unspent_utxo fn * adds test for success case Co-authored-by: teor <[email protected]>
Motivation
The transaction verifier currently checks that spent UTXOs of mempool transactions are in the state queue or on any chain, but it needs to verify that those spent UTXOs are on the best chain so that mined blocks including those transactions will be contextually valid.
Closes #5386
Solution
BestChainUtxo
request instead ofAwaitUtxo
when verifying mempool transactions.InputNotFound
variant toTransactionError
for when the spent UTXOs are not present on the best chain.Review
This PR is part of regular scheduled work.
Reviewer Checklist
Follow Up Work
retry_list
in the mempool storage for transactions that weren't verified withInputNotFound
?