Few Minor per-utxo assert-semantics re-adds and tweak #10537
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to per-utxo CCoins, we checked that no other in-mempool tx spent any of the given transaction's outputs, as we don't want to uncache that entire tx in such a case. However, we now are checking only that there exists no other mempool spends of the same output, which should clearly be impossible after we removed the transaction which was spending said output (barring massive mempool inconsistency). Thanks to @sdaftuar for the suggestion.
TheBlueMatt
force-pushed
the
2017-06-per-utxo-fixes
branch
from
05ae135 to
743bdb5
Compare
Member
|
utACK 743bdb5d52f7f7a0bcddfd55bccdac287140b491 |
ryanofsky
reviewed
Jun 9, 2017
Contributor
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK 743bdb5d52f7f7a0bcddfd55bccdac287140b491
It took me a little while to decipher first commit, but just to put it in my own words, the map mapNextTx.count(txin.prevout) check that this PR removes would always returns 0 because you can't have two transactions in the mempool spending the same prevout, and the one transaction that had been spending it just got removed out a few lines previously in RemoveStaged.
src/validation.cpp
Outdated
Contributor
There was a problem hiding this comment.
In commit "Return a bool in SpendCoin to restore pre-per-utxo assert semantics"
Maybe drop hungarian.
src/consensus/tx_verify.cpp
Outdated
Contributor
There was a problem hiding this comment.
In commit "Restore some assert semantics in sigop cost calculations"
Maybe update commit message to suggest why policy/atmp asserts aren't added back. Not possible there, or just not worth the effort?
Contributor
Author
There was a problem hiding this comment.
I figured it wasn't worth it - if we hit those errors in consensus code, we'd better crash cause we might fall out of consensus...if we accidentally add something to our mempool that should be out of policy...oh well, not a big deal. (I updated commit message).
TheBlueMatt
force-pushed
the
2017-06-per-utxo-fixes
branch
from
743bdb5 to
4cddd91
Compare
Since its free to do so, assert that Spends succeeded when we expect them to.
There are some similar asserts which are left removed in policy and ATMP (policy code being broken isn't a huge deal, but if we fail to verify some consensus rules, we should most definitely crash).
While the current implementation is pretty free, there is a lot of possibility for this to blow up in our face with future changes, especially as the backing map gets tweaked.
TheBlueMatt
force-pushed
the
2017-06-per-utxo-fixes
branch
from
4cddd91 to
9417d7a
Compare
Member
|
utACK 9417d7a |
sipa
added a commit
that referenced
this pull request
Jun 21, 2017
9417d7a Be much more agressive in AccessCoin docs. (Matt Corallo) f58349c Restore some assert semantics in sigop cost calculations (Matt Corallo) 3533fb4 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo) ec1271f Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo) Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Oct 26, 2017
…tweak 9417d7a Be much more agressive in AccessCoin docs. (Matt Corallo) f58349c Restore some assert semantics in sigop cost calculations (Matt Corallo) 3533fb4 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo) ec1271f Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo) Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca
codablock
pushed a commit
to codablock/dash
that referenced
this pull request
Oct 31, 2017
…tweak 9417d7a Be much more agressive in AccessCoin docs. (Matt Corallo) f58349c Restore some assert semantics in sigop cost calculations (Matt Corallo) 3533fb4 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo) ec1271f Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo) Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca
jasonbcox
pushed a commit
to Bitcoin-ABC/bitcoin-abc
that referenced
this pull request
Sep 13, 2019
Summary: 9417d7a Be much more agressive in AccessCoin docs. (Matt Corallo) f58349c Restore some assert semantics in sigop cost calculations (Matt Corallo) 3533fb4 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo) ec1271f Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo) Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca Backport of Core PR10537 bitcoin/bitcoin#10537 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3542
jonspock
pushed a commit
to jonspock/devault
that referenced
this pull request
Dec 22, 2019
Summary: 9417d7a33 Be much more agressive in AccessCoin docs. (Matt Corallo) f58349ca8 Restore some assert semantics in sigop cost calculations (Matt Corallo) 3533fb4d3 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo) ec1271f2b Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo) Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca Backport of Core PR10537 bitcoin/bitcoin#10537 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3542
proteanx
pushed a commit
to devaultcrypto/devault
that referenced
this pull request
Dec 23, 2019
Summary: 9417d7a33 Be much more agressive in AccessCoin docs. (Matt Corallo) f58349ca8 Restore some assert semantics in sigop cost calculations (Matt Corallo) 3533fb4d3 Return a bool in SpendCoin to restore pre-per-utxo assert semantics (Matt Corallo) ec1271f2b Remove useless mapNextTx lookup in CTxMemPool::TrimToSize. (Matt Corallo) Tree-SHA512: 158a4bce063eac93e1d50709500a10a7cb1fb3271f10ed445d701852fce713e2bf0da3456088e530ab005f194ef4a2adf0c7cb23226b160cecb37a79561f29ca Backport of Core PR10537 bitcoin/bitcoin#10537 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: Fabien, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3542
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
One small tweak which @sdaftuar commented and got missed, and two restore-previous-assert-semantics that I figured would be nice to re-add.