Skip to content
This repository has been archived by the owner on Dec 17, 2024. It is now read-only.

Add transaction validation #36

Merged
merged 5 commits into from
Jan 21, 2024
Merged

Add transaction validation #36

merged 5 commits into from
Jan 21, 2024

Conversation

tuommaki
Copy link
Contributor

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.

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.
@tuommaki tuommaki requested a review from musitdev January 12, 2024 13:09
@tuommaki tuommaki self-assigned this Jan 12, 2024
#[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,
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

@tuommaki tuommaki merged commit 3936d2d into proto Jan 21, 2024
4 checks passed
@tuommaki tuommaki deleted the add-tx-validation branch January 21, 2024 15:05
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants