init: ignore BIP-30 verification in DisconnectBlock for problematic blocks

seejee commented at 3:01 am on April 14, 2022: contributor

When using checklevel=4, block verification fails because of duplicate coinbase transactions involving blocks 91812 and 91722. There was already a check in place within ConnectBlock to ignore the problematic blocks, but DisconnectBlock did not contain a similar check to ignore these blocks when called from VerifyDB.

By ignoring these two blocks in DisconnectBlock, the block verification process succeeds at checklevel=4.

(Note to reviewers: this is my first contribution to Bitcoin Core, so any feedback is most welcome. Thanks in advance for reviewing!)

Steps to reproduce:

Use the following bitcoin.conf file and start bitcoind. I only used block data through block ~100000 so that the verification process was much faster.

0assumevalid=0
1checkblocks=0
2checklevel=4

Without this change, you will see the following error when the blocks are verified:

02022-04-14T02:56:44Z init message: Verifying blocks…
12022-04-14T02:56:44Z Verifying last 101881 blocks at level 4
22022-04-14T02:56:44Z [0%]...[10%]...[20%]...[30%]...[40%]...ERROR: VerifyDB(): *** coin database inconsistencies found (last 10160 blocks, 142571 good transactions before that)
3
42022-04-14T02:57:01Z : Corrupted block database detected.
5Please restart with -reindex or -reindex-chainstate to recover.
6: Corrupted block database detected.
7Please restart with -reindex or -reindex-chainstate to recover.

With this change, you will see this instead:

02022-04-14T02:32:29Z init message: Verifying blocks…
12022-04-14T02:32:29Z Verifying last 101746 blocks at level 4
22022-04-14T02:32:29Z [0%]...[10%]...[20%]...[30%]...[40%]...[50%]...[60%]...[70%]...[80%]...[90%]...[DONE].
32022-04-14T02:32:48Z No coin database inconsistencies in last 101746 blocks (226126 transactions)

Ignore problematic blocks in DisconnectBlock

When using checklevel=4, block verification fails because of duplicate coinbase transactions
involving blocks 91812 and 91722. There was already a check in place for ConnectBlock to
ignore the problematic blocks, but DisconnectBlock did not contain a similar check.

This change ignores the blocks where these inconsistencies surface so
that block verification will succeed at checklevel=4.

511eb7fdea

seejee force-pushed on Apr 14, 2022

in src/validation.cpp:1845 in 511eb7fdea outdated

1836@@ -1837,6 +1837,15 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
1837         return DISCONNECT_FAILED;
1838     }
1839 
1840+    // Ignore blocks that contain transactions which are 'overwritten' by later transactions,
1841+    // unless those are already completely spent.
1842+    // See https://github.com/bitcoin/bitcoin/issues/22596 for additional information.
1843+    // Note: the blocks specified here are different than the ones used in ConnectBlock because DisconnectBlock
1844+    // unwinds the blocks in reverse. As a result, the inconsistency is not discovered until the earlier
1845+    // blocks with the duplicate coinbase transactions are disconnected.

seejee commented at 3:06 am on April 14, 2022:

I’m still pretty new to Bitcoin terminology, so if there is a more precise way to describe the issue in this comment, I welcome the feedback.

in src/validation.cpp:1862 in 511eb7fdea outdated

1858@@ -1850,7 +1859,7 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
1859                 COutPoint out(hash, o);
1860                 Coin coin;
1861                 bool is_spent = view.SpendCoin(out, &coin);
1862-                if (!is_spent || tx.vout[o] != coin.out || pindex->nHeight != coin.nHeight || is_coinbase != coin.fCoinBase) {
1863+                if (fEnforceBIP30 && (!is_spent || tx.vout[o] != coin.out || pindex->nHeight != coin.nHeight || is_coinbase != coin.fCoinBase)) {

seejee commented at 3:11 am on April 14, 2022:

At first I tried to mimic the behavior in ConnectBlock and the put the fEnforceBIP30 guard around the entire loop that begins on line 1850, but that would still result in the verification failing later in the process. Moving the guard here allowed the verification process to complete successfully.

I’m assuming that we need to call SpendCoin in both cases because we still need to update the view’s state even if we ignore the is_spent result.

seejee renamed this:
~~consensus: ignore BIP-30 verification in DisconnectBlock for problematic blocks~~
init: ignore BIP-30 verification in DisconnectBlock for problematic blocks
on Apr 14, 2022

seejee marked this as ready for review on Apr 14, 2022

DrahtBot added the label Validation on Apr 14, 2022

jamesob commented at 3:13 pm on April 14, 2022: member

Concept ACK

Thanks for the contribution and welcome to the project. I’ve tested the patch locally and it works as advertised; I also perturbed each height value to ensure that the heights listed are those necessary to fix the VerifyDB issue.

My one comment is that since BIP30 violations are limited to coinbase transactions, it might be nice to restrict the exceptions accordingly, e.g. with a patch like this

 0diff --git a/src/validation.cpp b/src/validation.cpp
 1index 6d87ab88b3..ffad7e8402 100644
 2--- a/src/validation.cpp
 3+++ b/src/validation.cpp
 4@@ -1851,6 +1851,7 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
 5         const CTransaction &tx = *(block.vtx[i]);
 6         uint256 hash = tx.GetHash();
 7         bool is_coinbase = tx.IsCoinBase();
 8+        bool is_bip30_exception = (is_coinbase && !fEnforceBIP30);
 9 
10         // Check that all outputs are available and match the outputs in the block itself
11         // exactly.
12@@ -1859,8 +1860,10 @@ DisconnectResult CChainState::DisconnectBlock(const CBlock& block, const CBlockI
13                 COutPoint out(hash, o);
14                 Coin coin;
15                 bool is_spent = view.SpendCoin(out, &coin);
16-                if (fEnforceBIP30 && (!is_spent || tx.vout[o] != coin.out || pindex->nHeight != coin.nHeight || is_coinbase != coin.fCoinBase)) {
17-                    fClean = false; // transaction output mismatch
18+                if (!is_spent || tx.vout[o] != coin.out || pindex->nHeight != coin.nHeight || is_coinbase != coin.fCoinBase) {
19+                    if (!is_bip30_exception) {
20+                        fClean = false; // transaction output mismatch
21+                    }
22                 }
23             }
24         }

seejee commented at 3:52 pm on April 14, 2022: contributor

@jamesob Thanks for the review and good idea to ensure that we’re only allowing exceptions for coinbase transactions. I’ve included your suggested patch in the latest commit and retested to ensure things are working as expected. 👍

Thanks again!

jamesob commented at 4:02 pm on April 14, 2022: member

@seejee great. FWIW, looks like there’s a typo in your commit message (“trxs”); you can fix with git commit --amend. You can also add co-author credits by appending

0Co-authored-by: James O'Beirne <james.obeirne@pm.me>

if you’d like.

init: limit bip30 exceptions to coinbase txs

Co-authored-by: James O'Beirne <james.obeirne@pm.me>

e899d4ca6f

seejee force-pushed on Apr 14, 2022

seejee commented at 4:05 pm on April 14, 2022: contributor

@jamesob Fixed the typo, and I included your co-author credits. Sorry for not doing that in the first place! 🙇

jamesob approved

jamesob commented at 4:23 pm on April 14, 2022: member

(Biased) ACK e899d4ca6f44785b6e5481e200a6a9a8d2448612 (jamesob/ackr/24851.2.seejee.init_ignore_bip_30_verif)

Well documented and symmetric with the equivalent ConnectBlock conditionals; even better in terms of specifically acting on coinbase transactions. Tested locally per the PR description.

 0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK e899d4ca6f44785b6e5481e200a6a9a8d2448612 ([`jamesob/ackr/24851.2.seejee.init_ignore_bip_30_verif`](https://github.com/jamesob/bitcoin/tree/ackr/24851.2.seejee.init_ignore_bip_30_verif))
 4
 5Well documented and symmetric with the equivalent `ConnectBlock` conditionals; 
 6even better in terms of specifically acting on coinbase transactions. Tested
 7locally per the PR description.
 8
 9-----BEGIN PGP SIGNATURE-----
10
11iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmJYSjwACgkQepNdrbLE
12TwWLXg/8CvE6Lr2u2y6fuWoXCnTNyfh/sTKlxyE7VMXiCw9eV2RvmoIaGCPHNHZP
135onsUv0J8WQ+KJVdUZUJpVaW0Gg/dThjDeUEjqTaniYqy8JgUEcjy5fTayI6Cpkt
14YDY1M5oAX80Tq7zaHLxMonLUmqy/moYBJw8TmmAMyVoFPJh2UcSie7P5TLVsmPLe
15XQyFc0QjvBNr2ois/uLswN7rArtPPldakCnXrBeDWkDIMN7gcPmrNJXbeKl4vEYk
16dwtAvuacpGimm7YVNjIY5vnamL2KAp+0d+pw8g8IWX1AJmH7noMeJ09ZmUqpeooo
17vF8l6+Q5mWwKs05WRYpG3dmU+Lfhpn+q48tN0n2GFxYNtKBF1XVRUEvse3Vdo0JM
18GVr4ISVLYhKLU+HzVH1iNRQ+lZ5GgOutvZfgjzxrKVI71kv6XRcu3+8nFmuSfJNx
19wvDhXSNjlMCX0qbBi+UOuKYLscuzEN+dnltnF1UH/u3AyV/EDwcvbF6iwhKunuNU
20uo4Qf1g8ipaGVVVs2UavruE3AyAQx8ajmmGb64NsirqMuIVxbdPNMZiUHPeIj5S+
21z4z0GXpyII2p8lJlI4+lHXpH+EX/Cypo7XGqOzJItWYEfmn4BSVK8ieC5I8LcdSC
22VSBup0RTpyHIWg1gu1ZCy0lQKoEpu8tl8OA0WAEP/hcXRjdQiMc=
23=w1jg
24-----END PGP SIGNATURE-----

0Tested on Linux-5.10.0-11-amd64-x86_64-with-glibc2.31
1
2Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default
3
4Compiled with /usr/bin/ccache /usr/local/bin/clang++ -std=c++17 -mavx -mavx2 -mpclmul -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_top_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -fstack-clash-protection -msse4.1 -msse4.2 -msse4 -msha  i
5
6Compiler version: Debian clang version 13.0.1-++20220126092033+75e33f71c2da-1~exp1~20220126212112.63

in src/validation.cpp:1846 in e899d4ca6f

1841+    // unless those are already completely spent.
1842+    // See https://github.com/bitcoin/bitcoin/issues/22596 for additional information.
1843+    // Note: the blocks specified here are different than the ones used in ConnectBlock because DisconnectBlock
1844+    // unwinds the blocks in reverse. As a result, the inconsistency is not discovered until the earlier
1845+    // blocks with the duplicate coinbase transactions are disconnected.
1846+    bool fEnforceBIP30 = !((pindex->nHeight==91722 && pindex->GetBlockHash() == uint256S("0x00000000000271a2dc26e7667f8419f2e15416dc6955e5a6c6cdf3f2574dd08e")) ||

laanwj commented at 5:26 am on June 22, 2022:

Maybe it’d make sense to factor out this “EnforceBIP30(pindex)” logic into a function? I know, it will never change so it doesn’t hurt either to hardcode it in duplicate, but I’d like the symmetry of it better.

Edit: oh, it isn’t duplicated? The only other match I see is in src/index/coinstatsindex.cpp. Never mind, I was confused and didn’t read the comment well enough.

laanwj commented at 5:33 am on June 22, 2022: member

Code review ACK e899d4ca6f44785b6e5481e200a6a9a8d2448612

It would be real nice to have a functional test for the disconnection behavior, though not necessarily in this PR.

0xB10C commented at 8:54 am on August 13, 2022: contributor

Successfully tested with verifychain 4 0 on mainnet. Had to change source https://github.com/0xB10C/bitcoin/commit/eb0d5f078f03a5190d458ab4cc2d2e859e2492da to allow for a dbcache larger than 16GB. Ran it with 48GB and from what I can tell it used just under 32 GB of RAM. Took 14h.

DrahtBot commented at 12:37 pm on September 23, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

achow101 commented at 6:11 pm on October 13, 2022: member

ACK e899d4ca6f44785b6e5481e200a6a9a8d2448612

achow101 merged this on Oct 13, 2022

achow101 closed this on Oct 13, 2022

bitcoin locked this on Oct 13, 2023

init: ignore BIP-30 verification in DisconnectBlock for problematic blocks #24851

Steps to reproduce:

Conflicts