Vote extensions: Add consensus param for extension activation logic #9862
Conversation
… extension require height (#8547) This PR makes vote extensions optional within Tendermint. A new ConsensusParams field, called ABCIParams.VoteExtensionsEnableHeight, has been added to toggle whether or not extensions should be enabled or disabled depending on the current height of the consensus engine. Related to: #8453
| ext, err := cs.blockExec.ExtendVote(vote) | ||
| if err != nil { | ||
| return nil, err | ||
| if cs.state.ConsensusParams.ABCI.VoteExtensionsEnabled(cs.Height) { |
There was a problem hiding this comment.
I notice that the timeout logic is missing here, but I can't quite discern what it was doing to begin with
There was a problem hiding this comment.
I believe you refer to the (background) differences seen at this point in the diff-of-diffs.
I looked into it and the difference seems to be #6240. However its description is rather terse.
In any case, my guess is that those changes have to do with enhancing the privval interface, which (I think) has little to do with ABCI.
| @@ -0,0 +1,13 @@ | |||
| package test | |||
There was a problem hiding this comment.
this was in internal/test/factory/, if that doesn't exist, then here is fine, but leaving this comment here in case this was accidentally not moved to the proper path.
There was a problem hiding this comment.
Yeah, for some reason, code belonging to internal/test/factory/ started landing in internal/test/ last summer. So this is keeping up with that.
I think we'll need to re-think how to rearrange the code currently in internal/, since we no longer have "internal" production code. Maybe the way forward is to move again part of the production code to internal. Time will tell.
| var err error | ||
| // temporary extra key before consensus param protos are regenerated | ||
| // TODO(wbanfield) remove in next PR | ||
| tmpABCIKey, err = orderedcode.Append(nil, int64(10000)) |
There was a problem hiding this comment.
Assuming this is deleted in a future PR, this is fine. orderedcode is not used in main but was in v0.36.x so reference to it should not outlive this set of pull requests.
There was a problem hiding this comment.
Yes :-)
In the next one
Closes #9861
The first commit is a cherry-pick of #8547.
Subsequent commits are fixing the build and the unit tests
N.B: The changelog will be updated once branch
feature/abci++vefis merged back intomainPR checklist
CHANGELOG_PENDING.mdupdated, or no changelog entry neededdocs/) and code comments, or nodocumentation updates needed