Rewrite CreateNewBlock #6898

Merged
merged 6 commits into from Dec 1, 2015

Conversation

Projects
None yet
10 participants
@morcos
Member

morcos commented Oct 28, 2015

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 laanwj added the Mining label Oct 31, 2015

@luke-jr

View changes

src/txmempool.h
@@ -78,7 +80,8 @@ class CTxMemPoolEntry
public:
CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee,
- int64_t _nTime, double _dPriority, unsigned int _nHeight, bool poolHasNoInputsOf = false);
+ int64_t _nTime, double _dPriority, unsigned int _nHeight, bool poolHasNoInputsOf = false,
+ unsigned int nSigOps = 0);

This comment has been minimized.

@luke-jr

luke-jr Oct 31, 2015

Member

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

@luke-jr

luke-jr Oct 31, 2015

Member

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

This comment has been minimized.

@morcos

morcos Nov 1, 2015

Member

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

@morcos

morcos Nov 1, 2015

Member

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

This comment has been minimized.

@luke-jr

luke-jr Nov 1, 2015

Member

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

@luke-jr

luke-jr Nov 1, 2015

Member

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

This comment has been minimized.

@morcos

morcos Nov 3, 2015

Member

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.

@morcos

morcos Nov 3, 2015

Member

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.

@luke-jr

View changes

src/miner.cpp
- throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed");
+ if (!TestBlockValidity(state, *pblock, pindexPrev, false, false)) {
+ LogPrintf("ERROR: CNB TestBlockValidity failed: %s\n",FormatStateMessage(state));
+ return NULL;

This comment has been minimized.

@luke-jr

luke-jr Oct 31, 2015

Member

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

@luke-jr

luke-jr Oct 31, 2015

Member

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

This comment has been minimized.

@morcos

morcos Nov 3, 2015

Member

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.

@morcos

morcos Nov 3, 2015

Member

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.

This comment has been minimized.

@luke-jr

luke-jr Nov 3, 2015

Member

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

@luke-jr

luke-jr Nov 3, 2015

Member

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

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 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.

Member

morcos commented Nov 3, 2015

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 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.

Member

morcos commented Nov 12, 2015

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 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.

Member

morcos commented Nov 18, 2015

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 morcos changed the title from [WIP] Rewrite CreateNewBlock to Rewrite CreateNewBlock Nov 18, 2015

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 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.

Member

morcos commented Nov 18, 2015

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

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 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.

Member

morcos commented Nov 19, 2015

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.

@jtimon

View changes

src/miner.cpp
- if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false))
- throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed");
+ if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, false, false)) {
+ throw std::runtime_error("CreateNewBlock(): TestBlockValidity failed: " + FormatStateMessage(state));

This comment has been minimized.

@jtimon

jtimon Nov 23, 2015

Member

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

@jtimon

jtimon Nov 23, 2015

Member

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

@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 24, 2015

Member

Addressed @jtimon's nit.

Member

sdaftuar commented Nov 24, 2015

Addressed @jtimon's nit.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Nov 29, 2015

Member

Needs rebase.

Member

btcdrak commented Nov 29, 2015

Needs rebase.

@jameshilliard

This comment has been minimized.

Show comment
Hide comment
@jameshilliard

jameshilliard Nov 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.

Contributor

jameshilliard commented Nov 29, 2015

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

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Nov 30, 2015

Member

rebased

Member

morcos commented Nov 30, 2015

rebased

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Dec 1, 2015

Member

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

Edit: Hm. needs some not totally trivial rebasing.

Member

gmaxwell commented Dec 1, 2015

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

Edit: Hm. needs some not totally trivial rebasing.

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Dec 1, 2015

Contributor

ut ACK

Contributor

jgarzik commented Dec 1, 2015

ut ACK

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 1, 2015

Member

Needs rebase again...

Member

laanwj commented Dec 1, 2015

Needs rebase again...

@morcos

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Dec 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.

Member

morcos commented Dec 1, 2015

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.

morcos added some commits Oct 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Dec 1, 2015

Member

ACK

Member

sipa commented Dec 1, 2015

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

This comment has been minimized.

Show comment
Hide comment
@morcos

morcos Dec 1, 2015

Member

rebased

Member

morcos commented Dec 1, 2015

rebased

@sipa sipa merged commit 553cad9 into bitcoin:master Dec 1, 2015

1 check passed

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

sipa added a commit that referenced this pull request Dec 1, 2015

Merge pull request #6898
553cad9 Rewrite CreateNewBlock (Alex Morcos)
5f12263 Expose FormatStateMessage (Alex Morcos)
1f09287 Make accessing mempool parents and children public (Alex Morcos)
7230187 Add TxPriority class and comparator (Alex Morcos)
f3fe836 Add a score index to the mempool. (Alex Morcos)
c49d5bc Store the total sig op count of a tx. (Alex Morcos)
@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Dec 1, 2015

Member

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

Member

luke-jr commented Dec 1, 2015

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

luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Dec 1, 2015

Revert default block priority size to 50k
The priority area is often our only defense against spam, so disabling it should not be encouraged.
Additionally, the only benefit for disabling it is performance, which is no longer an issue with #6898.

While there may be arguably better policies for handling priority, these are not yet supported (nor even evaluated theoretically or tested in practice).
Until those are implemented and tested, we shouldn't regress default policy.
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 2, 2015

Member

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

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

Member

laanwj commented Dec 2, 2015

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

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

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