validation: count blocks correctly for check level < 3 #13431

pull kallewoof wants to merge 1 commits into bitcoin:master from kallewoof:verifydb_pindexstate_lvl0-2 changing 1 files +7 −7

kallewoof commented at 7:16 AM on June 11, 2018: member

As noted in #13428 (comment) there is a bug where if check level < 3, the resulting count for blocks is wrong, because pindexState is never updated.

Post-commit ./bitcoin-cli verifychain 1 3:


2018-06-11T07:12:28Z Verifying last 3 blocks at level 1
2018-06-11T07:12:28Z [0%]...[33%]...[66%]...[99%]...[DONE].
2018-06-11T07:12:28Z No coin database inconsistencies in last 3 blocks (0 transactions)

Pre-commit ./bitcoin-cli verifychain 1 3:

2018-06-11T07:13:34Z Verifying last 3 blocks at level 1
2018-06-11T07:13:34Z [0%]...[33%]...[66%]...[99%]...[DONE].
2018-06-11T07:13:34Z No coin database inconsistencies in last 0 blocks (0 transactions)

fanquake added the label Validation on Jun 11, 2018
kallewoof force-pushed on Jun 11, 2018
Empact commented at 7:34 AM on June 11, 2018: member

I think you can move the assignment to the for loop increment statement, because pindex isn't assigned anywhere else within the loop, and it's irrelevant if the ShutdownRequested early return occurs. This assumes the coin check is irrelevant to the update.
kallewoof commented at 7:41 AM on June 11, 2018: member

~~What about (fPruneMode && !(pindex->nStatus & BLOCK_HAVE_DATA) case? It would be incorrectly set to the wrong value if this occurred. Also~~ would have to make pindex available outside the for loop.

Edit: First statement is not right, second is. I guess it's okay to make pindex available, though.

Edit: Done.
kallewoof force-pushed on Jun 11, 2018
Empact commented at 7:48 AM on June 11, 2018: member

Check out the shadowing on 4056.
kallewoof commented at 7:56 AM on June 11, 2018: member

Good catch. Shouldn't affect the outcome, but bad for readability. Fixing.
kallewoof force-pushed on Jun 11, 2018
in src/validation.cpp:4060 in c995014f49 outdated
```
4056 | +        pindexState = pindex;
4057 | +    }
4058 |      // check level 4: try reconnecting blocks
4059 |      if (nCheckLevel >= 4) {
4060 | -        CBlockIndex *pindex = pindexState;
4061 | +        pindex = pindexState;
```
MarcoFalke commented at 2:31 PM on June 13, 2018:

Since you moved pindex to the outer scope and made it visible here, they are already the same (both of them are set to pindex->pprev at the end of the previous for loop)

At this point pindexState is only used to calculate the number of blocks, so it could make sense to just remove it altogether and calculate the number of blocks through a index_lowest or num_blocks.
MarcoFalke commented at 2:33 PM on June 13, 2018: member

utACK c995014f492bc9e2d8273b10a19659f9dfc4b182. Feel free to ignore the style-nit. (Though, if you fix it, you might as well do a rebase on your other fix, which was already merged.)
kallewoof force-pushed on Jun 15, 2018
kallewoof force-pushed on Jun 15, 2018
kallewoof commented at 2:21 AM on June 15, 2018: member

@MarcoFalke Sorry for delay. Good point. I removed pindexState. I had to add a block counter as the check level >= 4 case would move pindex, but I think it makes sense.

Also rebased on master.
kallewoof force-pushed on Jun 15, 2018
kallewoof renamed this:
~~validation: update pindexState for check level < 3~~
validation: count blocks correctly for check level < 3
on Jun 15, 2018
validation: count blocks correctly for check level < 3 f618ebc4e4
kallewoof force-pushed on Jun 15, 2018
MarcoFalke commented at 1:33 PM on June 15, 2018: member

utACK f618ebc4e40f32e5aa6a8afe3de76a47f811c562
Empact commented at 9:44 PM on June 15, 2018: member

utACK f618ebc
sipa commented at 1:03 AM on June 30, 2018: member

utACK f618ebc4e40f32e5aa6a8afe3de76a47f811c562

fanquake commented at 8:52 AM on July 1, 2018: member

Quickly tested using src/bitcoind -checkblocks=3 -checklevel=1 master (10ffca7429326c6cc3c428d7a34eb4a195eb727e):

2018-07-01T08:40:22Z init message: Verifying blocks...
2018-07-01T08:40:22Z Verifying last 3 blocks at level 1
2018-07-01T08:40:22Z [0%]...[33%]...[66%]...[99%]...[DONE].
2018-07-01T08:40:22Z No coin database inconsistencies in last 0 blocks (0 transactions)
2018-07-01T08:40:22Z  block index            4211ms

f618ebc:

2018-07-01T08:46:50Z init message: Verifying blocks...
2018-07-01T08:46:50Z Verifying last 3 blocks at level 1
2018-07-01T08:46:50Z [0%]...[33%]...[66%]...[99%]...[DONE].
2018-07-01T08:46:50Z No coin database inconsistencies in last 3 blocks (0 transactions)
2018-07-01T08:46:50Z  block index            3865ms

src/bitcoind -checkblocks=3 -checklevel=4

2018-07-01T08:49:43Z init message: Verifying blocks...
2018-07-01T08:49:43Z Verifying last 3 blocks at level 4
2018-07-01T08:49:43Z [0%]...[16%]...[33%]...[50%]...[DONE].
2018-07-01T08:49:43Z No coin database inconsistencies in last 3 blocks (901 transactions)
2018-07-01T08:49:43Z  block index            3359ms

MarcoFalke merged this on Jul 1, 2018
MarcoFalke closed this on Jul 1, 2018
MarcoFalke referenced this in commit 954f4a9c7c on Jul 1, 2018
kallewoof deleted the branch on Jul 1, 2018
jasonbcox referenced this in commit b925970984 on Sep 27, 2019
jonspock referenced this in commit c77e011d9c on Dec 24, 2019
jonspock referenced this in commit acd6561a03 on Dec 24, 2019
jonspock referenced this in commit 207d21d5da on Dec 24, 2019
jonspock referenced this in commit 266d6d9816 on Dec 24, 2019
jonspock referenced this in commit a78d9dcdcb on Dec 24, 2019
jonspock referenced this in commit 3f4b54ae76 on Dec 26, 2019
PastaPastaPasta referenced this in commit ae293d581b on Jul 7, 2020
PastaPastaPasta referenced this in commit c32d781d75 on Jul 7, 2020
PastaPastaPasta referenced this in commit 55d74a6de2 on Jul 8, 2020
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#13428 validation: check the specified number of blocks (off-by-one)