Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock #6445

jtimon · 2015-07-15T23:30:15Z

In all the 3 functions, reject transactions creating new money earlier.
Consensus::CheckTxInputs gets nTxFee as output parameter and is separated from main::CheckInputs [renamed CheckInputsScripts]. This continues #6061.

Detailed optimizations:

Consensus::CheckTxInputs (called by the rest):

Don't calculate nValueOut twice
Don't check nFees < 0 twice

main::AcceptToMemoryPool:

Don't calculate nValueOut 5 times
Don't calculate nValueIn 3 times
Don't call CCoinsViewCache::HaveInputs 3 times

miner::CreateNewBlock:

Don't calculate nValueOut 3 times
Don't calculate nValueIn twice
Don't call CCoinsViewCache::HaveInputs twice

main::ConnectBlock:

Don't calculate nValueOut 3 times
Don't calculate nValueIn twice
Still call CCoinsViewCache::HaveInputs twice

ghost · 2015-07-16T22:44:39Z

Have you benchmarked these optimisations?

jtimon · 2015-07-17T08:10:55Z

@NanoAkron Actually, no, I haven't. I don't really think the improvement will be that great, but hopefully it will be obvious to reviewers that performance can only be better, not worse (and the code is also cleaner IMO).
One thing is not easy to benchmark is changing the order of checks, for example, moving the check that impedes transactions from creating new money before checking the "standard-ness" of input scripts. Is that better? Well, that depends on how many of the transactions checked by acceptToMempool don't have standard scripts and how many are attempting to create new money, etc. So it basically depends on the test set and benchmarking that is not really that interesting. I can move those checks less if I'm asked to though.

But benchmarking the other changes (less repeated operations if the transaction is accepted) would be great. @sipa do you have some advice (or maybe a code snippet to copy) on how to benchmark this?

sipa · 2015-07-24T18:39:51Z

src/txmempool.cpp

@@ -275,7 +275,8 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
            waitingOnDependants.push_back(&it->second);
        else {
            CValidationState state;
-            assert(CheckInputs(tx, state, mempoolDuplicate, false, 0, false, NULL));
+            CAmount nTxFees;
+            assert(Consensus::CheckTxInputs(tx, state, mempoolDuplicate, GetSpendHeight(mempoolDuplicate), nTxFees));


Not your fault, but I wasn't aware of this call from mempool to main; I think we should get rid of it.

Sure, these 2 checks can be removed instead of adapted if that's preferrable.

jtimon · 2015-07-28T16:31:10Z

Needed rebase after #6077 . I also removed the checks in txmempool.cpp as suggested by @sipa

jtimon · 2015-07-28T16:45:31Z

Note that this is now kind of a de-optimization, since the cache has not being properly adapted (and thus the Consensus::CheckTxInputs calls are not using the cache anymore).
I'm not sure anymore which version is faster. This may not be faster than master after #6077 so benchmarks would be appreciated.
Or maybe I can just close this and "start" again. Something like jtimon@ddd505a can't be a "next step" to this anymore, and jtimon@a6d1e6e (very outdated) now looks more far away in the future than ever...

Yeah, probably closing unless someone really wants it open. Mhmm, this was the last consensus PR I had open...

jtimon · 2015-07-28T17:18:19Z

Maybe I should open one just removing the checks in txmempool.cpp ?

jtimon · 2015-07-29T11:52:29Z

Rebased and reopened after #6077 has been reverted.

…lock, and miner::CreateNewBlock and In all of them, reject transactions creating new money earlier. Consensus::CheckTxInputs gets nTxFee as output parameter and is separated from main::CheckInputs [renamed CheckInputsScripts] - Consensus::CheckTxInputs (called by the rest): Don't calculate nValueOut twice Don't check nFees < 0 twice - main::AcceptToMemoryPool: Don't call CCoinsViewCache::HaveInputs twice Don't calculate nValueIn 3 times Don't calculate nValueOut 5 times - miner::CreateNewBlock: Don't call CCoinsViewCache::HaveInputs twice Don't calculate nValueIn twice Don't calculate nValueOut 3 times - main::ConnectBlock: Still call CCoinsViewCache::HaveInputs twice Don't calculate nValueIn twice Don't calculate nValueOut 3 times

jtimon · 2015-08-20T23:08:39Z

Needed rebase after #6519 (which by the way it's a step forwards for completing libconsensus, thanks @laanwj ).

jtimon · 2015-09-06T22:53:14Z

Closing until one of #6526/#6625 gets merged and probably also until #6557 / TheBlueMatt@88a3b8d gets merged as well.

…it is checked that no money... 4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón) 3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón) 832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón) 3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón) Pull request description: ...is created by individual transactions to 2 places (but call only once in each): - ConnectBlock ( before calculated fees per txs twice ) - AcceptToMemoryPoolWorker ( before called CheckTxInputs 4 times and calculated fees per tx one extra time ) Also call tx.GetValueOut() only once per call of CheckTxInputs (instead of 2) For more motivation: ~~https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1493~~ jtimon/bitcoin@0.13-consensus-inputs...jtimon:0.13-consensus-inputs-comments EDIT: partially replaces #6445 Near-Bugfix as pointed out in #8498 (comment) Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346

… times it is checked that no money... 4e955c5 Near-Bugfix: Reestablish consensus check removed in 8d7849b (Jorge Timón) 3e8c916 Introduce CheckInputsAndUpdateCoins static wrapper in txmempool.cpp (Jorge Timón) 832e074 Optimization: Minimize the number of times it is checked that no money is created (Jorge Timón) 3f0ee3e Proper indentation for CheckTxInputs and other minor fixes (Jorge Timón) Pull request description: ...is created by individual transactions to 2 places (but call only once in each): - ConnectBlock ( before calculated fees per txs twice ) - AcceptToMemoryPoolWorker ( before called CheckTxInputs 4 times and calculated fees per tx one extra time ) Also call tx.GetValueOut() only once per call of CheckTxInputs (instead of 2) For more motivation: ~~https://github.com/bitcoin/bitcoin/blob/master/src/main.cpp#L1493~~ jtimon/bitcoin@0.13-consensus-inputs...jtimon:0.13-consensus-inputs-comments EDIT: partially replaces dashpay#6445 Near-Bugfix as pointed out in bitcoin#8498 (comment) Tree-SHA512: c71188e7c7c2425c9170ed7b803896755a92fd22f43b136eedaa6e554106696f0b10271d0ef0d0127c1eaafbc31d12eb19143df4f1b6882feecedf6ef05ea346

jtimon changed the title ~~Optimizations: Consensus: In main::AcceptToMemoryPool, main::ConnectBlock, and miner::CreateNewBlock~~ Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock Jul 15, 2015

jtimon force-pushed the consensus-txinputs-0.12.99 branch from dfaf01e to dc7db85 Compare July 16, 2015 00:05

jtimon mentioned this pull request Jul 16, 2015

Continuously limit the memory pool memory consumption #6421

Closed

jtimon force-pushed the consensus-txinputs-0.12.99 branch from dc7db85 to 2b23dd3 Compare July 16, 2015 10:56

jtimon mentioned this pull request Jul 17, 2015

Policy: Prepare AcceptToMemoryPool for encapsulated alternative replacement policies #6416

Closed

laanwj added the Consensus label Jul 17, 2015

jtimon mentioned this pull request Jul 20, 2015

Implement on-the-fly mempool size limitation #6448

Closed

sipa reviewed Jul 24, 2015
View reviewed changes

jtimon force-pushed the consensus-txinputs-0.12.99 branch from 2b23dd3 to 7a00431 Compare July 28, 2015 16:29

jtimon closed this Jul 28, 2015

jtimon mentioned this pull request Jul 28, 2015

Cache transaction validation successes #6077

Merged

jtimon reopened this Jul 29, 2015

jtimon force-pushed the consensus-txinputs-0.12.99 branch from 7a00431 to 15a98c5 Compare July 29, 2015 11:51

jtimon force-pushed the consensus-txinputs-0.12.99 branch from 15a98c5 to 1c87931 Compare August 20, 2015 22:22

jtimon force-pushed the consensus-txinputs-0.12.99 branch from 1c87931 to 1cce439 Compare August 20, 2015 23:07

This was referenced Aug 24, 2015

Remove TX priority and free transaction area from mempool, block creator. #6405

Closed

Policy: Remove free transactions special case code #6584

Closed

jtimon closed this Sep 6, 2015

jtimon mentioned this pull request Nov 11, 2015

BIP-68: Mempool-only sequence number constraint verification #6312

Closed

This was referenced Nov 23, 2015

nSequence-based Full-RBF opt-in #6871

Merged

Move maxTxFee out of mempool #7070

Merged

jtimon mentioned this pull request Jan 4, 2016

Report non-mandatory script failures correctly #7276

Merged

jtimon mentioned this pull request Aug 11, 2016

Near-Bugfix: Optimization: Minimize the number of times it is checked that no money... #8498

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock #6445

Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock #6445

Uh oh!

jtimon commented Jul 15, 2015

Uh oh!

ghost commented Jul 16, 2015

Uh oh!

jtimon commented Jul 17, 2015

Uh oh!

sipa Jul 24, 2015

Uh oh!

jtimon Jul 24, 2015

Uh oh!

jtimon commented Jul 28, 2015

Uh oh!

jtimon commented Jul 28, 2015

Uh oh!

jtimon commented Jul 28, 2015

Uh oh!

jtimon commented Jul 29, 2015

Uh oh!

jtimon commented Aug 20, 2015

Uh oh!

jtimon commented Sep 6, 2015

Uh oh!

Uh oh!

Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock #6445

Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock #6445

Uh oh!

Conversation

jtimon commented Jul 15, 2015

Uh oh!

ghost commented Jul 16, 2015

Uh oh!

jtimon commented Jul 17, 2015

Uh oh!

sipa Jul 24, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon Jul 24, 2015

Choose a reason for hiding this comment

Uh oh!

jtimon commented Jul 28, 2015

Uh oh!

jtimon commented Jul 28, 2015

Uh oh!

jtimon commented Jul 28, 2015

Uh oh!

jtimon commented Jul 29, 2015

Uh oh!

jtimon commented Aug 20, 2015

Uh oh!

jtimon commented Sep 6, 2015

Uh oh!

Uh oh!