Rewrite CreateNewBlock

morcos commented at 8:58 pm on October 28, 2015: member

WARNING: This hasn’t been used for mainnet mining yet as far as I know.

NOTE: This includes a commit which changes the default block priority size to 0. This code has been optimized for not including transactions in a priority space. If block priority space > 0 then the code does not offer too much of a performance improvement over the existing code. (EDIT: now its fast regardless)

The mempool is explicitly assumed to be responsible for maintaining consistency of transactions with respect to not spending non-existent outputs, not double spending, script validity and coinbase maturity. Only finality of transactions is checked before assembling a block.

A final call to TestBlockValidity is still performed. If an invalid block has been constructed, then an error is thrown ~~logged and a NULL pointer is returned.~~ (EDIT: reverted to original behavior with more informative error)

This code produces the same blocks as the code it replaces with the following exceptions:

The fee rate sort tie breaker is hash instead of priority
The priority sort has a secondary tie breaker of hash
~~The priority block is not limited to transactons above AllowFree()~~ (EDIT: fixed)
~~The fee rate sorted part of the block is not limited to transactions above minRelayTxFee~~ (EDIT: fixed)
(EDIT: The blocks are the same if you keep scanning to the end to try to fill up the last remaining space, but this code stops after trying 50 times to fill the last 1000 bytes)

Comparing this to the original code over 2000 calls to CreateNewBlock over the last 2 days. (blockprioritysize=0, maxmempool=300, dbcache=1000, maxsigcachesize=1000000) Time in ms

	Time to assemble block	Additional time to perform final TestBlockValidity
master	2602	240
this pull	14	438

The extra slowness in TestBlockValidity is because the cache used to be warmed up in the assembly code.

laanwj added the label Mining on Oct 31, 2015

in src/txmempool.h: in 8f84ec4230 outdated

79@@ -78,7 +80,8 @@ class CTxMemPoolEntry
80 
81 public:
82     CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
83-                    int64_t _nTime, double _dPriority, unsigned int _nHeight, bool poolHasNoInputsOf = false);
84+                    int64_t _nTime, double _dPriority, unsigned int _nHeight, bool poolHasNoInputsOf = false,
85+                    unsigned int nSigOps = 0);

luke-jr commented at 6:30 pm on October 31, 2015:

NACK defaulting nSigOps to 0 when we depend on it being accurate.

morcos commented at 1:22 am on November 1, 2015:

ok i agree, it makes more sense to default it high.

luke-jr commented at 1:38 am on November 1, 2015:

It does not make sense to default it period. Note the exact sigop count is part of the GBT response.

morcos commented at 4:16 pm on November 3, 2015:

OK agreed. The default is just for the tests, I think for now it makes sense to remove all defaults from the constructor and just make the tests fill them in. Unfortunately there are 2 other pulls open which also add arguments to the constructor and require changing the tests. I’m going to hold off updating the tests in this pull until we’re a bit closer assuming those might be merged first.

in src/miner.cpp: in 8f84ec4230 outdated

295@@ -351,8 +296,10 @@ CBlockTemplate* CreateNewBlock(const CScript& scriptPubKeyIn)
296         pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]);
297 
298         CValidationState state;
299-        if (!TestBlockValidity(state, *pblock, pindexPrev, false, false))
300-            throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed");
301+        if (!TestBlockValidity(state, *pblock, pindexPrev, false, false)) {
302+            LogPrintf("ERROR: CNB TestBlockValidity failed: %s\n",FormatStateMessage(state));
303+            return NULL;

luke-jr commented at 6:30 pm on October 31, 2015:

Why change exception to returning NULL here? IMO this should be an independent PR.

morcos commented at 4:17 pm on November 3, 2015:

The idea was this is now more likely to happen, and it seemed to meet that the JSON error from GetBlockTemplate would still be sufficient notification, and it would be better to not bring the node down. However, I’m happy to leave the existing behavior if people think crashing the node is the right outcome.

luke-jr commented at 5:33 pm on November 3, 2015:

Exceptions here should not crash the node… it just gets returned as a nicer JSON-RPC error.

morcos force-pushed on Nov 3, 2015

morcos commented at 4:27 pm on November 3, 2015: member

OK, this has been rewritten to use CTxMemPoolEntry::GetPriority for the priority sort. This will make it very fast even when maintaining a priority space in the block. So the commit changing the default blockprioritysize has been removed.

Of course until #6357 is merged or some other decision is taken regarding priority, it will not sort by an accurate measure of priority.

morcos force-pushed on Nov 3, 2015

gmaxwell added this to the milestone 0.12.0 on Nov 5, 2015

morcos commented at 2:02 pm on November 12, 2015: member

I addressed @luke-jr ’s comment about changing the method of failure return. I still modified it to give some extra state info. That commit and other fixes will get squashed.

Among the remaining outstanding items is to fix the constructor for CTxMemPoolEntry to not have any default arguments. It would be nice to know a merge order since this will conflict with #6357 and #6915 and involves tedious updating of a bunch of tests.

morcos force-pushed on Nov 12, 2015

morcos force-pushed on Nov 15, 2015

morcos force-pushed on Nov 18, 2015

morcos commented at 4:53 am on November 18, 2015: member

I rebased this and squashed fixes for comments so far.

I rebased it on top of #7008, because I needed to pick some path forward on how priority would be addressed in this PR. The first 3 commits are therefore part of that pull and not this one. If we decide to merge #6357 as well, then it should be an easy change.

I will continue testing, but I don’t have any other changes planned right now.

morcos renamed this:
~~[WIP] Rewrite CreateNewBlock~~
Rewrite CreateNewBlock
on Nov 18, 2015

morcos force-pushed on Nov 18, 2015

morcos commented at 8:10 pm on November 18, 2015: member

Thanks @sdaftuar for finding a bug in the way I was doing PrioritiseTransaction. There should probably be RPC tests for those functions.

morcos force-pushed on Nov 18, 2015

morcos force-pushed on Nov 19, 2015

morcos commented at 6:33 pm on November 19, 2015: member

Apologies for all the churn. Squashed again and rebased off a slightly modified index @sdaftuar created for #7062. This introduces one more minor difference from the original code in that fee rates are compared as doubles now.

If I correct for these differences:

using a score index which compares fee rates not doubles
make the original code use the hash tie breakers
search up 5000 txs to fill the final 1000 bytes
implement #6357 to correctly compute priority

Then I generate the exact same blocks as the old code modulo a few rare cases where double imprecision in the different priority calculation causes tx reordering in the priority space.

morcos force-pushed on Nov 19, 2015

in src/miner.cpp: in b0fb264e84 outdated

286@@ -350,8 +287,9 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const CScript& s
287         pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]);
288 
289         CValidationState state;
290-        if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false))
291-            throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed");
292+        if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false)) {
293+            throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed: " + FormatStateMessage(state));

jtimon commented at 1:06 pm on November 23, 2015:

can you strprintf("%: TestBlockValidity failed: %", __func__, ... instead?

sdaftuar commented at 1:43 pm on November 24, 2015: member

Addressed @jtimon’s nit.

btcdrak commented at 8:20 am on November 29, 2015: contributor

Needs rebase.

jameshilliard commented at 10:00 am on November 29, 2015: contributor

TestBlockValidity should probably be done in a background thread so that it does not delay GBT since TestBlockValidity is just a sanity check.

morcos force-pushed on Nov 30, 2015

morcos commented at 1:27 pm on November 30, 2015: member

rebased

gmaxwell commented at 10:09 am on December 1, 2015: contributor

Concept ACK. Some tested-ack. Will test more now.

Edit: Hm. needs some not totally trivial rebasing.

jgarzik commented at 12:06 pm on December 1, 2015: contributor

ut ACK

laanwj commented at 12:34 pm on December 1, 2015: member

Needs rebase again…

morcos commented at 3:00 pm on December 1, 2015: member

that was an expected merge conflict, will rebase shortly @laanwj my warning about not having mined, was just a literal warning because i don’t have sufficient hashpower to mine on mainnet. I have created 1000s upon 1000s of block templates that all pass TestBlockValidity both in code running this pull live for the past 2 weeks (and prior iterations for weeks before that) and in historical simulation over 6 weeks of data.

Store the total sig op count of a tx.

Store sum of legacy and P2SH sig op counts.  This is calculated in AcceptToMemory pool and storing it saves redoing the expensive calculation in block template creation.

c49d5bc9e6

Add a score index to the mempool.

The score index is meant to represent the order of priority for being included in a block for miners.  Initially this is set to the transactions modified (by any feeDelta) fee rate.  Index improvements and unit tests by sdaftuar.

f3fe83673e

Add TxPriority class and comparator 7230187b1d

Make accessing mempool parents and children public 1f09287c66

Expose FormatStateMessage 5f12263302

sipa commented at 4:26 pm on December 1, 2015: member

ACK

Rewrite CreateNewBlock

Use the score index on the mempool to only add sorted txs in order.  Remove much of the validation while building the block, relying on mempool to be consistent and only contain txs that can be mined.
The mempool is assumed to be consistent as far as not containing txs which spend non-existent outputs or double spends, and scripts are valid.  Finality of txs is still checked (except not coinbase maturity, assumed in mempool).
Still TestBlockValidity in case mempool consistency breaks and return error state if an invalid block was created.
Unit tests are modified to realize that invalid blocks can now be constructed if the mempool breaks its consistency assumptions and also updated to have the right fees, since the cached value is now used for block construction.

Conflicts:
	src/miner.cpp

553cad94e2

morcos force-pushed on Dec 1, 2015

morcos commented at 5:10 pm on December 1, 2015: member

rebased

sipa merged this on Dec 1, 2015

sipa closed this on Dec 1, 2015

sipa referenced this in commit 4077ad20d0 on Dec 1, 2015

luke-jr commented at 8:18 pm on December 1, 2015: member

This was not ready for merging. It needs #6357 first.

luke-jr referenced this in commit e4d7271457 on Dec 1, 2015

laanwj commented at 9:26 am on December 2, 2015: member

This was not ready for merging. It needs #6357 first.

#7008, which took the place of #6537 was merged

MarkLTZ referenced this in commit 7425415619 on Apr 27, 2019

random-zebra referenced this in commit 98cf582557 on Jun 27, 2020

random-zebra referenced this in commit e59d8e59fa on Aug 18, 2020

Fuzzbawls referenced this in commit a88b6ea83f on Feb 3, 2021

zkbot referenced this in commit 12af999d42 on Aug 10, 2021

zkbot referenced this in commit 44f7d7e4ea on Aug 11, 2021

zkbot referenced this in commit 7fa7d5700b on Aug 12, 2021

zkbot referenced this in commit fd462fd8c4 on Aug 13, 2021

MarcoFalke locked this on Sep 8, 2021

Rewrite CreateNewBlock #6898