Change Details

Was introduced in D2076. Passing check=true to AddCoins mainly just causes an extra HaveCoin lookup for each coin, which is a whole database lookup since it's basically always going to be a miss. That's a slowdown that we probably don't want to be doing. The comment is also confusing me: - CheckBlock does not check for duplicate transactions except the special case of the merkle tree vuln. - If we overwrite a coin from a previous blocks, this has nothing to do with double spends. These are transaction outputs. - BIP30 is not being enforced on recent blocks (again, because HaveCoin misses are slow), only BIP34 is enforced. -- see https://github.com/bitcoin/bitcoin/pull/6931 (We do have an in-block duplicate txid check in ContextualCheckBlock, which applies for CTOR blocks if the node had been upgraded CTOR rules when it downloaded the block. However that doesn't check against prior blocks.) For the most part we (and Core) really primarily rely on BIP34, i.e., we rely on the improbability of hash collisions. That said, our current code does require a certain backport and perhaps a new consensus rule sometime in the next 25 years, no rush. :D https://github.com/bitcoin/bitcoin/pull/12204 Did a quick test by doing -reindex-chainstate on testnet with a 300 MB dbcache, it took 20 mins with this patch compared to 29 minutes before.

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 https://github.com/bitcoin/bitcoin/pull/6931

Was introduced in D2076.In D2076 the check=true parameter was passed, which doesn't just Passing check=true to AddCoins mainly justsilently hide overwrites but also causes an extra HaveCoin lookup lookup for each coin,. whichThis is a whole database lookup since it'sregardless of the basically always going to be a miss.caches, Thasince it's a slowdown that wealways going to be a miss in normal operation. probably don't want to be doingWe're better off just catching the exception than doing the silent overwrite thing. The comment is also confusing me:As a quick benchmark, I ran reindex-chainstate on testnet without/with - CheckBlock does not check for duplicate transactions except thethis change, and the runtime changed from 29 minutes to 20 minutes. special case ofAnd that's even with the coins db on an SSD and probably fully in the OS cache. I found the merkle tree vuln.comment a bit confusing: - If we overwrite a coin from a previous blocks, this has nothing to- CheckBlock does not check for duplicate transactions except the do with double spends. These are transaction outputsspecial case of the merkle tree vuln. - BIP30 is nice but it's not being enforced on recent blocks (again, because HaveCoin because HaveCoin misses are slow), only BIP34 is enforced. -- see https://github.com/bitcoin/bitcoin/pull/6931 (We do have an in-block duplicate txid check in ContextualCheckBlock, which applies for CTOR blocks if the node had been upgraded CTOR rules when it downloaded the block. However that doesn't check against prior blocks.) For the most part we (and Core) really primarily rely on BIP34, i.e., we rely on the improbability of hash collisions. That said, our current code does require a certain backport and perhaps a new consensus rule sometime in the next 25 years, no rush. :D https://github.com/bitcoin/bitcoin/pull/12204 Did a quick test by doing -reindex-chainstate on testnet with a 300 MB dbcache, it took 20 mins with this patch compared to 29 minutes before.