incorrect blk file size calculation during reindex results in recoverable blk file corruption #24858

pull mruddy wants to merge 1 commits into bitcoin:master from mruddy:issue_21379 changing 5 files +68 −5
  1. mruddy commented at 11:40 pm on April 14, 2022: contributor

    Fixes #21379.

    The blocks/blk?????.dat files are mutated and become increasingly malformed, or corrupt, as a result of running the re-indexing process. The mutations occur after the re-indexing process has finished, as new blocks are appended, but are a result of a re-indexing process miscalculation that lingers in the block manager’s m_blockfile_info nSize data until node restart. These additions to the blk files are non-fatal, but also not desirable. That is, this is a form of data corruption that the reading code is lenient enough to process (it skips the extra bytes), but it adds some scary looking log messages as it encounters them.

    The summary of the problem is that the re-index process double counts the size of the serialization header (magic message start bytes [4 bytes] + length [4 bytes] = 8 bytes) while calculating the blk data file size (both values already account for the serialization header’s size, hence why it is over accounted).

    This bug manifests itself in a few different ways, after re-indexing, when a new block from a peer is processed:

    1. If the new block will not fit into the last blk file processed while re-indexing, while remaining under the 128MiB limit, then the blk file is flushed to disk and truncated to a size that is 8 greater than it should be. The truncation adds zero bytes (see FlatFileSeq::Flush and TruncateFile).
    2. If the last blk file processed while re-indexing has logical space for the new block under the 128 MiB limit:
      1. If the blk file was not already large enough to hold the new block, then the zeros are, in effect, added by fseek when the file is opened for writing. Eight zero bytes are added to the end of the last blk file just before the new block is written. This happens because the write offset is 8 too great due to the miscalculation. The result is 8 zero bytes between the end of the last block and the beginning of the next block’s magic + length + block.
      2. If the blk file was already large enough to hold the new block, then the current existing file contents remain in the 8 byte gap between the end of the last block and the beginning of the next block’s magic + length + block. Commonly, when this occcurs, it is due to the blk file containing blocks that are not connected to the block tree during reindex and are thus left behind by the reindex process and later overwritten when new blocks are added. The orphaned blocks can be valid blocks, but due to the nature of concurrent block download, the parent may not have been retrieved and written by the time the node was previously shutdown.
  2. mruddy force-pushed on Apr 15, 2022
  3. DrahtBot added the label Block storage on Apr 15, 2022
  4. DrahtBot added the label Build system on Apr 15, 2022
  5. DrahtBot added the label Validation on Apr 15, 2022
  6. mruddy commented at 2:16 am on April 15, 2022: contributor

    An easy way to reproduce this issue is to:

    1. setup a new temp node data directory containing only the blocks directory that contains only one 516 byte long file, blk00000.dat

      1. the blk00000.dat file contains
        1. the 8 byte serialization header for the genesis block
        2. the genesis block (blockhash 000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f)
        3. the 8 byte serialization header for block at height 2
        4. the block at height 2 (blockhash 000000006a625f06636b8bb6ac7b960a8d03705d1ace08b1a19da3fdcc99ddbd)

      in other words:

       0$ cd /tmp/btc && find .
       1.
       2./blocks
       3./blocks/blk00000.dat
       4$ sha256sum blocks/blk00000.dat
       5713be71e35a2bc25481cc66b465da1e439df1c2570d065fa0aecc11eaecbea94  blocks/blk00000.dat
       6$ xxd -g 4 blocks/blk00000.dat
       700000000: f9beb4d9 1d010000 01000000 00000000  ................
       800000010: 00000000 00000000 00000000 00000000  ................
       900000020: 00000000 00000000 00000000 3ba3edfd  ............;...
      1000000030: 7a7b12b2 7ac72c3e 67768f61 7fc81bc3  z{..z.,>gv.a....
      1100000040: 888a5132 3a9fb8aa 4b1e5e4a 29ab5f49  ..Q2:...K.^J)._I
      1200000050: ffff001d 1dac2b7c 01010000 00010000  ......+|........
      1300000060: 00000000 00000000 00000000 00000000  ................
      1400000070: 00000000 00000000 00000000 0000ffff  ................
      1500000080: ffff4d04 ffff001d 01044554 68652054  ..M.......EThe T
      1600000090: 696d6573 2030332f 4a616e2f 32303039  imes 03/Jan/2009
      17000000a0: 20436861 6e63656c 6c6f7220 6f6e2062   Chancellor on b
      18000000b0: 72696e6b 206f6620 7365636f 6e642062  rink of second b
      19000000c0: 61696c6f 75742066 6f722062 616e6b73  ailout for banks
      20000000d0: ffffffff 0100f205 2a010000 00434104  ........*....CA.
      21000000e0: 678afdb0 fe554827 1967f1a6 7130b710  g....UH'.g..q0..
      22000000f0: 5cd6a828 e03909a6 7962e0ea 1f61deb6  \..(.9..yb...a..
      2300000100: 49f6bc3f 4cef38c4 f35504e5 1ec112de  I..?L.8..U......
      2400000110: 5c384df7 ba0b8d57 8a4c702b 6bf11d5f  \8M....W.Lp+k.._
      2500000120: ac000000 00f9beb4 d9d70000 00010000  ................
      2600000130: 004860eb 18bf1b16 20e37e94 90fc8a42  .H`..... .~....B
      2700000140: 7514416f d75159ab 86688e9a 83000000  u.Ao.QY..h......
      2800000150: 00d5fdcc 541e25de 1c7a5add edf24858  ....T.%..zZ...HX
      2900000160: b8bb665c 9f36ef74 4ee42c31 6022c90f  ..f\.6.tN.,1`"..
      3000000170: 9bb0bc66 49ffff00 1d08d2bd 61010100  ...fI.......a...
      3100000180: 00000100 00000000 00000000 00000000  ................
      3200000190: 00000000 00000000 00000000 00000000  ................
      33000001a0: 000000ff ffffff07 04ffff00 1d010bff  ................
      34000001b0: ffffff01 00f2052a 01000000 43410472  .......*....CA.r
      35000001c0: 11a824f5 5b505228 e4c3d519 4c1fcfaa  ..$.[PR(....L...
      36000001d0: 15a456ab df37f9b9 d97a4040 afc073de  ..V..7...z@@..s.
      37000001e0: e6c89064 984f0338 5237d921 67c13e23  ...d.O.8R7.!g.>#
      38000001f0: 6446b417 ab79a0fc ae412ae3 316b77ac  dF...y...A*.1kw.
      3900000200: 00000000                             ....
      
    2. start the node with the command line similar to: bitcoin-qt -debug -datadir=/tmp/btc -reindex

    3. wait for header synchronization to finish and you see this in the debug log: UpdatedBlockTip: new block hash=00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048

    4. shutdown the node

    We setup like this in order to hit the case where an “Out of order block” is found during reindexing and it is left disconnected since it’s ancestor block at height 1 (blockhash 00000000839a8e6886ab5951d76f411475428afc90947ee320161bbf18eb6048) is not present in the blk00000.dat file for the reindexing process to attach.

    Now, if we examine the blk00000.dat file, we can see that the corruption has occured at offset 0x125 through 0x12c. The serialization header and data for block at height 1 follow at offset 0x12d:

     0$ xxd -g 4 -l 524 blocks/blk00000.dat
     100000000: f9beb4d9 1d010000 01000000 00000000  ................
     200000010: 00000000 00000000 00000000 00000000  ................
     300000020: 00000000 00000000 00000000 3ba3edfd  ............;...
     400000030: 7a7b12b2 7ac72c3e 67768f61 7fc81bc3  z{..z.,>gv.a....
     500000040: 888a5132 3a9fb8aa 4b1e5e4a 29ab5f49  ..Q2:...K.^J)._I
     600000050: ffff001d 1dac2b7c 01010000 00010000  ......+|........
     700000060: 00000000 00000000 00000000 00000000  ................
     800000070: 00000000 00000000 00000000 0000ffff  ................
     900000080: ffff4d04 ffff001d 01044554 68652054  ..M.......EThe T
    1000000090: 696d6573 2030332f 4a616e2f 32303039  imes 03/Jan/2009
    11000000a0: 20436861 6e63656c 6c6f7220 6f6e2062   Chancellor on b
    12000000b0: 72696e6b 206f6620 7365636f 6e642062  rink of second b
    13000000c0: 61696c6f 75742066 6f722062 616e6b73  ailout for banks
    14000000d0: ffffffff 0100f205 2a010000 00434104  ........*....CA.
    15000000e0: 678afdb0 fe554827 1967f1a6 7130b710  g....UH'.g..q0..
    16000000f0: 5cd6a828 e03909a6 7962e0ea 1f61deb6  \..(.9..yb...a..
    1700000100: 49f6bc3f 4cef38c4 f35504e5 1ec112de  I..?L.8..U......
    1800000110: 5c384df7 ba0b8d57 8a4c702b 6bf11d5f  \8M....W.Lp+k.._
    1900000120: ac000000 00f9beb4 d9d70000 00f9beb4  ................      <----- offset 0x125 through 0x12c demonstrate the incorrect offset calculated by the reindexing process. those bytes are from the original data that was already in the file and were erroneously not overwritten due to the bad offset.
    2000000130: d9d70000 00010000 006fe28c 0ab6f1b3  .........o......
    2100000140: 72c1a6a2 46ae63f7 4f931e83 65e15a08  r...F.c.O...e.Z.
    2200000150: 9c68d619 00000000 00982051 fd1e4ba7  .h........ Q..K.
    2300000160: 44bbbe68 0e1fee14 677ba1a3 c3540bf7  D..h....g{...T..
    2400000170: b1cdb606 e857233e 0e61bc66 49ffff00  .....W#>.a.fI...
    2500000180: 1d01e362 99010100 00000100 00000000  ...b............
    2600000190: 00000000 00000000 00000000 00000000  ................
    27000001a0: 00000000 00000000 000000ff ffffff07  ................
    28000001b0: 04ffff00 1d0104ff ffffff01 00f2052a  ...............*
    29000001c0: 01000000 43410496 b538e853 519c726a  ....CA...8.SQ.rj
    30000001d0: 2c91e61e c11600ae 1390813a 627c66fb  ,..........:b|f.
    31000001e0: 8be7947b e63c52da 75893795 15d4e0a6  ...{.<R.u.7.....
    32000001f0: 04f81417 81e62294 721166bf 621e73a8  ......".r.f.b.s.
    3300000200: 2cbf2342 c858eeac 00000000           ,.#B.X......
    
    1. If we now restart the node with the same command line as before, bitcoin-qt -debug -datadir=/tmp/btc -reindex we will soon see a message in the debug.log like: LoadExternalBlockFile: Deserialize or I/O error - ReadCompactSize(): size too large: iostream error
  7. mruddy commented at 9:39 am on April 15, 2022: contributor

    Attached here is the blk00000.dat file with sha256sum of 713be71e35a2bc25481cc66b465da1e439df1c2570d065fa0aecc11eaecbea94 used by my easy reproduction comment above #24858 (comment).

    Simply remove the “.txt” file extension that I had to add in order to upload it here.

    blk00000.dat.txt

  8. vincenzopalazzo commented at 9:31 pm on April 15, 2022: none

    Concept ACK, sounds good to me, and it is also well documented.

    I will test it in locally with your use case described

  9. mruddy commented at 12:54 pm on April 18, 2022: contributor
    noting for historical purposes that this updates a fix made by #5864 @theuni
  10. jamesob commented at 3:26 pm on April 18, 2022: member
    Concept ACK, will review this week.
  11. in src/node/blockstorage.cpp:619 in 2da96174b9 outdated
    615+        // pos.nPos points at the offset of the block data in the blk file. that already accounts for
    616+        // the serialization header present in the file (the 4 magic message start bytes + the 4 length bytes = 8 bytes).
    617+        // nAddSize already has 8 added to it too. we subtract 8 to avoid over accounting for the serialization
    618+        // header. such over accounting was a long standing bug that added undesirable 8 byte gaps into blk data files following
    619+        // a -reindex operation. for more info, see https://github.com/bitcoin/bitcoin/issues/21379
    620+        m_blockfile_info[nFile].nSize = std::max(pos.nPos + nAddSize - 8, m_blockfile_info[nFile].nSize);
    


    mzumsande commented at 10:54 pm on April 18, 2022:

    I think an alternative fix would be to have two distinct calls to FindBlockPos in BlockManager::SaveBlockToDisk conditional on whether dbp == nullptr (not adding 8 to nAddSize parameter if that is the case). Maybe that would be a little bit clearer than adding and substracting 8.

    In general, such a high percentage of code in FindBlockPos() is conditional on fKnown or !fKnown that I wonder whether it would make sense to refactor this into two separate functions (not here, but as a possible follow-up).


    mruddy commented at 11:57 pm on April 18, 2022:

    Thanks for the review!

    I resonate with what you said about not subtracting 8 within BlockManager::FindBlockPos for code clarity. Subtracting 8 was just the first simplest fix that I thought of.

    Building off of your idea, instead of making two different calls, we could get the same effect by doing something like the following in BlockManager::SaveBlockToDisk:

    0    const auto position_known = dbp != nullptr;
    1    const auto additional_size = nBlockSize + (position_known ? 0 : 8);
    2    if (!FindBlockPos(blockPos, additional_size, nHeight, active_chain, block.GetBlockTime(), position_known)) {
    

    mruddy commented at 1:58 am on April 19, 2022:
    made updates for this in 66b8d3c23d999651c79ac03e7558d4248a200ba1
  12. in src/test/blockmanager_tests.cpp:1 in 2da96174b9 outdated
    0@@ -0,0 +1,40 @@
    1+// Copyright (c) 2011-2022 The Bitcoin Core developers
    


    mzumsande commented at 10:54 pm on April 18, 2022:
    nit: remove 2011- , it’s new

    mruddy commented at 11:58 pm on April 18, 2022:
    ok, i will update when i update the commit along with updates from the other comments
  13. in src/validation.cpp:4392 in 2da96174b9 outdated
    4366+                // we could also be experiencing a storage system read error, or a read of a previous bad write. these are possible, but
    4367+                // less likely scenarios. we don't have enough information to tell a difference here.
    4368+                // the reindex process is not the place to attempt to clean and/or compact the block files. if so desired, a studious node operator
    4369+                // may use knowledge of the fact that the block files are not entirely pristine in order to prepare a set of pristine, and
    4370+                // perhaps ordered, block files for later reindexing.
    4371+                LogPrint(BCLog::REINDEX, "%s: unexpected data at file offset 0x%x - %s. continuing\n", __func__, (nRewind - 1), e.what());
    


    mzumsande commented at 11:04 pm on April 18, 2022:
    I wonder whether it might be better to keep this an unconditional log, so that there is a higher chance of getting reports such as the one in #21379. I think the expectation is that it shouldn’t be possible to get this warning (unless there is actual disk corruption) after this PR, unless you have an existing datadir that is already slightly corrupted by past reindexes.

    mruddy commented at 0:10 am on April 19, 2022:
    I did consider keeping this log message unconditional. I think we’d get a fair number of false positive reports if we leave this scary log message as it was. This bug has been around for about 7 years, and maybe more if you count the bugs that the last fix tried to handle. So, there are probably a lot of blk files that have been recoverably corrupted (i.e. corrupted in a way that is still processable). To me, it’s more of a notification for a concerned node operator that really cares about having a clean set of blk files. The new message gives the exact offset to go look, and maybe edit the file, if you really want to. But, The existing message doesn’t really do much to help the operator. It kind of just scares them, even though the processing goes on. Maybe there is a balance that we need to hit. But, I’m not sure what that looks like besides this right now.

    mzumsande commented at 12:33 pm on April 19, 2022:
    okay, makes sense. By the way, node operators also have the option to apply the linearize-data.py script in /contrib/linearize (which was adjusted in #16802 to deal with this bug, after a report in #14986), but this will also remove orphaned blocks from the blk files.

    maflcko commented at 11:41 am on October 13, 2022:
    unrelated: If the intent is to hide this, it would be good to clarify the log level. Currently it defaults to INFO, which is hidden, but this might be about to change, so maybe DEBUG might be better? cc @jonatack Not sure what the plan on the log levels currently is.

    jonatack commented at 1:46 pm on October 13, 2022:

    The default loglevel is currently debug and will become info.

    Warning and error levels are currently logged unconditionally, and the plan is to log info unconditionally as well.

    I’ve made a note to check this in the severity logging updates.

  14. mzumsande commented at 11:11 pm on April 18, 2022: contributor

    Concept ACK

    An alternative, extremely simple way to see that -reindex adds 8 bytes is is to just observe debug.log during startup: Here for regtest, with a datadir that has only the genesis block: Master, before Reindex: LoadBlockIndexDB: last block file info: CBlockFileInfo(blocks=1, size=293, heights=0...0, time=2011-02-02...2011-02-02)

    Master, after Reindex: LoadBlockIndexDB: last block file info: CBlockFileInfo(blocks=1, size=301, heights=0...0, time=2011-02-02...2011-02-02)

    The size goes up by 8 bytes from 293 to 301, where as it stays at 293 with this branch.

  15. mruddy force-pushed on Apr 19, 2022
  16. w0xlt commented at 3:40 am on April 19, 2022: contributor
    Concept ACK, will review soon.
  17. in src/node/blockstorage.cpp:791 in 66b8d3c23d outdated
    784@@ -785,19 +785,27 @@ bool ReadRawBlockFromDisk(std::vector<uint8_t>& block, const FlatFilePos& pos, c
    785     return true;
    786 }
    787 
    788-/** Store block on disk. If dbp is non-nullptr, the file is known to already reside on disk */
    


    jamesob commented at 2:21 pm on April 20, 2022:
    Maybe move this docstring to the header file?

    mruddy commented at 4:23 pm on April 20, 2022:
    i removed that doc string and put the meat of the comment into the context where it made sense on line 792. i figured that the remaining part of the comment “Store block on disk” was pretty self-evident for the method named “SaveBlockToDisk”. if i were to make more changes, then i’d probably rename the parameter to be meaningful and potentially add a comment. but then that begins to make the change larger. so, i’ll leave it for now unless i have to to get this merged.

    mruddy commented at 8:24 pm on April 20, 2022:
    update in 425fc8813307b8d96a48e4b45d0a7b00bbc2289e
  18. in src/validation.cpp:4380 in 66b8d3c23d outdated
    4355@@ -4356,7 +4356,18 @@ void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp)
    4356                     }
    4357                 }
    4358             } catch (const std::exception& e) {
    


    jamesob commented at 2:51 pm on April 20, 2022:

    (comment about existing code, not a criticism on this patch)

    The breadth of what this is catching is concerning to me - it makes it difficult to tell where we realistically expect exceptions to be thrown, and is making understanding this fix more difficult than I think it otherwise would be. From what I can tell, there are really two lines within this try that we expect the catch to handle,

    • blkdat >> block;
    • if (ReadBlockFromDisk(...
      • but maybe not even this line, since exceptions are handled within ReadBlockFromDisk

    Am I wrong about this? Is there a good reason the try block is so broad?

    If not, maybe (in future PRs?) we can limit this try/catch block to the specific line(s) where we expect to throw.


    mruddy commented at 12:54 pm on May 7, 2022:

    Thanks for looking at this. I agree, it’s likely an overly broad try-catch block and that it’s difficult to reason about what types of exceptions might be thrown, and from where, since there is no compile time concept of “checked” exceptions, like in Java. This is why I don’t want to try changing it for this PR :-) I’m sure I’ll miss something and it’ll break at runtime. And, it’ll just make the patch larger without impacting the usefulness of this specific fix.

    I can see that blkdat >> block; will throw at least std::ios_base::failure from at least a couple different places within the deserialization code. These are the exceptions that the original issue reported.

  19. in src/node/blockstorage.cpp:798 in 66b8d3c23d outdated
    797-    }
    798-    if (!FindBlockPos(blockPos, nBlockSize + 8, nHeight, active_chain, block.GetBlockTime(), dbp != nullptr)) {
    799+    } else {
    800+        // when known, blockPos.nPos points at the offset of the block data in the blk file. that already accounts for
    801+        // the serialization header present in the file (the 4 magic message start bytes + the 4 length bytes = 8 bytes).
    802+        // we add 8 only for new blocks since they will have the serialization header added when written to disk.
    


    jamesob commented at 3:09 pm on April 20, 2022:
    Aid to reviewers: the 8 bytes mentioned are added on writing the block to disk in blockstorage.cpp:WriteBlockToDisk().

    mruddy commented at 4:25 pm on April 20, 2022:
    also, the position comes from CChainState::LoadExternalBlockFile when it’s read in again. that position is past the serialization header already (it points directly at the block data) and thus why we don’t need to add 8 with the old code nBlockSize + 8.
  20. in src/node/blockstorage.cpp:802 in 66b8d3c23d outdated
    801+        // the serialization header present in the file (the 4 magic message start bytes + the 4 length bytes = 8 bytes).
    802+        // we add 8 only for new blocks since they will have the serialization header added when written to disk.
    803+        // this avoids over accounting for the serialization header on existing blocks. such over accounting was a long
    804+        // standing bug that added undesirable 8 byte gaps into blk data files following a -reindex operation.
    805+        // for more info, see https://github.com/bitcoin/bitcoin/issues/21379
    806+        nBlockSize += 8U;
    


    jamesob commented at 3:11 pm on April 20, 2022:

    Could also be phrased as nBlockSize += sizeof(unsigned int) + CMessageHeader::MESSAGE_START_SIZE to avoid hardcoding the expected header size.

    We could also include

    0constexpr size_t BLOCK_SERIALIZATION_HEADER_LEN = 
    1    sizeof(unsigned int) + CMessageHeader::MESSAGE_START_SIZE;
    

    nearby WriteBlockToDisk to avoid having that information live too far away from its source of truth.


    mruddy commented at 4:38 pm on April 20, 2022:
    maybe. this actually crossed my mind. i wasn’t sure if it was better to go with the hardcoded 8 that has been around for so long, or to go with the sizeof(unsigned int) that could be wrong for LP32 and ILP64 (https://en.cppreference.com/w/cpp/language/types). we may go with what you suggest, but i’d like to get some feedback and make this change on purpose.

    mruddy commented at 7:21 pm on April 20, 2022:
    i just found in src/compat/assumptions.h static_assert(sizeof(unsigned) == 4, "32-bit unsigned assumed"); that is what i need to feel safe making this change. i’ll do it in my next update.

    mruddy commented at 8:22 pm on April 20, 2022:
    update in 425fc8813307b8d96a48e4b45d0a7b00bbc2289e
  21. jamesob approved
  22. jamesob commented at 3:49 pm on April 20, 2022: member

    ACK 66b8d3c23d999651c79ac03e7558d4248a200ba1 (jamesob/ackr/24858.1.mruddy.reindex_log_test_incorre)

    Tested locally. The source of the bug (reading through SaveBlockToDisk -> FindBlockPos) is still somewhat unclear to me… I guess I’m trying to determine where those 8 header bytes are accounted for in the case of an existing block during reindex. In other words, exactly what line is responsible for the addition of those 8 bytes to the FlatFilePos* dbp, which is passed into SaveBlockToDisk.

    Near as I can figure, the answer to that question is somewhere in the LoadExternalBlockFile -> AcceptBlock call chain.

    [after writing the above and rereading the code]

    My best guess is that those 8 bytes are included in LoadExternalBlockFile (nRewind = blkdat.GetPos()) after an existing block is read (which then determines the value of dbp), whereas during the addition of a new block, no existing data is read and therefore those 8 header bytes are unaccounted for.

    Anyway, I’ve tested this patch pretty extensively:

    • multiple subsequent -reindex calls with -stopatheight=105000
    • using this branch with an existing mainnet datadir, waiting for new tips to come in, switching back to master and ensuring init happens properly
    • revert change in validation.cpp, ensure included unittests fail

    So I have good confidence that this is safe and correct.

    Thanks for working on this; I think this is a pretty important issue to iron out even if it doesn’t immediately cause true data corruption. It’s the kind of thing that could easily cascade into a bigger problem.

     0-----BEGIN PGP SIGNED MESSAGE-----
     1Hash: SHA512
     2
     3ACK 66b8d3c23d999651c79ac03e7558d4248a200ba1 ([`jamesob/ackr/24858.1.mruddy.reindex_log_test_incorre`](https://github.com/jamesob/bitcoin/tree/ackr/24858.1.mruddy.reindex_log_test_incorre))
     4
     5Tested locally. The source of the bug (reading through SaveBlockToDisk -> FindBlockPos) is still somewhat unclear to me... I guess I'm trying to determine where those 8 header bytes are accounted for in the case of an existing block during reindex. In other words, exactly what line is responsible for the addition of those 8 bytes to the `FlatFilePos* dbp`, which is passed into SaveBlockToDisk.
     6
     7Near as I can figure, the answer to that question is somewhere in the `LoadExternalBlockFile -> AcceptBlock` call chain.
     8
     9[after writing the above and rereading the code]
    10
    11My best guess is that those 8 bytes are included in LoadExternalBlockFile (`nRewind = blkdat.GetPos()`) after an existing block is read (which then determines the value of `dbp`), whereas during the addition of a new block, no existing data is read and therefore those 8 header bytes are unaccounted for.
    12
    13Anyway, I've tested this patch pretty extensively:
    14- - multiple subsequent -reindex calls with -stopatheight=105000
    15- - using this branch with an existing mainnet datadir, waiting for new tips to come in, switching back to master and ensuring init happens properly
    16- - revert change in validation.cpp, ensure included unittests fail
    17
    18So I have good confidence that this is safe and correct.
    19
    20Thanks for working on this; I think this is a pretty important issue to iron out even if it doesn't immediately cause true data corruption. It's the kind of thing that could easily cascade into a bigger problem.
    21
    22-----BEGIN PGP SIGNATURE-----
    23
    24iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmJgK0AACgkQepNdrbLE
    25TwXXJA/8Cq4RgrVcKBTB7zYjgqM5d2aQo7R++nMyF+5ZW+fPHY9kn9uR3YXJgVHb
    26u68TsFFAY/CS3ziNLmBsjsOS4yOvDzdz5m9f6Eso78qsLyf7qix3gOlCkGZGuGy3
    27Sr8FHYrgV4AuqdOQMlUnLLyZAf9ItDpF/OOcMm61IVM5MgMfXlnntod9bPil+AAI
    28G6COVrL9P4/RRgs1oxRuVeP+RXRwwyNzJbavjThd8pnpd5JAFpcs8z9FGDmLlQaV
    29Z4tVJvwoFohYhK5AoqnX13A+lIGRZoHqEihd2P++3wrzA1DiLQHXv2+3umRX6sSl
    30ApjhFBf68rH006BTe0OOoyakYMHPPJCleD+1W730Tk5/NxiuBO6ES4B7Yw4lEI4f
    31ZBXtVDha+3vRnUFXSUzAWIetfgoSOny3EC7rst9FOtd+I0W4lzbKGj4+Zh1PpB45
    32rx9ygnPV3h7shkAbla/zJL3L7luAlBgQkdYoHCEOTeGtuvtQ16yHpDDFgbRHlzh/
    331BeFr7/l03YM8i3rayB4UYUfuvEc2LuY4QOPYjyrgS0FZNY1z+lec+BWirul8Kz0
    34NsY25rHLiQYW70hh+5HcidFKs9LoiYfZ22eWuWpZTle07CClrV2xNGB36SO4AiuR
    35QComx/Z7uTFYdWD7tkwawgJzv/LHtxthEYpvacRJvro1Tj1rCQQ=
    36=R1e1
    37-----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
    
  23. mruddy commented at 4:54 pm on April 20, 2022: contributor

    My best guess is that those 8 bytes are included in LoadExternalBlockFile (nRewind = blkdat.GetPos()) after an existing block is read (which then determines the value of dbp), whereas during the addition of a new block, no existing data is read and therefore those 8 header bytes are unaccounted for. @jamesob Thanks for the review! You’re very close and you have the right idea. The exact line where they come from is in CChainState::LoadExternalBlockFile at the line dbp->nPos = nBlockPos; https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L4288 That is why blindly adding another 8 on the call to FindBlockPos in BlockManager::SaveBlockToDisk is wrong for already known blocks.

  24. mruddy force-pushed on Apr 20, 2022
  25. maflcko renamed this:
    reindex, log, test: incorrect blk file size calculation during reindex results in undesirable blk file malformedness
    incorrect blk file size calculation during reindex results in undesirable blk file malformedness
    on Apr 21, 2022
  26. maflcko removed the label Build system on Apr 21, 2022
  27. maflcko removed the label Validation on Apr 21, 2022
  28. droid192 commented at 3:52 pm on May 1, 2022: none
    please add error recovery to rescan such that from any ioerror the following blk.dat files can still be imported while/after the missing data is downloaded form a peer. Currently all considered blk.dat files after yield 0 imported blocks (btw one could stop after first ioerror right away!) also: does it reverse already padded blk.dats that got touched by reindex ? It reduces stress from network and oneself!
  29. mruddy commented at 2:23 pm on May 2, 2022: contributor

    Currently all considered blk.dat files after yield 0 imported blocks (btw one could stop after first ioerror right away!)

    That can be the case, but it is not the case a lot of the time. For example, this issue deals with the cases where reads fail, and get skipped, in-between blocks. The case where no subsequent blocks get imported is where an actual block read fails and that makes every subsequent block appear to be out of order. That’s a different thing.

    does it reverse already padded blk.dats

    No, this change does not attempt to alter the block files if an error is found (and skipped). This change attempts to stop setting up data that is used by later processing which winds up corrupting the block files.

    I’d like to keep this change focused on fixing the original issue and not trying to re-design the import processes.

  30. in src/node/blockstorage.cpp:656 in 425fc88133 outdated
    652@@ -653,6 +653,8 @@ bool BlockManager::FindUndoPos(BlockValidationState& state, int nFile, FlatFileP
    653     return true;
    654 }
    655 
    656+static constexpr size_t BLOCK_SERIALIZATION_HEADER_SIZE = CMessageHeader::MESSAGE_START_SIZE + sizeof(unsigned int);
    


    ryanofsky commented at 6:47 pm on May 3, 2022:

    In commit “reindex, log, test: fixes #21379” (425fc8813307b8d96a48e4b45d0a7b00bbc2289e)

    Could you add a comment documenting this variable? Would suggest /** Size of header written by WriteBlockToDisk() before a serialized CBlock */


    mruddy commented at 5:59 pm on May 4, 2022:
    Yes, I’ll add this comment in the next commit.
  31. in src/node/blockstorage.cpp:795 in 425fc88133 outdated
    791 FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp)
    792 {
    793     unsigned int nBlockSize = ::GetSerializeSize(block, CLIENT_VERSION);
    794     FlatFilePos blockPos;
    795-    if (dbp != nullptr) {
    796+    const auto position_known {dbp != nullptr};
    


    ryanofsky commented at 7:20 pm on May 3, 2022:

    In commit “reindex, log, test: fixes #21379” (425fc8813307b8d96a48e4b45d0a7b00bbc2289e)

    Just IMO, but I don’t think adding the position_known variable is helpful here. Just adds another piece of state and the name doesn’t convey the main thing this variable actually does, which is skip writing to disk.


    mruddy commented at 5:59 pm on May 4, 2022:
    I did this because dbp was compared to nullptr three times in this method, and I thought that giving a name to the concept that it represents seemed more descriptive/readable. Whether we have a known position for the block plays a large role in what goes on in BlockManager::FindBlockPos one call deeper, so, to me, it adds some continuity to the concept when you add in that context. By calling it position_known the reader gets an answer for why we don’t write it to disk again – because it’s position in a blk file is already known; it’s already written to disk.
  32. in src/node/blockstorage.cpp:802 in 425fc88133 outdated
    803+        // the serialization header present in the file (the 4 magic message start bytes + the 4 length bytes = 8 bytes = BLOCK_SERIALIZATION_HEADER_SIZE).
    804+        // we add BLOCK_SERIALIZATION_HEADER_SIZE only for new blocks since they will have the serialization header added when written to disk.
    805+        // this avoids over accounting for the serialization header on existing blocks. such over accounting was a long
    806+        // standing bug that added undesirable 8 byte gaps into blk data files following a -reindex operation.
    807+        // for more info, see https://github.com/bitcoin/bitcoin/issues/21379
    808+        nBlockSize += static_cast<unsigned int>(BLOCK_SERIALIZATION_HEADER_SIZE);
    


    ryanofsky commented at 7:43 pm on May 3, 2022:

    In commit “reindex, log, test: fixes #21379” (425fc8813307b8d96a48e4b45d0a7b00bbc2289e)

    It seems fragile to be passing FindBlockPos different lengths for the same block depending on whether dbp is null. The bug in the previous version of the code was not that it was passing the wrong length when dbp is non-null. The bug was that it was passing the wrong position. (It was passing a different position than the one the original FindBlockPos call returned when the block was first written). Fixing the new position being 8 bytes ahead by making the length 8 bytes shorter seems fragile because it assumes FindBlockPos will be ok with this. Better to just pass later FindBlockPos calls the same position earlier calls returned, than work around this by passing a different length.

    I also think this comment is a little confusing, and that it’s generally not helpful for comments to describe bugs in previous versions of the code that are no longer present. Information about the bug is helpful in the other part of the commit to understand the log statement, but I think is just distracting here in this part of the code.

    Suggested an alternate version of this fix in https://github.com/ryanofsky/bitcoin/commit/07c05a1b24bf42e57f8c229f505ed448a6df81f7


    mruddy commented at 7:42 pm on May 4, 2022:

    Thanks for the review. I can see it both ways. I’m not feeling a big difference either way. To me, it kind of feels like two sides of the same coin.

    What I’m thinking is that BlockManager::FindBlockPos is multi-purpose. It’s kind of funky because of how much within that method is conditioned upon the position being known, or not. In one use, it’s trying to figure out a spot where it can put the new header + block into a block file while respecting the file size limit. In another use, it’s trying to compute where it should put new blocks once we’re done processing blocks that we already have in a file. Since in the second use, it knows the actual position and the block size, it can make sense why the passed in size to add, nAddSize, is different between the two invocations.

    Thanks for providing an example of what you envisoned (07c05a1b24bf42e57f8c229f505ed448a6df81f7). I kind of like my commit because I don’t have to go back and add the header size. But, I can also see what you’re saying about passing FindBlockPos the same positions that it returns. I’m undecided. Any other thoughts?

    Lastly, I’m fine with trimming the comment to take out the info about past bug.

  33. in src/test/blockmanager_tests.cpp:37 in 425fc88133 outdated
    32+    // the actual block contents don't matter, just that it's a block.
    33+    // verify that the write position is at offset 0x12d.
    34+    // this is a check to make sure that https://github.com/bitcoin/bitcoin/issues/21379 does not recur
    35+    // 8 bytes (for serialization header) + 285 (for serialized genesis block) = 293
    36+    // add another 8 bytes for the second block's serialization header and we get 293 + 8 = 301
    37+    BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 1, chain, *params, nullptr).nPos, 301U);
    


    ryanofsky commented at 8:11 pm on May 3, 2022:

    In commit “reindex, log, test: fixes #21379” (425fc8813307b8d96a48e4b45d0a7b00bbc2289e)

    May be less fragile to write 8 + GetSerializeSize(GenesisBlock()) instead of 301


    mruddy commented at 5:58 pm on May 4, 2022:

    I could do this.

    My follow-up question is whether to move the BLOCK_SERIALIZATION_HEADER_SIZE constant into node/blockstorage.h and reference it here too? For example, node::BLOCK_SERIALIZATION_HEADER_SIZE + ::GetSerializeSize(params->GenesisBlock()), instead of using the magic 8.

    The tradeoff is that it moves the constant further from the code where it is used. Is that worth doing when the genesis block is immutable? I am fine with either approach. I kind of prefer putting it in the header file, but, I’d just like to get opinions before pushing up another commit.

  34. ryanofsky approved
  35. ryanofsky commented at 8:40 pm on May 3, 2022: contributor

    Code review ACK 425fc8813307b8d96a48e4b45d0a7b00bbc2289e, but I think the current fix is more confusing and fragile than it needs to be. I would suggest something like https://github.com/ryanofsky/bitcoin/commit/07c05a1b24bf42e57f8c229f505ed448a6df81f7 as a more straightforward fix that just passes new FindBlockPos calls the same position that the original FindBlockPos returned, instead of trying to work around the position being different by tweaking the size.

    Overall though this is a nice improvement adding a useful test and dealing with a scary looking bug.

  36. mruddy commented at 8:03 am on May 5, 2022: contributor

    @ryanofsky Thanks for the review. I’ve put updates related to your comments into 011f6ec54fffb7600ae300851cc9816423f3f4dd. I’ll squash the commits after positive code review of these.

    I still like what I have in SaveBlockToDisk regarding what is passed into FindBlockPos for the second parameter, nAddSize. When the position and size is known, we pass that in. When only size is known, we pass in block size + header size because we know that’s what we’ll have to add to the file. And, by doing it this way, we don’t have to re-adjust the known position on the way out of SaveBlockToDisk. We will never pass the same position into FindBlockPos for a given block because we either don’t know the position, or we pass in where we found the block. The only question is whether we should pass in the positon that FindBlockPos previously returned. I don’t see the advantage to doing that because it’s kind of like trying to pretend that we don’t know what we do know.

  37. jonatack commented at 8:08 am on May 5, 2022: contributor

    Concept ACK.

    (The linter needs appeasing when you repush; consider presenting this pull in its intended final form to ease review.)

    0The subject line of commit hash 011f6ec54fffb7600ae300851cc9816423f3f4dd is followed by a non-empty line. Subject lines should always be followed by a blank line.
    
  38. reindex, log, test: fixes #21379
    This fixes a blk file size calculation made during reindex that results in increased blk file malformity.
    The fix is to avoid double counting the size of the serialization header during reindex.
    This adds a unit test to reproduce the bug before the fix and to ensure that it does not recur.
    These changes include a log message change also so as to not be as alarming. This is a common and recoverable
    data corruption. These messages can now be filtered by the debug log reindex category.
    bcb0cacac2
  39. mruddy force-pushed on May 7, 2022
  40. mruddy commented at 1:07 pm on May 7, 2022: contributor
    ok, i squashed the commits and rebased onto the latest master. bcb0cacac28e98a39dc856c574a0872fe17059e9 is the latest after working through the feedback. i tested and it still looks good.
  41. mruddy commented at 9:52 am on May 12, 2022: contributor
    just to summarize, i think bcb0cacac28e98a39dc856c574a0872fe17059e9 is the final form of this patch. it’s a small, targeted, and uncomplicated bug fix. i think that if we can get some final reviews that this would be ready to merge. thanks!
  42. mruddy commented at 10:39 am on June 7, 2022: contributor
    Is there anything that I can do to get this merged? It’s ready to merge as far as I can tell.
  43. mruddy renamed this:
    incorrect blk file size calculation during reindex results in undesirable blk file malformedness
    incorrect blk file size calculation during reindex results in recoverable blk file corruption
    on Jun 9, 2022
  44. in src/node/blockstorage.cpp:802 in bcb0cacac2
    799         blockPos = *dbp;
    800+    } else {
    801+        // when known, blockPos.nPos points at the offset of the block data in the blk file. that already accounts for
    802+        // the serialization header present in the file (the 4 magic message start bytes + the 4 length bytes = 8 bytes = BLOCK_SERIALIZATION_HEADER_SIZE).
    803+        // we add BLOCK_SERIALIZATION_HEADER_SIZE only for new blocks since they will have the serialization header added when written to disk.
    804+        nBlockSize += static_cast<unsigned int>(BLOCK_SERIALIZATION_HEADER_SIZE);
    


    ryanofsky commented at 2:13 pm on June 13, 2022:

    In commit “reindex, log, test: fixes #21379” (bcb0cacac28e98a39dc856c574a0872fe17059e9)

    This comment doesn’t address the confusing thing here because it doesn’t say why nBlockSize is set to different sizes for the same block depending on position_known. Would suggest a comment that says why the code is using a different size in each case. Could use:

     0// When position_known is false, the CBlock has not been written to the
     1// disk yet, so the nBlockSize value passed to FindBlockPos needs to be
     2// set so FindBlockPos allocates enough bytes for the CBlock data *plus*
     3// the size of the header WriteBlockToDisk will write before the CBlock
     4// data.
     5//
     6// When position_known is true, WriteBlockToDisk will have already been
     7// called, and it will have adjusted blockPos.nPos to point to the CBlock
     8// data after the header, instead of to the header, so the nBlockSize
     9// value passed to FindBlockPos should only include the CBlock size,
    10// *not* the header size.
    

    I still prefer the approach of passing a consistent size and position to FindBlockPos like https://github.com/ryanofsky/bitcoin/commit/07c05a1b24bf42e57f8c229f505ed448a6df81f7 instead of dealing with an inconsistent position value by introducing an inconsistent size value. But both approaches have the same ultimate effect, so the more important thing is to document the code clearly.


    mruddy commented at 10:27 am on June 14, 2022:
    First, thanks for the feedback. While I’m not opposed to adding these comments if they help get this merged, I’d rather let the commit stay as it is so that the reviews can accumulate and we can get this stuff merged.

    mzumsande commented at 4:13 am on July 5, 2022:

    I also think this comment would be helpful, except for the point below.

    0// When position_known is true, WriteBlockToDisk will have already been
    1// called, and it will have adjusted blockPos.nPos to point to the CBlock
    2// data after the header
    

    Isn’t this incorrect? My understanding is that when position_known is true, blockPos.nPos was set by the LoadExternalBlockFile code which has read past the header in the existing block file and moved the position accordingly - but not WriteBlockToDisk, which wasn’t called for this block at all during this invocation of bitcoind.


    ryanofsky commented at 3:42 pm on July 5, 2022:

    re: #24858 (review)

    0// When position_known is true, WriteBlockToDisk will have already been
    1// called, and it will have adjusted blockPos.nPos to point to the CBlock
    2// data after the header
    

    Isn’t this incorrect?

    I think it is correct 😎. The line that changes blockPos.nPos to point after the header instead of before the header is here:

    https://github.com/bitcoin/bitcoin/blob/9fb2a2bc6768ab03fcada9155d52a16ce6f6a0cc/src/node/blockstorage.cpp#L676

    This is the critical code calculating the block position which is saved in the CBlockIndex struct, and written to BlockIndexDb and used when ReadBlockFromDisk is called to read back block data.

    My understanding is that when position_known is true, blockPos.nPos was set by the LoadExternalBlockFile code which has read past the header in the existing block file and moved the position accordingly

    I don’t think LoadExternalBlockFile is relevant to normal operation, only used when importing or reindexing is requested.

    but not WriteBlockToDisk, which wasn’t called for this block at all during this invocation of bitcoind.

    It’s true that WriteBlockToDisk may or may not have been called during this invocation of bitcoind, but it must have been called previously at some point, and the position it returns is written to BlockIndexDb and used to read blocks in future invocations.


    mzumsande commented at 4:42 pm on July 5, 2022:
    Hmm, but the suggested comment is not in ReadBlockFromDisk but in SaveBlockToDisk, and it is about the case when “position_known is true”, or dbp != nullptr. I think this is only possible when this function is called via AcceptBlock() from LoadExternalBlockFile which has set the blockPos - the normal operation calls to SaveBlockToDisk (via AcceptBlock() and ProcessNewBlock()) have dbp==nullptr, so position_known is false and the other half of the comment applies.

    ryanofsky commented at 5:21 pm on July 5, 2022:

    Sure, if you feel it is more useful for the comment to go into those things, feel free to make a suggestion along those lines. My suggestion was rejected anyway so maybe you will have better luck.

    Also, I wrote this comment to describe a piece of code that I think is unnecessarily confusing. I would personally prefer not to have this piece of code, and not to have this comment explaining it, and instead use a solution like https://github.com/ryanofsky/bitcoin/commit/07c05a1b24bf42e57f8c229f505ed448a6df81f7 which does not require understanding external places where SaveBlockToDisk is called to understand, and only requires understanding internally how it always returns the same position.


    mzumsande commented at 4:20 pm on July 6, 2022:

    I don’t have a strong preference - it seems that both approaches only affect the blockstorage-internal logic, and since neither solution changes the public interface SaveBlockToDisk, this sholdn’t change what external places will need to know: In particular, that if they pass dbp!=nullptr, dbp needs to point right after the header and before the block data (which should be documented in SaveBlockToDisk imo).

    However, I think that the internal function FindBlockPos() would be clearer if it was split up depending on whether the block is known, with so much code being conditional on it: If dbp!=nullptr , the function doesn’t “find” anything new but only updates the block info statistics (and sometimes does a Flush which I’m not convinced is necessary). I had an initial go at this in https://github.com/mzumsande/bitcoin/commits/202207_refactor_findblockpos , which is outside the scope of this PR, but might make sense as a follow-up.


    LarryRuane commented at 1:38 am on July 27, 2022:
    I agree with @ryanofsky’s comment (above) about consistency. I’d also note that adding 8 to nBlockSize is confusing because it’s no longer a block size!
  45. ryanofsky approved
  46. ryanofsky commented at 4:32 pm on June 13, 2022: contributor

    Code review ACK bcb0cacac28e98a39dc856c574a0872fe17059e9. This is a disturbing bug with an easy fix which seems well-worth merging.

    I still like what I have in SaveBlockToDisk regarding what is passed into FindBlockPos for the second parameter, nAddSize. When the position and size is known, we pass that in. When only size is known, we pass in block size + header size because we know that’s what we’ll have to add to the file.

    To me, this sounds more complicated that just passing the same size.

    And, by doing it this way, we don’t have to re-adjust the known position on the way out of SaveBlockToDisk.

    This is inaccurate because WriteBlockToDisk is adjusting the position here if dbp is null. Instead of SaveBlockToDisk sometimes adjusting the position, and sometimes not adjusting it, I think it is clearer if SaveBlockToDisk always adjusts the position as in https://github.com/ryanofsky/bitcoin/commit/07c05a1b24bf42e57f8c229f505ed448a6df81f7. Just making the adjustment in SaveBlockToDisk explicit at all is improvement. I’m pretty sure I stared at this code for an hour before I noticed the hidden adjustment in WriteBlockToDisk that was critical to making it work.

    We will never pass the same position into FindBlockPos for a given block because we either don’t know the position, or we pass in where we found the block. The only question is whether we should pass in the positon that FindBlockPos previously returned. I don’t see the advantage to doing that because it’s kind of like trying to pretend that we don’t know what we do know.

    I don’t know why you are mentioning the position-not-known case because that is the working case, not the broken case, and no change we are discussing affects it. Also no code is “trying to pretend” anything in that case that I can see.

    The advantage to passing the same position to FindBlockPos that it previously returned is that code is easier to explain and less likely to break if a variable has one meaning within a function, instead of two different meanings. This bug is directly caused by two different position values being used for the same block in FindBlockPos. You are working around two different position values being for being used for the same block by introducing two different size values for the same block. I think if we just used just one position and one size for a block in FindBlockPos, code would be easier to understand and less fragile.

  47. mruddy commented at 10:31 am on June 14, 2022: contributor

    @ryanofsky Thanks for the code review ACK!

    I appreciate your comments and I spent quite a while responding to them. In the end, I decided not to post my responses in order to avoid confusion and focus on this being ready to merge. I hear you though. I think we accomplish the same result just in a pretty minorly different way that comes down to personal preference.

  48. in src/test/blockmanager_tests.cpp:17 in bcb0cacac2
    12+
    13+using node::BlockManager;
    14+using node::BLOCK_SERIALIZATION_HEADER_SIZE;
    15+
    16+// use BasicTestingSetup here for the data directory configuration, setup, and cleanup
    17+BOOST_FIXTURE_TEST_SUITE(blockmanager_tests, BasicTestingSetup)
    


    mzumsande commented at 0:00 am on July 5, 2022:
    nit: With the convention "<source_filename>_tests.cpp", the name of the file/test suite should be blockstorage_tests.
  49. in src/validation.cpp:4381 in bcb0cacac2
    4377@@ -4378,7 +4378,18 @@ void CChainState::LoadExternalBlockFile(FILE* fileIn, FlatFilePos* dbp)
    4378                     }
    4379                 }
    4380             } catch (const std::exception& e) {
    4381-                LogPrintf("%s: Deserialize or I/O error - %s\n", __func__, e.what());
    4382+                // historical bugs added extra data to the block files that does not deserialize cleanly.
    


    mzumsande commented at 3:43 am on July 5, 2022:
    nit: I would drop the last two sentences (seems a bit verbose and out of place to explain what reindex is not and what studious node operators may do), and I find long comments like this one easier to read when full sentences start with capitals.
  50. in src/node/blockstorage.h:177 in bcb0cacac2
    173@@ -171,6 +174,7 @@ class BlockManager
    174     bool WriteUndoDataForBlock(const CBlockUndo& blockundo, BlockValidationState& state, CBlockIndex* pindex, const CChainParams& chainparams)
    175         EXCLUSIVE_LOCKS_REQUIRED(::cs_main);
    176 
    177+    /** Store block on disk. If dbp is not nullptr, then it provides the known position of the block within a block file on disk. */
    


    mzumsande commented at 4:30 am on July 5, 2022:
    nit: Just from reading this comment it’s not clear to me why this function is even called when the block is already on disk - it apparently doesn’t need to be stored in that case.
  51. mzumsande commented at 4:48 am on July 5, 2022: contributor

    ACK bcb0cacac28e98a39dc856c574a0872fe17059e9 (reviewed code and did some testing, I agree that it fixes the bug).

    nits below are non-blocking and could be left to follow-ups.

    In general, I got the impression that the SaveBlockToDisk / FindBlockPos code could benefit from doc improvements and possibly refactors to improve the quality of code, even though it’s obviously a critical area.

  52. in src/node/blockstorage.cpp:795 in bcb0cacac2
    792 FlatFilePos BlockManager::SaveBlockToDisk(const CBlock& block, int nHeight, CChain& active_chain, const CChainParams& chainparams, const FlatFilePos* dbp)
    793 {
    794     unsigned int nBlockSize = ::GetSerializeSize(block, CLIENT_VERSION);
    795     FlatFilePos blockPos;
    796-    if (dbp != nullptr) {
    797+    const auto position_known {dbp != nullptr};
    


    LarryRuane commented at 9:31 pm on July 23, 2022:

    nit, if you retouch (bcb0cacac28e98a39dc856c574a0872fe17059e9)

    0    const bool position_known{dbp != nullptr};
    
  53. LarryRuane commented at 1:39 am on July 27, 2022: contributor
    I want to propose my own alternative approach, which I think is easier to understand: https://github.com/LarryRuane/bitcoin/commit/e8b90d475b193c1c1b1a74d7751c9ec830c82867. This version hides the 8 bytes adjustment within FindBlockPos, which is where it belongs. In my view, SaveBlockToDisk should not need to be aware of this detail.
  54. in src/test/blockmanager_tests.cpp:31 in bcb0cacac2
    26+    // simulate what happens during reindex
    27+    // simulate a well-formed genesis block being found at offset 8 in the blk00000.dat file
    28+    // the block is found at offset 8 because there is an 8 byte serialization header
    29+    // consisting of 4 magic bytes + 4 length bytes before each block in a well-formed blk file.
    30+    FlatFilePos pos{0, BLOCK_SERIALIZATION_HEADER_SIZE};
    31+    BOOST_CHECK_EQUAL(blockman.SaveBlockToDisk(params->GenesisBlock(), 0, chain, *params, &pos).nPos, BLOCK_SERIALIZATION_HEADER_SIZE);
    


    LarryRuane commented at 3:53 am on July 27, 2022:

    This test is slightly unrealistic because it’s simulating a normal (new) block add followed by a reindex. A reindex, if it occurs, always happens immediately after the node starts up, before any blocks have been added from peers. As a result of this, this (the second) test call to SaveBlockToDisk causes m_blockfile_info.nBlocks to increment to 2 here, which is strange, since there’s only one block.

    This doesn’t cause any problems now, but some future change could cause this test to break in mysterious ways.

    A way to fix this would be to allocate a new BlockManager (and probably CChain) for this call, to simulate restarting the node. (The third call to SaveBlockToDisk, below, can use the same ones as this one does, because following a reindex, the real node does go ahead and start adding new blocks.)

    In case it’s not clear, I updated that commit (containing other suggestions) to show what I mean.

  55. hernanmarino approved
  56. hernanmarino commented at 1:07 am on July 28, 2022: contributor

    Tested ACK. Reproduced the issue on master and tested succesfully the fix proposed.

    Tested on Linux 5.15.0-41-generic x86_64 x86_64 x86_64 GNU/Linux Compiled with gcc 11.2.0

  57. pablomartin4btc approved
  58. pablomartin4btc commented at 5:53 pm on July 28, 2022: contributor

    Tested ACK.

    gcc version 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04)

    Linux 5.11.0-43-generic #47~20.04.2-Ubuntu SMP x86_64 x86_64 x86_64 GNU/Linux

  59. w0xlt approved
  60. w0xlt commented at 1:15 am on July 30, 2022: contributor

    tACK https://github.com/bitcoin/bitcoin/pull/24858/commits/bcb0cacac28e98a39dc856c574a0872fe17059e9

    Verified the error on master branch as described in #24858 (comment) and #24858#pullrequestreview-944897633. This PR fixes it.

  61. jamesob commented at 7:57 pm on August 4, 2022: member

    I think this would be a good bug to fix, so it would be nice to see some progress here. @mruddy do you think you’ll be able to address some of the outstanding feedback? I think it might be good to at least incorporate some of the clarifying commentary that @ryanofsky proposed.

    This PR is a step in the right direction; the fix could possibly clearer, along the lines of what @ryanofsky or @LarryRuane have suggested. But in any case, one of these approaches should be committed to, reviewed, and merged (with credit to @mruddy in any case for proposing the conceptual solution).

    Edit: to be clear, merging this PR as-is would be a big improvement over the current state of things. I think we’re bikeshedding a little at this point, and a follow-up could be done if necessary.

  62. DrahtBot commented at 10:07 am on August 5, 2022: contributor

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

    Conflicts

    No conflicts as of last run.

  63. mruddy commented at 7:50 pm on August 10, 2022: contributor
    If I have to change something, then I’d probably go back in the direction that @LarryRuane suggested with e8b90d475b193c1c1b1a74d7751c9ec830c82867. I’d have to review and test those changes closer, but at a glance, they seem plausible. They are close to my original fix in that they keep the change within FindBlockPos. I changed to the current approach trying to be responsive to feedback in April. I’d probably avoid adding comments in any potential future commit as those seem to cause a relatively large amount of friction. The current commit has gathered review, testing, and works, so I hate to start the process all over again. But, if this fix isn’t going to get merged for some reason, then let me know and I’ll do it.
  64. LarryRuane commented at 1:31 am on August 13, 2022: contributor
    tested code-review ACK bcb0cacac28e98a39dc856c574a0872fe17059e9 I strongly agree with @jamesob, this should get merged asap; any possible improvements can be done in a follow-up PR. My apologies for stirring the pot too much!
  65. jamesob commented at 2:35 pm on October 12, 2022: member
    I think this is ready for merge.
  66. glozow merged this on Oct 12, 2022
  67. glozow closed this on Oct 12, 2022

  68. mruddy deleted the branch on Oct 14, 2022
  69. sidhujag referenced this in commit e0075d67c2 on Oct 23, 2022
  70. bitcoin locked this on Oct 14, 2023

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-08 22:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me