Skip to content
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

[TEST] update to cometbft 0.38 #310

Closed
wants to merge 2 commits into from
Closed

[TEST] update to cometbft 0.38 #310

wants to merge 2 commits into from

Conversation

jchappelow
Copy link
Member

@jchappelow jchappelow commented Sep 21, 2023

DO NOT MERGE. For evaluation.

This is working as expected. Several updates:

  • Combined BeginBlock, all DeliverTx and EndBlock into one FinalizeBlock
  • The app hash is returned by FinalizeBlock, previously returned by Commit. Fortunately it was OK to move some code from commit up to finalize.
  • All methods of the abci/type.Application interface:
    • add a second return, an error (yay reason)
    • pass the request and response by pointer (yay reason again)
  • the comet Node is now given a "connection-synchronized" client instead of the previous one that has a mutex for all the "connections"
  • We had to update pkg/abci/cometbft/privval/privvalidator.go to deal with the new "vote extension". We might want to switch back to the supported FilePV type using an ephemoral key file, or PR a type that uses a key from memory.
  • The struct returned by BroadcastTxCommit changed a field name (hurray unstable APIs)

Builds but there's something funny with the new vote extensions and the signer.
panic: non-recoverable error when signing vote (30243/0): extensions must be present IFF vote is a non-nil Precommit; present false, vote type 2, is nil false
Signer needed updating.

@@ -122,7 +122,21 @@ func (v *ValidatorSigner) SignVote(chainID string, vote *tendermintTypes.Vote) e

signBytes := types.VoteSignBytes(chainID, vote)

// VoteExtensionSignBytes???
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this had to be added. We might want to go back to their FilePV because it'll be always right and maintained. There's a ton of stuff changed in just this borrowed code in the last 10 months that this vote extension feature has been in the works.
Took a couple hours to identify the issue and it'll probably happen again in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Umm, in abci++, the application also controls the votes. some work might be needed to support that

Copy link
Member Author

@jchappelow jchappelow Sep 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can control votes, but the app can decide to require vote extensions or not. The simple update approach is to return an empty byte slice in ExtendVote and requiring en empty slice in VerifyVoteExtension

https://docs.cometbft.com/v0.38/spec/abci/abci++_comet_expected_behavior#adapting-existing-applications-that-use-abci

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Background: cometbft/cometbft#1001

Our signer has to generate extension signatures. This update in this PR supports that.

@jchappelow jchappelow marked this pull request as ready for review September 22, 2023 03:38
@jchappelow
Copy link
Member Author

Can reopen when we're ready to update. Probably when there's a 0.38.1 or greater patch release.

@jchappelow jchappelow closed this Oct 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants