Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock

jtimon commented at 11:30 pm on July 15, 2015: contributor

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

jtimon renamed this:
~~Optimizations: Consensus: In main::AcceptToMemoryPool, main::ConnectBlock, and miner::CreateNewBlock~~
Optimizations: Consensus: In AcceptToMemoryPool, ConnectBlock, and CreateNewBlock
on Jul 15, 2015

jtimon force-pushed on Jul 16, 2015

ghost commented at 10:44 pm on July 16, 2015: none

Have you benchmarked these optimisations?

jtimon commented at 8:10 am on July 17, 2015: contributor

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

laanwj added the label Consensus on Jul 17, 2015

in src/txmempool.cpp: in 2b23dd340a outdated

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

sipa commented at 6:39 pm on July 24, 2015:

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

jtimon commented at 9:08 pm on July 24, 2015:

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

jtimon force-pushed on Jul 28, 2015

jtimon commented at 4:31 pm on July 28, 2015: contributor

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

jtimon commented at 4:45 pm on July 28, 2015: contributor

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 https://github.com/jtimon/bitcoin/commit/ddd505a4f62603d6b2ed07e0fecb213b81c8f795 can’t be a “next step” to this anymore, and https://github.com/jtimon/bitcoin/commit/a6d1e6e60d1757fc850f5798266bb2a4c7145193 (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 closed this on Jul 28, 2015

jtimon commented at 5:18 pm on July 28, 2015: contributor

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

jtimon reopened this on Jul 29, 2015

jtimon force-pushed on Jul 29, 2015

jtimon commented at 11:52 am on July 29, 2015: contributor

Rebased and reopened after #6077 has been reverted.

jtimon force-pushed on Aug 20, 2015

Optimizations: Consensus: In main::AcceptToMemoryPool, main::ConnectBlock, 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

1cce439120

jtimon force-pushed on Aug 20, 2015

jtimon commented at 11:08 pm on August 20, 2015: contributor

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

jtimon commented at 10:53 pm on September 6, 2015: contributor

Closing until one of #6526/#6625 gets merged and probably also until #6557 / https://github.com/TheBlueMatt/bitcoin/commit/88a3b8d52432f437a3d194748e446461f721cf9b gets merged as well.

jtimon closed this on Sep 6, 2015

jtimon referenced this in commit a61cec80c7 on Mar 16, 2016

laanwj referenced this in commit 0e3a411351 on Oct 11, 2017

codablock referenced this in commit 142ccecf6a on Sep 30, 2019

codablock referenced this in commit e48a86479b on Oct 2, 2019

codablock referenced this in commit 56d28c5fb3 on Oct 2, 2019

UdjinM6 referenced this in commit f230317796 on Oct 3, 2019

codablock referenced this in commit ee34d2081c on Oct 3, 2019

codablock referenced this in commit 8bcba5d4e1 on Oct 3, 2019

barrystyle referenced this in commit baabcd2c6e on Jan 22, 2020

DrahtBot locked this on Sep 8, 2021

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