feat: add --use-cached-genesis-state-hash paramater

[lyfsn]

PR description

When Besu starts, it reads the entire contents of the genesis file (besu.json) and traverses all accounts in memory to calculate the genesis state hash. This value is then compared with the contents of the database.

For custom or test networks with potentially large genesis files containing many accounts, this process can significantly lengthen startup times and increase memory usage due to the in-memory calculation of the genesis state hash.

In our testing environment, a genesis file around 1GB requires 16GB of memory and approximately 2 minutes to start the node.

In contrast, other clients like Nethermind do not read the genesis file if a genesis state hash is present in the database, resulting in significantly faster startup times, often just tens of seconds.

Other clients such as Geth, Erigon, and Reth separate initialization operations from node runtime operations, avoiding resource consumption for reading the genesis file during startup, thus achieving quick startup times.

Besu's practice of verifying the genesis file at every startup is largely to ensure user configurations are correct.

Therefore, we propose adding an option for advanced users who are certain they have not modified their genesis file and understand the implications of this option. This trade-off sacrifices some security for faster node startup times, a trade-off we believe advanced users will find acceptable.

With this option, our tests show a startup time of under 30 seconds for a 1GB genesis file, marking a significant improvement.

CLI Options and Configuration Profiles

We have introduced a new CLI option:

--use-cached-genesis-state-hash: When this option is used to start the node, if the genesis state hash value is already stored in the database, the node will directly utilize this value for startup.

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

Most advanced CI tests are deferred until PR approval, but you could:

• locally run all unit tests via: ./gradlew build
• locally run all acceptance tests via: ./gradlew acceptanceTest
• locally run all integration tests via: ./gradlew integrationTest
• locally run all reference tests via: ./gradlew ethereum:referenceTests:referenceTests

[jflo]

Great idea! This implementation looks good, but we'd like to see more test coverage. Suggest adding tests to cover the changes added to BesuControllerBuilder.

[lyfsn]

@jflo I added a unit test that already covers all the code of the BesuControllerBuilder.build() method. Could you check it and suggest more places that need modification?

[jflo]

@jflo I added a unit test that already covers all the code of the BesuControllerBuilder.build() method. Could you check it and suggest more places that need modification?

sure, need to cover the added lines after 577, including assertions that the genesis hash is not recomputed when the command line option is passed. Likely need to add a test case to BesuControllerBuilderTest that does so.

[lyfsn]

First, I renamed the parameter names. Next, I added unit tests in BesuControllerBuilderTest.

There are two new unit tests. The first one addresses the scenario where the node is started for the first time. At this point, the value for getGenesisStateHash is not present in VariablesStorage, so the process will proceed to the setGenesisStateHash branch and then to commit.

The second test addresses the scenario where the node is started for the second time. By then, the getGenesisStateHash already has a value, so it will not recompute the genesis state hash. Hence, it will not enter the setGenesisStateHash branch or acquire the updater.

In the unit tests, it's not directly tested whether a specific GenesisState.fromConfig was called, because it's a static method. In the mock framework, to test static methods, you need to explicitly set the return value of the static method, which is a complete genesisState object, to complete the unit test workflow. This can be challenging, especially to construct a complete GenesisState object that matches the one initialized by initMock.

Therefore, the unit tests include statements within the branch conditions around the GenesisState.fromConfig statement. Although it doesn't directly test which GenesisState.fromConfig was used, it is sufficient to prove which branch the code entered and which process it followed.

Additionally, for simulating the node's second start, the value of getGenesisStateHash is directly mocked. This is because the build method in the unit tests is idempotent and does not retain state. It's not possible to simulate the second node start scenario through committing data once and then getting it.

I hope you can review the current code to see if it meets the requirements. Thank you!

[lyfsn]

Additionally, this is the test coverage report, which shows that the coverage for the modified content is 100%:

Pic 1:

dditionally, this is the test coverage report, which shows that the coverage for the modified content is 100%:

Pic 2:

[jflo]

nailed it. thanks!

[lyfsn]

Just now, I rebased the code to the latest because this branch has conflicts that must be resolved in "besu/src/test/java/org/hyperledger/besu/controller/BesuControllerBuilderTest.java".

And I dropped the unit test about BesuControllerBuilder because BesuControllerBuilderTest.java has been deleted in there.

[lyfsn]

Due to the CI check failing, I rebased the branch code to the latest again, but did not change the source code, just rebased to the latest.
Because I ran "./gradlew integrationTest compileJmh" multiple times locally on macOS and Linux OS, and the integrationTest failure did not reoccur, I think there might be a sporadic error in the CI tool.

So, I request that you run the CI check again to confirm whether this PR's code passes the check.

[macfarla]

@lyfsn I'd like to get this merged in! but only you can update the branch

[lyfsn]

@macfarla
Every time I update the code to the latest version, I need to restart the CI checks. Is this a normal process? Because this means that the entire repository cannot have two mergeable PRs at the same time. Once one is merged, all others cannot be merged, requiring the branch to be updated and the checks to be run again?

Perhaps it was my mistake. When submitting the PR, I might have accidentally set something that prevented you from updating the code. Is there still a way to change this setting now?

[macfarla]

@macfarla Every time I update the code to the latest version, I need to restart the CI checks. Is this a normal process? Because this means that the entire repository cannot have two mergeable PRs at the same time. Once one is merged, all others cannot be merged, requiring the branch to be updated and the checks to be run again?
Yes we have a requirement that the CI checks must pass before a PR can be merged, and the PR must be up to date with main. So yes if one PR gets merged, other PRs must be updated.

Perhaps it was my mistake. When submitting the PR, I might have accidentally set something that prevented you from updating the code. Is there still a way to change this setting now?

it is repo/branch permissions on your repo. You can allow other users to push to your branch

Anyway I'm going to keep an eye on CI now because it's up to date so stay tuned!