RPC support for CometBFT 0.38 #1317

mzabaluev · 2023-05-19T17:03:50Z

The RPC changes for #1301

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/

In the v0_37+ dialect helper, support the new finalize_block_events field if it is present. Also default the begin_block_events and end_block_events fields to None if they are not present.

For abci::Event and abci::response::CheckTx, restore the Deserialize implementation on domain types themselves. The event serialization format as used in 0.37+ will be established as the "canonical" serialization.

Add the new fields and types without static differentiation for 0.38.x. Use the domain type ExecTxResult in deserializing the endpoint response types in the current (i.e. 0.37+) dialect for these endpoints: - /block_results - /broadcast_tx_commit - /tx - /tx_commit The Deserialize impls are restored on the response types, corresponding to the JSON schema for the current RPC dialect. Simplify the 0.34 dialect support for these responses, no longer using generics. In endpoint::tx_commit::Response, rename the deliver_tx field to tx_result to correspond to the current JSON schema. The Deserialize impl falls back to deliver_tx to also be able to parse 0.37 responses.

Some serde attributes were not migrated onto the ExecTxResult domain type when it has been tasked with deserializing the latest RPC dialect. On abci::response::CheckTx, add serde(default) on fields that are no longer present in CometBFT 0.38.

If the finalize_block_events is not present, default to an empty vector. Conversely, skip the field from serializing if it's set to an empty vector.

BeginBlock, EndBlock, FinalizeBlock are used to deserialize RPC responses.

Now that event data can have new fields, distinguish this with a new NewBlock enum variant, renaming the old one to LegacyNewBlock. The dialect serialization helpers for subscription events are separated to v0_34::DialectEvent, that does not recognize the 0.38 fields at all and parses the event attributes as base64-encoded, and latest::DialectEvent, which has provisions for 0.38 fields if they are present, and in this case converts to the NewBlock variant.

codecov-commenter · 2023-05-25T18:27:33Z

Codecov Report

Merging #1317 (e6e0285) into main (dba8242) will increase coverage by 1.6%.
The diff coverage is 88.2%.

❗ Current head e6e0285 differs from pull request most recent head 18ccbcc. Consider uploading reports for the commit 18ccbcc to get more accurate results

@@           Coverage Diff           @@
##            main   #1317     +/-   ##
=======================================
+ Coverage   58.1%   59.8%   +1.6%     
=======================================
  Files        281     283      +2     
  Lines      25478   26895   +1417     
=======================================
+ Hits       14824   16093   +1269     
- Misses     10654   10802    +148

Impacted Files	Coverage Δ
rpc/src/client/bin/main.rs	`0.3% <0.0%> (-0.1%)`	⬇️
rpc/src/dialect/v0_37.rs	`0.0% <ø> (-71.8%)`	⬇️
rpc/src/response.rs	`59.5% <ø> (ø)`
rpc/tests/kvstore_fixtures.rs	`100.0% <ø> (ø)`
rpc/src/endpoint/broadcast/tx_commit.rs	`42.3% <55.5%> (-16.6%)`	⬇️
rpc/src/event.rs	`55.0% <59.7%> (+5.8%)`	⬆️
rpc/tests/kvstore_fixtures/v0_37.rs	`93.9% <85.0%> (+<0.1%)`	⬆️
rpc/tests/kvstore_fixtures/v0_34.rs	`98.2% <87.8%> (-0.1%)`	⬇️
rpc/tests/kvstore_fixtures/v0_38.rs	`93.8% <93.8%> (ø)`
rpc/src/client/transport/websocket.rs	`64.4% <98.7%> (+2.3%)`	⬆️
... and 15 more

... and 5 files with indirect coverage changes

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

For consistency, define event::v0_37 and event::v0_38, aliasing the public definitions in event::latest.

🤦‍♂️

romac

Amazing work @mzabaluev! I just have some questions which I've left inline.

romac · 2023-06-01T07:40:14Z

rpc/src/client/compat.rs

@@ -49,6 +49,7 @@ impl CompatMode {
        match (version.major, version.minor) {
            (0, 34) => Ok(CompatMode::V0_34),
            (0, 37) => Ok(CompatMode::V0_37),
+            (0, 38) => Ok(CompatMode::V0_37),


This seems a bit counterintuitive, though I guess if the RPC client does not differentiate between 0.37 and 0.38 it makes sense.

I wonder though if we could rather add a CompatMode variant for 0.38 and then change the logic at use site to check whether the compat mode is set to 0.3X or higher on case-per-case basis, eg. with compat_mode >= CompatMode::v0_37 for when we want to check if the version is higher or equal to v0.37.

Correct, the RPC "dialect" does not change enough between 0.37 and 0.38 that we have to specially switch anything in order to talk to any node built on either of these versions. Naming could be improved; I tried to play with the Latest compact mode name in the past, perhaps anticipating this ambiguity.

romac · 2023-06-01T07:41:29Z

rpc/src/endpoint/broadcast/tx_commit.rs

+    ///
+    /// The JSON field carrying this data is named `deliver_tx` in
+    /// CometBFT versions before 0.38.
+    #[serde(alias = "deliver_tx")]


Let's perhaps add a comment to the whole struct to specify why we cannot derive Serialize here (ie. because of the field name change between <0.38 and >=0.38.

But we do derive Serialize on the struct, and it will serialize to the latest RPC schema, 0.38.
The DialectResponse from the submodule can be used to serialize in the 0.34 schema.

And thanks for bringing this up: this means we have nothing to serialize with for 0.37 JSON-RPC if anyone wants to. The RPC client does not use Serialize for responses, so I've been treating it as an afterthought. Maybe there should be another compatibility submodule just to enable this.

romac · 2023-06-01T11:15:57Z

rpc/src/event.rs

-            gas_used: msg.gas_used,
-            events: msg.events.into_iter().map(Into::into).collect(),
+/// Serialization helpers for the RPC protocol used since CometBFT 0.37
+pub mod latest {


Rather than have the latest module define directly items for the latest supported release, can we instead name it with the version for that release and have the latest module re-exports everything from that module?

The helpers in this module are actually a grab bag for deserializing anything that might be present in 0.37 or 0.38.
On the other hand, the Serialize impl can also emit any fields that are not set to None, which is rather sloppy.

I'll think on how to improve this in a more principled way.

romac · 2023-06-01T11:17:06Z

tendermint/src/abci/response/finalize_block.rs

    pub consensus_param_updates: Option<consensus::Params>,
+    // FIXME: decide if base64 is the canonical serialization for AppHash,


Just a note that there is a remaining FIXME here

Herein lies a question: do we want a "normal" way to serialize AppHash, even though in the current protocol there are instances of both hex and base64 encoding? I've asked this on Slack, but nobody provided an answer.

romac · 2023-06-01T11:18:20Z

rpc/src/event.rs

 #[derive(Debug, Clone, PartialEq, Eq)]
-// To be fixed in 0.24
-#[allow(clippy::large_enum_variant)]
 pub enum EventData {


Have we considered making EventData part of the dialect rather than introduce a legacy variant for NewBlock?

I thought of making the NewBlock variant in this API type carry both 0.37 and 0.38 fields as optionals just like in latest::DialectEventData, and decoding to a different set of fields getting Some values for each respective version, but this feels like we'd be ramifying the enum matching that typically needs to be done by the consumer across multiple levels of the data structure.
So I decided a more clean split of the old and the new data is needed, also because we should eventually deprecate and remove LegacyNewBlock when everything up to 0.37 is EOLd.

Or do you have another idea about the API for this? Consider the dialect stuff in submodules an implementation detail for decoding JSON-RPC; I think the whole dialect business should be made private, the only reason I haven't made it private is the "integration" tests that decode fixtures, but those more properly belong in unit tests.

NB: Hermes currently does not care about the changed fields at all, it just needs block as can be seen in informalsystems/hermes#3372.

The Serialize impl needs to be implemented differently for 0.37 and 0.38 in order to emit old or new fields for NewBlock. The Deserialize impl, however, flexibly handles both formats to avoid introducing another dialect where most other helpers would be identical. This means that Event gets two different helpers for each protocol version: DeEvent for Deserialize and SerEvent for Serialize.

Provide v0_37::DialectResponse as a way to serialize responses in the 0.37 JSON format.

Use the exact protocol version to serialize test server responses as appropriate. No new variant is added to CompatMode, as that is a client-side setting and the client is able to cope with both versions dynamically.

mzabaluev · 2023-06-07T09:19:35Z

rpc/src/client/transport/websocket.rs

+    enum TestRpcVersion {
+        V0_34,
+        V0_37,
+        V0_38,
+    }


It's almost like CompatMode could be given the new V0_38 variant as well, but it's avoidable on the client side and I think it would result in lots of unnecessary code duplication.

No opinions have been expressed about any canonical way to serialize app hashes, so the explicit serializer helper should be there for now.

mzabaluev added 6 commits May 19, 2023 19:57

rpc: 0.38 support for /block_results endpoint

9962a4f

In the v0_37+ dialect helper, support the new finalize_block_events field if it is present. Also default the begin_block_events and end_block_events fields to None if they are not present.

rpc: add app_hash field to /block_results response

8328820

tendermint: restore Deserialize on some types

320decd

For abci::Event and abci::response::CheckTx, restore the Deserialize implementation on domain types themselves. The event serialization format as used in 0.37+ will be established as the "canonical" serialization.

tendermint: serde impls for ExecTxResult

ad73c4b

rpc: recognize 0.38 in CompatMode::from_version

14cda47

mzabaluev added rpc domain-types Anything relating to the creation, modification or removal of domain types labels May 19, 2023

mzabaluev requested a review from romac May 19, 2023 17:03

mzabaluev added 5 commits May 22, 2023 14:07

Fix CompatMode parsing test

ba70456

Fix deserialization of RPC results

c8d8497

Some serde attributes were not migrated onto the ExecTxResult domain type when it has been tasked with deserializing the latest RPC dialect. On abci::response::CheckTx, add serde(default) on fields that are no longer present in CometBFT 0.38.

rpc: 0.37 compat on /block_results response

009e696

If the finalize_block_events is not present, default to an empty vector. Conversely, skip the field from serializing if it's set to an empty vector.

Add kvstore fixtures for 0.38

7ba4b1c

Derive serde impls for abci responses

057f169

BeginBlock, EndBlock, FinalizeBlock are used to deserialize RPC responses.

mzabaluev mentioned this pull request May 24, 2023

Multi-version support for CometBFT v0.38 #1301

Closed

mzabaluev added 4 commits May 25, 2023 17:13

base64 for app_hash in JSON of FinalizeBlock

07de311

rpc: adjusted tests for kvstore_fixtures/v0_38

c37c6c3

clippy fix

2cf5d78

Fix up kvstore-test

3c12478

mzabaluev mentioned this pull request May 26, 2023

Update to tendermint-rs 0.33.0 and adapt to RPC changes in CometBFT 0.38 informalsystems/hermes#3372

Merged

7 tasks

mzabaluev added 3 commits May 29, 2023 15:36

rpc: CometBFT 0.38 compat notes on new fields

e85d346

rpc: version-alias latest event serde helpers

a2015fb

For consistency, define event::v0_37 and event::v0_38, aliasing the public definitions in event::latest.

Changelog for #1317

fa7d4b1

mzabaluev marked this pull request as ready for review May 29, 2023 14:09

mzabaluev added 3 commits May 29, 2023 18:18

rpc: swap around dialect parsing in websocket

2406f5a

rpc: swap around dialects in websocket test

77b5b63

🤦‍♂️

rpc: websocket_client_happy_path test for 0.38

c00766a

romac reviewed Jun 1, 2023

View reviewed changes

mzabaluev added 3 commits June 7, 2023 00:55

rpc: 0.37 serialization for /broadcast_tx_commit

569db8c

Provide v0_37::DialectResponse as a way to serialize responses in the 0.37 JSON format.

rpc: Fix 0.38 websocket test by proper emulation

8d58a1d

Use the exact protocol version to serialize test server responses as appropriate. No new variant is added to CompatMode, as that is a client-side setting and the client is able to cope with both versions dynamically.

mzabaluev commented Jun 7, 2023

View reviewed changes

mzabaluev added 2 commits June 7, 2023 13:57

Merge branch 'main' into mikhail/cometbft-rpc-0.38

3d3dac8

Remove a FIXME comment

51e47d0

No opinions have been expressed about any canonical way to serialize app hashes, so the explicit serializer helper should be there for now.

romac approved these changes Jun 13, 2023

View reviewed changes

mzabaluev added 3 commits June 14, 2023 13:12

Merge branch 'main' into mikhail/cometbft-rpc-0.38

3d05019

rpc: add new fields to 0.38 fixture test

18ccbcc

Update changelog for #1317

e5f141b

mzabaluev merged commit 52f3d8d into main Jun 14, 2023

mzabaluev deleted the mikhail/cometbft-rpc-0.38 branch June 14, 2023 12:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RPC support for CometBFT 0.38 #1317

RPC support for CometBFT 0.38 #1317

mzabaluev commented May 19, 2023 •

edited

Loading

codecov-commenter commented May 25, 2023 •

edited

Loading

romac left a comment

romac Jun 1, 2023

mzabaluev Jun 1, 2023

romac Jun 1, 2023

mzabaluev Jun 1, 2023 •

edited

Loading

romac Jun 1, 2023

mzabaluev Jun 1, 2023

romac Jun 1, 2023

mzabaluev Jun 1, 2023 •

edited

Loading

romac Jun 1, 2023

mzabaluev Jun 1, 2023

mzabaluev Jun 7, 2023

		pub consensus_param_updates: Option<consensus::Params>,
		// FIXME: decide if base64 is the canonical serialization for AppHash,

RPC support for CometBFT 0.38 #1317

RPC support for CometBFT 0.38 #1317

Conversation

mzabaluev commented May 19, 2023 • edited Loading

codecov-commenter commented May 25, 2023 • edited Loading

Codecov Report

romac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mzabaluev commented May 19, 2023 •

edited

Loading

codecov-commenter commented May 25, 2023 •

edited

Loading

mzabaluev Jun 1, 2023 •

edited

Loading

mzabaluev Jun 1, 2023 •

edited

Loading