chore(Node): Do not keep the entire genesis file in memory during node operation by alesforz · Pull Request #4247 · cometbft/cometbft

alesforz · 2024-10-04T10:10:56Z

Closes #1290.

This PR tracks the work we are doing for #1290.
For details, please refer to #1290 discussion.

ToDo

Environment stores either a pointer to a GenesisDoc or the genesis' chunks, but not both. Merged to this branch with chore(RPC): Environment stores only one in-memory copy of the Genesis #4235.
Node does not keep a pointer to a GenesisDoc after initialization. Merged to this branch with chore(Node): Node does not keep a pointer to a GenesisDoc after initialization #4250.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

…is (#4235) Closes #4234. ### Changes The [`Environment`](https://github.com/cometbft/cometbft/blob/8273634cdefb2fb22765a14f9acb9fca608e9982/rpc/core/env.go#L72) type will: - Store a pointer to a `GenesisDoc` if the genesis file size is below a threshold currently set to 16 MB. - If the genesis file exceeds this threshold, it will split it into 16 MB chunks and store the hashes of each chunk in memory in a slice. In this case, the pointer to the `GenesisDoc` will be `nil`. Because the `InitGenesisChunks` method of `Environment` is responsible for creating the chunks, most of the changes of this PR focus on refactoring this method and adding unit tests for it. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments

…alization (#4250) Closes #4248. ### Changes The `Node` type will: - create a singleton instance of `Environment` when setting up the RPC APIs. - set the pointer to the `GenesisDoc` to `nil` once startup is complete so that the GC can collect the in-memory `GenesisDoc` object. - store a `GenesisTime` field. --- #### PR checklist - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments

It was missing. Interestingly, tests did not pick up on this.

It seems that the `base64.StdEncoding.EncodeToString` function does not make a full copy of its argument. Therefore creating each `chunk` as a direct sub-slice of `data` was preventing the GC from deallocating the `data` slice after the function returned (you could still see it as allocated in pprof graphs). Creating each `chunk` as a copy of the sub-slice of `data` solves the issue.

alesforz · 2024-10-16T15:01:05Z

See here for a visualization of the changes in memory usage after applying these changes.

alesforz · 2024-10-16T15:13:25Z

Note: despite our efforts, these changes break the Node public API. Because Node doesn't store the genesis in memory after startup, its exported method GenesisDoc() had to change:

it now fetches the genesis from disk and unmarshals it into a *types.GenesisDoc.
its signature changed to also return an error in case fetching the genesis from disk fails (this is the breaking change)

rpc/client/local/local.go

rpc/core/env.go

andynog

Hi @alesforz, I think the implementation looks good, I've added comments and suggestions. I've also tested the API using a sample file and seems to work.

I didn't not validate the content returned. Just realized that we should provide maybe a CLI command (or sample app) in the future that could download the chunks and re-assemble into a genesis doc. Expecting users and operators to download encoded chunks might not be the best UX

rpc/core/env.go

node/node.go

rpc/core/env.go

rpc/core/env_test.go

rpc/core/net.go

- Added a log message in `ConfigureRPC()` to announce the start of the creation of the genesis file chunks - Improved error message returned by `InitGenesisChunks()` - `TestInitGenesisChunks() now checks that the original genesis doc and the genesis doc reassembled from its chuks match.

andynog

thanks for addressing the comments and suggestions. Looks good to me.

node/node.go

rpc/core/env_test.go

rpc/core/net.go

jmalicevic · 2024-10-22T10:57:49Z

Are the genesis json files still needed for testing ?As you added strings, can they be removed from the PR ?

alesforz · 2024-10-23T10:17:27Z

Are the genesis json files still needed for testing ?As you added strings, can they be removed from the PR ?

@jmalicevic The files are still there to run the unit tests. They are fictitious though: genesis_ci.json is the default genesis we generate to run the e2e test ci.toml; genesis_big.json is a copy of genesis_ci.json to which I added meaningless strings to the app_state field in order to test with a big genesis. Neither have real addresses.

I can substitute the small genesis file with a string in the code, but the big genesis file needs to be there to unit test the chunking mechanism. I can reduce its size (it's about 42MB) if that's your concern.

ValarDragon · 2024-10-23T21:16:52Z

Have not reviewed the PR, but am looking forward to this feature!

.changelog/unreleased/improvements/4235-4234-environment-stores-one-genesis.md

node/node.go

.changelog/unreleased/breaking-changes/4250-node-no-genesis.md

…esting in code.

alesforz · 2024-10-24T09:48:41Z

Are the genesis json files still needed for testing ?As you added strings, can they be removed from the PR ?

@jmalicevic The files are still there to run the unit tests. They are fictitious though: genesis_ci.json is the default genesis we generate to run the e2e test ci.toml; genesis_big.json is a copy of genesis_ci.json to which I added meaningless strings to the app_state field in order to test with a big genesis. Neither have real addresses.

I can substitute the small genesis file with a string in the code, but the big genesis file needs to be there to unit test the chunking mechanism. I can reduce its size (it's about 42MB) if that's your concern.

@jmalicevic Updated the testing code to generate the big genesis docs at runtime rather than having a file in the repo 😄 See last couple of commits

jmalicevic

Thanks for this @alesforz !

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

…e operation (cometbft#4247) Closes cometbft#1290. This PR tracks the work we are doing for cometbft#1290. For details, please refer to cometbft#1290 discussion. ### ToDo - [x] Environment stores either a pointer to a GenesisDoc or the genesis' chunks, but not both. Merged to this branch with cometbft#4235. - [x] [Node](https://github.com/cometbft/cometbft/blob/87a8c8948bf9926aff37f344b4120163f12cb4ad/node/node.go#L50) does not keep a pointer to a GenesisDoc after initialization. Merged to this branch with cometbft#4250. --- #### PR checklist - [x] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [x] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

alesforz self-assigned this Oct 4, 2024

alesforz linked an issue Oct 4, 2024 that may be closed by this pull request

Do not keep the entire genesis file in memory during node operation #1290

Closed

3 tasks

alesforz added backlog A prioritized task in the team's backlog P:tech-debt Priority: Technical debt that needs to be paid off to enable us to move faster, reliably P:storage-optimization Priority: Give operators greater control over storage and storage optimization labels Oct 4, 2024

alesforz added this to the 2024-Q4 milestone Oct 14, 2024

Alessandro Sforzin added 5 commits October 16, 2024 16:57

added .changelog entry for #4250

b4d0a73

Initialize Node.genesisTime field.

ab9c0ba

It was missing. Interestingly, tests did not pick up on this.

alesforz force-pushed the alesforz/genesis-in-memory branch from d28d4d6 to 6bfe1e1 Compare October 16, 2024 14:57

alesforz marked this pull request as ready for review October 16, 2024 14:58

alesforz requested a review from a team as a code owner October 16, 2024 14:58

alesforz requested review from a team, andynog and jmalicevic October 16, 2024 14:58

alesforz mentioned this pull request Oct 17, 2024

chore(RPC): Reduce Genesis Chunks Size #4299

Merged

2 tasks

jmalicevic reviewed Oct 17, 2024

View reviewed changes

rpc/client/local/local.go Outdated Show resolved Hide resolved

jmalicevic reviewed Oct 17, 2024

View reviewed changes

rpc/core/env.go Show resolved Hide resolved

jmalicevic reviewed Oct 17, 2024

View reviewed changes

rpc/core/env.go Show resolved Hide resolved

alesforz requested a review from jmalicevic October 17, 2024 14:07

andynog approved these changes Oct 17, 2024

View reviewed changes

alesforz requested a review from andynog October 18, 2024 09:53

andynog approved these changes Oct 21, 2024

View reviewed changes

node/node.go Show resolved Hide resolved

rpc/core/env_test.go Show resolved Hide resolved

rpc/core/net.go Show resolved Hide resolved

Merge branch 'main' into alesforz/genesis-in-memory

8eb4a65

Merge branch 'main' into alesforz/genesis-in-memory

a35f311

mergify bot added 5 commits October 24, 2024 05:35

Merge branch 'main' into alesforz/genesis-in-memory

f491b02

Merge branch 'main' into alesforz/genesis-in-memory

05b8b89

Merge branch 'main' into alesforz/genesis-in-memory

5f4a55b

Merge branch 'main' into alesforz/genesis-in-memory

23e267f

Merge branch 'main' into alesforz/genesis-in-memory

fa78376

melekes reviewed Oct 24, 2024

View reviewed changes

Alessandro added 2 commits October 24, 2024 13:46

Moved contents of genesis_ci.json into a const string

c8aaf37

Deleted genesis_big.json because we now generate genesis docs for t…

38c730a

…esting in code.

jmalicevic approved these changes Oct 24, 2024

View reviewed changes

Alessandro Sforzin and others added 2 commits October 25, 2024 14:44

updated .changelog as suggested by code reviews

0c9bb5e

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

improved error messages

8cfb389

alesforz added this pull request to the merge queue Oct 25, 2024

Merged via the queue into main with commit d6a661a Oct 25, 2024

alesforz deleted the alesforz/genesis-in-memory branch October 25, 2024 13:30

jmalicevic mentioned this pull request Feb 25, 2025

fix(statesync): if statesync fails, handshake with the app #4978

Merged

Conversation

alesforz commented Oct 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

ToDo

PR checklist

Uh oh!

alesforz commented Oct 16, 2024

Uh oh!

alesforz commented Oct 16, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

andynog 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!

andynog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmalicevic commented Oct 22, 2024

Uh oh!

alesforz commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ValarDragon commented Oct 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alesforz commented Oct 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmalicevic left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

alesforz commented Oct 4, 2024 •

edited

Loading

alesforz commented Oct 23, 2024 •

edited

Loading

alesforz commented Oct 24, 2024 •

edited

Loading