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

Multi-version support for CometBFT v0.38 #1301

Closed
2 tasks done
thanethomson opened this issue Apr 19, 2023 · 3 comments
Closed
2 tasks done

Multi-version support for CometBFT v0.38 #1301

thanethomson opened this issue Apr 19, 2023 · 3 comments
Assignees
Labels
enhancement New feature or request

Comments

@thanethomson
Copy link
Contributor

thanethomson commented Apr 19, 2023

Description

Since the CometBFT v0.38.0-alpha.1 release has been cut and it appears as though the API is mostly stable, we should look at implementing support for v0.38.0 when the final release comes out later in Q2 of 2023.

Definition of "done"

When tendermint-rs supports CometBFT v0.34, v0.37 and v0.38.

Tasks

  1. domain-types rpc
@thanethomson thanethomson added the enhancement New feature or request label Apr 19, 2023
@mzabaluev mzabaluev self-assigned this Apr 19, 2023
@mzabaluev
Copy link
Contributor

mzabaluev commented Apr 24, 2023

I started off a branch by generating the v0_38 module in tendermint-proto alongside v0_34 and v0_37 and using the newly generated code in appropriate places.

The main items of work here appear to be:

@mzabaluev
Copy link
Contributor

mzabaluev commented May 15, 2023

Here are my notes on the RPC adaptation.

The main difference is a few fields in the response to /block_results: the event lists in begin_block_events/end_block_events have been replaced with a single list in finalize_block_events. Another, smaller change is the deliver_tx field, renamed to tx_result in the response to /broadcast_tx_commit.

I have started out, again, on a "clean" solution: statically separate, versioned endpoint traits to expose the differences at the type level. But again, the impact on the consumer does not seem to be worth the weight of these few changes. I think it's better to expose both old and new fields side by side on the API types representing RPC endpoint responses.

Specifically, endpoint::block_results::Response would have its begin_block_events and end_block_events fields only populated with values when a pre-0.38 node is queried, otherwise these fields will have empty vectors, but finalize_block_events will have the 0.38+ event data. In time, the former two fields will be deprecated as we drop support for older versions of the protocol.

mzabaluev referenced this issue May 18, 2023
* proto-compiler: Improve error reporting

Print out the prost-build error to display protoc output line by line
rather than as an unreadable Debug dump.

* Generate proto from tendermint 0.37.0-alpha.1

Also adapted the tendermint-abci crate as necessary.
Committing this helps take stock of the protocol changes since 0.34,
before multi-version support can be implemented.

* proto: sort includes in generated tendermint.rs

This normalizes the content of the module for protocol updates.

* proto: generate structs for 0.34 and 0.37

Modify proto-compiler to generate protobuf modules for both versions
of the Tendermint protocol. The different versions are disambiguated
by module paths, presently tendermint::v0_34 and tendermint::v0_37,
with the latest supported version reimported as tendermint::*.

* tendermint: Remove Evidence::ConflictingHeaders

This variant does not get expressed in RPC or protobuf,
seems like this protocol change was not taken as per
https://github.com/tendermint/tendermint/blob/91fba07e49cee43048fd761c8b2c2ec3c017acc8/docs/architecture/adr-047-handling-evidence-from-light-client.md

* Add serializers::allow_null

This should be used in preference to nullable where `nil` in the format
could be met as a quirk admitted by the Go implementation,
but otherwise the preferred form is some, possibly default value.

* Adapt domain types to v0.37 and add multi-version conversions.

* Multi-version protobuf support for request types

* Add ABCI response types for PrepareProposal, ProcessProposal

* Multi-version protobuf conversions for response types

* Fix conversion from account::Id to Bytes

* Fix compiler warnings

* light-client: Restore Supervisor::report_evidence

Keep it as a stub to avoid more extensive changes.

* Restore serialization of Block and Evidence

Define hand-written Serialize/Deserialize impls for Evidence.

* Fix clippy lints

* proto-compiler: Improve copying of generated files

- Filter out the empty non-Tendermint files.
- Improve use of the WalkDir iterator.
- Use OS-agnostic path construction.

* proto: Regenerate v0_34 with v0.34.22

* Add domain type for RemoteSignerError

With multi-protocol stuff, it should be done properly and conversions
defined from/to proto stuff.

* Define more multi-protocol conversions

Temporarily remove the import of crate::v0_37::* to the root of
tendermint-proto.

* p2p: version-qualify dependencies on protobuf

Where there are direct dependencies on the proto definitions,
clarify them with the protocol version for later audit.

* Make clippy happy

* Derive Eq where clippy suggests

* tendermint: version-specific Request and Response

Separate Request and Response definitions pertaining to
protocol versions v0.34 and v0.37, namespaced in top-level
modules v0_34 and v0_37 respectively.

* Remove last version aliases of Request, Response

Need to check where we depend in downstream branches.

* Re-generate protos with prost-build 0.11.4

Also update v0_34 to 0.34.24.

* rpc: add endpoints header and header_by_hash

Provided by v0.37 nodes.

* rpc: Separate Client traits for v0.34 and v0.37

Add new endpoints /header and /header_by_hash to
the v0_37::Client trait, and re-export it as new crate::Client.

* rpc: feature-gate the Client traits

The same way as the old Client trait was.

* xla/multi-tc-versionsupport/fix (#1254)

* tendermint: post-merge fix

* Add missing ConsensusRequest and ConsensusResponse mappings.

* abci: change serialization to unsigned varint

As the protocol used by tendermint-abci has been updated
to 0.37, (with no compat mode for 0.34 at the moment), the encoding
is changed to match.

* abci: changelog notice about varint encoding

This is a breaking change.

* rpc: multi-version support through generics

Introduce Dialect trait and data structures in modules dialect,
v0_34::dialect, v0_37::dialect, to realize differences in serialization
between RPC versions. Parameterize data structures that need to be
differently serialized by generic parameters containing the differences.

* rpc: remove generic default from request::Wrapper

* Implement both Client dialect traits for websocket

* Rework Event serialization with helper types

DialectEvent and its sub-field structs take over the dialectal generics,
while Event and its inner structs remain public types free from
serialization concerns.

* rpc: Refactor subscriptions to support dialects

* rpc: fix the fixture tests

Have to make DialectEvent and friends public for these
"integration" tests.

* rpc: fix websocket tests

* tendermint: Fix test_sign_bytes_compatibility

Badly merged code broke the test.

* Versioned type aliases for WebSocketClient

Also for WebSocketClientDriver.
The specializations for v0_37 are also re-exported under crate root,
providing some backward compatibility.

* Fix tools build

* Remove dialect parameter for SubscriptionClient

The trait does not need it and the Subscription public API is erased.
Only WebSocketClient needs the dialect parameter.

* Derive PartialEq, Eq on ProdCommitValidator

Also add test code that ensures ProdVerifier supports the common derived
traits.

* tendermint: restore Serialize impl on Event

Also on ABCI domain types containing Event instances.
This is needed for CLI.

* rpc: de-genericized result types

Add associated type Output to SimpleRequest to designate the
output type produced from the response. Implement so that the
output type is a generics-free Response, while the Result type for
the Request trait is a DialectResponse where it needs to be.

* rpc: make the client module public

The purpose is to expose the generic WebSocketClient and
WebSocketClientDriver types, though not under the crate root
that's reserved for the type aliases specialized for the
latest protocol version.

* rpc: rename DefaultDialect to LatestDialect

* rpc: dynamic compat mode for HttpClient

Instead of using statically selected, but mostly identical
Client traits to specify the protocol version, add the dynamic
compatibility mode parameter with enum type CompatMode,
and implement v0.34 RPC compatibility mode with its different
JSON serialization if the parameter specifies so.

* rpc: eliminate dialect Client traits

Get back to the single Client trait, with the clients supporting
protocol dialects through the compatibility mode selected at runtime.
Add CompatMode support to WebSocketClient and make the web socket
client types back non-generic.

HttpClient gets a set_compat_mode method to update the mode
on the fly. No such thing is provided for WebSocketClient, though,
because the compat mode is set into the subscription driver
and cannot be updated.

* rpc: unit test for CompatMode version parsing

* rpc: make WebSocketConfig struct more usable

Make the fileds public, derive Debug.

* rpc: expose CompatMode::from_version

* rpc: debug received events in WebSocketTransport

* Quick fix for running `rpc-probe` against a Comet 0.37 node

* Fix doc for overriding env variable in rpc-probe README

* rpc: added kvstore test fixtures for 0.37

Obtained with rpc-probe against a Docker image cometbft/cometbft:v0.37.x
(ID 4ab97039d4c42dd67e62ecfe72307e1552f9b9c0e48ec15958197d637f1fdde9).

The fixtures for 0.34 have been moved alongside.

* rpc: adjust some v0_37 tests to match fixtures

Some of the fixtures needed adjusting as well, due to
time-dependent assumptions made by tests, or randomness
that could not be guessed ahead of time (incoming/block_by_hash).

* rpc: fudge data in subscribe_newblock_1

Same as was done in the v0_34 fixtures in the past, it lets us test
parsing of the struct containing events.

* rpc: adjust kvstore_fixtures tests for v0_37

* rpc: fixed and expanded unit tests using fixtures

Use the versioned path to the fixture files,
and add a copy of tests for v0_37 which was previously not covered.

* abci: more sensible default impl of ABCI++ methods

Returning blank responses does not make a good migration path
for applications that are yet unaware of the ABCI++ proposal handling
phases.
Implement the simplest sensible behaviors by default for
Application::prepare_proposal and Application::process_proposal
to fulfill the specification.

* abci: overflow-proof impl of prepare_proposal

* rpc: improve CompatMode enum

Instead of Latest as a variant, have only the explicit list of supported
protocol versions as the enum variants. Get the latest via the associated const fn
CompatMode::latest().

* rpc: builder API for clients

Replace the *Config structs with a proper builder API for better
extensibility.

* Add missing serde bounds on WebSocketClientUrl

* Add missing Display impl to WebSocketClientUrl

* rpc: do version discovery in CLI

---------

Co-authored-by: xla <self@xla.is>
Co-authored-by: Akosh Farkash <aakoshh@gmail.com>
Co-authored-by: Romain Ruetschi <romain@informal.systems>
@mzabaluev
Copy link
Contributor

Similarly, in WebSocket subscription events, there may be a result_finalize_block field alongside result_begin_block/result_end_block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants