Page MenuHomePhabricator

ConnectBlock: fix slow usage of AddCoins
ClosedPublic

Authored by markblundeberg on Jan 27 2020, 17:54.

Details

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

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Ah OK, nevermind, as soon as I posted this I realized what's going on ... I get it now. :)

Regtest never activates BIP34 and so BIP30 is always active. So, it's hard to test this at all in the functional tests.

It seems we might want to make AddCoins just take overwrite as a parameter and not bother with HaveCoin. The comment could read something like this:
"Since BIP30 is not usually enforced, we have not yet excluded the possibility that this block contains duplicate transactions from same or previous blocks. We unconditionally overwrite coins here, with the understanding that duplicate coinbase transactions will have been already excluded via BIP34, and duplicate normal transactions will trigger a double-spend error in the following loop."
Note that unconditional overwrite is also a (smaller) performance penalty, since it doesn't mark coins "FRESH" and therefore we get much more db flushes (every utxo now bloats the cache, even if spent in same/next block).

The (slight) danger of using AddCoins with check=false is that a block with a duplicate transaction will cause an exception if it gets to this point. I don't think this can really happen in practice: currently we have CheckBlock (ensures first tx is coinbase and nothing else is coinbase) and new blocks are checked with CTOR (excludes duplicate transactions among non-coinbases), and we have checkpoints to ensure alternative pre-CTOR blocks are not considered.

This all is impossible to test in the functional tests, because regtest has bip30 always active and so it will always catch duplicates first.

Anyway, the performance impact of HaveCoin is quite real.

markblundeberg retitled this revision from ConnectBlock: fix slow usage of AddCoins and fix comment to ConnectBlock: fix slow usage of AddCoins.
markblundeberg edited the summary of this revision. (Show Details)
markblundeberg edited the test plan for this revision. (Show Details)

update

src/validation.cpp
1910 ↗(On Diff #15843)

Not strictly related and I'm happy to remove, but I was trying to convince myself why this is all correct and so it's partly a note to remind myself what is going on.

An alternative possibility is to keep the current performance hit of using HaveCoin, and put it to work: reintroduce BIP30 checking for all new blocks. Then we aren't relying on our confidence in BIP34 to save us from bad times (which we maybe shouldn't be so confident about -- I wonder if there might be yet something else that every has overlooked).

After more digging, it looks like this is what's core is doing. Let's go for it.

This revision is now accepted and ready to land.Feb 2 2020, 22:26