-
Notifications
You must be signed in to change notification settings - Fork 226
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
Changes from 32 commits
Commits
Show all changes
88 commits
Select commit
Hold shift + click to select a range
d37f063
proto-compiler: Improve error reporting
mzabaluev ce68dbd
Generate proto from tendermint 0.37.0-alpha.1
mzabaluev 95b962a
proto: sort includes in generated tendermint.rs
mzabaluev c6807d2
proto: generate structs for 0.34 and 0.37
mzabaluev df05a47
tendermint: Remove Evidence::ConflictingHeaders
mzabaluev eb160fb
Add serializers::allow_null
mzabaluev 8e04a06
Adapt domain types to v0.37 and add multi-version conversions.
mzabaluev d33945a
Multi-version protobuf support for request types
mzabaluev 7414aa9
Add ABCI response types for PrepareProposal, ProcessProposal
mzabaluev 30408ed
Multi-version protobuf conversions for response types
mzabaluev df28559
Fix conversion from account::Id to Bytes
mzabaluev ca50ce4
Fix compiler warnings
mzabaluev 3277ac3
light-client: Restore Supervisor::report_evidence
mzabaluev 47e285b
Restore serialization of Block and Evidence
mzabaluev 53a546c
Fix clippy lints
mzabaluev 9b7089c
proto-compiler: Improve copying of generated files
mzabaluev f40b6f4
proto: Regenerate v0_34 with v0.34.22
mzabaluev 276bc6d
Add domain type for RemoteSignerError
mzabaluev 18f51e4
Define more multi-protocol conversions
mzabaluev b18264e
p2p: version-qualify dependencies on protobuf
mzabaluev d1a76a9
Make clippy happy
mzabaluev d397090
Derive Eq where clippy suggests
mzabaluev 4796bc4
tendermint: version-specific Request and Response
mzabaluev 9730636
Remove last version aliases of Request, Response
mzabaluev 1aa5fee
Merge branch 'main' into mikhail/multi-tc-version-support
mzabaluev d7ee353
Re-generate protos with prost-build 0.11.4
mzabaluev 0bea5cf
rpc: add endpoints header and header_by_hash
mzabaluev d7e7100
rpc: Separate Client traits for v0.34 and v0.37
mzabaluev 6a24461
rpc: feature-gate the Client traits
mzabaluev a00a69d
xla/multi-tc-versionsupport/fix (#1254)
xla 14b3080
Merge branch 'main' into mikhail/multi-tc-version-support
mzabaluev 08702c5
tendermint: post-merge fix
mzabaluev 011dc7b
Add missing ConsensusRequest and ConsensusResponse mappings.
aakoshh e6fdbcd
Merge branch 'mikhail/multi-tc-version-support' of github.com:aakoshh…
mzabaluev 1f782ea
rpc: clean up change noise
mzabaluev c2f06e3
Changelog entries for #1193
mzabaluev c2fa95a
Revert "rpc: clean up change noise"
mzabaluev 620aa99
abci: change serialization to unsigned varint
mzabaluev 483be77
abci: changelog notice about varint encoding
mzabaluev 0af8aba
rpc: multi-version support through generics
mzabaluev 0695093
Merge branch 'main' into mikhail/multi-tc-version-support
mzabaluev 91b984d
rpc: remove generic default from request::Wrapper
mzabaluev 6054e3e
Implement both Client dialect traits for websocket
mzabaluev ab16785
Rework Event serialization with helper types
mzabaluev 03eda10
rpc: Refactor subscriptions to support dialects
mzabaluev 6beba5f
rpc: fix the fixture tests
mzabaluev d708fd0
rpc: fix websocket tests
mzabaluev 251ff89
Merge branch 'main' into mikhail/multi-tc-version-support
mzabaluev 580b367
tendermint: Fix test_sign_bytes_compatibility
mzabaluev 0305ba8
Versioned type aliases for WebSocketClient
mzabaluev 75c212f
Fix tools build
mzabaluev 546170e
Remove dialect parameter for SubscriptionClient
mzabaluev 52197f0
Derive PartialEq, Eq on ProdCommitValidator
mzabaluev 1610120
tendermint: restore Serialize impl on Event
mzabaluev 2d864bb
rpc: de-genericized result types
mzabaluev 504bbe7
rpc: make the client module public
mzabaluev e81f7bf
Remove WebSocketClient::*_with_dialect
mzabaluev 064534b
rpc: rename DefaultDialect to LatestDialect
mzabaluev fca0f1a
rpc: dynamic compat mode for HttpClient
mzabaluev 31cf889
rpc: eliminate dialect Client traits
mzabaluev 1c1432e
rpc: Add client::compat::discover
mzabaluev b587e6e
rpc: unit test for CompatMode version parsing
mzabaluev 7691ba0
rpc: make WebSocketConfig struct more usable
mzabaluev 3866d0e
rpc: expose CompatMode::from_version
mzabaluev 62f6e5d
rpc: make mod client::compat private
mzabaluev 4c55808
rpc: account for "v" prefixing Tendermint version
mzabaluev b526b2a
rpc: fix websocket tests
mzabaluev c6c5aa6
rpc: debug received events in WebSocketTransport
mzabaluev f0d9e6a
Quick fix for running `rpc-probe` against a Comet 0.37 node
romac 4f8f122
Fix doc for overriding env variable in rpc-probe README
romac 4deecf4
Merge branch 'main' into mikhail/multi-tc-version-support
mzabaluev ab8b54e
rpc: added kvstore test fixtures for 0.37
mzabaluev bacb64b
rpc: adjust some v0_37 tests to match fixtures
mzabaluev 6946849
rpc: fudge data in subscribe_newblock_1
mzabaluev 295ef2a
rpc: adjust kvstore_fixtures tests for v0_37
mzabaluev 7fedc09
rpc: fixed and expanded unit tests using fixtures
mzabaluev 7e79039
abci: more sensible default impl of ABCI++ methods
mzabaluev 0fe73a1
abci: overflow-proof impl of prepare_proposal
mzabaluev 9e25f3f
Update #1193 changelog for tendermint-rpc changes
mzabaluev 74b8793
Fill out .changelog entry for #1193
mzabaluev f3e161e
rpc: improve CompatMode enum
mzabaluev 84d9cad
rpc: builder API for clients
mzabaluev 7d9572c
Update #1193 changelog about RPC client builders
mzabaluev 679eba1
Change CompatMode::LATEST to a const fn
romac 11c2c82
Add missing serde bounds on WebSocketClientUrl
romac e68643b
Add missing Display impl to WebSocketClientUrl
romac fe4c0ab
Change to self-consuming builder API
romac 2d4a333
rpc: do version discovery in CLI
mzabaluev File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
//! Supervisor and Handle implementation. | ||
|
||
use crossbeam_channel as channel; | ||
use tendermint::evidence::{ConflictingHeadersEvidence, Evidence}; | ||
use tendermint::evidence::Evidence; | ||
|
||
use crate::{ | ||
errors::Error, | ||
|
@@ -284,19 +284,15 @@ impl Supervisor { | |
} | ||
|
||
/// Report the given evidence of a fork. | ||
// TODO: rework to supply LightClientAttackEvidence data | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In progress in my branch |
||
fn report_evidence( | ||
&mut self, | ||
provider: PeerId, | ||
primary: &LightBlock, | ||
witness: &LightBlock, | ||
_primary: &LightBlock, | ||
_witness: &LightBlock, | ||
) -> Result<(), Error> { | ||
let evidence = ConflictingHeadersEvidence::new( | ||
primary.signed_header.clone(), | ||
witness.signed_header.clone(), | ||
); | ||
|
||
self.evidence_reporter | ||
.report(Evidence::ConflictingHeaders(Box::new(evidence)), provider) | ||
.report(Evidence::LightClientAttackEvidence, provider) | ||
.map_err(Error::io)?; | ||
|
||
Ok(()) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note that the defaults will probably not work (they will propose empty blocks and think reject all proposals). The spec has instructions for adaptation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right; sorry about this,
tendermint-abci
has not been my top priority for updating to 0.37. Will fix.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand it, this whole default implementation only provides the most trivial default responses.
It's up to the application to fill the methods in where necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I agree; what I'm trying to say is that by leaving it on defaults, filling in these methods will always be necessary.
I can't vouch for all the other cases, but I would expect for those methods the default values represent an acceptable behaviour, like
check_tx
not finding any error, orend_block
not changing the power table.But with these two new methods, the defaults are not accepted by Tendermint:
prepare_proposal
, that effectively will remove all transactions Tendermint wanted to propose, resulting in an empty block instead. This is accepted but breaks the examples that usebroadcast_tx_commit
, and the following error is logged by Tendermint:Error on broadcastTxCommit module=rpc err="timed out waiting for tx to be included in a block"
ProcessProposal::Unknown
(which is the default, although it could beAccept
) instead ofProcessProposal::Accept
, we get the following error:CONSENSUS FAILURE!!! module=consensus err="ProcessProposal responded with status UNKNOWN"
. That is because Tendermint now considers us insane: we just asked it to propose a block we ourselves do not accept.So, if we want to have a transition to v0.37 where users of the
Application
trait don't have to override unless they have something to change from the default behaviour, and they expect Tendermint to just work, then the default method implementations have a bit more work to do than usual.(NB the specs explicitly say that TC may pass more transactions in
PrepareProposal
than what fits into the maximum size; otherwise the implementation would be a trivial copy).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good points! Let's remove the default implementations of these methods then.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be a breaking change for all the
Application
users.Can the library crate assume these behaviours are sensible on behalf of the applications, unless overridden:
prepare_proposal
;Accept
inprocess_proposal
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aakoshh Would that work for you or would you rather leave out the default impls?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for asking @romac, my vote goes for having a default implementation that copies transactions and accepts proposals, unless overridden, like @mzabaluev said. This is so people don't have to come up with boilerplate to do this every time they implement the trait, similar to how they don't have to override
check_tx
unless they want to actually perform some logic.For example I forked
tower-abci
to point it at this branch oftendermint-rs
and had to implement this logic for the kvstore example, where it seems a bit accidental among all the otherDefault::default()
s. After that I took inspiration from theApplication
trait intendermint-rs
and provided an async version which takes care of these defaults, so I think it's the sensible thing to do.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @aakoshh for your feedback.
The more elaborate (but still very naive) default behaviours have now been implemented in 7e79039