-
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
Changes from 68 commits
d37f063
ce68dbd
95b962a
c6807d2
df05a47
eb160fb
8e04a06
d33945a
7414aa9
30408ed
df28559
ca50ce4
3277ac3
47e285b
53a546c
9b7089c
f40b6f4
276bc6d
18f51e4
b18264e
d1a76a9
d397090
4796bc4
9730636
1aa5fee
d7ee353
0bea5cf
d7e7100
6a24461
a00a69d
14b3080
08702c5
011dc7b
e6fdbcd
1f782ea
c2f06e3
c2fa95a
620aa99
483be77
0af8aba
0695093
91b984d
6054e3e
ab16785
03eda10
6beba5f
d708fd0
251ff89
580b367
0305ba8
75c212f
546170e
52197f0
1610120
2d864bb
504bbe7
e81f7bf
064534b
fca0f1a
31cf889
1c1432e
b587e6e
7691ba0
3866d0e
62f6e5d
4c55808
b526b2a
c6c5aa6
f0d9e6a
4f8f122
4deecf4
ab8b54e
bacb64b
6946849
295ef2a
7fedc09
7e79039
0fe73a1
9e25f3f
74b8793
f3e161e
84d9cad
7d9572c
679eba1
11c2c82
e68643b
fe4c0ab
2d4a333
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
- [`tendermint`] Version-specific definitions for ABCI `Request` and `Response` | ||
enums under `v0_34::abci` and `v0_37::abci`, containing only the method variants | ||
present in each of the respective protocol versions. | ||
`Request` and `Response` defined under `v0_37` are re-exported under | ||
the non-versioned `abci` module name, but the `SetOption` variant is not present | ||
in these latest versions of the enums. | ||
([#1193](https://github.com/informalsystems/tendermint-rs/pull/1193)) | ||
- [`tendermint-abci`] Change the frame length encoding in the ABCI wire protocol | ||
to unsigned varint, to correspond to the changes in Tendermint Core 0.37. | ||
No compatibility with 0.34 is provided at the moment. | ||
([#1193](https://github.com/informalsystems/tendermint-rs/pull/1193)). |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
- [`tendermint-proto`] Generate prost bindings for Tendermint 0.34 and 0.37 side by side. | ||
The version-specific structs are placed under the `tendermint::v0_34` and | ||
`tendermint::v0_37` module namespaces, respectively. The names under | ||
`tendermint::v0_37` are also re-exported under `tendermint`. | ||
([#1193](https://github.com/informalsystems/tendermint-rs/pull/1193)) | ||
- [`tendermint`] New and updated ABCI domain types for Tendermint Core v0.37 | ||
([#1193](https://github.com/informalsystems/tendermint-rs/pull/1193)). | ||
- [`tendermint`] Protobuf conversions provided for both `v0_34` and `v0_37` | ||
versions of the generated [`tendermint-proto`] structs, where applicable. | ||
([#1193](https://github.com/informalsystems/tendermint-rs/pull/1193)). | ||
- [`tendermint-rpc`] Provide separate `Client` traits for Tendermint Core 0.34 | ||
and 0.37, placed under the `v0_34::client` and `v0_37::client` modules. | ||
The latest version is re-exported as `crate::Client`. | ||
The websocket and HTTP client implement both traits, it's up to the | ||
application which one to import for use. | ||
([#1193](https://github.com/informalsystems/tendermint-rs/pull/1193)) | ||
- [`tendermint-abci`] Port ABCI application support to 0.37 Tendermint Core API. | ||
No legacy support for 0.34 is provided at the moment. | ||
([#1193](https://github.com/informalsystems/tendermint-rs/pull/1193)). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ use std::{ | |
|
||
use bytes::{Buf, BufMut, BytesMut}; | ||
use prost::Message; | ||
use tendermint_proto::abci::{Request, Response}; | ||
use tendermint_proto::v0_37::abci::{Request, Response}; | ||
|
||
use crate::error::Error; | ||
|
||
|
@@ -130,7 +130,7 @@ where | |
message.encode(&mut buf).map_err(Error::encode)?; | ||
|
||
let buf = buf.freeze(); | ||
encode_varint(buf.len() as u64, &mut dst); | ||
prost::encoding::encode_varint(buf.len() as u64, &mut dst); | ||
dst.put(buf); | ||
Ok(()) | ||
} | ||
|
@@ -142,11 +142,11 @@ where | |
{ | ||
let src_len = src.len(); | ||
let mut tmp = src.clone().freeze(); | ||
let encoded_len = match decode_varint(&mut tmp) { | ||
let encoded_len = match prost::encoding::decode_varint(&mut tmp) { | ||
Ok(len) => len, | ||
// We've potentially only received a partial length delimiter | ||
Err(_) if src_len <= MAX_VARINT_LENGTH => return Ok(None), | ||
Err(e) => return Err(e), | ||
Err(e) => return Err(Error::decode(e)), | ||
}; | ||
let remaining = tmp.remaining() as u64; | ||
if remaining < encoded_len { | ||
|
@@ -164,14 +164,3 @@ where | |
Ok(Some(res)) | ||
} | ||
} | ||
|
||
// encode_varint and decode_varint will be removed once | ||
// https://github.com/tendermint/tendermint/issues/5783 lands in Tendermint. | ||
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. Has this landed in 0.34? 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. No, it's a breaking change from 0.34. It was originally supposed to be included in 0.35. I don't know if it's slated for inclusion in 0.37. 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. The encoding has changed in 0.37, and in this PR, 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. We could also choose to commit to multi-version support here as well, but this will require more work. |
||
pub fn encode_varint<B: BufMut>(val: u64, mut buf: &mut B) { | ||
prost::encoding::encode_varint(val << 1, &mut buf); | ||
} | ||
|
||
pub fn decode_varint<B: Buf>(mut buf: &mut B) -> Result<u64, Error> { | ||
let len = prost::encoding::decode_varint(&mut buf).map_err(Error::decode)?; | ||
Ok(len >> 1) | ||
} |
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(()) | ||
|
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