owned this note
owned this note
Published
Linked with GitHub
# Regen Cosmos SDK Architecture Review
Zoom link: https://zoom.us/j/489240122
**Date:** May 7, 2020 5pm CET / 11am ET / 8am PT
**Attendees:** Aaron C, Cory L, Bez, Alex Peters, Simon W, Anil, Sahith, Marie G, Alessio
## Agenda Items
- [x] Review ADR020 (https://github.com/cosmos/cosmos-sdk/pull/6111)
- [x] Canonicalization vs Raw Bytes signing
- [x] Unknown fields
- [x] Critical vs Non-critical field - distinguish by field number range (i.e. 1-1023 for critical, 1024+ for non-critical)
- [x] Embedded `TxBody`
- [x] Inclusion of `SignerInfo` in `TxBody`
- [ ] `PubKey` format - `Any`, `oneof`, or `enum`
- [ ] Current work on `Any` (Amino compatibility)
- [ ] Review ADR023 (https://github.com/cosmos/cosmos-sdk/pull/6083)
## Notes
- Aaron: Why should we have a separate `TxBody` ?
- basically comes down to whether we are going to be signing based on the raw bytes, vs a canonicalized version of the protobuf
- Tony's argument: canonicalization w/ protobuf means your server will reject any thing from the client with unknown fields
- that can be a good thing, a server should reject an unknown message
- but you may cause breakage of clients sending upgraded versions, if server hasn't upgraded
- Proposed idea: having critical field number range (1-1023).
- Client would sign the raw bytes, as opposed to putting fields in order and signing the message
- Simon: requirements that you have for TLS is that any client can connect to any server (which is diff from blockchain usecase)
- upgrading happens in a more coordinated way. So there shouldn't be a use case where there is a "newer client" ever being deployed and in use at scale before the server upgrade
- In general, agree with Tony's solution for hte problem, but don't agree that we have hte problem
- Aaron: We could face this problem in a multi-chain use case. There may be clients that support "any chain that supports staking module", and zones that are running that staking module could be running different versions of the staking module
- Bez: Seems like signing over hte raw bytes, and not dealing w/ canonicalization is a more pragmatic approach
- Simon: I would be really happy if we could avoid canonicalization in general... but there seems to be 3 levels (of envelope) where we are discussing possibilty of having to deal w/ canonicalization
- level 1: transaction
- level 2: transaciton body
- level 3: messages
- Seems like we are talking about how to deal with canonicalizatino of level 1 or 2, but not eliminating the requirement for it entirely
- Alex: are we optimizing for the wrong use case?
- On the server side, we should optimize for the 90% case and not spend too much time. How often will we be actually dealing with optional fields?
- Aaron: If we add this requirement for critical fields, we still should sign raw bytes
- Bez: are we in agreement to move forward with for now not allowing unknown fields, and later we can enable uknown fields outside hte critical range?
- Simon: We should be more clear about what document we are talking about.
- Simon: i would be super happy if we could get rid of canonicalization entirely.
- I would canonicalize layer 1 and 2, but leave 3 untouched, as it is defined already as raw bytes serialized from protobuf
- Cory: what is an example of support for new unknown fields for this layer in the envelope?
- Aaron: the `timeout_height` addition, is an example of us adding something that dont want to break clients communicating with older servers, where the message is at this envelope
- Aaron: The way I see it, the job of a client developer is simpler if they don't have to canonicalize level 2. The only upside that I see towards doing the canonicalization approach is that you don't have to use these 2 raw types on the server side.
- Aaron: Yes, you still need to implement canonicalization of layer one (those 4 fields in the top envelope), but with so few fields it should be very simple to canonicalize
- Simon: It would be great to remove 0 from the sequence numbers. If we can do this, it would help us remove the need for canonicalization at all.
- Loose consensus:
- Approach that's laid out in ADR020 as is, which the change that sequence number can never be 0 (the first time you submit a transaction, sequence number is 1)
- Will proceed with reserved fields (1-1024)
- Benefits:
- Client doesn't need to know about SignDocRaw, they just use SignDoc, and send it over
- Public key (encode as `Any` vs `oneOf` vs `enum`)
- Bez: we're only using the pub key for verification right?
- We include it in the signature simply bc the pubkey can include additional metadata
- Aaron: there was a proposal from zaki to include metadata in the signing mode.
- Aaron: assuming we include it in the txbody, do we include it as Any?
*Time ran out, will followup offline for the rest of this conversation*
## Follow-ups
- @cory to writeup decision on ADR020 PR
- @aaron to writeup a comment about outstanding PubKey conversation, so we can come to a decision asynchronously