-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add preFinalizeBlockHook to allow VE persistence #16898
Conversation
This comment has been minimized.
This comment has been minimized.
app.logger.Info("InitChain", "initialHeight", req.InitialHeight, "chainID", req.ChainId) | ||
|
||
// Set the initial height, which will be used to determine if we are proposing | ||
// or processing the first block or not. | ||
app.initialHeight = req.InitialHeight | ||
if app.initialHeight == 0 { // If initial height is 0, set it to 1 | ||
app.initialHeight = 1 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bringing this from the PR that was reverted in order to make sure that PrepareProposal/ProcessProposal get the right context on the first block.
|
||
// GasMeter must be set after we get a context with updated consensus params. | ||
gasMeter := app.getBlockGasMeter(app.finalizeBlockState.ctx) | ||
app.finalizeBlockState.ctx = app.finalizeBlockState.ctx.WithBlockGasMeter(gasMeter) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lines above are to simplify context setting. The stuff done in the else
bracket are also done outside of it, so we might as well do it once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing 💯
Changes look great. I think we need to add a section to the building apps doc though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we get a e2e test with vote extensions?
+1 for docs update in this PR. |
@tac0turtle see here bc261cd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doc changes look good to me. Thanks @facundomedica 👍
this doesnt look like its testing resync issue, we should also test how this works with state sync, i could see that possibly being an issue. |
The major theme here is that VE should be handled when @facundomedica I think a test should ensure that |
we can do it in a separate PR but like you said this is the happy path, we should get better testing on this feature. Many things that we have found thus far would have been caught earlier with better tests |
Most of the tests are not calling ProcessProposal and calling FinalizeBlock right away. That was something I wanted to change before finding this issue (but we might leave as is).
Agree, but I'm not sure how to test it effectively tbh. |
I personally would merge this. We should explore (in a separate issue and PR), more sophisticated and real-world-esque testing of ABCI++ in our E2E test suite. |
@Mergifyio backport release/v0.50.x |
✅ Backports have been created
|
Co-authored-by: Aleksandr Bezobchuk <[email protected]> (cherry picked from commit 38f1014)
…) (#17001) Co-authored-by: Facundo Medica <[email protected]>
Description
Closes: #16883
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.md
Reviewers 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