ModifyNewCoins saves database lookups #6932
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
When processing a new transaction, in addition to spending the Coins of its txin's it creates a new Coins for its outputs. The existing ModifyCoins function will first make sure this Coins does not already exist. It can not exist due to BIP 30, but because of that the lookup can't be cached and always has to go to the database. Since we are creating the coins to match the new tx anyway, there is no point in checking if they exist first anyway. However this should not be used for coinbase tx's in order to preserve the historical behavior of overwriting the two existing duplicate tx pairs.
Contributor
|
ConceptACK, will review further and test. |
Member
|
Untested, code review ACK. Needs a unit test, though. |
Member
|
Together with #5967 it should be possible to also avoid the db read that's still done for coinbase transactions. |
Contributor
Author
|
@sipa I'm not sure if this was the kind of unit test that you had in mind? I thought it was important to actually test UpdateCoins instead of modifyNewCoins directly because it's how it is used that matters. This test passes on master, passes on this PR, but fails when UpdateCoins is changed to just mark coinbases as un-FRESH but still skip the lookup (assuming the assert is commented out to permit this). However if #5967 is merged then the test passes once again. |
Member
|
@morcos Awesome test. |
morcos
force-pushed
the
newCoinsThinAir
branch
from
dd338ca to
1cf3dd8
Compare
Contributor
Author
|
ok fixed the mess with the random nValue. |
Member
|
ACK |
Contributor
|
lightly tested ACK |
Member
There was a problem hiding this comment.
If the purpose is to do this every 100 iterations, why use random?
Either the comment or the code is wrong :)
Member
There was a problem hiding this comment.
It's copied from the test above that I wrote. It's intended to be random, so the comment is wrong :)
Member
|
Code review ACK |
laanwj
added a commit
that referenced
this pull request
Nov 18, 2015
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
May 14, 2018
Bitcoin 0.12 performance improvements Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6918 - bitcoin/bitcoin#6932 Part of #2074.
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
May 31, 2018
Bitcoin 0.12 performance improvements Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6918 - bitcoin/bitcoin#6932 Part of #2074.
zkbot
added a commit
to zcash/zcash
that referenced
this pull request
Nov 30, 2018
Bitcoin 0.12 performance improvements Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6918 - bitcoin/bitcoin#6932 Part of #2074.
milesmanley
pushed a commit
to RunOnFlux/fluxd
that referenced
this pull request
Mar 5, 2019
Bitcoin 0.12 performance improvements Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#6918 - bitcoin/bitcoin#6932 Part of #2074.
random-zebra
added a commit
to PIVX-Project/PIVX
that referenced
this pull request
Aug 8, 2020
9d97db0 Add unit test for UpdateCoins (random-zebra) 0965d3a Make CCoinsViewTest behave like CCoinsViewDB (Alex Morcos) 7d23d5c [Core] PIVX: remove db lookup for coinbase outputs (random-zebra) 1e88b7f ModifyNewCoins saves database lookups (Alex Morcos) Pull request description: Adapted version of bitcoin#6932 > When processing a new transaction, in addition to spending the Coins of its txin's, it creates a new Coins for its outputs. The existing ModifyCoins function will first make sure this Coins does not already exist. It can not exist due to BIP 30, but because of that, the lookup can't be cached and always has to go to the database. Since we are creating the Coins to match the new tx anyway, there is no point in checking if it exists first anyway. However this should not be used for coinbase tx's in order to preserve the historical behavior of overwriting the two existing duplicate tx pairs. > > In conjunction with bitcoin#6931 this will help ConnectBlock be much more efficient with caching access to the database. We remove the restriction on coinbases, and thus use always `ModifyNewCoins` instead of `ModifyCoins`, since duplicated coinbases are not possible on PIVX (ref. #1775). Accordingly, we remove the logic for checking the effects of duplicate coinbase txs in the unit test. ACKs for top commit: furszy: code looking good, tests and reindex went well too, ACK 9d97db0 Fuzzbawls: utACK 9d97db0 Tree-SHA512: e427f00699dfda29a9536b4e43919aecbfea90cf30195fe5a3835a49a715abf4f353a4672e6e99213b2016fb5b93f4ebfc94998a420ee8dc10626658069993af
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.
When processing a new transaction, in addition to spending the Coins of its txin's, it creates a new Coins for its outputs. The existing ModifyCoins function will first make sure this Coins does not already exist. It can not exist due to BIP 30, but because of that, the lookup can't be cached and always has to go to the database. Since we are creating the Coins to match the new tx anyway, there is no point in checking if it exists first anyway. However this should not be used for coinbase tx's in order to preserve the historical behavior of overwriting the two existing duplicate tx pairs.
In conjunction with #6931 this will help ConnectBlock be much more efficient with caching access to the database.
This still needs unit tests which exercise the new functionality.