Skip BIP 30 verification where not necessary #6931
Conversation
|
Do you have a benchmark with both PRs together? utACK, aside from the testnet3 coinbase issue |
|
Concept ACK, but the logic seems more complicated than necessary. Couldn't you just remove the BIP30 checking code entirely now that BIP34 has activated? Why do the work in the old chain, when we know the old chain doesn't violate the rules (and have headers-first to prevent somebody from feeding us a low-work bogus chain)? |
|
@gavinandresen Headers-first doesn't actually prevent someone from doing the attack -- for example the single peer a node chooses to download headers from could feed a bogus chain before a node ever learns about the real chain. Concept ACK |
|
@instagibbs I'm working on putting together some benchmarks. There are a set of pulls that I've been working on to speed up ConnectBlock and CreateNewBlock. I'll post an issue identifying them all and with some benchmarks. But roughly speaking we're talking about these times on average over some recent data for CreateNewBlock (with 1GB dbcache and 1M sigcachesize)
* I actually didn't do the coinbase lookup required in 6932 for this test, so that can be the difference between 0 and 1 db lookup. @gavinandresen I'd be interested if you have any thoughts on how best to benchmark this as well? |
|
utACK (will test) |
|
@gavinandresen I wanted to do that first too when this brought up, see
#6916.
|
|
Some random stats (generated using #6965): 2015-11-06 23:22:03 UpdateTip: new best=00000000000000000e168ce1bbcdab4551cc35e9d5128b75e5d603143cdc970a height=357552 log2_work=82.822239 tx=69465120 date=2015-05-22 12:42:52 progress=0.799217 cache=1120.9MiB(601684tx) 2015-11-06 23:32:37 UpdateTip: new best=00000000000000000e168ce1bbcdab4551cc35e9d5128b75e5d603143cdc970a height=357552 log2_work=82.822239 tx=69465120 date=2015-05-22 12:42:52 progress=0.799210 cache=1120.9MiB(601684tx) |
|
In other words, of some random set of blocks (~356331-357552), this pull took the ConnectBlock time from 230s to 160s! |
|
Concept & code review ACK. |
|
utACK modulo duplicate testnet coinbases issue. |
|
Just confirmed that all coinbases from block #0 to block #21112 on testnet3 are unique. Also successfully did a sync from scratch on testnet3. ACK |
|
Untested ACK |
|
Also confirmed unique coinbases on tesnet3. |
Skip BIP 30 verification where not necessary bitcoin#6931
bitcoin#6931 BIP-0034 should've been active by the time crown started. PLEASE TEST. Former-commit-id: c765f36
Skip BIP 30 verification where not necessary bitcoin#6931 Former-commit-id: dbea57a
5b8b387 Fix overly eager BIP30 bypass (Alex Morcos) Pull request description: In #6931 we introduced a possible consensus breaking change by misunderstanding how completely BIP 34 obviated the need for BIP 30. Unfixed, this could break consensus after block height about 1.9M. Explained in code comment. h/t @sdaftuar Tree-SHA512: 8f798c3f203432fd4ae1c1c08bd6967b4a5ec2064ed5f6a7dcf3bff34ea830952838dd4ff70d70b5080cf4644f601e5526b60456c08f43789e4aae05621d9d6b
Summary: In D2076 the check=true parameter was passed, which doesn't just silently hide overwrites but also causes an extra HaveCoin lookup for each coin. This is a whole database lookup regardless of the caches, since it's always going to be a miss in normal operation. We're better off just catching the exception than doing the silent overwrite thing. As a quick benchmark, I ran reindex-chainstate on testnet without/with this change, and the runtime changed from 29 minutes to 20 minutes. And that's even with the coins db on an SSD and probably fully in the OS cache. I found the comment a bit confusing: - CheckBlock does not check for duplicate transactions except the special case of the merkle tree vuln. - BIP30 is nice but it's not being enforced on recent blocks (again, because HaveCoin misses are slow), only BIP34 is enforced. -- see bitcoin/bitcoin#6931 Test Plan: `ninja check-extended` (since CTOR is always enabled on regtest, and also regtest always enforces BIP30, it is unfortunately not possible to write a functional test for this.) Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D5081
c894e8f [Core] Remove BIP30 check (random-zebra) Pull request description: A bit of history ---- Two transactions can have the same txid if their parents are identical, since the txids of the parents are included in a transaction. Coinbases have no parents, so it used to be possible for two of them to be identical. Further, by building on duplicate coinbases, duplicate normal transactions were possible as well (http://r6.ca/blog/20120206T005236Z.html). In order to remove the possibility of having duplicate transaction ids, Bitcoin introduced, with BIP30, the following consensus rule: - Blocks are not allowed to contain a transaction whose identifier matches that of an earlier, not-fully-spent transaction in the same chain. [[1](https://github.com/bitcoin/bips/blob/master/bip-0030.mediawiki)] This rule was enforced by verifying (in `ConnectBlock`) that none of the transactions in the block was overwriting an already existent non-pruned CCoins entry. BIP34 was later added in Bitcoin to enforce better transaction uniqueness, with the update of block version to 2, which introduced the following consensus rule: - the first serialized element in the scriptSig of coinbase transactions must be the height of the chain. [[2](https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki)] After the complete activation of BIP34, there seemed to be no longer need for the check in `ConnectBlock`, added for BIP30, as duplicated coinbases were rendered impossible with BIP34 (bitcoin#6931). This assumption was later revisited, when several blocks were found on Bitcoin's main chain (before BIP34 activation), having coinbase scripts with an height higher than the current chain height (and higher than the BIP34 activation height). Thus, coinbases for blocks at these "future" heights would have given the opportunity for BIP30 to be violated even with BIP34 enforced (bitcoin#12204). Motivation for this PR ---- PIVX has BIP30 and BIP34 consensus rules already implemented since the chain start. The first block after the genesis (height=1) has version 3. The "block.nVersion=2 rule" is enforced in `ContextualCheckBlock` https://github.com/PIVX-Project/PIVX/blob/af3dd41f86684a5e4a18cfb85d471a199cc980da/src/main.cpp#L3605-L3612 However the code still has the (somewhat expensive) BIP30 check in `ConnectBlock`, which wasn't needed at all, given the full enforcement of BIP34 since the start of the chain. This PR removes it. *Side Note*: Even without BIP34, with Proof-of-Stake, coinbase transactions have empty scriptPubKey (thus unspendable outputs), therefore there would have been no need for BIP30 checks in any case (at least after PoS activation height). ACKs for top commit: furszy: Great find! ACK c894e8f. Fuzzbawls: ACK c894e8f Tree-SHA512: bc0dec1ef68c05db35ee0a132d0817c851667ce47ba5ec7eae31d29f1a44266f987db4462dc16d2320684bf76a6088c3fdbe77e08a9312ab6a1b7a58b6632661
9d97db0 Add unit test for UpdateCoins (random-zebra) 0965d3a Make CCoinsViewTest behave like CCoinsViewDB (Alex Morcos) 7d23d5c [Core] PIVX: remove db lookup for coinbase outputs (random-zebra) 1e88b7f ModifyNewCoins saves database lookups (Alex Morcos) Pull request description: Adapted version of bitcoin#6932 > When processing a new transaction, in addition to spending the Coins of its txin's, it creates a new Coins for its outputs. The existing ModifyCoins function will first make sure this Coins does not already exist. It can not exist due to BIP 30, but because of that, the lookup can't be cached and always has to go to the database. Since we are creating the Coins to match the new tx anyway, there is no point in checking if it exists first anyway. However this should not be used for coinbase tx's in order to preserve the historical behavior of overwriting the two existing duplicate tx pairs. > > In conjunction with bitcoin#6931 this will help ConnectBlock be much more efficient with caching access to the database. We remove the restriction on coinbases, and thus use always `ModifyNewCoins` instead of `ModifyCoins`, since duplicated coinbases are not possible on PIVX (ref. #1775). Accordingly, we remove the logic for checking the effects of duplicate coinbase txs in the unit test. ACKs for top commit: furszy: code looking good, tests and reindex went well too, ACK 9d97db0 Fuzzbawls: utACK 9d97db0 Tree-SHA512: e427f00699dfda29a9536b4e43919aecbfea90cf30195fe5a3835a49a715abf4f353a4672e6e99213b2016fb5b93f4ebfc94998a420ee8dc10626658069993af
A bit of history ---- Two transactions can have the same txid if their parents are identical, since the txids of the parents are included in a transaction. Coinbases have no parents, so it used to be possible for two of them to be identical. Further, by building on duplicate coinbases, duplicate normal transactions were possible as well (http://r6.ca/blog/20120206T005236Z.html). In order to remove the possibility of having duplicate transaction ids, Bitcoin introduced, with BIP30, the following consensus rule: - Blocks are not allowed to contain a transaction whose identifier matches that of an earlier, not-fully-spent transaction in the same chain. [[1](https://github.com/bitcoin/bips/blob/master/bip-0030.mediawiki)] This rule was enforced by verifying (in `ConnectBlock`) that none of the transactions in the block was overwriting an already existent non-pruned CCoins entry. BIP34 was later added in Bitcoin to enforce better transaction uniqueness, with the update of block version to 2, which introduced the following consensus rule: - the first serialized element in the scriptSig of coinbase transactions must be the height of the chain. [[2](https://github.com/bitcoin/bips/blob/master/bip-0034.mediawiki)] After the complete activation of BIP34, there seemed to be no longer need for the check in `ConnectBlock`, added for BIP30, as duplicated coinbases were rendered impossible with BIP34 (bitcoin/bitcoin#6931). This assumption was later revisited, when several blocks were found on Bitcoin's main chain (before BIP34 activation), having coinbase scripts with an height higher than the current chain height (and higher than the BIP34 activation height). Thus, coinbases for blocks at these "future" heights would have given the opportunity for BIP30 to be violated even with BIP34 enforced (bitcoin/bitcoin#12204). Squorum has BIP30 and BIP34 consensus rules already implemented since the chain start. The first block after the genesis (height=1) has version 4. However the code still has the (somewhat expensive) BIP30 check in `ConnectBlock`, which wasn't needed at all, given the full enforcement of BIP34 since the start of the chain. This commit removes it. *Side Note*: Even without BIP34, with Proof-of-Stake, coinbase transactions have empty scriptPubKey (thus unspendable outputs), therefore there would have been no need for BIP30 checks in any case (at least after PoS activation height).
5b8b387 Fix overly eager BIP30 bypass (Alex Morcos) Pull request description: In bitcoin#6931 we introduced a possible consensus breaking change by misunderstanding how completely BIP 34 obviated the need for BIP 30. Unfixed, this could break consensus after block height about 1.9M. Explained in code comment. h/t @sdaftuar Tree-SHA512: 8f798c3f203432fd4ae1c1c08bd6967b4a5ec2064ed5f6a7dcf3bff34ea830952838dd4ff70d70b5080cf4644f601e5526b60456c08f43789e4aae05621d9d6b
5b8b387 Fix overly eager BIP30 bypass (Alex Morcos) Pull request description: In bitcoin#6931 we introduced a possible consensus breaking change by misunderstanding how completely BIP 34 obviated the need for BIP 30. Unfixed, this could break consensus after block height about 1.9M. Explained in code comment. h/t @sdaftuar Tree-SHA512: 8f798c3f203432fd4ae1c1c08bd6967b4a5ec2064ed5f6a7dcf3bff34ea830952838dd4ff70d70b5080cf4644f601e5526b60456c08f43789e4aae05621d9d6b
5b8b387 Fix overly eager BIP30 bypass (Alex Morcos) Pull request description: In bitcoin#6931 we introduced a possible consensus breaking change by misunderstanding how completely BIP 34 obviated the need for BIP 30. Unfixed, this could break consensus after block height about 1.9M. Explained in code comment. h/t @sdaftuar Tree-SHA512: 8f798c3f203432fd4ae1c1c08bd6967b4a5ec2064ed5f6a7dcf3bff34ea830952838dd4ff70d70b5080cf4644f601e5526b60456c08f43789e4aae05621d9d6b
5b8b387 Fix overly eager BIP30 bypass (Alex Morcos) Pull request description: In bitcoin#6931 we introduced a possible consensus breaking change by misunderstanding how completely BIP 34 obviated the need for BIP 30. Unfixed, this could break consensus after block height about 1.9M. Explained in code comment. h/t @sdaftuar Tree-SHA512: 8f798c3f203432fd4ae1c1c08bd6967b4a5ec2064ed5f6a7dcf3bff34ea830952838dd4ff70d70b5080cf4644f601e5526b60456c08f43789e4aae05621d9d6b
5b8b387 Fix overly eager BIP30 bypass (Alex Morcos) Pull request description: In bitcoin#6931 we introduced a possible consensus breaking change by misunderstanding how completely BIP 34 obviated the need for BIP 30. Unfixed, this could break consensus after block height about 1.9M. Explained in code comment. h/t @sdaftuar Tree-SHA512: 8f798c3f203432fd4ae1c1c08bd6967b4a5ec2064ed5f6a7dcf3bff34ea830952838dd4ff70d70b5080cf4644f601e5526b60456c08f43789e4aae05621d9d6b
Since BIP 34 became active, it has been impossible to generate duplicate coinbases, and therefore not possible to generate any other duplicate transaction ID's.
This pull will allow us to skip BIP 30 verification if we know we are on a chain after the point BIP 34 was activated and there are no latent duplicate transaction chains.
On mainnet there were 2 cases of duplicate coinbases, and in both cases the duplicate overwrote the original before there was an opportunity to spend it.
I have not actually verified that there are no duplicate coinbases on testnet3, and we need to verify that or turn off the skipping on testnet before merging this pull.(EDIT: verified)In conjunction with #6932 this will help ConnectBlock be much more efficient with caching access to the database.