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

Cache transaction validation successes #6077

Merged
merged 2 commits into from Jul 27, 2015

Conversation

Projects
None yet
6 participants
@sipa
Member

sipa commented Apr 28, 2015

This is an alternative to @gavinandresen's #5835, which caches transaction validation results in an mru cache in main, rather than relying on the mempool in consensus code. The cache keeps the last 33k results, and uses around 3 MB on 64-bit systems.

This pull request also contains a rebased version of the unit test from #5835, and the optimizations from #6064.

@sipa sipa changed the title from Cache transactionc validation successes to Cache transaction validation successes Apr 28, 2015

@laanwj laanwj added the Validation label Apr 29, 2015

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 29, 2015

Member

Reimplemented the mrumap using boost::multiindex, which is more compact, and offers erasing elements too. Entries are now removed from the tx validation cache when they are accepted into a block, meaning the last 33k valid unconfirmed transactions are cached, rather than just the last 33k whatsoever.

Member

sipa commented Apr 29, 2015

Reimplemented the mrumap using boost::multiindex, which is more compact, and offers erasing elements too. Entries are now removed from the tx validation cache when they are accepted into a block, meaning the last 33k valid unconfirmed transactions are cached, rather than just the last 33k whatsoever.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Apr 30, 2015

Member

Rebased on new #6064.

Member

sipa commented Apr 30, 2015

Rebased on new #6064.

@gavinandresen

This comment has been minimized.

Show comment
Hide comment
@gavinandresen

gavinandresen May 15, 2015

Contributor

Concept ACK. I hope to run some benchmarks to see how much this speeds up validation with a stream of real 'tx' and 'block' messages and different -dbcache settings, but that probably won't happen until I'm back from New York late next week.

I also want to benchmark #5967 and #6102 .

Contributor

gavinandresen commented May 15, 2015

Concept ACK. I hope to run some benchmarks to see how much this speeds up validation with a stream of real 'tx' and 'block' messages and different -dbcache settings, but that probably won't happen until I'm back from New York late next week.

I also want to benchmark #5967 and #6102 .

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 14, 2015

Member

Rebased.

Member

sipa commented Jun 14, 2015

Rebased.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 16, 2015

Member

Actually rebased.

Member

sipa commented Jun 16, 2015

Actually rebased.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jun 21, 2015

Member

Been running on bitcoin.sipa.be for a week, no problems.

Member

sipa commented Jun 21, 2015

Been running on bitcoin.sipa.be for a week, no problems.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 27, 2015

Member
Member

sipa commented Jul 27, 2015

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Jul 27, 2015

Contributor

"it works" positive & negative test + review ACK

Contributor

jgarzik commented Jul 27, 2015

"it works" positive & negative test + review ACK

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 27, 2015

Member

Rebased.

Member

sipa commented Jul 27, 2015

Rebased.

Unit test doublespends in new blocks
As suggested by Greg Maxwell-- unit test to make sure a block
with a double-spend in it doesn't pass validation if half of
the double-spend is already in the memory pool (so full-blown
transaction validation is skipped) when the block is received.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 27, 2015

Member

ACK

Member

laanwj commented Jul 27, 2015

ACK

@laanwj laanwj merged commit 517e6dd into bitcoin:master Jul 27, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Jul 27, 2015

Merge pull request #6077
517e6dd Unit test doublespends in new blocks (Gavin Andresen)
17b1142 Cache transaction validation successes (Pieter Wuille)

sipa added a commit that referenced this pull request Jul 27, 2015

Cache transaction validation successes
Conflicts:
	src/main.cpp
	src/test/test_bitcoin.cpp

Github-Pull: #6077
Rebased-From: 17b1142 517e6dd
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 27, 2015

Member

Backported to 0.11 as bc484ef

Member

laanwj commented Jul 27, 2015

Backported to 0.11 as bc484ef

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 28, 2015

Member

I missed this..
We want to cache unconfirmed transactions, sure. But why cache the transactions that are being confirmed in connectBlock?

EDIT: As in, couldn't this have been done in AcceptToMemoryPool instead of CheckScript (which, by the way, it's called twice inside AcceptToMemoryPool)?

Member

jtimon commented Jul 28, 2015

I missed this..
We want to cache unconfirmed transactions, sure. But why cache the transactions that are being confirmed in connectBlock?

EDIT: As in, couldn't this have been done in AcceptToMemoryPool instead of CheckScript (which, by the way, it's called twice inside AcceptToMemoryPool)?

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 28, 2015

Member

@jtimon To avoid rechecking the same transaction when it enters a block, when it has already been verified for the mempool (or whatever other reason).

Member

sipa commented Jul 28, 2015

@jtimon To avoid rechecking the same transaction when it enters a block, when it has already been verified for the mempool (or whatever other reason).

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 28, 2015

Member

@sipa Thanks, I guess this the simplest way to do that right now.

Member

jtimon commented Jul 28, 2015

@sipa Thanks, I guess this the simplest way to do that right now.

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 28, 2015

Member

As mentioned on IRC, I think there's a problem with transaction checks involving block height. Consider a coinbase spend that is valid when the transaction arrives and is accepted into the mempool, but is invalid at any earlier block height because of the coinbase maturity test. If there's a reorg, then when validating blocks at earlier heights, we'd consider blocks valid even if they include this transaction at a too-early height, because of this validation cache. (A test that demonstrates this is here: https://gist.github.com/sdaftuar/23a3db523e68541a3195)

Member

sdaftuar commented Jul 28, 2015

As mentioned on IRC, I think there's a problem with transaction checks involving block height. Consider a coinbase spend that is valid when the transaction arrives and is accepted into the mempool, but is invalid at any earlier block height because of the coinbase maturity test. If there's a reorg, then when validating blocks at earlier heights, we'd consider blocks valid even if they include this transaction at a too-early height, because of this validation cache. (A test that demonstrates this is here: https://gist.github.com/sdaftuar/23a3db523e68541a3195)

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Jul 28, 2015

Member

@sdaftuar now that you say it, that's why I repeated the checks even when the transaction had been fully validated here: jtimon@935ee1e#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR779
Maybe I was too fast closing #6445 (which should solve the problem) after all...

Member

jtimon commented Jul 28, 2015

@sdaftuar now that you say it, that's why I repeated the checks even when the transaction had been fully validated here: jtimon@935ee1e#diff-7ec3c68a81efff79b6ca22ac1f1eabbaR779
Maybe I was too fast closing #6445 (which should solve the problem) after all...

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 28, 2015

Member

Going to revert, thanks for pointing this out, @sdaftuar.

Member

sipa commented Jul 28, 2015

Going to revert, thanks for pointing this out, @sdaftuar.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Aug 21, 2015

Member

This wasn't fully reverted, sipa@517e6dd broke #6235 and #6382
@laanwj don't merge #6235 because that would break the build on master. Fixing now...

Member

jtimon commented Aug 21, 2015

This wasn't fully reverted, sipa@517e6dd broke #6235 and #6382
@laanwj don't merge #6235 because that would break the build on master. Fixing now...

laanwj added a commit that referenced this pull request Oct 9, 2015

zkbot added a commit to zcash/zcash that referenced this pull request Mar 21, 2018

Auto merge of #2390 - str4d:2132-mapargs-prep, r=<try>
Misc upstream PRs

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#6077
  - Second commit only (first was already applied to 0.11.X and then reverted)
- bitcoin/bitcoin#6284
- bitcoin/bitcoin#6489
- bitcoin/bitcoin#6462
- bitcoin/bitcoin#6647
- bitcoin/bitcoin#6235
- bitcoin/bitcoin#6905
- bitcoin/bitcoin#6780
  - Excluding second commit (QT) and third commit (requires bitcoin/bitcoin#6993)
- bitcoin/bitcoin#6961
  - Excluding QT parts, and a small `src/policy/policy.cpp` change which depends on a bunch of other PRs, which we'll have to remember to come back to.
- bitcoin/bitcoin#7044
- bitcoin/bitcoin#8856
- bitcoin/bitcoin#9002

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