Skip to content
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

abci: Add domain types #862

Closed

Conversation

hdevalence
Copy link
Collaborator

@hdevalence hdevalence commented Apr 14, 2021

Incomplete implementation of #856

Still to do:

  • Referenced an issue explaining the need for the change
  • Updated all relevant documentation in docs
  • Updated all code comments where relevant
  • Wrote tests
  • Updated CHANGELOG.md

@hdevalence
Copy link
Collaborator Author

hdevalence commented Apr 14, 2021

Update 2021-09-10, some of these are no longer relevant, so I've crossed them out.

  • [ ] I regenerated the protos against tendermint master to avoid having to do extra work later to align with changes that already exist upstream now. The tests seem to still pass, but if the idea is that tendermint-rs is tracking a specific release I guess this PR will have to wait.
  • I commented out some doctests that failed locally prior to making any changes -- not sure whether that's a local machine problem, or if the CI isn't running the doctests.
  • The conversions between domain types and protos have a lot of &'static str errors. I'm not sure what the crate error modeling strategy is like, so that could be changed in this PR or in a later one. After looking at Retiring anomaly #669 , I think the best thing would be to leave errors as &'static str, and have that be changed as part of a forthcoming error handling overhaul.
  • I still need to fill in the request types and arrange those nicely, and touch up the docs.
  • There's existing stub code in the abci module that seems to be used for handling some RPC calls, and it should be replaced by complete modeling of the ABCI types.
    • This requires defining JSON serialization for the new domain types. This serialization format is different from the proto format for historical reasons. I think the best way to support this is not to put serde attributes on the domain types themselves, but to inject them into the generated tendermint_proto types and use TryFrom/From attributes on the domain types. This ensures that all serialization details are in one place.
    • To avoid frontloading this work onto this PR, which is large enough as is, this code decouples the RPC implementation from the ABCI module by coping the old ABCI-for-RPC types into the RPC crate.
  • Some of the domain types could be further refined to make use of Tendermint structures, like AppHash, block::Height, etc.
  • [ ] There are a bunch of *Params structures that aren't properly part of ABCI, but come from other proto definitions. Presumably this means they shouldn't live in the abci module, but somewhere else. I'm not sure where. Hopefully I didn't duplicate anything, but I didn't see any existing ones.
  • There are no tests for the conversions. Ideally, it would be possible to use proptest to generate randomized messages and check the serialization round-trips, but I'm not sure if that infrastructure already exists. If it doesn't, I would not like to implement a bunch of one-off custom serialization tests, since I think that doing that has relatively low assurance vs effort (compared to, e.g., just using the code to talk to an ABCI client).

Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much for all this work! 🎉 I've done a first pass here and had a few comments/questions, but it's already looking like it's in really good shape.

tendermint/src/abci/kind.rs Show resolved Hide resolved
tendermint/src/abci/response.rs Outdated Show resolved Hide resolved
tendermint/src/abci/response.rs Outdated Show resolved Hide resolved
tendermint/src/abci/response.rs Outdated Show resolved Hide resolved
tendermint/src/abci/response.rs Outdated Show resolved Hide resolved
tendermint/src/abci/response.rs Show resolved Hide resolved
tendermint/src/abci/types.rs Outdated Show resolved Hide resolved
@xla xla changed the title [WIP] Add ABCI domain types to tendermint::abci. [WIP] abci: Add domain types Jun 8, 2021
@cwgoes
Copy link

cwgoes commented Jun 22, 2021

Bump. What's the status here / is there anything we can help with? (ref penumbra-zone#1)

@thanethomson
Copy link
Contributor

Bump. What's the status here / is there anything we can help with? (ref hdevalence#1)

Also happy to help on this!

@hdevalence
Copy link
Collaborator Author

Hey, sorry for the delay -- I wasn't able to pick it up again in June after the initial review because of some other work, but I've rebased it on top of the current state of the master branch to get ready to drive it to completion.

@hdevalence hdevalence force-pushed the abci-domain-types branch 2 times, most recently from 36069f3 to 2a78e5f Compare August 5, 2021 05:54
@hdevalence
Copy link
Collaborator Author

hdevalence commented Aug 5, 2021

Update:

  • I regenerated the protos against the current upstream master rather than the master as of mid-April when I opened the PR, and fixed any inconsistencies the upstream proto changes introduced relative to the rest of tendermint-rs.
  • I decoupled the original ABCI-for-RPC types from the new ABCI-for-ABCI types by copying them to the RPC crate. While this means the RPC crate now has duplicate definitions of ABCI types, it means that that code can be refactored with the ABCI code in chunks, rather than getting it all done in this PR (which is big enough as is, and has been unmerged for long enough to start risking problems -- sorry about that!)

Remaining work:

  • Another pass over the ABCI types to catch types that could be refined (e.g., sticking in Power, AppHash, etc
  • Aligning the original ABCI types with the new ones (without respect to the RPC code). For instance, using the response code structure, etc.

I'll post an update when I've finished those two and this is ready for re-review.

@hdevalence
Copy link
Collaborator Author

I've rebased this branch on top of current master to be able to get it to completion.

Currently, it's broken, because the conversions between the domain types and the proto types used &'static str errors, but it's no longer possible to use a &'static str as a source error.

Before trying to correct this, I'd like to make sure I understand how the error modeling is supposed to work. I looked at the flex-error docs, but it seems to be an abstraction over other error crates, so I'm not sure what the intended goal is or how to use it. The change to flex-error was in PR #923, which references discussion about an error overhaul in #669, but that issue just discusses some possibilities, and doesn't have any discussion motivating the creation of flex-error.

@hdevalence hdevalence force-pushed the abci-domain-types branch 2 times, most recently from 726beaa to 45c3946 Compare September 10, 2021 01:05
@thanethomson
Copy link
Contributor

Before trying to correct this, I'd like to make sure I understand how the error modeling is supposed to work. I looked at the flex-error docs, but it seems to be an abstraction over other error crates, so I'm not sure what the intended goal is or how to use it. The change to flex-error was in PR #923, which references discussion about an error overhaul in #669, but that issue just discusses some possibilities, and doesn't have any discussion motivating the creation of flex-error.

Sure I can give some context there! flex-error does, as you say, abstract over other error handling crates, and our main goals there are:

  1. Harmonizing the error handling approach across both tendermint-rs and ibc-rs
  2. no_std support for as many crates in tendermint-rs and ibc-rs as possible/necessary

hdevalence added a commit to penumbra-zone/penumbra that referenced this pull request Sep 10, 2021
This is useful until
informalsystems/tendermint-rs#862 is merged
upstream.
hdevalence and others added 18 commits November 15, 2021 16:20
The BlockParams and ConsensusParams structs moved out of the ABCI
protos.
This fixes a compile error introduced by upstream proto changes that add
an Sr25519 variant.
The previous data modeling allowed a user to construct an `Err(0)` value that
would be serialized and deserialized as `Ok`.
This changes the Serialize/Deserialize implementations for Block to convert
to/from the proto-generated `RawBlock`, and use the derived serialization for
that type.

This is much cleaner and more maintainable than keeping Serde annotations for
each sub-member of the data structure, because all of the serialization code is
kept in one place, and there's only one validation path (the TryFrom conversion
from RawBlock to Block) that applies to both kinds of serialization.
This changes the Block type to hold the transactions as a plain
`Vec<Vec<u8>>`, rather than as a custom `abci::transaction::Data` type
(which is itself a wrapper for an `Option<Vec<Transaction>>`, the
`Transaction` type being a wrapper for a `Vec<u8>`).

This also updates the `Block` getter functions; it's not clear (to me)
why they're there, since they access the public fields and aren't used
anywhere else.
The existing contents of the `tendermint::abci` module note that they're
only for the purpose of implementing the RPC endpoints, not a general
ABCI implementation.

Moving that code from the `tendermint` crate to the `tendermint-rpc`
crate decouples the RPC interface from improvements to the ABCI domain
modeling. Eventually, it would be good to eliminate this code and align
it with the new ABCI domain types.
These types mirror the generated types in tendermint_proto, but have better
naming.

The documentation for each request type is adapted from the ABCI Methods and
Types spec document. However, the same logical request may appear three
times, as a struct with the request data, as a Request variant, and as a
CategoryRequest variant.

To avoid duplication, this PR uses the `#[doc = include_str!(...)]`
functionality stabilized in Rust 1.54 to keep common definitions of the
documentation.
The code in the `abci` module had more complete documentation from the ABCI
docs, so I copied it onto the existing structure.
@hdevalence
Copy link
Collaborator Author

@thanethomson I've rebased this again and deduplicated types per your suggestions (thanks for pointing those out!). I also aligned all the errors, and then rebased again to eliminate the placeholder commits labeled "drop me".

So, this should be good to go.

@thanethomson
Copy link
Contributor

Superseded by (and included in) #1022.

@hdevalence
Copy link
Collaborator Author

🎊 Thanks so much for getting this over the finish line!!

hdevalence added a commit to penumbra-zone/tower-abci that referenced this pull request Dec 8, 2021
hdevalence added a commit to penumbra-zone/tower-abci that referenced this pull request Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants