-
Notifications
You must be signed in to change notification settings - Fork 224
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
Removed amino folder #592
Removed amino folder #592
Conversation
This commit fixes the header hashing mechanism to match that of the current Go version. It also adds a test to ensure the happy path works. Signed-off-by: Thane Thomson <[email protected]>
Demonstrates that the previous commit (fixing the header hash) fixes #600 when executed against a running Tendermint node. Signed-off-by: Thane Thomson <[email protected]>
Signed-off-by: Thane Thomson <[email protected]>
Signed-off-by: Thane Thomson <[email protected]>
Prost seems to encode an empty raw protobuf BlockId as an empty vector of bytes, whereas the Go encoder encodes the same object to [18, 0]. This commit adds a workaround to demonstrate that this is the case when testing against Tendermint v0.34 (otherwise header hashing would fail). Signed-off-by: Thane Thomson <[email protected]>
It seems as though the previous commit data was generated by an older version of Tendermint. The new commit fixtures here are generated by Tendermint 0.34.0-99aea7b0 running in a Docker container on my local machine. Signed-off-by: Thane Thomson <[email protected]>
Signed-off-by: Thane Thomson <[email protected]>
Signed-off-by: Thane Thomson <[email protected]>
Unfortunately one of the model-based tests (single_step_tests) in light-client started acting up since a merge from master: the commit_hash is invalid. I think those commit hashes might have been created with the old tendermint source code before Thane's fixes. |
Codecov Report
@@ Coverage Diff @@
## master #592 +/- ##
========================================
+ Coverage 41.3% 42.4% +1.0%
========================================
Files 163 166 +3
Lines 11311 11220 -91
Branches 2559 2527 -32
========================================
+ Hits 4682 4763 +81
+ Misses 6258 6081 -177
- Partials 371 376 +5
Continue to review full report at Codecov.
|
pub trait SignableMsg { | ||
/// Sign this message as bytes | ||
fn sign_bytes<B: BufMut>( | ||
&self, | ||
chain_id: chain::Id, | ||
sign_bytes: &mut B, | ||
) -> Result<bool, EncodeError>; | ||
|
||
/// Set the Ed25519 signature on the underlying message | ||
fn set_signature(&mut self, sig: &ed25519::Signature); | ||
fn validate(&self) -> Result<(), validate::Error>; | ||
fn consensus_state(&self) -> Option<consensus::State>; | ||
fn height(&self) -> Option<i64>; | ||
fn msg_type(&self) -> Option<SignedMsgType>; | ||
} |
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.
Will there be an equivalent of this trait? Currently, some of the types have to_signable_bytes
, so the trait implementations can just use that.
In tmkms, it's used for over different requests: https://github.com/iqlusioninc/tmkms/blob/develop/src/session.rs#L171
Or is it expected that the equivalent of this trait would be moved to tmkms?
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.
I'll be honest, I didn't see the need for a trait here. (Your link shows that this might be incorrect, but I didn't find that specific part of the KMS code before.)
Since the trait is only used in two types: Vote and Proposal, the trait implementation is a lot of extra code for very little benefit. (Unfortunately, traits are not interfaces.)
I'm going to go through that code to see what the expectations are. One option I would like to investigate is a message type enum that includes all types of messages. In that case the code can be replaced with a match message {}
and call the functions in there. Since we're talking about two types of structs, this is not that overbearing. (If the number of structs can explode, then maybe the trait is a good call.)
If this doesn't pan out, we can always reintroduce the trait.
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.
Ah, I think I found it: https://github.com/iqlusioninc/tmkms/blob/3e3ae75fee35d1d81258ed61c286b2efb240be40/src/session.rs#L113
You can see that when a request is identified as a Vote or a Proposal, it calls the generic .sign()
function to get the signable bytes. But if we already identified the structs, there's no need to generalize over them any more.
Instead of
Request::SignProposal(req) => self.sign(req)?,
it could be something like:
Request::SignProposal(req) => SignProposalRequest::try_from(req)?.to_signable_bytes(),
where the try_from ensures that the incoming data is valid.
This removes both the trait and the generic sign
function and we got a clearer understanding of what's happening when calling that code.
Does this way of thinking help? Honest question. I feel it's clearer to me but I understand it's a different concept than previous iterations.
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.
Extra note: I went further into the code and I see that the req
is an amino_type (or as we call it in the new version, a Raw type). I have a feeling that it could be upgraded into the domaintype even and all checks of incoming data moved into the read
function. The benefits of domain types are that there is a strict input check when creating the domain type and it can happen very early in the process. By the time the .to_signable_bytes()
function has to be called, you might have a valid type already with no need to check for invalid data.
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.
I'll try to look more into tmkms, but yes, I think using the domain types may make the code simpler there
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.
I'll just say I'm not a fan of this trait either
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.
Wow, what an undertaking! Thank you so much for putting in all this work.
Just went through the Rust code, everything looks good to me, only left some minor comments.
Let's wait until someone can review the changes in the TLA models before approving.
Thanks Romain for the great feedback! Your observations are appreciated, I'll fix the code tomorrow. Most of the suboptimal code is because of late-night coding. Some of the suboptimal code is copy-pasted from the earlier amino_types folder, we can and should fix those too. I was looking for that transpose function, thanks for pointing it out. |
Great stuff, @greg-szabo! Now it looks like the model-based tests are failing because |
tendermint/src/vote/sign_vote.rs
Outdated
} | ||
|
||
/// SignedVoteResponse is a response containing a signed vote or an error | ||
#[derive(Clone, PartialEq)] |
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.
#[derive(Clone, PartialEq)] | |
#[derive(Clone, PartialEq, Debug)] |
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, exactly the type of feedback I need to make the lib more usable. Added to the next commit.
} | ||
|
||
/// SignedProposalResponse is response containing a signed proposal or an error | ||
#[derive(Clone, PartialEq)] |
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.
#[derive(Clone, PartialEq)] | |
#[derive(Clone, PartialEq, Debug)] |
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, added to the next commit
CanonicalProposal::new(self.clone(), chain_id).encode_length_delimited_vec() | ||
} | ||
|
||
/// Consensus state from this proposal - This doesn't seem to be used anywhere. |
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 is used in tmkms via that trait: https://github.com/iqlusioninc/tmkms/blob/develop/src/session.rs#L315
but I guess it could be simpler / clearer without it
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 think so too. It's a similar case to the previous. It seems it's generating a State{}
struct and matching whatever the incoming request was. We can easily do that with a
consensus_state = State::from(vote);
or consensus_state = State::from(proposal);
call. Which tells me that implementing this From trait would be a good idea. I'll check the KMS code a bit further, I'm interested how this state is used.
|
||
/// Maximum length of a `chain::Id` name. Matches `MaxChainIDLen` from: | ||
/// <https://github.com/tendermint/tendermint/blob/develop/types/genesis.go> | ||
// TODO: update this when `chain::Id` is derived from a digest output | ||
pub const MAX_LENGTH: usize = 50; | ||
|
||
/// Chain identifier (e.g. 'gaia-9000') | ||
#[derive(Copy, Clone)] | ||
pub struct Id([u8; MAX_LENGTH]); | ||
#[derive(Clone)] |
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.
#[derive(Clone)] | |
#[derive(Copy, Clone)] |
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, unfortunately won't work, because the internally implemented String
does not have the Copy
trait. If you need bytes, you can use as_bytes()
or as_str()
. (You can read about the issue with the Copy trait and String in the Rust book. It's a size-in-memory thing.)
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.
Alternatively, we can revert back to using [u8] internally but it seemed to me that String
is a better representation with the added functions. Up to the team to decide.
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.
Awesome, thanks Greg! This is just an initial review, haven't gone through it all, but let's merge and continue to address anything else in a follow up
@@ -35,7 +35,6 @@ contracts = "0.4.0" | |||
crossbeam-channel = "0.4.2" | |||
derive_more = "0.99.5" | |||
futures = "0.3.4" | |||
prost-amino = "0.6.0" |
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.
🔥
(low.value() + (high.value() + 1 - low.value()) / 2).into() | ||
(low.value() + (high.value() + 1 - low.value()) / 2) | ||
.try_into() | ||
.unwrap() // Will panic if midpoint is higher than i64::MAX |
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.
Can we label these with something consistent so it's easy to find them in the future if we change how we handle this? Maybe should be an XXX or something easy to find...
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 idea, although in this case it's more of a theoretical possibility than a real one.
You can get the biggest number with high.value()==i64::MAX
and midpoint is going to be i64::MAX/2
in that case. So this is an unwrap case that can never fail, unless the developer changes the types without concern to the note.
Maybe it's better to remove it, since it's theoretical only.
@@ -208,7 +208,7 @@ fn broadcast_tx_commit_null_data() { | |||
fn commit() { | |||
let response = endpoint::commit::Response::from_string(&read_json_fixture("commit")).unwrap(); | |||
let header = response.signed_header.header; | |||
assert_eq!(header.chain_id.as_ref(), EXAMPLE_CHAIN); | |||
assert_eq!(header.chain_id.as_ref(), "dockerchain"); |
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.
Why not keep EXAMPLE_CHAIN or else why not make a variable for this?
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.
Uh, I'm not sure, it was completely fine as it was. I don't think it was changed by me, it seems it was changed when the commit.json was regenerated. (The previous one had cosmoshub-2
as the chain ID and the current one is dockerchain
.) I don't mind it this way because most other assertions use constants to check validity. But we can revert to a const too.
type Error = Error; | ||
|
||
fn try_from(value: i64) -> Result<Self, Self::Error> { | ||
Ok(Height(value.try_into().map_err(|_| Kind::NegativeHeight)?)) |
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 catches negative numbers?
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, it's a kind of Rust inherent magic: Height is defined as Height(u64)
. Hence the value's try_from()
method is going to try to convert the number to u64 from the original i64. This fails if the value is negative (if it's positive it fits without overflow) so the resultant error is because the height was negative. The TryInto trait's error is not compatible with the defined Self::Error, so it has to be converted.
This can fail miserably if/when we redefine the Height newtype. We can change it to u64::try_from(value)
which is more explicit and resolves into the same code.
} | ||
} | ||
|
||
impl TryFrom<RawCanonicalBlockId> for Id { |
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.
Do we actually need this anywhere? I would have thought we only have to marshal from domain types to the canonical type, but we never need to go back from canonical types, since the canonical types are just for signing - maybe I'm missing something?
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.
We don't use it. There are multiple weak reasons why it's there:
- Symmetric implementation of all conversions:
Domain::TryFrom(Raw)
andRaw::From(Domain)
. This sets a good precedent for the developer and standard expectations for all types and conversions. It's a weak reason because if we know that we don't use one conversion, we could simply omit it from the code. I kept it because this is a library and I wasn't sure who uses it for what. - The above symmetry is a prerequisite for the domaintype trait. It's a weak argument, because the trait is only available with exactly one raw type, and in this case, it's implemented with the raw BlockId and not the CanonicalBlockId. So, there's no real need for the same symmetric implementation because the DomainType trait doesn't really depend on it.
So it's there for good measure. We can remove it, if unnecessary.
|
||
/// Maximum length of a `chain::Id` name. Matches `MaxChainIDLen` from: | ||
/// <https://github.com/tendermint/tendermint/blob/develop/types/genesis.go> | ||
// TODO: update this when `chain::Id` is derived from a digest output | ||
pub const MAX_LENGTH: usize = 50; | ||
|
||
/// Chain identifier (e.g. 'gaia-9000') | ||
#[derive(Copy, Clone)] | ||
pub struct Id([u8; MAX_LENGTH]); |
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.
Do we have any idea why this didn't just use a String before?
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.
Recording it for any outsiders:
We discussed this on Slack and the reason might be because of the Copy
trait (which was removed when this became a String). The Copy
trait is not available for Strings. But Strings are more convenient to use in the code and the Clone trait is good enough for copying the structure where necessary.
step: 0, | ||
block_id: None, | ||
}; | ||
|
||
let oldest = State { | ||
height: block::Height::default(), | ||
round: 0, | ||
round: block::Round::try_from(0).unwrap(), |
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.
Is the try_from/unwrap dance really necessary even with a constant?
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.
LOL, we can change that to Round::default()
and get on with life. (Maybe in the next PR.)
The longer answer for the try-unwrap dance for constants is this:
We made it a strict requirement that the domain structs we use are safe or we invite abuse of the structs and the possibility of bad code. If constants can add any value without consequences, invalid structs can be created. A Round is a positive value between 0 and i32::MAX. There's no primitive value that can store this in Rust, hence the safe struct that only accepts and emits values between these two. If There's a Round(pub i32)
definition, then I can create rounds with negative numbers. If there's a Round(pub u32)
definition, I can create rounds with bigger numbers than i32::MAX. Both create problems when we want to encode to the wire and the current expectation is that encoding to the wire is infallible.
Also, we're only going to deal with constants in our tests. In "real code", we get other variables coming in from the wire. The try-unwrap dance is warranted there. (And it's possibly a try-?
dance.)
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.
Obviously, after my long-winded explanation last night, Romain woke up this morning and asked me something that made me realize how overcomplicated my explanation is. (I always learn something from Romain.)
So, we implemented the From trait for Height in #628 and it will come for the Round struct too with types that will definitely fit into the struct. In the case of Round, you can do for example: Round::from(0_u16)
.
tendermint/src/error.rs
Outdated
NegativeValidatorIndex, | ||
|
||
/// Wrong hash size in part_set_header | ||
#[error("Wrong hash: expected Hash size to be 32 bytes")] |
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.
Can/should we parametrize this hard coded 32 value or omit it?
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 was copied from the amino_types/error.rs . Yeah, eventually this should be more general. Right now all our hashes that can have invalid length, are supposed to be 32 bytes.
(I might create issues tomorrow from these notes.)
Addressed or follow up in future PRs
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.
🚀
Closes: #536 #585 #600
Removed all amino_types structs and implemented DomainType for the relevant Tendermint structs.