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

Serialize sequence number with transaction signature #6966

Closed
4 tasks
ethanfrey opened this issue Aug 6, 2020 · 11 comments · Fixed by #6997
Closed
4 tasks

Serialize sequence number with transaction signature #6966

ethanfrey opened this issue Aug 6, 2020 · 11 comments · Fixed by #6997

Comments

@ethanfrey
Copy link
Contributor

Summary

Adding 2-3 bytes per signature by serializing the sequence number signing the transaction would enable much better introspection by the clients.

Problem Definition

Currently, if we wish to validate the signature of a transaction, beyond the transaction itself (posted to the blockchain), we need some other information included in "sign bytes". This is chain_id, account_number and sequence. chain_id and account_number are static for the lifetime of the chain and should be know (or queried) at the current height. However, there is no way to get the proper sequence number used to sign, except by querying the blockchain for the account state at the height the tx was committed (which is likely not available for long ago).

If a client wants to validate a signature is valid and do a spot-check audit on the full nodes, there is no way to do so. It is also hard to present a tx as cryptographic evidence.

Further, when getting error messages, we get "maybe chain-id, account-number, or sequence is wrong", but this means there is no way to distinguish out of order delivery with failed signatures.

Maybe we want a client/proxy that can do stateless validation of transactions. Like before it even hits a full node. In any case, we cannot even verify the self-consistency of the transaction without this one critical piece of information, which is just implied.

For a larger project, imagine we are running a "proof of existence" app (this has been proposed to me many times, starting in 2017). Two parties hash a pdf document and both sign the hash and commit it to the blockchain. 4 years later you can query the blockchain and show that this agreement was made 4 years ago and by showing the preimage, you can back-date the info. However, why do we know it was 4 years ago? Just cuz the blockchain state said so? Maybe the state was dumped a few times in the meantime. Maybe something changed in one of the genesis migrations. There are lots of variables here.

If I wished to present this a strong cryptographic evidence, I would like to have the original transaction and be able to verify it was correctly signed by all involved public keys. This one transaction alone could be presented, and then the block headers/commits it belonged to.

Proposal

I would add one more field to the signature uint64 sequence. This will store the sequence that was used to sign it, and the same one we currently use in signBytes.

Since this uses protobuf varint encoding, it will take 2 bytes for all sequences <= 127, and 3 bytes for sequences <= 16256, and maybe in some amazingly rare cases 4 bytes. This should not be a significant burden to the size of the blockchain to remove the usability enhancements acheived by including this number along with the signature.


For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@alexanderbez
Copy link
Contributor

Ahhh we've come full circle. I still remember when this was initially brought up by @ValarDragon a la #2952. I'm in favor of adding the sequence (nonce) back to the tx.

@aaronc
Copy link
Member

aaronc commented Aug 6, 2020

Thanks for writing this up @ethanfrey. I'm assuming this could live in the unsigned content of the transaction? Could it be optional with zero as default maybe?

If we do this it should happen before 0.40 I think because it could be an incompatible change. Would you be willing to take on this work @ethanfrey ?

@ethanfrey
Copy link
Contributor Author

I'm assuming this could live in the unsigned content of the transaction

This would live in the Signature, maybe many times per tx. And it would be implicitly signed (as it is in the sign bytes) but it would not have to be in the tx body at all. We would not have to change the sign bytes or sign process, just the info we encode when posting.

If we do this it should happen before 0.40 I think because it could be an incompatible change. Would you be willing to take on this work @ethanfrey ?

I agree it should be in 0.40. If there is favor for it, I can make a clear spec of the change I would do, and if approved, I can do a PR for it.

@ValarDragon
Copy link
Contributor

ValarDragon commented Aug 6, 2020

I think adding the nonce back to the tx makes sense to improve UX short term and in general for stateless clients.

I do think that if you don't have stateless clients, the ideal architecture would be the bundle the sequence number in the p2p layer, and just not include it in the final tx. (But thats probably an optimization that can be done later)

@aaronc
Copy link
Member

aaronc commented Aug 6, 2020

Currently signatures are simply repeated bytes in proto so it probably makes sense to add this as a fourth field in Tx that's basically repeated uint64 sequences = 4. If the field is omitted it will be ignored in tx verification.

@aaronc
Copy link
Member

aaronc commented Aug 7, 2020

In our meeting today, we agreed to add account_sequence as a field on SignerInfo and remove it from SignDoc. The seems like a more logical place than as a 4th field on Tx.

This does make signing behavior even worse for multi-signer transactions, but if and when that becomes an issue we can reopen the discussion around SIGN_MODE_DIRECT_AUX.

@ethanfrey
Copy link
Contributor Author

Thank you Aaron.

Multisig and sequences was always a tricky issue and worked best with very lightly used accounts.

Do you still want me to work on implementating it now that the path forward is clear? Or will the core team do it?

I do agree it makes more sense in SignerInfo

@webmaster128
Copy link
Member

If you want to see something scary – this is a way to obtain sequence numbers from an indexed transaction: https://github.com/CosmWasm/cosmjs/blob/v0.22.0/packages/launchpad/src/sequence.ts#L7-L46. The code works, but is not used in any application as far as I know.

@aaronc aaronc added this to the v0.40 [Stargate] milestone Aug 7, 2020
@aaronc
Copy link
Member

aaronc commented Aug 7, 2020

Thank you Aaron.

Multisig and sequences was always a tricky issue and worked best with very lightly used accounts.

Do you still want me to work on implementating it now that the path forward is clear? Or will the core team do it?

I do agree it makes more sense in SignerInfo

We'll need to think about it a bit @ethanfrey . At first glance, it seems almost trivial. On second glance, maybe not so much. If I have time I'll take it... Or maybe someone else on our team has context. We will probably need to change the internal SignatureV2 struct to have an AccountSequence field so that it gets set before sign bytes are generated. @amaurymartiny you've worked with that code a bit. What do you think?

@amaury1093
Copy link
Contributor

We will probably need to change the internal SignatureV2 struct to have an AccountSequence field so that it gets set before sign bytes are generated.

Yeah, that's the way I would go too.

At first glance, it seems almost trivial. On second glance, maybe not so much.

IMO the second glance is more accurate. I also gave a quick look, there are a lot of functions which create a SignatureV2 internally, and we now need to pass to the account sequence as input to these functions.

@ethanfrey If you didn't start any work on this yet, I can focus on this this week.

@ethanfrey
Copy link
Contributor Author

@amaurymartiny I am busy preparing for a testnet launch tomorrow. If you can start on it, that would be great. Please mention me on any (draft) PR and I can review and/or push some code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants