Support for CometBFT proto 0.38 #1312
Conversation
Check out the protos from the v0.38.0-alpha.1 release tag and add the v0_38 fork of generated modules to tendermint-proto.
The AbciParams message/object was added in 0.38, used as the "abci" field in ConsensusParams. Add protobuf conversions for v0_38 protos, and serde.
This is needed for the more detailed VoteInfo in 0.38.
The BlockSignatureInfo enum is an attempt to represent both the pre-0.38 encoding with the signed_last_block flag, and the current one where the signature information is given by the block_id_flag field.
Rename to Signature::into_bytes to follow Rust naming conventions.
The serialization of extension fields is not tested yet.
Change the tendermint-proto structs in use to v0_38 and update the application API accordingly.
Only change enough to compile, will likely not work.
As commented in the declaration of the CheckTx domain type, I'm not sure it's the right thing to do.
There was no apparent value added by defining the impls manually, IDK why the attributes were previously shifted to the enum type.
Use v0.34.28 and v0.37.1
Add members to VoteInfo to be able to convert from and to the ExtendedVoteInfo message type as defined in 0.38 proto. Define conversions from and to the proto type, as well as conversions between CommitInfo and ExtendedCommitInfo in the proto. As a helper to the above, provide Bytes conversions for the Signature domain type.
Add the v0_38::abci::{Request,Response} enums and the new domain types
representing request and response messages added in CometBFT 0.38.
mzabaluev
force-pushed
the
mikhail/cometbft-0.38
branch
from
6d755a2
to
36eca7a
Compare
Define {ConsensusRequest, InfoRequest, MempoolRequest, SnapshotRequest}
separately for each of the v0_*::abci versioned modules, enumerating
just the requests that can be used with the respective version.
No panic.
mzabaluev
force-pushed
the
mikhail/cometbft-0.38
branch
from
e9941b0
to
f169a93
Compare
Codecov Report
@@ Coverage Diff @@
## main #1312 +/- ##
=======================================
- Coverage 60.3% 58.1% -2.3%
=======================================
Files 275 281 +6
Lines 24420 25467 +1047
=======================================
+ Hits 14733 14802 +69
- Misses 9687 10665 +978
... and 3 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Just like with abci::Request and abci::Response, the categorized request and response enums get re-exported from the v0_38::abci module.
The ExtendedVoteInfo and ExtendedCommitInfo protobuf messages can be encoded only in RequestPrepareProposal. In other contexts the extension fields are not useful.
tendermint/src/abci/types.rs
Outdated
| pub enum BlockSignatureInfo { | ||
| /// Full information available, as determined by the `BlockIdFlag` value. | ||
| Flag(block::BlockIdFlag), | ||
| /// In CometBFT versions before 0.38, the `signed_last_block` field has | ||
| /// the value of true. | ||
| /// | ||
| /// This variant should not be used in CometBFT 0.38 or later | ||
| /// and will result in the "undefined" encoding in protobuf. | ||
| LegacySigned, | ||
| } |
There was a problem hiding this comment.
@sergio-mena This is my attempt to represent the block signing information in VoteInfo and ExtendedVoteInfo without data loss also when converted from older versions.
Something to consider in connection with cometbft/cometbft#780
There was a problem hiding this comment.
I see a problem here. How about the case where we have the "legacy" field set to false?
If these data structures are to work for, e.g., legacy-to-legacy interoperability, you are not able to handle the case where the signature is absent.
There was a problem hiding this comment.
A couple of minor comments, but otherwise this looks great. Nice work @mzabaluev!
As suggested by Thane.
Add versioning information to comments on the fields of domain types that have been added in 0.38.
There was a problem hiding this comment.
LGTM!
I'd be interested to know what you think, @hdevalence and @romac, if you have time to review.
| // FIXME: do we want to support this for older versions of the protocol? | ||
| // Does anybody use it? | ||
| //pub data: Bytes, |
There was a problem hiding this comment.
I'm also restoring this field for the sake of pre-0.38 applications.
Protobuf-related changes for #1301
.changelog/