Add QuorumSignInfo and merge CertifiedTransaction with other Transaction types
#1686
Conversation
lxfind
requested review from
huitseeker,
patrickkuo,
sblackshear,
gdanezis,
kchalkias and
oxade
| @@ -327,6 +372,10 @@ where | |||
| #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] | |||
| #[serde(remote = "TransactionEnvelope")] | |||
| pub struct TransactionEnvelope<S> { | |||
| // This is a cache of an otherwise expensive to compute value. | |||
| // DO NOT serialize or deserialize from the network or disk. | |||
| #[serde(skip)] | |||
| // in this ser/de context it's relevant to compare signatures | ||
| assert_eq!(o1.signatures, o2.signatures); | ||
| assert_eq!(o1.auth_sign_info.signatures, o2.auth_sign_info.signatures); |
There was a problem hiding this comment.
nice, I was originally puzzled re why do we compare signatures as they can be malleable, but this is for serde tests which makes sense.
sui_types/src/crypto.rs
Outdated
| @@ -485,10 +485,30 @@ impl PartialEq for AuthoritySignInfo { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] | |||
| pub struct QuorumSignInfo { | |||
There was a problem hiding this comment.
We usually refer to it as auth_sign_info, ideally we should pick a consistent name, either auth or quorum
There was a problem hiding this comment.
Maybe AuthQuorumSignInfo? We already have AuthSignInfo to represent a single authority signature
| @@ -1081,7 +1081,7 @@ impl CertifiedTransaction { | |||
| is_checked: false, | |||
| data: transaction.data, | |||
| tx_signature: transaction.tx_signature, | |||
| auth_sign_info: QuorumSignInfo { | |||
| auth_sign_info: AuthorityQuorumSignInfo { | |||
There was a problem hiding this comment.
Thanks, the readability problem is still remaining (cause while you read this you expect to see an AuthoritySignInfo), what about renaming auth_sign_info to quorum_sign_info and keep your previous QuorumSignInfo?
There was a problem hiding this comment.
auth_sign_info can be one of EmptySignInfo, AuthoritySignInfo and AuthorityQuorumSignInfo.
So I don't think we want to rename it to quorum_sign_info.
There was a problem hiding this comment.
Hm.. ok, let's leave it unchanged for now, we might consider renaming it to the more generic sign_info later, but don't change yet, auth_sign_info works for now.
There was a problem hiding this comment.
The same structure also contains transaction signature (tx_signature). So this is named auth_sign_info to emphasize that the signature is from authority side instead of from transaction signer.
But yeah open to better names!
| @@ -485,10 +485,30 @@ impl PartialEq for AuthoritySignInfo { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug, Serialize, Deserialize, JsonSchema)] | |||
There was a problem hiding this comment.
Also, we should note that certified txs can carry more than quorum sigs (every authority might in theory sign), and wondering if we should put a comment here to clarify it (unless quorum implies quorum+)
lxfind
force-pushed
the
crypto-add-quorum-sign-info
branch
from
037dd9d to
aab7077
Compare
lxfind
force-pushed
the
crypto-add-quorum-sign-info
branch
from
aab7077 to
feea7d1
Compare
…saction types (#1686) * Add QuorumSignInfo * Add cached digest to Tx envelope * Merge CertifiedTransaction * Rename QuorumSignInfo
…saction types (#1686) * Add QuorumSignInfo * Add cached digest to Tx envelope * Merge CertifiedTransaction * Rename QuorumSignInfo
CertifiedTransactionis also just a Transaction that's signed by authorities, making it very similar to SignedTransaction.This PR introduces
QuorumSignInfoas a new authority signature trait type, and defineCertifiedTransactionusing it.This also allows us to unify some of the code, for example, the
Transactiontype now also has a cached digest field.