prune, import: allow pruning to work during loadblock import #24957

pull mruddy wants to merge 1 commits into bitcoin:master from mruddy:issue_23852_import_prune changing 1 files +19 −1
  1. mruddy commented at 12:33 pm on April 24, 2022: contributor

    Fixes #23852

    This allows pruning to work during the -loadblock import process.

    An example use case is where you have a clean set of block files and you want to create a pruned node from them, but you don’t want to alter the input set of block files.

    #23852 noted that pruning was not working reliably during the loadblock import process. The reason why the loadblock process was not pruning regularly as it progressed is that the pruning process (BlockManager::FindFilesToPrune) checks the tip height of the active chainstate, and CChainState::ActivateBestChain was not called (which updates that tip height) in ThreadImport until after all the import files were processed.

    An example bash command line that makes it easy to import a bunch of block files:

    0./src/qt/bitcoin-qt -debug -logthreadnames -datadir=/tmp/btc -prune=550 -loadblock=/readonly/btc/main/blk{00000..00043}.dat
    

    One interesting side note is that CChainState::ActivateBestChain can be called while the import process is running (in the loadblk thread) by concurrent network message processing activity in the msghand thread. For example, one way to reproduce this easily is with the getblockfrompeer RPC (requesting a block with height greater than 100000) run from a node connected to an importing node. There are other ways too, but this is an easy way. I only mention this to explain how the max_prune_height=225719 log message in the original issue came to occur.

  2. DrahtBot added the label Validation on Apr 24, 2022
  3. mruddy renamed this:
    prune, import: fixes #23852
    prune, import: allow pruning to work during loadblock import
    on Apr 24, 2022
  4. vincenzopalazzo commented at 9:15 am on April 25, 2022: none
    Concept ACK
  5. luke-jr approved
  6. luke-jr commented at 8:59 pm on May 8, 2022: member
    Code safety utACK. Did not verify if it fixes a bug or impacts performance.
  7. luke-jr referenced this in commit c86f129fd1 on May 21, 2022
  8. mruddy closed this on Jun 15, 2022

  9. jamesob commented at 8:52 pm on June 17, 2022: member
    Concept ACK. I think this should be reopened, as it’s a small diff and a good change. I can review more thoroughly next week.
  10. laanwj commented at 8:55 pm on June 17, 2022: member
    Yeah, concept ACK (adding up for grabs label just in case)
  11. laanwj added the label Up for grabs on Jun 17, 2022
  12. mruddy reopened this on Jun 18, 2022

  13. mruddy commented at 9:41 am on June 18, 2022: contributor
    I closed this to garbage collect because I thought it was dead. But, since there’s interest I’ve reopened it. Thanks!
  14. fanquake removed the label Up for grabs on Jun 20, 2022
  15. mruddy closed this on Sep 9, 2022

  16. mruddy deleted the branch on Sep 9, 2022
  17. mruddy restored the branch on Oct 21, 2022
  18. mruddy commented at 2:01 pm on October 27, 2022: contributor
    Reopening since I have time to usher this to completion now. Same patch, just rebased onto current master.
  19. mruddy reopened this on Oct 27, 2022

  20. mruddy force-pushed on Oct 27, 2022
  21. mruddy force-pushed on Oct 27, 2022
  22. mruddy force-pushed on Oct 27, 2022
  23. mruddy force-pushed on Oct 28, 2022
  24. mruddy force-pushed on Oct 30, 2022
  25. in src/validation.cpp:4471 in 2f62704430 outdated
    4466+                    // this is a tradeoff to conserve disk space at the expense of time
    4467+                    // spent updating the tip to be able to prune.
    4468+                    // otherwise, ActivateBestChain won't be called by the import process
    4469+                    // until after all of the block files are loaded. ActivateBestChain can be
    4470+                    // called by concurrent network message processing. but, that is not
    4471+                    // reliable for the puspose of pruning while importing.
    


    aureleoules commented at 3:06 pm on October 31, 2022:
    0                    // reliable for the purpose of pruning while importing.
    

    mruddy commented at 6:41 pm on October 31, 2022:
    ah, thanks. will update with my next push.
  26. aureleoules commented at 4:36 pm on October 31, 2022: member

    I’m not sure how to test this? I let a fresh bitcoind node sync (enough to generate 20 blk.dat files) and tried importing on another fresh bitcoind node these blk files using ./src/bitcoind -debug -logthreadnames -datadir=/tmp/btc -prune=550 -loadblock=/tmp/btc1/blocks/blk{00000..00020}.dat

    where /tmp/btc1 contains the 20 blk.dat files

    but I get these logs for every block (both on master and this PR):

    02022-10-31T16:30:15Z [loadblk] [reindex] LoadExternalBlockFile: Out of order block 000000000000061a9e7540272dd4cb607350b5f704d89bf87d4757078e0d2aa5, parent 0000000000000627a077a5b46189adeb9b345d3f5b9a4c8cfd847f5ba67af26c not known
    1...
    22022-10-31T16:30:16Z [loadblk] Loaded 0 blocks from external file in 1135ms
    
  27. mruddy commented at 6:40 pm on October 31, 2022: contributor

    @aureleoules thanks for looking into this. your block files have blocks out of order. it’s nothing that you did wrong. it’s just that with the -loadblock option, there is no unknown-parent tracking like there is with -reindex. i forget the historical reason for why that is at the moment. but, that’s how it is. for reference: https://github.com/bitcoin/bitcoin/blob/master/src/node/blockstorage.cpp#L885 https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L4431-L4439

    There is a tool to make some block files for testing where the blocks are in order and loadable with -loadblock at https://github.com/bitcoin/bitcoin/tree/master/contrib/linearize

    If I remember correctly, another snag you’ll come across is that you need to have at least 100,000 blocks for mainnet due to nPruneAfterHeight see https://github.com/bitcoin/bitcoin/blob/master/src/chainparams.cpp#L109

    I’ll make a todo item for myself to see how I can make testing this easier via a follow-up comment, and maybe a set of test input block files or something.

  28. mruddy force-pushed on Nov 1, 2022
  29. mruddy referenced this in commit adc1e623fe on Nov 1, 2022
  30. mruddy commented at 2:59 pm on November 1, 2022: contributor

    @aureleoules I put a set of linearized block files that are useful for testing this at:

    https://github.com/mruddy/test_blk_files

    A way to use the files and be able to watch what is going on in the debugger is with:

    0mkdir -p /tmp/btc && gdb --tui --args ./src/qt/bitcoin-qt -debug -logthreadnames -datadir=/tmp/btc -prune=550 -loadblock=/readonly/btc/main/blk{00000..00014}.dat -connect=0
    

    The debug log will log related messages and can be found easily with grep like the following examples:

    0grep 'removed 1 blk/rev pairs' /tmp/btc/debug.log
    12022-11-01T14:50:40Z [loadblk] [prune] target=550MiB actual=388MiB diff=161MiB max_prune_height=140633 removed 1 blk/rev pairs
    2
    3grep 'unlink pruned files' /tmp/btc/debug.log
    42022-11-01T14:50:46Z [loadblk] [bench] FlushStateToDisk: unlink pruned files started
    52022-11-01T14:50:46Z [loadblk] [bench] FlushStateToDisk: unlink pruned files completed (28.22ms)
    

    Depending on what you’re looking to understand, various breakpoints can be set within gdb, for example:

    0b ThreadImport
    1b LoadExternalBlockFile
    2b FlushStateToDisk
    3b FindFilesToPrune
    4b PruneOneBlockFile
    
  31. aureleoules approved
  32. aureleoules commented at 4:03 pm on November 1, 2022: member

    ACK 488682e785194f807f21b4dc6826df7714d78a25

    Thanks @mruddy for the linearized block files! I was able to verify with the given block files that pruning occurs during the import process in this PR and not at the end (master). If I understand correctly, this would allow cloning a full node into a pruned node without needing the double amount of disk space during the import process and without needing a workaround (-connect=localhost).

  33. luke-jr commented at 10:21 pm on November 3, 2022: member
    In the future, please don’t rebase for no reason
  34. luke-jr commented at 1:40 am on November 16, 2022: member
    Should we be doing this check later on when processing out of order successor blocks?
  35. luke-jr commented at 1:50 am on November 16, 2022: member

    P.S. Needs rebase. Silent conflict with #16981

     0diff --git a/src/validation.cpp b/src/validation.cpp
     1index 40d15b60d63..285aeb16d4e 100644
     2--- a/src/validation.cpp
     3+++ b/src/validation.cpp
     4@@ -4366,6 +4366,7 @@ void Chainstate::LoadExternalBlockFile(
     5                 // next block, but it's still possible to rewind to the start of the current block (without a disk read).
     6                 nRewind = nBlockPos + nSize;
     7                 blkdat.SkipTo(nRewind);
     8+                std::shared_ptr<CBlock> recent_block_maybe_not_this_block;
     9                 {
    10                     LOCK(cs_main);
    11                     // detect out of order blocks, and store them for later
    12@@ -4383,7 +4384,7 @@ void Chainstate::LoadExternalBlockFile(
    13                     if (!pindex || (pindex->nStatus & BLOCK_HAVE_DATA) == 0) {
    14                         // This block can be processed immediately; rewind to its start, read and deserialize it.
    15                         blkdat.SetPos(nBlockPos);
    16-                        std::shared_ptr<CBlock> pblock{std::make_shared<CBlock>()};
    17+                        auto& pblock = recent_block_maybe_not_this_block = std::make_shared<CBlock>();
    18                         blkdat >> *pblock;
    19                         nRewind = blkdat.GetPos();
    20 
    21@@ -4416,7 +4417,7 @@ void Chainstate::LoadExternalBlockFile(
    22                     // called by concurrent network message processing. but, that is not
    23                     // reliable for the purpose of pruning while importing.
    24                     BlockValidationState state;
    25-                    if (!ActivateBestChain(state, pblock)) {
    26+                    if (!ActivateBestChain(state, recent_block_maybe_not_this_block)) {
    27                         LogPrint(BCLog::REINDEX, "failed to activate chain (%s)\n", state.ToString());
    28                         break;
    29                     }
    
  36. mruddy force-pushed on Nov 18, 2022
  37. DrahtBot commented at 8:42 am on November 18, 2022: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK achow101
    Concept ACK vincenzopalazzo, jamesob, laanwj
    Stale ACK luke-jr, aureleoules

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  38. mruddy commented at 8:54 am on November 18, 2022: contributor

    @luke-jr Thanks! I just rebased and had to make a small change due to the #16981 updates. Those updates moved the block data read from disk into the code block that is guarded by cs_main. I couldn’t move my updates for this PR into that code block because ActivateBestChain asserts that cs_main is not held. So, I brought pblock one level back up out of that cs_main guarded scope (leaving the read where #16981 moved it) so that I could access it without the lock being held.

    Should we be doing this check later on when processing out of order successor blocks?

    No, because -loadblock doesn’t support the out of order processing, and I don’t want to make -prune work with -reindex for the reasons that I listed at #23852 (comment)

  39. mruddy force-pushed on Nov 18, 2022
  40. in src/validation.cpp:4477 in 734355b470 outdated
    4473@@ -4471,6 +4474,21 @@ void Chainstate::LoadExternalBlockFile(
    4474                     }
    4475                 }
    4476 
    4477+                if (fPruneMode && !fReindex && pblock) {
    


    luke-jr commented at 9:37 pm on November 23, 2022:
    Any reason not to just move this up to after we accept the block?

    mruddy commented at 4:03 pm on November 24, 2022:
    Yes, I can’t move my updates for this PR into that code block because ActivateBestChain asserts that cs_main is not held.
  41. luke-jr approved
  42. luke-jr commented at 9:37 pm on November 23, 2022: member
    utACK 734355b470764c523ef08b25320a5fabb73358a6
  43. mruddy commented at 12:09 pm on December 7, 2022: contributor
    Thanks to those that have reviewed/tested this already, I think it’s ready to go. @jamesob Is there a chance that you might be able to review or test this PR? Thanks!
  44. aureleoules approved
  45. aureleoules commented at 5:28 pm on January 18, 2023: member
    re-tACK 734355b470764c523ef08b25320a5fabb73358a6
  46. achow101 commented at 2:39 am on February 16, 2023: member
    ACK 734355b470764c523ef08b25320a5fabb73358a6
  47. achow101 commented at 2:43 am on February 16, 2023: member

    Looks like there’s a silent merge conflict:

     0../../../src/validation.cpp: In member function ‘void Chainstate::LoadExternalBlockFile(FILE*, FlatFilePos*, std::multimap<uint256, FlatFilePos>*)’:
     1../../../src/validation.cpp:4518:21: error: ‘fPruneMode’ was not declared in this scope; did you mean ‘node::fPruneMode’?
     2 4518 |                 if (fPruneMode && !fReindex && pblock) {
     3      |                     ^~~~~~~~~~
     4      |                     node::fPruneMode
     5In file included from ../../../src/validation.h:22,
     6                 from ../../../src/validation.cpp:6:
     7../../../src/node/blockstorage.h:51:13: note: ‘node::fPruneMode’ declared here
     8   51 | extern bool fPruneMode;
     9      |             ^~~~~~~~~~
    10make[2]: *** [Makefile:10619: libbitcoin_node_a-validation.o] Error 1
    
  48. prune, import: fixes #23852
    allows pruning to work during the loadblock import process.
    c4981e7f63
  49. mruddy force-pushed on Feb 22, 2023
  50. mruddy commented at 10:30 am on February 22, 2023: contributor
    @achow101 Thanks! I rebased and then resolved the silent merge conflict by changing fPruneMode to m_blockman.IsPruneMode().
  51. achow101 commented at 5:34 pm on February 22, 2023: member
    re-ACK c4981e7f63a3e0aeec1ef3dec040261e993dd724
  52. DrahtBot requested review from aureleoules on Feb 22, 2023
  53. DrahtBot requested review from luke-jr on Feb 22, 2023
  54. fanquake requested review from jamesob on Feb 22, 2023
  55. achow101 merged this on May 3, 2023
  56. achow101 closed this on May 3, 2023

  57. sidhujag referenced this in commit d03f4ab2d6 on May 4, 2023
  58. mruddy deleted the branch on May 5, 2023
  59. luke-jr referenced this in commit 50e3f6c9a2 on Jun 27, 2023
  60. Fabcien referenced this in commit 2c2af69440 on Dec 19, 2023
  61. bitcoin locked this on May 4, 2024

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-11-21 09:12 UTC

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