Paths

Table of Contentst

Differential D5081

ConnectBlock: fix slow usage of AddCoins
ClosedPublic
Actions

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

Details

Reviewers

deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGING1e1a278bdee6: ConnectBlock: fix slow usage of AddCoins
rABC1e1a278bdee6: ConnectBlock: fix slow usage of AddCoins

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

Branch

connectblock_addcoins

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 9157
Build 16272: Default Diff Build & Tests
Build 16271: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jan 27 2020, 17:54

Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 27 2020, 17:54

Build Bitcoin-ABC / Diffs / Diff Testing started.

markblundeberg edited the summary of this revision. (Show Details)Jan 27 2020, 17:55

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9157: Diff 15818.Jan 27 2020, 18:01

markblundeberg planned changes to this revision.Jan 27 2020, 18:03

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.

update

Build Bitcoin-ABC / Diffs / Diff Testing started.

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9173: Diff 15843.Jan 28 2020, 14:45

markblundeberg added inline comments.Jan 28 2020, 16:05

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).

Build Bitcoin-ABC / Diffs (Resource Intensive) / Resource Intensive Diff Testing started.

markblundeberg added a reviewer: deadalnix.Jan 30 2020, 10:49

Build Bitcoin-ABC / Diffs (Resource Intensive) / Resource Intensive Diff Testing passed.

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

Closed by commit rABC1e1a278bdee6: ConnectBlock: fix slow usage of AddCoins (authored by Mark Lundeberg <markblundeberg@users.noreply.github.com>). · Explain WhyFeb 3 2020, 08:27

This revision was automatically updated to reflect the committed changes.

Mark Lundeberg <markblundeberg@users.noreply.github.com> added a commit: rABC1e1a278bdee6: ConnectBlock: fix slow usage of AddCoins.

Mark Lundeberg <markblundeberg@users.noreply.github.com> added a commit: rSTAGING1e1a278bdee6: ConnectBlock: fix slow usage of AddCoins.Apr 7 2020, 22:46