refactor(proto)!: update SignedTransaction to contain Any for transaction

[noot]

Summary

as title says

Background

protobuf encoding is not deterministic, so we want to verify the signature over the exact same bytes signed.

Changes

• update SignedTransaction to contain Any for transaction instead of UnsignedTransaction

Testing

unit test

Breaking Changelist

• the SignedTransaction type is changed
• tx hashes are no longer the same

Related Issues

closes #310

[SuperFluffy]

I think we could amend the SignedTransaction type to contain the raw bytes that we signed over, i.e. do something like this:

struct SignedTransaction {
    signature: Signature,
    verification_key: VerificationKey,
    transaction: UnsignedTransaction,
    transaction_bytes: Bytes,
}

Because we don't allow mutation of the signed transaction we can ensure that the signature always stays valid.

[SuperFluffy]

Please prost::Name::type_url for this.

[SuperFluffy]

You can impl this method in terms of into_any (there is no perf advantage right now I think in specializing it and it avoids dulicating code):

pub fn to_any(&self) -> pbjson_types::Any {
    self.clone().into_any()
}

[SuperFluffy]

I believe we are deriving prost::Name for all types:

if any.type_url != UnsignedTransactionError::type_url() {

[SuperFluffy]

Useful to provide a bit more info:

Suggested change

	#[error("invalid type URL")]
	InvalidTypeUrl,
	#[error("encountered invalid type URL when converting from `google.protobuf.Any`; got `{got}`, expected `{}`", UnsignedTransaction::type_url())]
	InvalidTypeUrl { got: String, },

[SuperFluffy]

I would be a tad more specific about this. It might be that

Suggested change

	#[error("invalid bytes in Any")]
	#[error("failed to decode `google.protobuf.Any` to `{}`", UnsignedTransaction::type_url())]

[SuperFluffy]

Suggested change

	InvalidAnyBytes(#[source] prost::DecodeError),
	DecodeAny(#[source] prost::DecodeError),

[SuperFluffy]

I am not generally opposed to doing this (to save some allocations), but we must ensure that const matches the generated type URL of the prost::Name trait. So either use UnsignedTransaction::type_url() everywhere, or alternatively use the hardcoded value here, but add a unit test like below.

#[test]
fn hardcoded_type_url_matches_generated_trait() {
    assert_eq!(UNSIGNED_TRANSACTION_TYPE_URL, &UnsignedTransaction::type_url(),);
}

Read my other comments re the use of Name::type_url() with this in mind - either use the trait method everywhere, or ensure that the const always stays valid.

[noot]

updated to use type_url()

[noot]

@SuperFluffy addressed comments! what's the point of keeping the tx bytes inside the SignedTransaction? there isn't any case right now where we would need to re-verify the signature, as SignedTransaction can only be constructed if the signature was valid.

[joroshiba]

What is the use case for calling into_raw or to_raw on a SignedTransaction? Since we don't persist the serialized bytes in try_from_raw we lose the determinism of the what is returned here. The signature may not match the transaction bytes on a roundtrip. If you deserialized from raw, reserializing into is not safe.

My thinking is we can either remove this if there isn't a use for them, or we should maintain a copy of the bytes of the transaction when creating SignedTransaction and then build and return the any object here. I know not as pretty, and in some usecases it would be fine (ie I just constructed a new transaction and signed it), but the loops wouldn't be safe and avoiding them ever happening would require we add some data saying like "derived from raw" and then not allow reserializing (assuming we aren't perfect at code review).

[noot]

yeah, i updated this to keep the transaction_bytes in SignedTransaction so converting to raw is now guaranteed to be the same

[SuperFluffy]

@noot

@SuperFluffy addressed comments! what's the point of keeping the tx bytes inside the SignedTransaction? there isn't any case right now where we would need to re-verify the signature, as SignedTransaction can only be constructed if the signature was valid.

My thinking is in similar vain to what @joroshiba had in their comment: it's not about the signature but about maintaining the exact same byte-representation when going SignedTransaction <> raw::SignedTransaction. Imagine this flow:

network bytes --1> raw Signed Transaction --2> checked Signed Transaction --3> raw Signed Transaction --4> network bytes

raw Signed Transaction contains UnsignedTransaction in the form of an Any.

Right now, if you do step 2, i.e. raw Signed Transaction --2> checked Signed Transaction, you throw away the original bytes that made up the unsigned transaction. So when you go checked Signed Transaction --3> raw Signed Transaction you re-encode you don't have a guarantee that you get the same bytes.

If you keep the original bytes around you can completely avoid the issue.

[noot]

@SuperFluffy that makes sense, added to SignedTransaction!