-
Notifications
You must be signed in to change notification settings - Fork 49
Conversation
This is the first step towards having validation for transactions. It verifies that transaction signature is correct and that current workflow restrictions are checked. The transaction is validated on entering mempool, which covers currently all the key use cases in the program.
crates/node/src/types/transaction.rs
Outdated
#[derive(Clone, Debug, Default, Deserialize, Serialize, PartialEq)] | ||
pub struct Transaction { | ||
pub hash: Hash, | ||
pub payload: Payload, | ||
pub nonce: u64, | ||
pub signature: Signature, | ||
pub rec_id: u8, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was wondering. The rec id act as a sender pubkey using the signature. It uses less space, but for RPC access and db query it can complicate a lot of the query processing if we can't get the Tx for a sender.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it can complicate a lot of the query processing if we can't get the Tx for a sender.
Can you elaborate on this a bit more, what do you mean?
In general I'm not very happy with this myself either, but at the same time, it felt a bit weird to add the author's public key separately into tx.
One option that I also though was to merge rec_id
and signature
into single field. This would require quite a bit more processing for Signature
as currently it's merely a wrapper for libsecp256k-1
one (because I needed some extra functionality for it). What would you think about this option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My wondering is that the Tx sender is an important query parameter when you develop an application or monitor the BC. For example, getSignaturesForAddress is an important RPC function in Solana that is problematic because it's often use and need a lot of processing to answer.
So if the sender is not in the Tx at some point it should be added somewhere, in the Db storage for example or in the block or when the Tx is extracted. I don't know, but if it misses, it can be problematic.
So, rec_id is used to recreate the sender pub key. Perhaps we should add the sender public key in the Tx too.
About where to store the rec_id, in my opinion, it depends on its role. If the sender is part of the Tx it should be put in the signature because it's part of the signature domain. If not, it act has a sort of sender ID with the signature so it can be in the Tx. But as it depends on the signature, it's a little strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if the sender is not in the Tx at some point it should be added somewhere, in the Db storage for example or in the block or when the Tx is extracted. I don't know, but if it misses, it can be problematic.
The node needs to extract the public key from the tx on reception in all cases in order to verify the signature. It could then also store it separately in DB for easier request processing.
About where to store the rec_id, in my opinion, it depends on its role. If the sender is part of the Tx it should be put in the signature because it's part of the signature domain
If the sender pub key was stored in tx, is the rec_id needed for anything then? It's not needed for signature verification. Only for recovering the public key from the signature, but since that would exist already, then it could be just omitted completely?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For a conception point of view, the sender is part of the Tx with the signature. Sender are verified with the signature when the Tx is received to be sure both are compatible. It's an important security point. Removing the sender, by changing it with the rec_id, is an optimization to decrease the size of the Tx.
So we've to see if this size optimization doesn't add more overhead else where or some security issue.
We've to add the sender when the Tx is saved in the db, but it's not all the place where we need it. For example, most of the time there's notification channel (WebSocket for example). You'll have to add it there too and so on (in the log or tracing, ...).
So each time a Tx is read, you mostly need the sender. That why I say that this optimization can lead to more complexity and processing, so it don't really help.
I think we should keep the sender pubkey to ease the use of the Tx.
If we want to keep the rec_id, we should put if in the signature. It allows getting the pubkey with the sign. Perhaps there's some use case where it can help. I don't know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we've to see if this size optimization doesn't add more overhead else where or some security issue.
I strongly doubt this has anything to do with security, because rec_id
can have four different values (0-3) so brute forcing those is very easy and efficient, if necessary.
So each time a Tx is read, you mostly need the sender. That why I say that this optimization can lead to more complexity and processing, so it don't really help.
Here I start to align with you.
I think we should keep the sender pubkey to ease the use of the Tx.
Yep, let's do it this way.
If we want to keep the rec_id, we should put if in the signature. It allows getting the pubkey with the sign. Perhaps there's some use case where it can help. I don't know.
This one I doubt. If we already have the public key of the tx author, then rec_id
is just kinda redundancy. I'd rather just trust the algorithm/implementation and rely that signature verification against the author's public key always holds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one I doubt. If we already have the public key of the tx author, then rec_id is just kinda redundancy. I'd rather just trust the algorithm/implementation and rely that signature verification against the author's public key always holds.
Yes, for the Transaction point of view, but I was saying that it's part of the signature domain. It means that is there a use case where you only have the signature, and you need the public key? To answer it, we need to put the rec_id
in the signature. If not, we don't need.
This is the first step towards having validation for transactions.
It verifies that transaction signature is correct and that current workflow restrictions are checked.
The transaction is validated on entering mempool, which covers currently all the key use cases in the program.