feat!: [Comet v0.38 Integration] Vote Extensions #15766
Conversation
alexanderbez
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
@alexanderbez your pull request is missing a changelog! |
tac0turtle
force-pushed
the
bez/abci++_vote_ext
branch
from
2dc6194
to
75873b6
Compare
| // Since the application can get access to FinalizeBlock state and write to it, | ||
| // we must be sure to reset it in case ProcessProposal timeouts and is called | ||
| // again in a subsequent round. However, we only want to do this after we've | ||
| // processed the first block, as we want to avoid overwriting the finalizeState | ||
| // after state changes during InitChain. | ||
| if req.Height > app.initialHeight { | ||
| app.setState(execModeFinalize, header) | ||
| } |
There was a problem hiding this comment.
Okay, now I get what you meant the other day. So it's not possible that ProcessProposal gets called more than once during the first block?
There was a problem hiding this comment.
So it's not possible that ProcessProposal gets called more than once during the first block?
It is possible. Why does that matter? This conditional is irrelevant to the round number in the first block.
There was a problem hiding this comment.
Then I'm not so sure I understood you the other day lol. In other words, why this wouldn't cause an issue? I understand that we would be having multiple writes to state without discarding what was done in the previous processProposal.
1. InitChain writes to FinalizeState
2. ProcessProposal writes to FinalizeState
3. ProcessProposal gets called again and writes to FinalizeState
So step 3 would be seeing the results of steps 1+2, but I think it should only be seeing results of step 1.
There was a problem hiding this comment.
That's not the full correct understanding of what's happening. What is actually happening is, in the context of the first block:
1. InitChain writes to FinalizeState
2. ProcessProposal writes to a CACHED copy of FinalizeState
3. If ProcessProposal from (2) times out, (2) is called again
So step (3) would be a on a fresh cached copy of FinalizeState from step (1).
The big caveat here though is that a chain should NOT write to FinalizeState on the first block. I'll update the godoc. This isn't a concern really though because vote extensions can be used until at least the block after the 1st block.
There was a problem hiding this comment.
Ahh thanks for taking the time to explain me, I was a bit too worried 😅
| // If vote extensions are not enabled, as a safety precaution, we return an | ||
| // error. | ||
| cp := app.GetConsensusParams(app.voteExtensionState.ctx) | ||
| if cp.Abci != nil && cp.Abci.VoteExtensionsEnableHeight <= 0 { |
There was a problem hiding this comment.
Do we need a consensus module migration here? Or can we capture this somehow so we don't forget
There was a problem hiding this comment.
An app would have to set the VoteExtensionsEnableHeight in it's upgrade handler, yes. I dont think this is something we can do for them. That being said, you mean should we set default values in x/consensus? If so, yeah I think we can do that.
Mhh what do you mean exactly? What are the newly added items? |
Description
runTxIOTA ->execModeValidateVoteExtensionshelperPrepareProposalandProcessProposalabci_utils.goref: #12272
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