Effects of a nil Sha256Checksum in a ChecksummedGenesisDoc

[andynog]

The GenesisDocProvider is a type alias for a func

cometbft/node/setup.go

Line 55 in ce68e90

type GenesisDocProvider func() (ChecksummedGenesisDoc, error)

When the logic was implemented, I believe that the expectation was that this GenesisDocProvider was going to be 'provided' using DefaultGenesisDocProviderFunc, which calculates the sha256 checksum from the jsonBlob of the genesis file content

cometbft/node/setup.go

Line 59 in ce68e90

func DefaultGenesisDocProviderFunc(config *cfg.Config) GenesisDocProvider {

The problem is that this provider could come from a custom provider. For example, the Cosmos-SDK has a different genesis signature and it needs to pass it's own provider logic, something like:

	appGenesisProvider := func() (node.ChecksummedGenesisDoc, error) {
		appGenesis, err := genutiltypes.AppGenesisFromFile(cmtCfg.GenesisFile())
		if err != nil {
			return node.ChecksummedGenesisDoc{}, err
		}
		gen, err := appGenesis.ToGenesisDoc()
		if err != nil {
			return node.ChecksummedGenesisDoc{}, err
		}
		return node.ChecksummedGenesisDoc{GenesisDoc: gen}, nil
	}

which returns a func() (ChecksummedGenesisDoc, error) that can be used in a node.NewNode() as the GenesisDocProvider, e.g.

tmNode, err := node.NewNode(
		context.TODO(),
		...
		appGenesisProvider,
		...
	)

but this might cause issues in

cometbft/node/setup.go

Line 573 in ce68e90

func LoadStateFromDBOrGenesisDocProviderWithConfig(

because the Sha256Checksum field is not validated (look at the example above (return node.ChecksummedGenesisDoc{GenesisDoc: gen}) so it will be nil.

It will fail when trying to save it in the stateDB here because a nil value cannot be saved on the database

cometbft/node/setup.go

Line 606 in ce68e90

if err = stateDB.SetSync(genesisDocHashKey, csGenDoc.Sha256Checksum); err != nil {

and there is other logic in this function that relies on a Sha256Checksum, so in my opinion, we might need to account for that in the code or at least add more tests. I agree that in part this is caused by a integrator not adding the checksum (maybe we need to add some more information to the docs to say that is expected), but at the same time as a defensive measure we might need to ensure we can handle this scenario and that it will not cause other issues in the future.

[sergio-mena]

Can you come up with a failing unit test that demonstrates the problem?

[alesforz]

Can you come up with a failing unit test that demonstrates the problem?

Check #3526.