Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix possible block db breakage during re-index #5864

Merged
merged 1 commit into from Mar 11, 2015

Conversation

theuni
Copy link
Member

@theuni theuni commented Mar 7, 2015

I could really use some more eyes on this. I discussed it briefly with @sipa on IRC a few days ago.

I noticed this while looking into #5668 . This is one possible explanation I can come up with for overlapping block data. Whether it has anything to do with that issue or not, I think this still needs to be addressed.

When re-indexing, there are a few cases where garbage data may be skipped in the block files. In these cases, the indices are correctly written to the index db, however the pointer to the next position for writing in the current block file is calculated by adding the sizes of the valid blocks found.

As a result, when the re-index is finished, the index db is correct for all existing blocks, but the next block will be written to an incorrect offset, likely overwriting existing blocks.

Rather than using the sum of all valid blocks to determine the next write position, use the end of the last block written to the file. Don't assume that the current block is the last one in the file, since they may be read out-of-order.

I was able to trigger this problem by inserting some garbage data between two valid blocks on disk in the last .dat file, then reindexing. After that, run normally for a few min in order to write a few new blocks to disk, then run with -checkblocks=0.

Before this change, I would get different errors (de-serialization, eof, etc) depending on what garbage i add and where. After the change, it appears to survive the re-index without issue regardless of the garbage.

@sipa
Copy link
Member

sipa commented Mar 7, 2015

Seems very reasonable to me. Even if this doesn't fix anything, it should be safe.

@pstratem
Copy link
Contributor

pstratem commented Mar 9, 2015

I ran into this issue today.

Power failured during IBD.

Restarted with -reindex=1

Waited for IBD to finish and then called getblock RPC call for all blocks and got about a dozen ReadBlockFromDisk

vinfoBlockFile[nFile].AddBlock(nHeight, nTime);
if (fKnown && vinfoBlockFile[nFile].nSize < (pos.nPos + nAddSize))
vinfoBlockFile[nFile].nSize = pos.nPos + nAddSize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, although I'm not sure about the fKnown && vinfoBlockFile[nFile].nSize >= (pos.nPos + nAddSize) case. Would we want to increase nSize at all? Or is this better:

if (fKnown)
    vinfoBlockFile[nFile].nSize = std::max(pos.nPos + nAddSize, vinfoBlockFile[nFile].nSize);
else
    vinfoBlockFile[nFile].nSize += nAddSize;

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK this suggested change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When can I fetch latest source code, compile and test?

On Mon, Mar 9, 2015 at 1:01 PM, Jeff Garzik notifications@github.com
wrote:

In src/main.cpp
#5864 (comment):

 vinfoBlockFile[nFile].AddBlock(nHeight, nTime);
  • if (fKnown && vinfoBlockFile[nFile].nSize < (pos.nPos + nAddSize))
  •    vinfoBlockFile[nFile].nSize = pos.nPos + nAddSize;
    

+1


Reply to this email directly or view it on GitHub
https://github.com/bitcoin/bitcoin/pull/5864/files#r26029884.

Mvh
-fredrik-normann-

Sent from my Gmail Account

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@laanwj Yes, thanks for catching that.

@morcos
Copy link
Member

morcos commented Mar 9, 2015

I tested this in the this little RPC test, https://gist.github.com/morcos/ae506817284cd776d5b2.
Warning it does some raw file IO to munge your block files.
Tested and works with and without the suggested change, but fails on master.

@theuni
Copy link
Member Author

theuni commented Mar 9, 2015

Updated for @laanwj's suggestion.

@morcos Big thanks for that, much nicer than my manual testnet dd+hexedit hackery :)

When re-indexing, there are a few cases where garbage data may be skipped in
the block files. In these cases, the indices are correctly written to the index
db, however the pointer to the next position for writing in the current block
file is calculated by adding the sizes of the valid blocks found.

As a result, when the re-index is finished, the index db is correct for all
existing blocks, but the next block will be written to an incorrect offset,
likely overwriting existing blocks.

Rather than using the sum of all valid blocks to determine the next write
position, use the end of the last block written to the file. Don't assume that
the current block is the last one in the file, since they may be read
out-of-order.
@laanwj laanwj merged commit bb6acff into bitcoin:master Mar 11, 2015
laanwj added a commit that referenced this pull request Mar 11, 2015
bb6acff fix possible block db breakage during re-index (Cory Fields)
laanwj pushed a commit that referenced this pull request Mar 11, 2015
When re-indexing, there are a few cases where garbage data may be skipped in
the block files. In these cases, the indices are correctly written to the index
db, however the pointer to the next position for writing in the current block
file is calculated by adding the sizes of the valid blocks found.

As a result, when the re-index is finished, the index db is correct for all
existing blocks, but the next block will be written to an incorrect offset,
likely overwriting existing blocks.

Rather than using the sum of all valid blocks to determine the next write
position, use the end of the last block written to the file. Don't assume that
the current block is the last one in the file, since they may be read
out-of-order.

Rebased-From: bb6acff
Github-Pull: #5864
@laanwj
Copy link
Member

laanwj commented Mar 11, 2015

Cherry-picked to 0.10 as 002c8a2

reddink pushed a commit to reddcoin-project/reddcoin-3.10 that referenced this pull request May 27, 2020
When re-indexing, there are a few cases where garbage data may be skipped in
the block files. In these cases, the indices are correctly written to the index
db, however the pointer to the next position for writing in the current block
file is calculated by adding the sizes of the valid blocks found.

As a result, when the re-index is finished, the index db is correct for all
existing blocks, but the next block will be written to an incorrect offset,
likely overwriting existing blocks.

Rather than using the sum of all valid blocks to determine the next write
position, use the end of the last block written to the file. Don't assume that
the current block is the last one in the file, since they may be read
out-of-order.

Rebased-From: bb6acff
Github-Pull: bitcoin#5864
(cherry picked from commit 002c8a2)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants