Simplify return values of GetCoin/HaveCoin(InCache) #10581
Conversation
|
@morcos @ryanofsky Feel like having a look at this? |
src/test/coins_tests.cpp
Outdated
There was a problem hiding this comment.
What's the point of the two InsecureRandBits calls skipping the new BOOST_CHECKs? It seems like the checks should always pass and aren't very expensive. Is the point to avoid creating cache entries during the test? Maybe add a comment saying why this only randomly checks HaveCoin results.
There was a problem hiding this comment.
Added a comment, and shuffled the code a bit to make it clearer.
|
@ryanofsky Oh yes, indeed! Rebased on top of #10559. |
ryanofsky
left a comment
ryanofsky
left a comment
There was a problem hiding this comment.
utACK 04087f7d83360482e031e9ecbcc97a29fb3b1b8a. Only new changes are test cleanup and rebase.
src/test/coins_tests.cpp
Outdated
There was a problem hiding this comment.
micro nit eachSPCother
|
Rebased. |
This removes the possibility for GetCoin/HaveCoin/HaveCoinInCache to return true while the respective coin is spent. By doing it across all calls, some extra checks can be eliminated. coins_tests is modified to call HaveCoin sometimes before and sometimes after AccessCoin. A further change is needed because the semantics for GetCoin slightly changed, causing a pruned entry in the parent cache to not be pulled into the child in FetchCoin.
|
utACK 21180ff, certainly makes the API better |
21180ff Simplify return values of GetCoin/HaveCoin(InCache) (Pieter Wuille) Tree-SHA512: eae0aa64fa1308191100cdc7cdc790c825f33b066c200a18b5895d7d5806cee1cc4caba1766ef3379a7cf93dde4bbae2bc9be92947935f5741f5c126d3ee991b
|
posthumous utACK @sipa, I'd misinterpreted this #10559 (comment) to imply you wanted ViewMempool functions to include whether the mempool contains a spend. But now I think you didn't want that and are happy with where we are now? If so, so am I. |
|
@morcos Yes, the CCoinsViewMemPool view is internally consistent and follows all expectations of a CCoinsView. It just still contains outputs spent inside the mempool. |
|
Postumous utACK, there are a bunch of places that are only barely safe before this change, and this makes them much more clearly safe 👍 . |
21180ff Simplify return values of GetCoin/HaveCoin(InCache) (Pieter Wuille) Tree-SHA512: eae0aa64fa1308191100cdc7cdc790c825f33b066c200a18b5895d7d5806cee1cc4caba1766ef3379a7cf93dde4bbae2bc9be92947935f5741f5c126d3ee991b
21180ff Simplify return values of GetCoin/HaveCoin(InCache) (Pieter Wuille) Tree-SHA512: eae0aa64fa1308191100cdc7cdc790c825f33b066c200a18b5895d7d5806cee1cc4caba1766ef3379a7cf93dde4bbae2bc9be92947935f5741f5c126d3ee991b
Summary: 21180ff Simplify return values of GetCoin/HaveCoin(InCache) (Pieter Wuille) Tree-SHA512: eae0aa64fa1308191100cdc7cdc790c825f33b066c200a18b5895d7d5806cee1cc4caba1766ef3379a7cf93dde4bbae2bc9be92947935f5741f5c126d3ee991b Backport of Core PR10581 bitcoin/bitcoin#10581 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3620
Summary: 21180ff Simplify return values of GetCoin/HaveCoin(InCache) (Pieter Wuille) Tree-SHA512: eae0aa64fa1308191100cdc7cdc790c825f33b066c200a18b5895d7d5806cee1cc4caba1766ef3379a7cf93dde4bbae2bc9be92947935f5741f5c126d3ee991b Backport of Core PR10581 bitcoin/bitcoin#10581 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3620
Summary: 21180ff Simplify return values of GetCoin/HaveCoin(InCache) (Pieter Wuille) Tree-SHA512: eae0aa64fa1308191100cdc7cdc790c825f33b066c200a18b5895d7d5806cee1cc4caba1766ef3379a7cf93dde4bbae2bc9be92947935f5741f5c126d3ee991b Backport of Core PR10581 bitcoin/bitcoin#10581 Test Plan: make check test_runner.py Reviewers: deadalnix, Fabien, jasonbcox, markblundeberg, O1 Bitcoin ABC, #bitcoin_abc Reviewed By: deadalnix, O1 Bitcoin ABC, #bitcoin_abc Differential Revision: https://reviews.bitcoinabc.org/D3620
7 participants
Superset of #10559
This removes the possibility for GetCoin/HaveCoin/HaveCoinInCache to return
truewhile the respective coin is spent. By doing it across all calls, some extra checks can be eliminated.coins_tests is modified to call HaveCoin sometimes before and sometimes after AccessCoin. A further change is needed because the semantics for GetCoin slightly changed, causing a pruned entry in the parent cache to not be pulled into the child in FetchCoin.