blockstorage: Adjust fastprune limit if block exceeds blockfile size #27191

mzumsande · 2023-03-02T17:13:03Z

The debug-only -fastprune option used in several tests is not always safe to use:
If a -fastprune node receives a block larger than the maximum blockfile size of 64kb bad things happen: The while loop in BlockManager::FindBlockPos never terminates, and the node runs oom because memory for m_blockfile_info is allocated in each iteration of the loop.
The same would happen if a naive user used -fastprune on anything other than regtest (so this can be tested by syncing on signet for example, the first block that crashes the node is at height 2232).

Change the approach by raising the blockfile size to the size of the block, if that block otherwise wouldn't fit (idea by TheCharlatan).

DrahtBot · 2023-03-02T17:13:06Z

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	ryanofsky, TheCharlatan, pinheadmz

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
#27039 (blockstorage: do not flush block to disk if it is already there by pinheadmz)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

pinheadmz

concept ACK

I think assert is the appropriate move here to prevent the infinite memory loop! We mine as well just crash early. It might also make sense to not allow -fastprune on anything besides regtest as well. I'm not sure if that is covered by ArgsManager::DEBUG_ONLY

pinheadmz · 2023-03-02T18:47:35Z

src/node/blockstorage.cpp

    bool finalize_undo = false;
    if (!fKnown) {
-        while (m_blockfile_info[nFile].nSize + nAddSize >= (gArgs.GetBoolArg("-fastprune", false) ? 0x10000 /* 64kb */ : MAX_BLOCKFILE_SIZE)) {
+        const unsigned int max_blockfile_size{gArgs.GetBoolArg("-fastprune", false) ? 0x10000 : MAX_BLOCKFILE_SIZE};


This cleanup is already addressed in #27039 ;-)

Good, should make rebasing easy! Here, it's not a cleanup but needed to use max_blockfile_size in the added assert.

mzumsande · 2023-03-03T05:49:27Z

It might also make sense to not allow -fastprune on anything besides regtest as well. I'm not sure if that is covered by ArgsManager::DEBUG_ONLY

Not sure if suppressing options for specific networks is something we usually do (are there any other examples for this?). But I added some more explanation to the -fastprune documentation in init with the latest push.

TheCharlatan · 2023-03-03T12:10:08Z

Not sure how I feel about this change. It is trivial to create blocks exceeding this limit on regtest. If this happens, it should at least log a message explaining why it failed. The size limit constants used by fastprune seem arbitrary to me. What is their rationale, just fewer resources allocated/used while running the pruning functional tests? If so, why are the revision files not size restricted as well?

mzumsande · 2023-03-03T16:46:00Z

Not sure how I feel about this change. It is trivial to create blocks exceeding this limit on regtest. If this happens, it should at least log a message explaining why it failed.

I'll add a log message with my next push.

The size limit constants used by fastprune seem arbitrary to me. What is their rationale, just fewer resources allocated/used while running the pruning functional tests? If so, why are the revision files not size restricted as well?

It was introduced in c286a22 (#15946) without much discussion - I'd say it's a quick-and-dirty way to allow testing the pruning code in functional tests - tests like feature_index_prune.py would probably take hours without it.

I stumbled upon this while reviewing #27125, when I had the great idea to quickly sync on signet with -fastprune to make sure that it still works the same, and after my system crashed due to running oom I thought this could be handled a bit better...

src/node/blockstorage.cpp

mzumsande · 2023-03-21T19:31:10Z

I changed the approach to @TheCharlatan's idea (added you as coauthor) - now we don't assert anymore but raise the size of the blockfile to the size of the added block if needed.

TheCharlatan · 2023-03-21T20:55:54Z

ACK e930814 fwiw :P
Thank you for following up on this @mzumsande.

pinheadmz

ACK e930814

I also wrote a test for this that freezes (infinite loop) on master but passes on this branch. Take it or leave it, I just wanted to see the mechanism work before acking:

pinheadmz@c576efa

Show Signature

pinheadmz's public key is on keybase

ryanofsky

Code review ACK e930814, but I think it would be great to add the test written by @pinheadmz #27191 (review) because the blockmanager code is fragile and a regression could happen, and because the test setup is very clean so it could probably be used to branch out and test other things.

ryanofsky · 2023-04-14T15:42:52Z

src/node/blockstorage.cpp

+        if (gArgs.GetBoolArg("-fastprune", false)) {
+            max_blockfile_size = 0x10000; // 64kiB
+            if (nAddSize >= max_blockfile_size) {
+                max_blockfile_size = nAddSize + 1;


In commit "blockstorage: Adjust fastprune limit if block exceeds blockfile size" (e930814)

What is the +1 for? Would be helpful to add a code comment saying what the extra byte is.

I checked this carefully again and don't think anymore that the + 1 is necessary.

I think it is necessary, as long as the while condition says
while (m_blockfile_info[nFile].nSize + nAddSize >= max_blockfile_size)
it won't terminate if nAddSize == max_blockfile_size, because m_blockfile_info[nFile].nSize will be 0 if we skip to the next block.

Maybe we could lose the + 1 if we'd change the >= from the while loop into a >, however I really don't want to change this critical code in this PR because it would mean we'd try to squeeze another byte into all blocks, affecting blockstorage in normal operation as well, not just the test-only -fastprune mode.
So I'll add a comment.

Makes sense the +1 is needed to be consistent with >= below. It does look like there is a minor bug here that will cause block files to be at most MAX_BLOCKFILE_SIZE-1 bytes and never reach the maximum size. But if this is a bug, it goes back to 5382bcf when this code was introduced. Agree it would not be good to change that behavior here.

The functional test covers the +1 nicely as well. Without the +1 (and removing the assertion on L632) the test will hang until timed out at 30s

ryanofsky · 2023-04-14T15:49:34Z

src/node/blockstorage.cpp

+                max_blockfile_size = nAddSize + 1;
+            }
+        }
+        while (m_blockfile_info[nFile].nSize + nAddSize >= max_blockfile_size) {


In commit "blockstorage: Adjust fastprune limit if block exceeds blockfile size" (e930814)

Would be good to add to add assert(nAddSize <= max_blockfile_size); here to make it clear what this function is assuming and catch the problem instead of going into an infinite loop if outside code changes.

done, although I think we should assert nAddSize < max_blockfile_size; - see explanation above.

ryanofsky · 2023-04-14T16:07:32Z

I think assert is the appropriate move here to prevent the infinite memory loop!

I think throwing an exception or calling abort() would be ok, but better to reserve the assert macro for conditions which are actually impossible and not use it for error handling, because that would water down the meaning of other asserts.

mzumsande · 2023-04-18T18:39:29Z

Thanks for the reviews! I pushed an update to address the outstanding comments and added the test by @pinheadmz (thanks!).

maflcko · 2023-04-19T14:50:33Z

unrelated: Red CI can be ignored, or if you care a lot, you can rebase

If the added block exceeds the blockfile size in test-only -fastprune mode, the node would get stuck in an infinite loop and run out of memory. Avoid this by raising the blockfile size to the size of the added block in this situation. Co-authored-by: TheCharlatan <seb.kung@gmail.com>

ryanofsky

Code review ACK 8f14fc8. Added new assert, test, and comment since last review

ryanofsky · 2023-05-01T17:45:43Z

src/node/blockstorage.cpp

+        if (gArgs.GetBoolArg("-fastprune", false)) {
+            max_blockfile_size = 0x10000; // 64kiB
+            if (nAddSize >= max_blockfile_size) {
+                max_blockfile_size = nAddSize + 1;


Makes sense the +1 is needed to be consistent with >= below. It does look like there is a minor bug here that will cause block files to be at most MAX_BLOCKFILE_SIZE-1 bytes and never reach the maximum size. But if this is a bug, it goes back to 5382bcf when this code was introduced. Agree it would not be good to change that behavior here.

TheCharlatan

ACK 8f14fc8

pinheadmz

ACK 8f14fc8

Show Signature

pinheadmz's public key is on keybase

maflcko · 2023-05-02T12:19:22Z

test/functional/feature_fastprune.py

+        height = self.nodes[0].getblockcount() + 1
+        block = create_block(hashprev=tip, ntime=time, txlist=[tx], coinbase=create_coinbase(height=height))
+        add_witness_commitment(block)
+        block.solve()


Any reason to make this simple test more complicated than it needs to be? See #27553

…ceeds blockfile size 8f14fc8 test: cover fastprune with excessive block size (Matthew Zipkin) 271c23e blockstorage: Adjust fastprune limit if block exceeds blockfile size (Martin Zumsande) Pull request description: The debug-only `-fastprune` option used in several tests is not always safe to use: If a `-fastprune` node receives a block larger than the maximum blockfile size of `64kb` bad things happen: The while loop in `BlockManager::FindBlockPos` never terminates, and the node runs oom because memory for `m_blockfile_info` is allocated in each iteration of the loop. The same would happen if a naive user used `-fastprune` on anything other than regtest (so this can be tested by syncing on signet for example, the first block that crashes the node is at height 2232). Change the approach by raising the blockfile size to the size of the block, if that block otherwise wouldn't fit (idea by TheCharlatan). ACKs for top commit: ryanofsky: Code review ACK 8f14fc8. Added new assert, test, and comment since last review TheCharlatan: ACK 8f14fc8 pinheadmz: ACK 8f14fc8 Tree-SHA512: df2fea30613ef9d40ebbc2416eacb574f6d7d96847db5c33dda22a29a2c61a8db831aa9552734ea4477e097f253dbcb6dcb1395d43d2a090cc0588c9ce66eac3

Summary: If the added block exceeds the blockfile size in test-only -fastprune mode, the node would get stuck in an infinite loop and run out of memory. Avoid this by raising the blockfile size to the size of the added block in this situation. Co-authored-by: TheCharlatan <seb.kung@gmail.com> This is a backport of [[bitcoin/bitcoin#27191 | core#27191]] Depends on D16028 Note that this fix does not address the non-test issue of blocks larger than 128 MiB. But at least now we get a meaningful assertion failure rather than an OOM crash. Test Plan: `ninja all check-all` Check that without the change in blockstorage.cpp the new test slowly consumes all the memory and the test eventually fails. Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D16023

DrahtBot added the Block storage label Mar 2, 2023

pinheadmz reviewed Mar 2, 2023

View reviewed changes

DrahtBot mentioned this pull request Mar 3, 2023

refactor, kernel: Decouple ArgsManager from blockstorage #27125

Merged

mzumsande force-pushed the 202302_fastprune_oom branch from fe6d866 to d526dbb Compare March 3, 2023 05:42

DrahtBot mentioned this pull request Mar 3, 2023

blockstorage: do not flush block to disk if it is already there #27039

Merged

TheCharlatan reviewed Mar 4, 2023

View reviewed changes

src/node/blockstorage.cpp Outdated Show resolved Hide resolved

mzumsande force-pushed the 202302_fastprune_oom branch from d526dbb to e930814 Compare March 21, 2023 19:29

mzumsande changed the title ~~blockstorage: add an assert to avoid running oom with -fastprune~~ blockstorage: Adjust fastprune limit if block exceeds blockfile size Mar 21, 2023

pinheadmz approved these changes Mar 22, 2023

View reviewed changes

fanquake requested a review from ryanofsky March 22, 2023 18:01

ryanofsky approved these changes Apr 14, 2023

View reviewed changes

mzumsande force-pushed the 202302_fastprune_oom branch from e930814 to 19e9073 Compare April 18, 2023 18:37

mzumsande force-pushed the 202302_fastprune_oom branch from 19e9073 to af16fc9 Compare April 19, 2023 14:17

DrahtBot added the CI failed label Apr 19, 2023

mzumsande and others added 2 commits April 19, 2023 11:25

test: cover fastprune with excessive block size

8f14fc8

mzumsande force-pushed the 202302_fastprune_oom branch from af16fc9 to 8f14fc8 Compare April 19, 2023 15:25

ryanofsky approved these changes May 1, 2023

View reviewed changes

DrahtBot requested review from TheCharlatan and pinheadmz May 1, 2023 17:49

TheCharlatan approved these changes May 1, 2023

View reviewed changes

pinheadmz approved these changes May 1, 2023

View reviewed changes

fanquake merged commit 8a373a5 into bitcoin:master May 2, 2023

mzumsande deleted the 202302_fastprune_oom branch May 2, 2023 11:54

maflcko reviewed May 2, 2023

View reviewed changes

bitcoin locked and limited conversation to collaborators May 1, 2024

blockstorage: Adjust fastprune limit if block exceeds blockfile size #27191

blockstorage: Adjust fastprune limit if block exceeds blockfile size #27191

Uh oh!

Conversation

mzumsande commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 2, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzumsande commented Mar 3, 2023

Uh oh!

TheCharlatan commented Mar 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mzumsande commented Mar 3, 2023

Uh oh!

Uh oh!

mzumsande commented Mar 21, 2023

Uh oh!

TheCharlatan commented Mar 21, 2023

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Apr 14, 2023

Uh oh!

mzumsande commented Apr 18, 2023

Uh oh!

maflcko commented Apr 19, 2023

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheCharlatan left a comment

Choose a reason for hiding this comment

Uh oh!

pinheadmz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

mzumsande commented Mar 2, 2023 •

edited

Loading

DrahtBot commented Mar 2, 2023 •

edited

Loading

TheCharlatan commented Mar 3, 2023 •

edited

Loading