[Bug]: unable to add ConsensusParams into ctx within the same block execution containing migration

[mmsqe]

Summary of Bug

Manager cannot add ConsensusParams into ctx after calling BeginBlock of UpgradeModule. Since only ctx doesn't refresh after upgrade module, other modules within for loop can't not access consensus params.

Version

v0.47.3

Steps to Reproduce

Other module like feemarket get empty consensus params in BeginBlock, a workaround is to call consensusParamsKeeper to get from store but not a longterm solution.

[tac0turtle]

interesting, thanks for the report. We will look into this, this week

[mmsqe]

interesting, thanks for the report. We will look into this, this week

Thanks, but we might close this issue now, since I see main branch move to context.Context, I thought we might change to *ctx but that's super breaking api change.

[yihuang]

interesting, thanks for the report. We will look into this, this week

Thanks, but we might close this issue now, since I see main branch move to context.Context, I thought we might change to *ctx but that's super breaking api change.

if there's a solution in main branch, can we port to 0.47?

[mmsqe]

if there's a solution in main branch, can we port to 0.47?

Since main branch already use context.Context, I guess we could use consensusParamsKeeper 1st?

[yihuang]

if there's a solution in main branch, can we port to 0.47?

Since main branch already use context.Context, I guess we could use consensusParamsKeeper 1st?

there are logics in sdk that rely on ctx.ConsensusParams() as well.

[tac0turtle]

i think we will need a way to continue getting it from context.

[alexanderbez]

i think we will need a way to continue getting it from context.

If Consensus Params are needed on the Context object, they should be set in the relevant ABCI method in baseapp I would imagine.

[yihuang]

i think we will need a way to continue getting it from context.

If Consensus Params are needed on the Context object, they should be set in the relevant ABCI method in baseapp I would imagine.

The problem here is it's to be migrated, so it don't exists before upgrade module migrate it

[mmsqe]

Not sure if there's better way to allow other modules access updated ctx right after migrate without changing BeginBlock signature

[yihuang]

I guess we should just run migration before everything else, under a minimal context, a special hook just for that, before the BeginBlocker.

[mmsqe]

I guess we should just run migration before everything else, under a minimal context, a special hook just for that, before the BeginBlocker.

Do you mean sth like this, test pass but I can't find v0.48.x to based on since it's consensus breaking change.

[yihuang]

I guess we should just run migration before everything else, under a minimal context, a special hook just for that, before the BeginBlocker.

Do you mean sth like this, test pass but I can't find v0.48.x to based on since it's consensus breaking change.

https://github.com/cosmos/cosmos-sdk/pull/16575 looks a little bit hacky though ;D

[mmsqe]

I guess we should just run migration before everything else, under a minimal context, a special hook just for that, before the BeginBlocker.

Do you mean sth like this, test pass but I can't find v0.48.x to based on since it's consensus breaking change.

#16575 looks a little bit hacky though ;D

I cleanup a bit rebase from main now, but might need manual backport later.

[skosito]

@mmsqe @tac0turtle @yihuang i see some fixes for this, but none of them in 0.47.x, what is the way to fix this in 0.47.x?

[tac0turtle]

the correct fix would be to query the parameters from the consensus module itself instead of using context.ConsensusParams. That should suffice

[skosito]

the correct fix would be to query the parameters from the consensus module itself instead of using context.ConsensusParams. That should suffice

thanks! i misunderstood somehow from discussion above that was not enough, but it seems ok