Reduce default mempool size in -blocksonly mode #26471

willcl-ark · 2022-11-08T15:22:52Z

When -blocksonly has been set reduce default mempool size to avoid surprising resource usage via sharing un-used mempool cache space with dbcache.

In comparison to #9569 which either set maxmempool size to 0 when -blocksonly was set or else errored on startup, this change will permit maxmempool options being set.

This preserves the current (surprising?) behaviour of having a functional mempool in -blocksonly mode, to permit whitelisted peer transaction relay, whilst reducing average runtime memory usage for blocksonly nodes which either use the default settings or have otherwise configured a maxmempool size.

To use the previous old defaults node operators can configure their node with: -blocksonly -maxmempool=300.

stickies-v

It took me a surprisingly long time to figure out the rationale here. I'll write down my understanding both to have it validated/corrected, as well as hopefully to inform other people looking at this.

My understanding:

Users running with -blocksonly expect to have a near zero-sized mempool, because it'll only contain transactions broadcasted by their own wallet (disabled by default) or transactions received from whitelisted peers (non-default). As such, when provisioning their system, a user would expect to only have to allocate dbcache amount of memory, instead of currently actually requiring dbcache + maxmempool, which could lead to stability issues.

Without -blocksonly, a user needs to ensure that his system is provisioned to support both the full dbcache and mempool. Since it's provisioned anyway, we might as well use the unused mempool space for dbcache, which is especially helpful during IBD.

The misunderstanding for me was that the resource sharing between dbcache and maxmempool is not a resource optimization (in which case "we would benefit even more with -blocksonly, so why disable the optimization?!") but rather an accounting trick. Users running in -blocksonly should just increase their dbcache by whatever size maxmempool is for a similar effect (but not entirely, as explained below)

If my understanding is correct, I'm Concept ACK (i.e. making resource usage less surprising) but not yet Approach ACK, because:

this would be a performance degradation for users running -blocksonly that don't realize they should update their -dbcache. Users running -blocksonly could get higher default dbcache defaults, but that seems messy too.
users can't really replicate current behaviour even if they're aware of the change. The intuitive solution would be to increase -dbcache by -maxmempool, however, dbcache is shared by multiple caches, of which the coins cache receives 25-50%. It seems there's no way to allocate an extra 300MB (assuming default) to just the coins cache without increasing the dbcache by a total of 600MB - 1.2GB?

src/validation.cpp

src/init.cpp

willcl-ark · 2022-11-11T11:16:26Z

Thanks for your review at this conceptual stage @stickies-v. Your understanding of the problem and solution matches my own, although I had not fully considered the knock-on effect that:

users can't really replicate current behaviour even if they're aware of the change ... without increasing the dbcache by a total of 600MB - 1.2GB?

I also concur with you that altering default dbcache size for users who enable blocksonly seems kind of messy, although it could be made to work.

Therefore I see x options available from here:

Keep current behaviour, document it better, and close -blocksonly should disable sharing of mempool with dbcache #9526 as wontfix
Implement (something like) this change which introduces the performance reduction in IBD for blocksonly nodes and document it
Add special-casing for dbcache size when nodes are run in blocksonly mode.

In writing the options out like that, my first preference might actually be 1), followed by 3) and last 2).

stickies-v · 2022-11-11T12:47:06Z

Therefore I see x options available from here:

I think I'm not in favour of 1 & 3. Imo the direction should always be to reduce exceptional behaviour, if the short-term cost of that change is not exorbitant. I think in this case, the cost (potentially slower IBD which can be fixed with user config) is not exorbitant and increased stability (because of reduction of unexpected behaviour) is a worthwhile improvement.

I can think of two other alternatives, both building on top of what you've currently implemented:

add a -increase-coins-cache=<n> parameter to allow users keeping the current behaviour (and maybe even set it to 300 by default if -blocksonly in the first release to give users time to transition, and then change it to 0 in the release after. Pretty user-friendly/straightforward option, but it introduces another kinda one-off parameter.
Allowing users to manually define sub-cache allocations (overriding the defaults), e.g. through -dbcache.coins=<n|f> where n is an absolute amount and f a 0-1 float indicating a percentage of total dbcache. This seems like the most transparent and generic solution, but probably at the cost of reduced user-friendliness. It would also be a significantly bigger code change introducing the new parameters as well as a bunch of validation, and allowing the auto-allocation to deal with partial specification (e.g. only coins and tx_index are specified but not the other ones)

willcl-ark · 2022-11-11T13:38:26Z

Yes 4) is a possible solution.

IMO by the time we hit 5), we're probably almost talking about people who have such an understanding of the system (developers) that they can most likely make the change to the source themselves before compiling if they truely need such fine-grained settings changes.

Will leave it as is to see if we get any other suggestions, and mull it over myself...

luke-jr · 2022-11-14T23:13:54Z

The intuitive solution would be to increase -dbcache by -maxmempool, however, dbcache is shared by multiple caches, of which the coins cache receives 25-50%. It seems there's no way to allocate an extra 300MB (assuming default) to just the coins cache without increasing the dbcache by a total of 600MB - 1.2GB?

FWIW, I have a related thought process for Knots, where it makes sense to leave the coins cache unlimited (and let memory pressure trigger flushing), yet it isn't sane to just increase -dbcache to the system memory size, because that would also increase other cache sizes which aren't handled when under memory pressure.

Therefore I see x options available from here:

support specifying "+mempool" on the end of the -dbcache option to get the current behaviour even in blocksonly mode.

glozow · 2023-01-12T17:01:27Z

It seems the key issue is "surprising memory usage," i.e. -dbcache=n -blocksonly can use n+300 while the user may have expected n. The trick of using -blocksonly to add mempool's memory to the coins cache seems quite esoteric (lmk if I'm mistaken), so preserving it would be a bit less important imo.

the current (surprising) behaviour of having a functional mempool in -blocksonly mode

Given how surprising this is to so many people (including those affected by this problem), I can see how disabling mempool in blocksonly would be the easiest option. But can also see how it's not acceptable for users that rely on the current whitelist relay behavior according to #9569.

Perhaps setting a very small mempool m_max_size_bytes when in blocksonly mode unless overridden by -maxmempool, would be one solution? That seems to preserve the trick for those using it intentionally, while reducing the amount of "surprising memory usage" for those that assume 0 memory for -blocksonly. Those who benefit from the extra 300MB by accident can perhaps get their answer from the release notes if they see a performance reduction. If they want to continue doing what they were doing, they just need to set -maxmempool.

DrahtBot · 2023-01-12T17:01:29Z

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	ajtowns
Stale ACK	aureleoules
Ignored review	stickies-v

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

willcl-ark · 2023-01-13T19:54:13Z

Perhaps setting a very small mempool m_max_size_bytes when in blocksonly mode unless overridden by -maxmempool, would be one solution? That seems to preserve the trick for those using it intentionally, while reducing the amount of "surprising memory usage" for those that assume 0 memory for -blocksonly. Those who benefit from the extra 300MB by accident can perhaps get their answer from the release notes if they see a performance reduction. If they want to continue doing what they were doing, they just need to set -maxmempool.

I think this is a neat solution which covers all requirements so implemented it in this second branch (it's also much cleaner implementation-wise).

The only question I had was how small is "smaller"? I opted for 5MB in blocksonly mode as this should be enough for quite a few transactions to be relayed for whitelisted peers, I think...

If this is preferred I will move it over to this branch...?

glozow · 2023-01-16T15:05:35Z

The only question I had was how small is "smaller"? I opted for 5MB in blocksonly mode as this should be enough for quite a few transactions to be relayed for whitelisted peers, I think...

That definitely seems sufficient, and they can still make it higher if they need to. 5MB is the minimum -maxmempool config allowed as well, as the smaller the mempool size, the easier it is for your mempool min feerate to get driven up really high. Thinking out loud I guess this isn't as much of a concern for -blocksonly since you won't consider any transactions from strangers, so maybe smaller would be okay... but I can't say a different number with confidence.

If this is preferred I will move it over to this branch...?

I think it's a relatively simple change to lessen this problem with minimal side effects? 🤷 though I don't know if we could say we've completely fixed it. fwiw I think it's sensible regardless since we're probably unlikely to need 300MB in blocksonly mode.

willcl-ark · 2023-01-17T09:12:24Z

OK I have moved the commits across, and updated the PR title and description. I also added a new release note commit which I thought was worth it, but happy to be persuaded otherwise.

src/init.cpp

doc/reduce-memory.md

src/init.cpp

doc/reduce-memory.md

aureleoules

ACK 931924f5cc1dd9c82306a0ce59b62cf5c9276565
Tested with -blocksonly and -blocksonly -maxmempool=n and verified the logs, seems to work as intended.

(Please avoid rebasing when there is no need.)

src/init.cpp

stickies-v

Approach ACK 931924f5cc1dd9c82306a0ce59b62cf5c9276565, except for setting mempool_opts.max_size_bytes in two different locations and times.

I think this approach tackles a nice balance between addressing the most important issue (node crashing because of unexpected resource usage), reducing additional code overhead (compared to the other approaches considered) and allowing users to maintain existing performance profile without too much admin overhead. The biggest (but manageable) downside is that everyone that upgrades and doesn't manually override -maxmempool will silently experience a performance degradation, especially during IBD. I expect this performance degradation to be manageable, especially since most users that have the resources available, will probably have manually increased their dbcache anyway for IBD, lowering the relative impact of this change.

doc/reduce-memory.md

doc/release-notes-26471.md

src/init.cpp

aureleoules

reACK ed5ce6d0c911ac66bd5870a1711edef03a18ebeb - Wording updated and logic cleaned up since my last review.

stickies-v

ACK ed5ce6d0c

src/kernel/mempool_options.h

doc/release-notes-26471.md

ajtowns

Approach ACK

doc/reduce-memory.md

doc/release-notes-26471.md

src/node/mempool_args.cpp

src/kernel/mempool_options.h

src/node/mempool_args.cpp

doc/reduce-memory.md

doc/release-notes-26471.md

When -blockonly is set, reduce mempool size to 5MB unless -maxmempool is also set. See bitcoin#9569

Changes to the default mempool allocation size now documented. Provides users with guidance on the mempool implications of -blocksonly mode, along with instructions on how to re-enable old behaviour.

Adds a release note detailing the new mempool sizing behaviour when running in blocksonly mode, and instruction on how to override the new defaults.

willcl-ark · 2023-01-20T16:13:16Z

Thanks for the comments, I've updated the branch taking them into consideration.

ajtowns

ACK 8e85164

stickies-v

re-ACK 8e85164

fanquake · 2023-01-22T14:57:04Z

src/kernel/mempool_options.h


 /** Default for -maxmempool, maximum megabytes of mempool memory usage */
 static constexpr unsigned int DEFAULT_MAX_MEMPOOL_SIZE_MB{300};
+//** Default for -maxmempool when blocksonly is set */


Suggested change

//** Default for -maxmempool when blocksonly is set */

/** Default for -maxmempool when blocksonly is set */

maflcko · 2023-01-23T09:09:23Z

doc/reduce-memory.md

+- Since `0.14.0`, unused memory allocated to the mempool (default: 300MB) is shared with the UTXO cache, so when trying to reduce memory usage you should limit the mempool, with the `-maxmempool` command line argument.

-  - Do not use this when using the client to broadcast transactions as any transaction sent will stick out like a sore thumb, affecting privacy. When used with the wallet it should be combined with `-walletbroadcast=0` and `-spendzeroconfchange=0`. Another mechanism for broadcasting outgoing transactions (if any) should be used.
+- To disable most of the mempool functionality there is the `-blocksonly` option. This will reduce the default memory usage to 5MB and make the client opt out of receiving (and thus relaying) transactions, except from whitelisted peers and as part of blocks.


nit: Instead of whitelisted, it would be good to mention the exact permission flag.

8e85164 doc: release note on mempool size in -blocksonly (willcl-ark) ae79746 doc: Update blocksonly behaviour in reduce-memory (willcl-ark) 1134686 mempool: Don't share mempool with dbcache in blocksonly (willcl-ark) Pull request description: Fixes bitcoin#9526 When `-blocksonly` has been set reduce default mempool size to avoid surprising resource usage via sharing un-used mempool cache space with dbcache. In comparison to bitcoin#9569 which either set `maxmempool` size to 0 when `-blocksonly` was set or else errored on startup, this change will permit `maxmempool` options being set. This preserves the current (surprising?) behaviour of having a functional mempool in `-blocksonly` mode, to permit whitelisted peer transaction relay, whilst reducing average runtime memory usage for blocksonly nodes which either use the default settings or have otherwise configured a `maxmempool` size. To use the previous old defaults node operators can configure their node with: `-blocksonly -maxmempool=300`. ACKs for top commit: ajtowns: ACK 8e85164 stickies-v: re-ACK bitcoin@8e85164 Tree-SHA512: 1c461c24b6f14ba02cfe4e2cde60dc629e47485db5701bca3003b8df79e3aa311c0c967979f6a1dca3ba69f5b1e45fa2db6ff83352fdf2d4349d5f8d120e740d

glozow · 2023-01-30T09:37:43Z

@willcl-ark will you be taking the followups #26471 (comment) and #26471 (comment)?

willcl-ark · 2023-01-30T09:39:02Z

Yes I can do those @glozow

47c174d doc: NetPermissionFlags for tx relay in blocksonly (willcl-ark) e325e0f doc: Fix comment syntax error (willcl-ark) Pull request description: Fix syntax error and specify `NetPermissionFlags` for whitelisted tx relay ACKs for top commit: w0xlt: ACK 47c174d Tree-SHA512: eb579dc599a96a3ea79c01ac3e76160ec59cf71c2486c9401da8fbbd96ae756ba647aa9ba874835946bc76ba02782729da788617f982ae5a852139e10e7dfd75

47c174d doc: NetPermissionFlags for tx relay in blocksonly (willcl-ark) e325e0f doc: Fix comment syntax error (willcl-ark) Pull request description: Fix syntax error and specify `NetPermissionFlags` for whitelisted tx relay ACKs for top commit: w0xlt: ACK bitcoin@47c174d Tree-SHA512: eb579dc599a96a3ea79c01ac3e76160ec59cf71c2486c9401da8fbbd96ae756ba647aa9ba874835946bc76ba02782729da788617f982ae5a852139e10e7dfd75

glozow added the Resource usage label Nov 10, 2022

stickies-v reviewed Nov 10, 2022

View reviewed changes

src/validation.cpp Outdated Show resolved Hide resolved

src/validation.cpp Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

willcl-ark force-pushed the blocksonly_mempool_cache branch 2 times, most recently from ab64a8a to dd43259 Compare January 16, 2023 21:32

willcl-ark changed the title ~~Don't share mempool with dbcache in -blocksonly mode~~ Reduce default mempool size in -blocksonly mode Jan 17, 2023

willcl-ark marked this pull request as ready for review January 17, 2023 09:09

aureleoules reviewed Jan 17, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

doc/reduce-memory.md Outdated Show resolved Hide resolved

src/init.cpp Outdated Show resolved Hide resolved

doc/reduce-memory.md Outdated Show resolved Hide resolved

willcl-ark force-pushed the blocksonly_mempool_cache branch from dd43259 to 931924f Compare January 17, 2023 10:51

aureleoules approved these changes Jan 17, 2023

View reviewed changes

src/init.cpp Outdated Show resolved Hide resolved

stickies-v reviewed Jan 17, 2023

View reviewed changes

willcl-ark force-pushed the blocksonly_mempool_cache branch 2 times, most recently from 2789dcd to ed5ce6d Compare January 17, 2023 15:31

glozow added the Mempool label Jan 17, 2023

aureleoules approved these changes Jan 19, 2023

View reviewed changes

glozow mentioned this pull request Jan 19, 2023

refactor: Make m_mempool optional in PeerManager #26247

Closed

stickies-v approved these changes Jan 19, 2023

View reviewed changes

src/kernel/mempool_options.h Outdated Show resolved Hide resolved

doc/release-notes-26471.md Outdated Show resolved Hide resolved

ajtowns reviewed Jan 19, 2023

View reviewed changes

doc/reduce-memory.md Outdated Show resolved Hide resolved

doc/release-notes-26471.md Outdated Show resolved Hide resolved

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

glozow reviewed Jan 20, 2023

View reviewed changes

willcl-ark added 2 commits January 20, 2023 12:53

mempool: Don't share mempool with dbcache in blocksonly

1134686

When -blockonly is set, reduce mempool size to 5MB unless -maxmempool is also set. See bitcoin#9569

doc: Update blocksonly behaviour in reduce-memory

ae79746

Changes to the default mempool allocation size now documented. Provides users with guidance on the mempool implications of -blocksonly mode, along with instructions on how to re-enable old behaviour.

willcl-ark force-pushed the blocksonly_mempool_cache branch from ed5ce6d to dceef81 Compare January 20, 2023 13:31

doc: release note on mempool size in -blocksonly

8e85164

Adds a release note detailing the new mempool sizing behaviour when running in blocksonly mode, and instruction on how to override the new defaults.

willcl-ark force-pushed the blocksonly_mempool_cache branch from dceef81 to 8e85164 Compare January 20, 2023 13:54

ajtowns approved these changes Jan 21, 2023

View reviewed changes

stickies-v approved these changes Jan 22, 2023

View reviewed changes

fanquake reviewed Jan 22, 2023

View reviewed changes

fanquake merged commit ad09b76 into bitcoin:master Jan 22, 2023

fanquake added this to the 25.0 milestone Jan 22, 2023

willcl-ark deleted the blocksonly_mempool_cache branch January 22, 2023 20:52

maflcko reviewed Jan 23, 2023

View reviewed changes

bitcoin locked and limited conversation to collaborators Jan 30, 2024

	//** Default for -maxmempool when blocksonly is set */
	/** Default for -maxmempool when blocksonly is set */

Reduce default mempool size in -blocksonly mode #26471

Reduce default mempool size in -blocksonly mode #26471

Uh oh!

Conversation

willcl-ark commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willcl-ark commented Nov 11, 2022

Uh oh!

stickies-v commented Nov 11, 2022

Uh oh!

willcl-ark commented Nov 11, 2022

Uh oh!

luke-jr commented Nov 14, 2022

Uh oh!

glozow commented Jan 12, 2023

Uh oh!

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

Uh oh!

Reviews

Uh oh!

willcl-ark commented Jan 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow commented Jan 16, 2023

Uh oh!

willcl-ark commented Jan 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

stickies-v left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aureleoules left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stickies-v left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

willcl-ark commented Jan 20, 2023

Uh oh!

ajtowns left a comment

Choose a reason for hiding this comment

Uh oh!

willcl-ark commented Nov 8, 2022 •

edited

Loading

stickies-v left a comment •

edited

Loading

DrahtBot commented Jan 12, 2023 •

edited

Loading

willcl-ark commented Jan 13, 2023 •

edited

Loading

stickies-v left a comment •

edited

Loading

aureleoules left a comment •

edited

Loading