search

sipa / bitcoin

Bitcoin integration/staging tree
http://www.bitcoin.org
MIT License
88 stars 21 forks source link

Fix RewindBlockIndex for pruning #85

Closed sdaftuar closed 8 years ago

sdaftuar commented 8 years ago

Turns out this is harder than I thought. This is not suitable for merging because there's a bug (see below), and after further thought I now wonder whether trying to fix all this is worth it.

I ran into two issues: first, after we were calling RewindBlockIndex, in init we'd then call VerifyDB, which for a pruning node would fail because we just rewound back to the point where we have no more data, so trying to read the next earlier block from disk would fail.

I tried to workaround this by moving the VerifyDB call to happen before we call RewindBlockIndex, but then realized that this doesn't work, because eg if you were using checklevel=4, then the blocks that need to be rewound wouldn't in general pass VerifyDB (since we didn't download those blocks with the likely-needed witness data).

Maybe one solution to this problem would be to somehow bypass calling VerifyDB in the particular case that you're a pruning node and we just rewound the chain? But that seems hacky. Any better ideas?

The second issue I ran into had to do with wallet rescans taking place after RewindBlockIndex. I figured there should be no problem, because the wallet ought to be ahead of the tip, which should be fine. However, it turns out that FindForkInGlobalIndex(chainActive, wallet-locator) can return a blockindex that is earlier than chainActive.Tip() even if the wallet locator's tip is a successor of our tip. So I added a quick "fix" to FindForkInGlobalIndex that would detect this situation. It's not clear to me if there might be side effects (eg performance side effects, or new behavior because the function now returns different values in some situations) worth worrying about.

Also, I discovered that the node is in a pretty brittle state if it's a pruning node that is rewound this way... If you were to, say, bring the node down after the chain has been rewound and before new blocks are downloaded from peers, then on restart the node will die in VerifyDB. So at this point, I am wondering if we shouldn't perhaps just put a big warning in the release notes that pruning nodes should try to upgrade before activation in order to avoid redownloading the blockchain, along with graceful detection so that if the user fails to do so, they get a nicer error message?

I can take a stab at that approach if you agree that it makes more sense.

sdaftuar commented 8 years ago

Ok, I think this works now -- first commit changes the behavior of -checkblocks to not stomp on pruning nodes. Hopefully the change to FindForkInGlobalIndex is okay...

maflcko commented 8 years ago

(Note: The last commit is https://github.com/bitcoin/bitcoin/pull/8076 )

sdaftuar commented 8 years ago

Rebased (and got rid of unintended code move in init.cpp that was in the prior version).

sipa commented 8 years ago

Rebase on top of merged #8076?

sdaftuar commented 8 years ago

I cherry picked that commit from #8076 and rebased on sipa/segwit-master.

sipa commented 8 years ago

@sdaftuar Woops, I missed this is against segwit-master and not against upstream!

sipa commented 8 years ago

Including in next batch.