Enable --Xbonsai-limit-trie-logs-enabled by default #7181

siladu · 2024-06-05T16:54:59Z

PR description

Enable --Xbonsai-limit-trie-logs-enabled by default, unless sync-mode=FULL.
More info: https://wiki.hyperledger.org/display/BESU/Limit+Trie+Logs+for+Bonsai

When sync-mode=FULL and data-storage-format=BONSAI you get a config validation error at startup...

besu --sync-mode=FULL --data-storage-format=BONSAI
2024-06-06 17:36:12.806+01:00 | main | INFO  | Besu | Starting Besu
2024-06-06 17:36:13.027+01:00 | main | ERROR | Besu | Failed to start Besu
...
Cannot enable --Xbonsai-limit-trie-logs-enabled with sync mode FULL

I will promote to a Stable option in a separate PR.

This option has been tested on our canaries and by the community since https://github.com/hyperledger/besu/releases/tag/24.3.0

Added a fallback value for the option otherwise specifying --Xbonsai-limit-trie-logs-enabled on the command line, without a value, toggles it back off again.

The minimalist-profile has been updated from bonsai-historical-block-limit=128 to bonsai-historical-block-limit=512 to avoid further code changes and because I believe there is only a ~60MB difference so using 128 has limited value, see #6560 (comment)

I will conservatively set Xbonsai-limit-trie-logs-enabled=false in the enterprise profile for now as not sure it is desirable.

Fixed Issue(s)

Fixes #6560

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

jframe · 2024-06-06T03:47:33Z

besu/src/main/java/org/hyperledger/besu/cli/options/stable/DataStorageOptions.java

@@ -86,6 +86,8 @@ public static class Unstable {
    @CommandLine.Option(
        hidden = true,
        names = {BONSAI_LIMIT_TRIE_LOGS_ENABLED, "--Xbonsai-trie-log-pruning-enabled"},
+        defaultValue = "true",


Should this be enabled by default for all profiles? In the The enterprise-private profile probably makes more sense to not prune I think. @non-fungible-nelson What do you think?

Probably not. I thought it might be alright initially since it's scoped to BONSAI, but I know we have a follow-on story related to profiles somewhere...I will check if we need to play that at the same time.

This is relevant #6560

I think this may have been the PR I was thinking about, which doesn't actually touch the profiles directly but had some impact on them #6561

I disabled this in enterprise-profile 👍 - side note: was surprised to find enterprise was defaulting to BONSAI, addressed that here: #7186

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

…ogs-enabled-by-default Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

fab-10

LGTM, just a couple of nits

fab-10 · 2024-06-06T15:19:57Z

besu/src/test/java/org/hyperledger/besu/cli/BesuCommandTest.java

@@ -1244,8 +1244,13 @@ public void ethStatsContactOptionCannotBeUsedWithoutEthStatsServerProvided() {
  }


any test to verify it is enabled by default?

added here 100bf43 (#7181)

fab-10 · 2024-06-06T15:21:55Z

...um/core/src/main/java/org/hyperledger/besu/ethereum/worldstate/DataStorageConfiguration.java

    long MINIMUM_BONSAI_TRIE_LOG_RETENTION_LIMIT = DEFAULT_BONSAI_MAX_LAYERS_TO_LOAD;
    int DEFAULT_BONSAI_TRIE_LOG_PRUNING_WINDOW_SIZE = 30_000;
    boolean DEFAULT_BONSAI_FULL_FLAT_DB_ENABLED = true;
    boolean DEFAULT_BONSAI_CODE_USING_CODE_HASH_ENABLED = true;

    DataStorageConfiguration.Unstable DEFAULT =
-        ImmutableDataStorageConfiguration.Unstable.builder().build();
+        ImmutableDataStorageConfiguration.Unstable.builder()
+            .bonsaiLimitTrieLogsEnabled(true)


this should be redundant since the default is true now, right?

well spotted, an artefact from an earlier implementation.

removed here 100bf43 (#7181)

Address PR comments Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

non-fungible-nelson · 2024-06-06T18:55:16Z

Commit LGTM - thanks @siladu for the last minute fixes.

matthew1001 · 2024-06-21T14:42:49Z

FYI @siladu in PR #7140 I was planning to change the fail-on-start behaviour if FULL sync mode is enabled and bonsai-trie-log-pruning-enabled is set, and instead set bonsai-trie-log-pruning-enabled to false if sync mode is FULL (with a log message to state that the default has been overridden).

Let me know if you have any concerns about that change - maybe via comments in that PR?

* Enable --Xbonsai-limit-trie-logs-enabled by default Signed-off-by: Simon Dudley <simon.dudley@consensys.net> Signed-off-by: George Tebrean <george@web3labs.com>

* Enable --Xbonsai-limit-trie-logs-enabled by default Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

siladu added 2 commits June 5, 2024 17:50

Enable --Xbonsai-limit-trie-logs-enabled by default

91eb0fc

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Use defaultValue and fallbackValue

641ec86

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

jframe reviewed Jun 6, 2024

View reviewed changes

siladu added 4 commits June 6, 2024 10:06

Only need fallbackValue and fix tests

d095d8f

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Merge remote-tracking branch 'upstream/main' into bonsai-limit-trie-l…

bd29664

…ogs-enabled-by-default Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Update profile defaults

d996521

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Only display limit trie log config if using bonsai

79b2aed

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

siladu marked this pull request as ready for review June 6, 2024 11:31

siladu added 3 commits June 6, 2024 14:56

Merge branch 'main' into bonsai-limit-trie-logs-enabled-by-default

7946fc6

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Add breaking change warning to changelog

052323a

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

small changelog update

76df04c

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

fab-10 approved these changes Jun 6, 2024

View reviewed changes

siladu added 3 commits June 6, 2024 18:04

Startup error if sync-mode=FULL

100bf43

Address PR comments Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

Merge branch 'main' into bonsai-limit-trie-logs-enabled-by-default

e14733c

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

javadoc

5b004d2

Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

jframe approved these changes Jun 6, 2024

View reviewed changes

jframe merged commit 7c21eed into hyperledger:main Jun 6, 2024
40 checks passed

siladu deleted the bonsai-limit-trie-logs-enabled-by-default branch June 7, 2024 05:28

This was referenced Jun 7, 2024

Promote option - bonsai-limit-trie-logs-enabled and bonsai-trie-logs-pruning-window-size #7192

Merged

Promote --bonsai-limit-trie-logs-enabled to stable #7193

Merged

gtebrean pushed a commit to gtebrean/besu that referenced this pull request Jun 26, 2024

Enable --Xbonsai-limit-trie-logs-enabled by default (hyperledger#7181)

99375d7

* Enable --Xbonsai-limit-trie-logs-enabled by default Signed-off-by: Simon Dudley <simon.dudley@consensys.net>

This was referenced Jul 19, 2024

Use getDefaultSyncModeIfNotSet during bonsai-limit-trie-log-enabled validation #7354

Closed

Disable bonsai-limit-trie-logs-enabled if sync-mode=FULL #7357

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable --Xbonsai-limit-trie-logs-enabled by default #7181

Enable --Xbonsai-limit-trie-logs-enabled by default #7181

siladu commented Jun 5, 2024 •

edited

Loading

jframe Jun 6, 2024

siladu Jun 6, 2024

siladu Jun 6, 2024

siladu Jun 6, 2024 •

edited

Loading

fab-10 left a comment

fab-10 Jun 6, 2024

siladu Jun 6, 2024 •

edited

Loading

fab-10 Jun 6, 2024

siladu Jun 6, 2024

siladu Jun 6, 2024

non-fungible-nelson commented Jun 6, 2024

matthew1001 commented Jun 21, 2024

		@@ -1244,8 +1244,13 @@ public void ethStatsContactOptionCannotBeUsedWithoutEthStatsServerProvided() {
		}

Enable --Xbonsai-limit-trie-logs-enabled by default #7181

Enable --Xbonsai-limit-trie-logs-enabled by default #7181

Conversation

siladu commented Jun 5, 2024 • edited Loading

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

jframe Jun 6, 2024

Choose a reason for hiding this comment

siladu Jun 6, 2024

Choose a reason for hiding this comment

siladu Jun 6, 2024

Choose a reason for hiding this comment

siladu Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

fab-10 left a comment

Choose a reason for hiding this comment

fab-10 Jun 6, 2024

Choose a reason for hiding this comment

siladu Jun 6, 2024 • edited Loading

Choose a reason for hiding this comment

fab-10 Jun 6, 2024

Choose a reason for hiding this comment

siladu Jun 6, 2024

Choose a reason for hiding this comment

siladu Jun 6, 2024

Choose a reason for hiding this comment

non-fungible-nelson commented Jun 6, 2024

matthew1001 commented Jun 21, 2024

siladu commented Jun 5, 2024 •

edited

Loading

siladu Jun 6, 2024 •

edited

Loading

siladu Jun 6, 2024 •

edited

Loading