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

Remove account num, sequence num from serialized std signature struct #2952

Closed
ValarDragon opened this issue Nov 29, 2018 · 4 comments
Closed

Comments

@ValarDragon
Copy link
Contributor

ValarDragon commented Nov 29, 2018

Currently the serialized StdSignature struct contains a uint64 account number, and a uint64 sequence number. Clearly these need to be signed over, but they make no sense being serialized in the tx. The reason being, to verify a signature you must fetch the account anyway to ensure correct accNum, sequenceNum. So we don't need this to have ever been included in the tx.

Removing these two values saves us 4-20 bytes per signature!! Mostly like 6 bytes total, (2 bytes for field numbers, 2-18 bytes for raw data types in var int encoding. account number we can expect to be over 128, so it takes at least 2 bytes, and it takes 3 bytes at 16k total accounts.)

Originally posted by @ValarDragon in https://github.com/_render_node/MDIzOlB1bGxSZXF1ZXN0UmV2aWV3VGhyZWFkMTQ0MTIxODE2OnYy/pull_request_review_threads/discussion

@alexanderbez
Copy link
Contributor

Just to clarify, the StdSignature in the auth.StdTx.

@sunnya97
Copy link
Member

sunnya97 commented Dec 7, 2018

I don't think this was a good idea. For one, it makes inspecting the correctness of the tx harder. I want to verify that a signature is correct on a transaction that I created locally, but I don't know what accnum and seqnum it was over.

Also, if a tx doesn't have its sequence number and account number in the tx itself, then how will the txs signature be verified by CheckTx? Getting the data from the CheckTx state? Then that would require that a tx can't go into the mempool, until the previous tx from that account gets committed.

@ValarDragon
Copy link
Contributor Author

ValarDragon commented Dec 7, 2018

I don't think this was a good idea. For one, it makes inspecting the correctness of the tx harder. I want to verify that a signature is correct on a transaction that I created locally, but I don't know what accnum and seqnum it was over.

Thats not true. Have signatures that you create also provide the sequence number and account number. Now when you go to broadcast, just don't send that.

Also, if a tx doesn't have its sequence number and account number in the tx itself, then how will the txs signature be verified by CheckTx? Getting the data from the CheckTx state? Then that would require that a tx can't go into the mempool, until the previous tx from that account gets committed.

Currently CheckTx does query the check state (which gets updated for every tx in the mempool). (This is how it has always been)
Its already the case that a tx can't go into the mempool, unless it has the correct sequence number. The CheckTx sequence number changes as txs go into the mempool.

imo you do want the mempool checking all of these sequence numbers and account numbers. The reason being, otherwise I'd just go make 10000 txs with wrong account numbers, but fill up the entire networks mempools.

@jackzampolin
Copy link
Member

Going to go ahead and close this.

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

No branches or pull requests

4 participants