fix: ensure height is correct on first block #16259
Conversation
alexanderbez
added
backport/0.46.x
github-prbot
requested review from
a team,
JeancarloBarrios and
likhita-809
and removed request for
a team
| _ = app.Commit() | ||
|
|
||
| ctx := app.GetContextForCheckTx(nil) | ||
| require.Equal(t, int64(1), ctx.BlockHeight()) |
There was a problem hiding this comment.
w/o this fix, this value is actually 0 which would cause CheckTx to fail for txs prior to the 1st block.
| // handler, the block height is zero by default. However, after Commit is called | ||
| // the height needs to reflect the true block height. | ||
| initHeader.Height = req.InitialHeight | ||
| app.checkState.ctx = app.checkState.ctx.WithBlockHeader(initHeader) |
There was a problem hiding this comment.
can we set this in header info as well?
There was a problem hiding this comment.
Header info? What do you mean by that?
| @@ -74,6 +74,16 @@ func (app *BaseApp) InitChain(req abci.RequestInitChain) (res abci.ResponseInitC | |||
| } | |||
| } | |||
|
|
|||
| defer func() { | |||
There was a problem hiding this comment.
defer is usually for resource cleanup, I think it's clearer to simply put the code after the commit call, for example if panic happens before it, there's no point to still run this code.
There was a problem hiding this comment.
Although the tutorial on defer states that, there's no idiomatic usage of it.
defer is often used where e.g. ensure and finally would be used in other languages.
Finally is exactly what we're trying to do here. I think if we do it in Commit, we'll have to explicitly check if height == 0.
There was a problem hiding this comment.
I mean just after the initChainer call, I think we don't want to run this if there's any errors or panics?
There was a problem hiding this comment.
ahh yes, the reason why I added in the defer is if initChainer == nil, you'll need to add it before the return. That's why I added in the defer instead of having it twice.
tac0turtle
removed
backport/0.46.x
|
removed backport, dont think this should be backported. It is a bug but its not anything critical for users |
|
That's fair. I think dydx is launching with v0.47 and I'd imagine they'd want this but I guess they can wait or (temp) fork. |
Description
The block height after genesis and before the 2nd block is wrong. This is because we do not set the height in the default case on
InitChain. I've added a fix and a test case to reflect this.If you actually look at any major chain, you won't see any txs in the first block.
Bug was found by dydx team.
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!in the type prefix if API or client breaking change