Rebase ABCI Domain Types onto v0.34.20 branch. #1185

hdevalence · 2022-08-17T23:15:13Z

After a second round of rebasing:

The original ABCI domain types work used chrono; this temporarily re-adds
chrono as a dependency, so that the changes that eliminated it can be
replayed on top of this rebasing.
The original ABCI domain types work was, for some reason, mixed in with
changes to the RPC test harness for 0.35 compatibility, and that may break
this code. I didn't attempt to fix this, because I didn't touch the RPC tests
when writing the ABCI domain types.
The RPC tests don't pass, for reasons that are unclear to me. I split the changes to the RPC code into ones that seemed relevant to the ABCI vendoring and the changes for 0.35 compatibility. The changes for 0.35 compatibility, which are not contained in this PR, are here, for reference: penumbra-zone@46bf130

Replace chrono with time 0.3 (informalsystems#1030) * pbt-gen: Converted from chrono to time 0.3 chrono has soundness issues (see RUSTSEC-2020-0159) and does not seem to be maintained. * Replace dependency on chrono with time 0.3 Change Time implementation to crate time: chrono has soundness issues (see RUSTSEC-2020-0159) and does not seem to be actively maintained. * Add Time methods checked_add and checked_sub These should be used instead of the overloaded operators that broke the operator convention by returning a Result. * proto: Don't use formatting methods of time Drop the "formatting" feature of time, as this brings in std. * pbt-gen: Add arb_datetime_for_rfc3339 With crate time, the range of supported dates stretches to the ancient past beyond the common era, which is not representable in RFC 3339. Add a generator to produce `OffsetDateTime` values that are within the RFC 3339 spec. * Ensure Time can only have values good for RFC 3339 As the time values are meant for human-readable representation in the RFC 3339 format, make it not possible to construct Time with values that fall outside this standard representation. Conversion from time::OffsetDateTime is made fallible with a TryFrom impl replacing the From impl. Conversion from Unix timestamps is also affected. * rpc: Replaced chrono with time 0.3 * testgen: Replaced chrono with time 0.3 * Less allocatey ways of formatting date-times Provide and use another helper in proto mod serializers::timestamp, one that formats into a provided fmt::Write object rather than allocating a string. * light-client: port from chrono to time * Revert to Add/Sub returning Result * light-client: changed the MBTs from chrono to time * tendermint: Remove a comment referencing chrono We use ErrorDetail::DurationOutOfRange without the source error from the time library because that is uninformative in the context. Also move the DateOutOfRange close with DurationOutOfRange since they can occur in the same operations and are generally similar. * light-client: add std feature for time The clock needs OffsetDateTime::now_utc(). * tendermint: Simplify Time::duration_since * testgen: minor code cleanup * Restrict valid Time years to 1-9999 Exclude year 0 because the spec on Google protobuf Timestamp forbids it. Add more helpers in pbt-gen to produce protobuf-safe values in both OffsetDateTime and RFC 3339 strings. Restrict the property tests to only use the protobuf-safe values where expected. * Test Time::checked_add and Time::checked_sub * Changelog entries for informalsystems#1030 * Improve documentation of tendermint::Time * proto: remove the chrono type conversions The From impls are panicky and do not enforce compliance with protobuf message value restrictions. * tendermint: remove direct uses of chrono types Replace with tendermint::Time. * Harden Timestamp conversions and serde Require the timestamp to be in the validity range (years 1-9999 in UTC) and the nanosecond member value to not exceed 999_999_999. Rename ErrorDetail::TimestampOverflow to TimestampNanosOutOfRange, because the old variant was not very informative (the chained TryFromIntError did not help) and we also use it for the above-range case now which is not an overflow. * Changelog entry about changed error variants * Restore nanosecond range check in Time::from_unix_timestamp Add a unit test to exercise the check. * proto: Improve timestamp::fmt_as_rfc3339_nanos - More ergonomic signature - A non-allocating implementation * Fix component name in changelog for 1030-remove-chrono Co-authored-by: Thane Thomson <thane@informal.systems> * time: Use Self instead of the type name in methods Co-authored-by: Thane Thomson <thane@informal.systems> * Comment on the inner representation of `Time` * Don't alias crate time in testgen * Document the Time::from_utc helper Co-authored-by: Thane Thomson <thane@informal.systems>

mzabaluev · 2022-09-21T11:58:54Z

0.35 compatibility

How much should we care about 0.35 at this point? This version has been discarded by cosmos-sdk in favor of 0.37, and judging by my previous work on side-by-side support for it in #1143, the protocol differences with 0.34 are more significant than between 0.34 and 0.37.

proto/src/prost/tendermint.abci.rs

mzabaluev · 2022-09-21T12:54:26Z

rpc/src/endpoint/net_info.rs

@@ -39,7 +39,7 @@ pub struct Response {
    pub n_peers: u64,

    /// Peer information
-    pub peers: Vec<PeerInfo>,
+    pub peers: Option<Vec<PeerInfo>>,


Is there a special meaning for None that would be different to Some(vec![]) here?

I don't know, this is the existing domain type modeling.

My only guess is, it could matter for serialization where the peers field can also have a null value (it cannot be missing/omitted from the struct because that would require serde attributes to allow). But I'd like to know what's the purpose of this special case as a domain type and how does it interop with responses from the wire. In the Go implementation, the field type is an ordinary array, and it's specified as such in the OpenAPI manifest as well. Without more information, I'd suggest removing the Option wart for the 1.0 API.

Cool, removing the Option sounds good.

I suspect the Go code slips in a null instead of an array value somewhere, and this gets encoded into JSON accordingly. But I'd like to see a case where this is valid or otherwise has to be grandfathered in.

mzabaluev · 2022-09-21T13:00:05Z

tendermint/src/block.rs

@@ -128,7 +129,7 @@ impl Block {
    }

    /// Get data
-    pub fn data(&self) -> &transaction::Data {
+    pub fn data(&self) -> &Vec<Vec<u8>> {


The return type looks dodgy; borrowing as a slice would be better perhaps?

Suggested change

pub fn data(&self) -> &Vec<Vec<u8>> {

pub fn data(&self) -> &[Vec<u8>] {

A domain Data type might be stylistically better, but I don't have much history with this API to strongly recommend that.

I think changes to the existing API should be made after backporting it, rather than as part of the backport, and after a semver-compatible release, so that it's actually possible for downstream users to use the crate at all.

hdevalence · 2022-09-21T15:09:44Z

0.35 compatibility

How much should we care about 0.35 at this point? This version has been discarded by cosmos-sdk in favor of 0.37, and judging by my previous work on side-by-side support for it in #1143, the protocol differences with 0.34 are more significant than between 0.34 and 0.37.

There's no compatibility with 0.35; the changes for 0.35 compatibility are the ones not included in this rebase.

mzabaluev · 2022-09-30T11:12:44Z

Superseded by #1203.

hdevalence force-pushed the abci-domain-types-on-034 branch 2 times, most recently from 3a318ed to 7b414c2 Compare August 18, 2022 00:24

hdevalence force-pushed the abci-domain-types-on-034 branch from 7b414c2 to b699af9 Compare September 20, 2022 16:34

hdevalence added 2 commits September 20, 2022 13:55

split A.4.1, proto tooling changes

3021889

split A.2, changelog abci

38a5de3

hdevalence force-pushed the abci-domain-types-on-034 branch from b699af9 to c6cd0a3 Compare September 20, 2022 18:28

hdevalence added 2 commits September 20, 2022 14:39

split A.1, tendermint and abci changes

b7f3125

split A.3, abci-in-rpc

8bc6a97

hdevalence force-pushed the abci-domain-types-on-034 branch from c6cd0a3 to ebd3429 Compare September 20, 2022 19:17

hdevalence and others added 9 commits September 20, 2022 15:24

clean up imports in abci crate

3096775

Return to 0.34 ABCI encoding

e403925

fixup! split A.3, abci-in-rpc

b9b6596

split A.4.1.1, check_event_attrs

c02739a

split A.4.1.2, some slice change?

2e429e7

split A.4.1.3, b.data.get(0).is_none()

064fb82

split A.4.1.4, remove test attrs

39f7210

split A.4.1.5, someting optiony

60dafb4

hdevalence force-pushed the abci-domain-types-on-034 branch from ebd3429 to 60dafb4 Compare September 20, 2022 19:24

clippy fix

61e6bf5

hdevalence marked this pull request as ready for review September 20, 2022 20:25

mzabaluev reviewed Sep 21, 2022

View reviewed changes

hdevalence mentioned this pull request Sep 21, 2022

Rollback to Tendermint 0.34 penumbra-zone/penumbra#1271

Closed

5 tasks

mzabaluev mentioned this pull request Sep 23, 2022

Rebase ABCI domain types onto main #1203

Merged

mzabaluev closed this Sep 30, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebase ABCI Domain Types onto v0.34.20 branch. #1185

Rebase ABCI Domain Types onto v0.34.20 branch. #1185

hdevalence commented Aug 17, 2022 •

edited

Loading

mzabaluev commented Sep 21, 2022

mzabaluev Sep 21, 2022

hdevalence Sep 21, 2022

mzabaluev Sep 21, 2022 •

edited

Loading

hdevalence Sep 26, 2022

mzabaluev Sep 26, 2022 •

edited

Loading

mzabaluev Sep 21, 2022

mzabaluev Sep 21, 2022

hdevalence Sep 21, 2022

hdevalence commented Sep 21, 2022

mzabaluev commented Sep 30, 2022

	pub fn data(&self) -> &Vec<Vec<u8>> {
	pub fn data(&self) -> &[Vec<u8>] {

Rebase ABCI Domain Types onto v0.34.20 branch. #1185

Rebase ABCI Domain Types onto v0.34.20 branch. #1185

Conversation

hdevalence commented Aug 17, 2022 • edited Loading

mzabaluev commented Sep 21, 2022

mzabaluev Sep 21, 2022

Choose a reason for hiding this comment

hdevalence Sep 21, 2022

Choose a reason for hiding this comment

mzabaluev Sep 21, 2022 • edited Loading

Choose a reason for hiding this comment

hdevalence Sep 26, 2022

Choose a reason for hiding this comment

mzabaluev Sep 26, 2022 • edited Loading

Choose a reason for hiding this comment

mzabaluev Sep 21, 2022

Choose a reason for hiding this comment

mzabaluev Sep 21, 2022

Choose a reason for hiding this comment

hdevalence Sep 21, 2022

Choose a reason for hiding this comment

hdevalence commented Sep 21, 2022

mzabaluev commented Sep 30, 2022

hdevalence commented Aug 17, 2022 •

edited

Loading

mzabaluev Sep 21, 2022 •

edited

Loading

mzabaluev Sep 26, 2022 •

edited

Loading