-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Drop -dbcache limit #28358
base: master
Are you sure you want to change the base?
Drop -dbcache limit #28358
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
4e27771
to
f311e45
Compare
Can you explain this? Why is there a requirement that the whole utxo set must fit in the cache? This can easily lead to OOM, as you explain, in which case the user would have to start from zero, because potentially nothing has been persisted at all. Also, for slow storage it could lead to hangs, which could lead the user (or OS) to kill the process, causing the same issues. Finally, with assumeutxo, I wonder why there is any need to chase this? No objection to this pull, but I wonder if this really makes users happier on average, or will just lead to more hangs, crashes and |
doc/release-notes-25358.md
Outdated
Node | ||
------ | ||
|
||
- The maximum value for `-dbcache` has been increased to 32,000 MiB due to recent UTXO set growth. Be sure to check that you have enough RAM before increasing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of implying that users should check their RAM and then increase this, it would be more helpful to explain why a user should change the setting in the first place, what they can expect, and what the tradeoffs are, but this seems better put in a separate document or the -help
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was not the intended implication. If they feel like increasing it, they should check their RAM first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is just that this is the only thing you mention in the release notes. In any case, I don't think the release notes are the right place for extended documentation. Either put it in the -help
or a separate md.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dropped the "check RAM" sentence.
Wonder if it's worth warning/failing if the dbcache param exceeds available memory. |
My recollection is that an IBD without a single flush is faster, but have not benched this in recent years. We can always recommend a lower setting if that turns out to be better. By not increasing it to infinity at least we constrain the worst case here, e.g. if it turns out the failures you mean get exponentially worse as the UTXO set keeps growing. While at the same time not removing the existing ability to sync in one go. |
My understanding was that we can't measure available RAM reliably. |
A single flush throughout the entire multi-hour IBD will not make a noticeable difference to a user. My initial version of #28280 had 3 extra pointers per cache entry, which caused the |
Concept ACK. This does not change defaults, just gives more options to power users. And UTXO set will grow in future. For some server environments 16 GB more or less will mean nothing. |
Concept ACK. |
1 similar comment
Concept ACK. |
Concept ACK. Except for:
This breaks scripts that set dbcache to a % of available RAM. And is inconsistent with it rounding up silently rather than fail to start if the value is too low. |
Concept ACK on increasing the maximum I had to manually bump the dbcache to run |
Concept ACK on giving more options. |
Do you know any example of people doing that? In general I think we should fail to startup if the user provides an invalid configuration option, as this often leads to problems. I believe this has been gradually introduced for various other options too (see also #16545). At the same time I don't want to needless break scripts out there. From my own experience I was very surprised to find that this rounding down behavior happened, which is which I prefer an error. But we could also print a warning in the debug log.
@sipa any thoughts on a sane maximum? |
My project Bails puts this in It's an alpha with about 20 users, makes a portable USB stick node that keeps Bitcoin Core up to date, none probably own a 64GB computer, but it will break if they do. I found a couple other projects with the idea: They'll waste time if they miss the change notes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK
In general I think we should fail to startup if the user provides an invalid configuration option, as this often leads to problems. I believe this has been gradually introduced for various other options too (see also #16545).
#27632 was another recent example of switching from a warning to raising an error. It's too easy for users, even experienced ones, not to notice a warning line in the log.
src/init.cpp
Outdated
@@ -446,7 +446,7 @@ void SetupServerArgs(ArgsManager& argsman) | |||
argsman.AddArg("-conf=<file>", strprintf("Specify path to read-only configuration file. Relative paths will be prefixed by datadir location (only useable from command line, not configuration file) (default: %s)", BITCOIN_CONF_FILENAME), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
argsman.AddArg("-datadir=<dir>", "Specify data directory", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
argsman.AddArg("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::OPTIONS); | |||
argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |||
argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). Make sure you have enough RAM. In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While touching this help, I think the following would be much clearer (particularly s/for/with
).
argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). Make sure you have enough RAM. In addition, unused mempool memory is shared for this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); | |
argsman.AddArg("-dbcache=<n>", strprintf("Maximum database cache size <n> MiB (%d to %d, default: %d). Make sure you have enough RAM. In addition, unused memory allocated to the mempool is shared with this cache (see -maxmempool).", nMinDbCache, nMaxDbCache, nDefaultDbCache), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken
src/init.cpp
Outdated
@@ -925,6 +925,10 @@ bool AppInitParameterInteraction(const ArgsManager& args) | |||
return InitError(Untranslated("Cannot set -listen=0 together with -listenonion=1")); | |||
} | |||
|
|||
if (args.GetIntArg("-dbcache", nDefaultDbCache) > nMaxDbCache) { | |||
return InitError(Untranslated(strprintf("-dbcache must be at most %d MiB", nMaxDbCache))); | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could add a test.
Edit: maybe -dbcache configuration option can be at most %d MiB, but %d was passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used the text, added a test.
doc/release-notes-25358.md
Outdated
Node | ||
------ | ||
|
||
- The maximum value for `-dbcache` has been increased to 32,000 MiB due to recent UTXO set growth. Be sure to check that you have enough RAM before increasing this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The maximum value for `-dbcache` has been increased to 32,000 MiB due to recent UTXO set growth. Be sure to check that you have enough RAM before increasing this. | |
- The maximum allowed value for the `-dbcache` configuration option has been increased to 32,000 MiB due to recent UTXO set growth. Be sure to check that you have enough RAM before increasing this. (#28358) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I might also add something like "Even with sufficient RAM the maximum setting may not be optimal."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes -- and why, or link to an improved In-memory caches
section in doc/reduce-memory.md
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Taken the suggested text diff.
doc/release-notes-25358.md
Outdated
@@ -0,0 +1,5 @@ | |||
Node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe (I'm not sure this matters).
Node | |
Updated settings |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Release notes get edited later anyway, but I'll change once I retouch.
(waiting for more inputs in the discussion on warning/failure and the max size)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
Perhaps it should be set so high that you can't easily buy a Desktop with more RAM than the limit. I could go to Microcenter today and walk out with a 64GB gaming tower, or a Xeon workstation with 128GB, likely a 64GB laptop as well. Lots of guides on internet (foolishly) suggest setting it to half the total RAM. |
src/init.cpp
Outdated
@@ -925,6 +925,10 @@ bool AppInitParameterInteraction(const ArgsManager& args) | |||
return InitError(Untranslated("Cannot set -listen=0 together with -listenonion=1")); | |||
} | |||
|
|||
if (args.GetIntArg("-dbcache", nDefaultDbCache) > nMaxDbCache) { | |||
return InitError(Untranslated(strprintf("-dbcache must be at most %d MiB", nMaxDbCache))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this will break software out there, why not simply keep the previous behavior and leave it as-is? What problem are you trying to solve?
Printing a warning and/or updating the docs should be more than enough.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That indeed seems like the best approach. Silently rounding down is annoying, but doesn't break anything, so a warning should be fine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OTOH, it seems that raising would be a clearer user interface than rounding down silently as now, or with an added warning. See https://en.wikipedia.org/wiki/Principle_of_least_astonishment. I don't want to break anything, but the only software mentioned so far that this might marginally affect is that of @BenWestgate (thank you for reporting), who would be aware of the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaving this error as is for now, unless there are other examples of breaking software.
I would also rather drop nMaxDbCache
entirely than quietly enforce it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See https://en.wikipedia.org/wiki/Principle_of_least_astonishment
If I imagined myself running into this error. I'd be indeed astonished and ask myself why the dbcache is capped at all. Again, if there is a reason for this change, it should be explained. It can't hurt to put the reason in the error string.
Why do we have an upper limit at all?
#19873 does exactly that |
src/txdb.h
Outdated
@@ -38,7 +38,7 @@ static const int64_t nDefaultDbCache = 450; | |||
//! -dbbatchsize default (bytes) | |||
static const int64_t nDefaultDbBatchSize = 16 << 20; | |||
//! max. -dbcache (MiB) | |||
static const int64_t nMaxDbCache = sizeof(void*) > 4 ? 16384 : 1024; | |||
static const int64_t nMaxDbCache = sizeof(void*) > 4 ? 32768 : 1024; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might as well make it constexpr
while you're touching it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
f311e45
to
1e10a82
Compare
Rebased, incorporated most of the text feedback above. Dropped the RAM warning from the release note, since that's already in the help text.
@sipa seemed worried about users going overboard with this setting: #28249 (comment) |
Why would the value be "invalid"? |
If there is a limit, a value above the limit is invalid. That is the entire point of a limit. Which is why I asked if you prefer to drop the limit. |
I don't know if it is safe to drop the limit. For example, one could run into a signed integer overflow for some values. If that is the reason for the limit, you should add a comment for it. However, if there is no reason for the limit, I don't see why a limit should exist. |
39a3d48
to
5f31349
Compare
@@ -94,8 +94,7 @@ OptionsDialog::OptionsDialog(QWidget* parent, bool enableWallet) | |||
ui->setupUi(this); | |||
|
|||
/* Main elements init */ | |||
ui->databaseCache->setMinimum(nMinDbCache); | |||
ui->databaseCache->setMaximum(nMaxDbCache); | |||
ui->databaseCache->setRange(nMinDbCache, std::numeric_limits<int>::max()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even on a 32 bit system the maximum would be 2 petabyte (2 billion MiB), so not a practical concern imo.
On 32-bit systems we shouldn't raise the cache size too close to 4 GiB or more, as it'll just result in memory allocation errors. On 64-bit systems, the limit is much less necessary, but I'd still worry about wildly too large numbers, as they also affect leveldb cache sizes. Setting those (way) too large is probably counterproductive. |
@sipa since 32 bit systems can't have more than 4 GiB of RAM, the "Make sure you have enough RAM" wording in the documentation should cover that. Though perhaps "Stay at least 1 GiB below your RAM size" may be safer, also because of swap behaviour (see e.g. #29603 (comment)) With something like #19873 we could warn the use if they pick a value too close to the limit (on both 32 and 64 bit systems).
We could suggest an optimum, but without further experimentation we don't know where that is. Ideally we would measure performance in real time and flush when it starts degrading (unless the user really insists that we don't). |
Built 5f31349 and ran bitcoin-qt because I was worried that setting a very high max value might cause issues with the QT UI control. It's not some kind of horizontal slider that goes crazy, but rather a textbox with up/down arrows. It ignores key inputs that would make the value go further than |
Will do if I need to retouch. |
After #28233 keeping the entire utxo set in memory will also help connect new blocks as fast as possible, not just for IBD. |
5f31349
to
2a71819
Compare
Rebased to make it easier to test the interaction with (merged) #28233. However, any potential benefit goes away after the first restart. The only time -dbcache is filled beyond its previous 16GB limit is during IDB. In order to benefit from having the full UTXO set in RAM after a restart, we'd have to proactively load the entire UTXO set into -dbcache if space permits. Or maybe fill half the available space with coins, picked randomly. Added "Make sure you have enough RAM." warning to the GUI. |
I'm sure it will happen if you restart your node now and leave it running for long enough ;)
I did make a crude attempt at that years ago #18941. Not sure it is worth it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crACK
doc/release-notes-25358.md
Outdated
------ | ||
|
||
- The maximum value for `-dbcache` has been increased to 32,000 MiB due to recent UTXO set growth. Be sure to check that you have enough RAM before increasing this. | ||
- Setting `-dbcache` above the maximum causes the node to abort startup. Previously it would round the number down. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer it quietly round down to the maximum. Especially since it rounds up to the minimum.
I know a few scripts people use that set dbcache to a percentage of available memory that this part of the PR would break for 64GB RAM.
What problem is failing to start meant to solve?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
I don't see much of a philosophical difference between having no limit and having a limit of 16GB on systems where a user has less than 16GB actually available... If we're already requiring users to do something smart, we can keep doing that -- and if we are worried about someone using an inappropriate value we can try to solve that separately (maybe by testing that we can allocate that much at startup or something?).
The best would be never setting it greater than total RAM. Since other reviewers mentioned only wildly excessive values are seriously detrimental with swap, capping to max RAM is sensible.
Some have used
That's barely safe without swap, Bitcoin-QT and the OS will use most of the 1 GiB offset leaving it pretty close to locking up. Without a swap file, an offset from total ram (1-2 GiB?) may avoid OOM, but this becomes arbitrary and may limit performance if set too low. It's better users w/o swap who set it too high OOM, than to limit performance of systems w/ swap that may benefit from more. So total memory is still the best limit, if any is enforced.
I've done a lot of tests on 4 GB and 8 GB RAM machines with zram swap and the -datadir on USB sticks and With Suggesting |
2a71819
to
bb3b980
Compare
@BenWestgate thanks for the analysis. If we want to add a recommendation to the help text, I suggest we do that in another PR though. It's probably going to take multiple people doing benchmark on wildly different machines. Checking against RAM can also be done in a followup. I rebased and changed the .md file name. |
Due to recent UTXO set growth, the current maximum value for
-dbcache
of 16GB isjust months away from beinginsufficient (for those who wish to complete IBD with the UTXO set held in RAM).This drops the limit. It also adds a warning that it's up to users to check that they have enough RAM.
Fixes #28249.
A previous version of this PR increased the maximum to 64GB. It also made startup abort if the value provided is too high, rather than quietly round it down. But this didn't get much support.