node: verify genesis doc hash against the file contents rather than remarshalled JSON by mzabaluev · Pull Request #1293 · cometbft/cometbft

mzabaluev · 2023-09-01T06:34:58Z

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

node/node.go

mzabaluev · 2023-09-01T12:14:25Z

I don't think this needs a changelog entry, as this is a follow-up to #1017 and we haven't released the functional changes yet.

sergio-mena · 2023-09-01T14:29:20Z

@mzabaluev PR #1295 is likely to affect this PR. Sorry for the inconveniences...

mzabaluev · 2023-09-01T22:19:19Z

@mzabaluev PR #1295 is likely to affect this PR. Sorry for the inconveniences...

No worries, I will wait (and review) and rebase once that one is merged.

jmalicevic · 2023-09-01T22:25:26Z

Merging the PR now so you are free to rebase! Thanks Mikhail !

Marshalling the doc data back into JSON to checksum is wasteful. In the future, the hash of the genesis file may be passed as a CLI or config parameter to help control coordinated upgrades. We want that hash to be the same as the hash stored in the database.

Don't put the requirement to verify the JSON content checksum onto every genesis doc provider, do it in one place in the main code.

jmalicevic · 2023-09-04T08:50:16Z

node/setup.go

 // DefaultGenesisDocProviderFunc returns a GenesisDocProvider that loads
 // the GenesisDoc from the config.GenesisFile() on the filesystem.
 func DefaultGenesisDocProviderFunc(config *cfg.Config) GenesisDocProvider {
-	return func() (*types.GenesisDoc, error) {


I wonder whether instead of changing the return value of this function (which is used in many other places), we can not move the checksum logic into the LoadStateFromDBOrGenesisProvider to be executed only once if there is no hash in the DB or if we have a flag provided via cli? Basically move it to line 576 and put the logic of reading the file and computing the sum there?

I wonder whether instead of changing the return value of this function (which is used in many other places), we can not move the checksum logic into the LoadStateFromDBOrGenesisProvider to be executed only once if there is no hash in the DB or if we have a flag provided via cli? Basically move it to line 576 and put the logic of reading the file and computing the sum there?

I have not come across any other places where the GenesisDocProvider signature was used. My concern is about eventually reworking this for streamability: the provider should not need to store the whole file content in RAM to parse the JSON or calculate the checksum. This signature allows implementing it with an incremental parser (e.g. json.Decoder from the standard library) and an incremental SHA-256 hasher. I have left a FIXME comment about this in the default provider function. Maybe I should turn it into a backlog issue.

Filed away as #1302

DefaultGenesisProvider is being used a lot although mostly to create a new node so technically it should be fine. But I am not sure whether it is used by users as well .

So instead of changing a public interface for a future optimization, could we not do one of two things:

Leave this until we do the actual optimization and then we can even create a DefaultStreamingDocProvider or

Keep the genesisProvider returning just the doc and have a special createChecksum function that could afterwards be altered to not read the file but rather stream through it based on a given path. This function is then called to get the hash (the fact that the genesisProvider has it in memory does not change anything as we anyways still keep it in memory).

Also, we cannot really do this without changing the signature and contract for GenesisDocProvider in some way: in main, it only returns the parsed doc, and both retrieval or JSON content from the source and parsing is supposed to happen inside the provider function.

There is a backward-compatible alternative: add a checksum field to the GenesisDoc struct and mandate every genesis doc provider set it with the computed checksum. But: 1) it feels conceptually wrong; 2) this special field has to be fenced away from normal handling of the GenesisDoc fields by the JSON marshal and unmarshal implementations.

Yes, you are right it might be wrong.. What abut the solution of changing the signature of LoadStateFromDBOrGenesisDocProvider by adding it a path to the genesis file (as we will anyways change this function signature to pass the hash from command line)? Both are breaking, just not sure which is more (as in breaking the way people are used to use these things).

The SDK uses GenesisDocProvider in three places (if I didn't miss any): here, here, and here.

If we include a changelog explaining the signature change, I doesn't seem highly disruptive. Thoughts?

If we include a changelog explaining the signature change, I doesn't seem highly disruptive. Thoughts?

I support this. But I do need to add a changelog, after all.

I also agree with the change. Even my proposed solution about breaking the API of the other function is not going to work. As Mikhail pointed out on Slack, the whole point of the provider is to have an abstract provider so passing a path to a function is def not that.

sergio-mena

🙏

mzabaluev · 2023-09-05T17:53:21Z

The tests seem flaky: I've got failures in two different test buckets on subsequent pushes that did not add anything to the changes.

…emarshalled JSON (#1293) * node: verify genesis doc hash against the file Marshalling the doc data back into JSON to checksum is wasteful. In the future, the hash of the genesis file may be passed as a CLI or config parameter to help control coordinated upgrades. We want that hash to be the same as the hash stored in the database. * node: centralize genesis doc hash verification Don't put the requirement to verify the JSON content checksum onto every genesis doc provider, do it in one place in the main code. * node: link the issue on the recent FIXME comment * Changelog entry for #1293

This avoids saving the genesisDoc to database. When using goleveldb and ~4GiB+ genesis files, it causes a panic during snappy encoding (panic: snappy: decoded block is too large). refs akash-network/support#280 backports: - cometbft#1017 - cometbft#1293 Signed-off-by: Artur Troian <troian.ap@gmail.com>

This avoids saving the genesisDoc to database. When using goleveldb and ~4GiB+ genesis files, it causes a panic during snappy encoding (panic: snappy: decoded block is too large). refs akash-network/support#280 ports: - cometbft#1017 - cometbft#1293 Signed-off-by: Artur Troian <troian@users.noreply.github.com>

mzabaluev requested a review from a team as a code owner September 1, 2023 06:34

mzabaluev requested a review from a team September 1, 2023 06:34

mzabaluev marked this pull request as draft September 1, 2023 06:35

jmalicevic reviewed Sep 1, 2023

View reviewed changes

node/node.go Outdated Show resolved Hide resolved

node/node.go Outdated Show resolved Hide resolved

mzabaluev marked this pull request as ready for review September 1, 2023 12:14

jmalicevic mentioned this pull request Sep 3, 2023

state: remove genesis file from database #1297

Merged

3 tasks

mzabaluev added 2 commits September 4, 2023 10:06

node: centralize genesis doc hash verification

0f13216

Don't put the requirement to verify the JSON content checksum onto every genesis doc provider, do it in one place in the main code.

mzabaluev force-pushed the mikhail/gen-doc-hash-from-file branch from ae9d6ea to 0f13216 Compare September 4, 2023 07:16

jmalicevic reviewed Sep 4, 2023

View reviewed changes

mzabaluev mentioned this pull request Sep 4, 2023

Use incremental JSON parser and checksum hasher in genesis doc provider #1302

Open

node: link the issue on the recent FIXME comment

9893882

jmalicevic mentioned this pull request Sep 4, 2023

cli/genesis: allow operators to provide Comet with a hash of the genesis file to be verified on load #1303

Closed

Changelog entry for #1293

9a99da1

mzabaluev added the breaking A breaking change label Sep 4, 2023

jmalicevic approved these changes Sep 4, 2023

View reviewed changes

sergio-mena approved these changes Sep 4, 2023

View reviewed changes

Merge branch 'main' into mikhail/gen-doc-hash-from-file

479284d

mzabaluev added this pull request to the merge queue Sep 6, 2023

Merged via the queue into main with commit 147a47d Sep 6, 2023

mzabaluev deleted the mikhail/gen-doc-hash-from-file branch September 6, 2023 12:38

melekes mentioned this pull request Jan 9, 2024

feat(state)!: do not save genesis file in DB (backport #1295, #1293, #1297) #1997

Closed

troian mentioned this pull request Feb 6, 2025

refactor: backport remove genesis persistence from state akash-network/cometbft#2

Merged

troian mentioned this pull request Jul 14, 2025

feat: port remove genesis persistence from state akash-network/cometbft#5

Merged

Conversation

mzabaluev commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

Uh oh!

Uh oh!

mzabaluev commented Sep 1, 2023

Uh oh!

sergio-mena commented Sep 1, 2023

Uh oh!

mzabaluev commented Sep 1, 2023

Uh oh!

jmalicevic commented Sep 1, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mzabaluev Sep 4, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sergio-mena left a comment

Choose a reason for hiding this comment

Uh oh!

mzabaluev commented Sep 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

mzabaluev commented Sep 1, 2023 •

edited

Loading

mzabaluev Sep 4, 2023 •

edited

Loading