ModifyNewCoins saves database lookups #6932

Merged
merged 3 commits into from Nov 18, 2015

Conversation

Projects
None yet
5 participants
@morcos
Member

morcos commented Nov 3, 2015

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.

ModifyNewCoins saves database lookups
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.
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 3, 2015

Member

ConceptACK, will review further and test.

Member

gmaxwell commented Nov 3, 2015

ConceptACK, will review further and test.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 11, 2015

Member

Untested, code review ACK. Needs a unit test, though.

Member

sipa commented Nov 11, 2015

Untested, code review ACK. Needs a unit test, though.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 11, 2015

Member

Together with #5967 it should be possible to also avoid the db read that's still done for coinbase transactions.

Member

sipa commented Nov 11, 2015

Together with #5967 it should be possible to also avoid the db read that's still done for coinbase transactions.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 12, 2015

Member

@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 commented Nov 12, 2015

@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.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 12, 2015

Member

@morcos Awesome test.

Member

sipa commented Nov 12, 2015

@morcos Awesome test.

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 12, 2015

Member

ok fixed the mess with the random nValue.

Member

morcos commented Nov 12, 2015

ok fixed the mess with the random nValue.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 12, 2015

Member

ACK

Member

sipa commented Nov 12, 2015

ACK

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Nov 12, 2015

Contributor

lightly tested ACK

Contributor

jgarzik commented Nov 12, 2015

lightly tested ACK

+ }
+
+ if (insecure_rand() % 100 == 0) {
+ // Every 100 iterations, change the cache stack.

This comment has been minimized.

@laanwj

laanwj Nov 13, 2015

Member

If the purpose is to do this every 100 iterations, why use random?
Either the comment or the code is wrong :)

@laanwj

laanwj Nov 13, 2015

Member

If the purpose is to do this every 100 iterations, why use random?
Either the comment or the code is wrong :)

This comment has been minimized.

@sipa

sipa Nov 13, 2015

Member

It's copied from the test above that I wrote. It's intended to be random, so the comment is wrong :)

@sipa

sipa Nov 13, 2015

Member

It's copied from the test above that I wrote. It's intended to be random, so the comment is wrong :)

+ // Every 100 iterations, change the cache stack.
+ if (stack.size() > 0 && insecure_rand() % 2 == 0) {
+ stack.back()->Flush();
+ delete stack.back();

This comment has been minimized.

@laanwj

laanwj Nov 13, 2015

Member

If this was in production code I'd prefer to use a RAII pointer type (such as boost::shared_ptr) inside stack instead of explicit delete, because of exception safety and memory leaks.
As it is just the tests, mehh.

@laanwj

laanwj Nov 13, 2015

Member

If this was in production code I'd prefer to use a RAII pointer type (such as boost::shared_ptr) inside stack instead of explicit delete, because of exception safety and memory leaks.
As it is just the tests, mehh.

This comment has been minimized.

@sipa

sipa Nov 13, 2015

Member
@sipa

sipa via email Nov 13, 2015

Member

This comment has been minimized.

@laanwj

laanwj Nov 13, 2015

Member

Right - I think shared_ptr is the only boost pointer you can use inside containers.

Edit: with c++11 unique_ptr it should be possible, though, I guess we can change it by then :)

@laanwj

laanwj Nov 13, 2015

Member

Right - I think shared_ptr is the only boost pointer you can use inside containers.

Edit: with c++11 unique_ptr it should be possible, though, I guess we can change it by then :)

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 13, 2015

Member

Code review ACK

Member

laanwj commented Nov 13, 2015

Code review ACK

@laanwj laanwj merged commit 1cf3dd8 into bitcoin:master Nov 18, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Nov 18, 2015

Merge pull request #6932
1cf3dd8 Add unit test for UpdateCoins (Alex Morcos)
03c8282 Make CCoinsViewTest behave like CCoinsViewDB (Alex Morcos)
14470f9 ModifyNewCoins saves database lookups (Alex Morcos)

@dagurval dagurval referenced this pull request in bitcoinxt/bitcoinxt Dec 21, 2017

Merged

Add test for UpdateCoins #285

@str4d str4d referenced this pull request in zcash/zcash May 14, 2018

Open

Bitcoin 0.12 performance improvements #3262

zkbot added a commit to zcash/zcash that referenced this pull request May 14, 2018

Auto merge of #3262 - str4d:2074-perf-1, r=<try>
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 14, 2018

Auto merge of #3262 - str4d:2074-perf-1, r=<try>
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

Auto merge of #3262 - str4d:2074-perf-1, r=<try>
Bitcoin 0.12 performance improvements

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6918
- bitcoin/bitcoin#6932

Part of #2074.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment