Consensus failure when manifest.Params doesn't contain params.Inflation

[fmorency]

Updating the manifest module parameters without supplying Inflation leads to a consensus failure.

The culprit is

	if !params.Inflation.AutomaticEnabled {
		k.Logger().Debug("Automatic inflation is disabled")
		return nil
	}

in BeginBlocker

We should add a check.

CC @Reecepbcups
CC @chalabi2

[Reecepbcups]

leads to a consensus failure as in the nodes all halt / app hash, or the message fails?
and this only occurs from the POA multisig incorrectly setting it yea?

Sounds like it just needs a Validate() inflation not nil check ?

[fmorency]

leads to a consensus failure as in the nodes all halt / app hash, or the message fails? and this only occurs from the POA multisig incorrectly setting it yea?

Sounds like it just needs a Validate() inflation not nil check ?

Here's the message I'm getting in the node logs

12:45PM ERR CONSENSUS FAILURE!!! err="runtime error: invalid memory address or nil pointer dereference" module=consensus stack="goroutine 339 [running]:\nruntime/debug.Stack()\n\truntime/debug/stack.go:24 +0x5e\ngithub.com/cometbft/cometbft/consensus.(*State).receiveRoutine.func2()\n\tgithub.com/cometbft/[email protected]/consensus/state.go:801 +0x46\npanic({0x2b3f1c0?, 0x59c40f0?})\n\truntime/panic.go:770 +0x132\ngithub.com/liftedinit/manifest-ledger/x/manifest.BeginBlocker({_, _}, {{0x3ef1b90, 0xc0017a28b0}, {0x3edf4d8, 0xc0022ea540}, {{0x3ef1b90, 0xc0017a28b0}, {0x3e9e460, 0xc00148c240}, ...}, ...}, ...)\n\tgithub.com/liftedinit/manifest-ledger/x/manifest/abci.go:36 +0x17c\ngithub.com/liftedinit/manifest-ledger/x/manifest.AppModule.BeginBlock(...)\n\tgithub.com/liftedinit/manifest-ledger/x/manifest/module.go:167\ngithub.com/cosmos/cosmos-sdk/types/module.(*Manager).BeginBlock(_, {{0x3ed6640, 0x5b85860}, {0x3ef25a0, 0xc00652a0c0}, {{0x0, 0x0}, {0xc002912210, 0xa}, 0x19, ...}, ...})\n\tgithub.com/cosmos/[email protected]/types/module/module.go:779 +0x188\ngithub.com/liftedinit/manifest-ledger/app.(*ManifestApp).BeginBlocker(...)\n\tgithub.com/liftedinit/manifest-ledger/app/app.go:933\ngithub.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).beginBlock(0xc000524488, 0xc006544900?)\n\tgithub.com/cosmos/[email protected]/baseapp/baseapp.go:731 +0xb5\ngithub.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).internalFinalizeBlock(0xc000524488, {0x3ed6640, 0x5b85860}, 0xc006544900)\n\tgithub.com/cosmos/[email protected]/baseapp/abci.go:760 +0xdfe\ngithub.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).FinalizeBlock(0xc000524488, 0xc006544900)\n\tgithub.com/cosmos/[email protected]/baseapp/abci.go:884 +0x16e\ngithub.com/cosmos/cosmos-sdk/server.cometABCIWrapper.FinalizeBlock(...)\n\tgithub.com/cosmos/[email protected]/server/cmt_abci.go:44\ngithub.com/cometbft/cometbft/abci/client.(*localClient).FinalizeBlock(0x3ef30f0?, {0x3ed6a30?, 0x5b85860?}, 0x7f5c0aa287a0?)\n\tgithub.com/cometbft/[email protected]/abci/client/local_client.go:185 +0xcd\ngithub.com/cometbft/cometbft/proxy.(*appConnConsensus).FinalizeBlock(0xc00168e0c0, {0x3ed6a30, 0x5b85860}, 0xc006544900)\n\tgithub.com/cometbft/[email protected]/proxy/app_conn.go:104 +0x170\ngithub.com/cometbft/cometbft/state.(*BlockExecutor).ApplyBlock(_, {{{0xb, 0x0}, {0xc0023858fa, 0x6}}, {0xc002385910, 0xa}, 0x1, 0x18, {{0xc00586f280, ...}, ...}, ...}, ...)\n\tgithub.com/cometbft/[email protected]/state/execution.go:213 +0x5c5\ngithub.com/cometbft/cometbft/consensus.(*State).finalizeCommit(0xc001461888, 0x19)\n\tgithub.com/cometbft/[email protected]/consensus/state.go:1771 +0xb6c\ngithub.com/cometbft/cometbft/consensus.(*State).tryFinalizeCommit(0xc001461888, 0x19)\n\tgithub.com/cometbft/[email protected]/consensus/state.go:1682 +0x2e8\ngithub.com/cometbft/cometbft/consensus.(*State).enterCommit.func1()\n\tgithub.com/cometbft/[email protected]/consensus/state.go:1617 +0x9c\ngithub.com/cometbft/cometbft/consensus.(*State).enterCommit(0xc001461888, 0x19, 0x0)\n\tgithub.com/cometbft/[email protected]/consensus/state.go:1655 +0xc3b\ngithub.com/cometbft/cometbft/consensus.(*State).addVote(0xc001461888, 0xc0017b29c0, {0xc000ed20c0, 0x28})\n\tgithub.com/cometbft/[email protected]/consensus/state.go:2334 +0x1c6d\ngithub.com/cometbft/cometbft/consensus.(*State).tryAddVote(0xc001461888, 0xc0017b29c0, {0xc000ed20c0?, 0x0?})\n\tgithub.com/cometbft/[email protected]/consensus/state.go:2066 +0x26\ngithub.com/cometbft/cometbft/consensus.(*State).handleMsg(0xc001461888, {{0x3ea8080, 0xc001593878}, {0xc000ed20c0, 0x28}})\n\tgithub.com/cometbft/[email protected]/consensus/state.go:929 +0x39c\ngithub.com/cometbft/cometbft/consensus.(*State).receiveRoutine(0xc001461888, 0x0)\n\tgithub.com/cometbft/[email protected]/consensus/state.go:836 +0x3f1\ncreated by github.com/cometbft/cometbft/consensus.(*State).OnStart in goroutine 286\n\tgithub.com/cometbft/[email protected]/consensus/state.go:398 +0x10c\n"

I'm not sure what the idiomatic fix is but the x/manifest/abci.go fix in #62 works.

Happy to get your input.

Edit: The issue occurs when I execute the following Group proposal

  manifestUpdateProposal = &manifesttypes.MsgUpdateParams{
    Authority: groupAddr,
    Params: manifesttypes.Params{
      StakeHolders: []*manifesttypes.StakeHolders{
      {
        Address:    accAddr,
        Percentage: 50_000_000,
      },
      {
        Address:    acc2Addr,
        Percentage: 50_000_000,
      },
    },
  },
}

where the Inflation parameter is missing from Params.

Edit2: As you might have guessed, the POA Admin is a Group policy.

[Reecepbcups]

I think params.Inflation == nil like you did should work & prob the best solution.

Need to look more into & see if instead if we cast a type to not allow it to be nil, thus getting defaults. Worry there is that it would change params to default 0 values, which means the pointer case is ideal to not change others not touched

[fmorency]

I think params.Inflation == nil like you did should work & prob the best solution.

Need to look more into & see if instead if we cast a type to not allow it to be nil, thus getting defaults. Worry there is that it would change params to default 0 values, which means the pointer case is ideal to not change others not touched

Like you said, pointer case is ideal. If the fix is good for you then the case is closed. No need to spend more time on this.

[fmorency]

For the sake of posterity, I had to add the same fix in PayoutStakeholders().