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

Side-by-side support for Tendermint 0.34 and 0.37 #1193

Merged
merged 88 commits into from
Mar 2, 2023

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Sep 20, 2022

ADR: #1274

A cross-chain bridge agent, such as a relayer, may need to communicate with nodes using different Tendermint versions. To enable this use with a single linked instance of tendermint-rpc /tendermint-abci crate, and also to disentangle tendermint-rs versioning from the versions of the Tendermint Core protocol, change tendermint-proto to have generated protobuf structs for all supported tendermint versions in side-by-side modules, and adapt the higher level client APIs to be able to select the version expected from the node.

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Added entry in .changelog/

@mzabaluev mzabaluev added enhancement New feature or request rpc abci labels Sep 20, 2022
@mzabaluev
Copy link
Contributor Author

mzabaluev commented Sep 20, 2022

in my understanding, an MVP of multi-version tendermint-rs may need the following changes:

  • Adapt the tendermint-abci client to be able to talk both versions.
    • It is not used by Hermes, are there other customers for this?
    • The Application/ Server part can just serve the latest supported version; I see no use case for multi-version here.
  • Add support for the RPC endpoints new in 0.37 to tendermint-rpc and conditionally disable them for the 0.34 mode, or just rely on the node to respond with an error.

@hdevalence
Copy link
Collaborator

I don't think it's currently possible to implement support for both 0.34 and 0.37 on main, because main does not even have support for 0.34 -- ABCI support only exists for 0.35. Before trying to add support for 0.37, the main branch should have support for 0.34. Merging something like this now will generate even more massive conflicts.

@mzabaluev
Copy link
Contributor Author

I think it should be possible to have limited legacy support for 0.34 in tendermint-rs, by generating the v0_34 structs in tendermint-proto, implementing domain type conversions for these, and adapting tendermint-rpc to be able to interop with 0.34 nodes (which should be pretty much there due to backward compatibility). As we discussed with @thanethomson, we are not interested in multi-version adaptations for tendermint-abci at the moment.

@mzabaluev mzabaluev force-pushed the mikhail/multi-tc-version-support branch from 820c013 to fae22f9 Compare October 19, 2022 16:57
@mzabaluev mzabaluev force-pushed the mikhail/multi-tc-version-support branch from f52b30b to c44c2de Compare October 27, 2022 06:29
@codecov-commenter
Copy link

codecov-commenter commented Oct 27, 2022

Codecov Report

Merging #1193 (b40a935) into main (0c37d3a) will decrease coverage by 0.2%.
The diff coverage is 75.5%.

❗ Current head b40a935 differs from pull request most recent head 7d9572c. Consider uploading reports for the commit 7d9572c to get more accurate results

@@           Coverage Diff           @@
##            main   #1193     +/-   ##
=======================================
- Coverage   64.3%   64.2%   -0.2%     
=======================================
  Files        250     271     +21     
  Lines      21618   24268   +2650     
=======================================
+ Hits       13915   15583   +1668     
- Misses      7703    8685    +982     
Impacted Files Coverage Δ
abci/src/application.rs 13.0% <0.0%> (-5.9%) ⬇️
abci/src/client.rs 41.6% <ø> (+1.6%) ⬆️
abci/src/error.rs 0.0% <ø> (-92.7%) ⬇️
abci/tests/echo_app.rs 100.0% <ø> (ø)
abci/tests/kvstore_app.rs 100.0% <ø> (ø)
p2p/src/secret_connection.rs 86.8% <ø> (ø)
p2p/src/secret_connection/amino_types.rs 0.0% <ø> (ø)
p2p/src/secret_connection/protocol.rs 59.8% <ø> (ø)
proto/src/serializers/allow_null.rs 0.0% <0.0%> (ø)
proto/tests/unit.rs 94.8% <ø> (ø)
... and 162 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@mzabaluev mzabaluev force-pushed the mikhail/multi-tc-version-support branch from 2e4d38b to c72063e Compare November 14, 2022 12:20
@mzabaluev mzabaluev force-pushed the mikhail/multi-tc-version-support branch from d7ea496 to c8505bd Compare November 20, 2022 17:39
@mzabaluev mzabaluev force-pushed the mikhail/multi-tc-version-support branch from 950205c to e1e7a43 Compare November 28, 2022 12:09
Print out the prost-build error to display protoc output line by line
rather than as an unreadable Debug dump.
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.
This normalizes the content of the module for protocol updates.
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::*.
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.
Keep it as a stub to avoid more extensive changes.
Define hand-written Serialize/Deserialize impls for Evidence.
- Filter out the empty non-Tendermint files.
- Improve use of the WalkDir iterator.
- Use OS-agnostic path construction.
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.
rpc/src/client/transport/http.rs Outdated Show resolved Hide resolved
rpc/src/client/transport/websocket.rs Outdated Show resolved Hide resolved
},
TendermintVersion {
ident: "v0_37",
commitish: "v0.37.0-alpha.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the last tag in Tendermint Core where the proto files had bundled Google well known types, before switching to buf to refer to dependencies. Adapting to buf will require more changes to the proto-compiler tool, and I have already done this already once in an older branch.

Here I'm leaving the migration to buf, and switching to CometBFT, as follow-on tasks.
There is no difference in protobuf message formats with the latest state in CometBFT branch v0.37.x (as per commit d26bdfe1d424ff24c9676fee5bef9a180870315e).

@romac romac added the breaking label Feb 28, 2023
rpc/src/client/compat.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@mzabaluev mzabaluev left a comment

Choose a reason for hiding this comment

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

Notes from the review call:

tendermint/src/abci/event.rs Show resolved Hide resolved
mzabaluev and others added 4 commits February 28, 2023 18:40
Instead of Latest as a variant, have only the explicit list of supported
protocol versions as the enum variants. Make LATEST an associated const.
Replace the *Config structs with a proper builder API for better
extensibility.
Copy link
Member

@romac romac left a comment

Choose a reason for hiding this comment

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

Amazing work there @mzabaluev! I think we can finally get this in :)

I have one question left, but I don't believe it should block this PR from being merged.

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

Successfully merging this pull request may close these issues.

8 participants