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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
93d6d2b
Add transaction validation
tuommaki Jan 12, 2024
3259bbb
Refactor tx construction & validation
tuommaki Jan 21, 2024
c73a260
address clippy
tuommaki Jan 21, 2024
fb79fe1
Merge branch 'proto' into add-tx-validation
tuommaki Jan 21, 2024
7a0bba6
Update transaction construction in CLI as well
tuommaki Jan 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions crates/node/src/mempool/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ impl Mempool {
}

pub async fn add(&mut self, tx: Transaction) -> Result<()> {
// First validate transaction.
tx.validate()?;

let mut tx = tx;
self.storage.set(&tx).await?;

Expand Down
2 changes: 2 additions & 0 deletions crates/node/src/scheduler/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -305,6 +305,7 @@ impl TaskManager for Scheduler {
nonce,
signature: Signature::default(),
propagated: false,
rec_id: 0,
},
TaskKind::Verification => Transaction {
hash: Hash::default(),
Expand All @@ -316,6 +317,7 @@ impl TaskManager for Scheduler {
nonce,
signature: Signature::default(),
propagated: false,
rec_id: 0,
},
TaskKind::PoW => {
todo!("proof of work tasks not implemented yet");
Expand Down
5 changes: 5 additions & 0 deletions crates/node/src/storage/database/entity/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ pub struct Transaction {
pub nonce: sqlx::types::Decimal,
pub signature: Signature,
pub propagated: bool,
pub rec_id: i8,
}

impl From<&types::Transaction> for Transaction {
Expand All @@ -47,6 +48,8 @@ impl From<&types::Transaction> for Transaction {
nonce: value.nonce.into(),
signature: value.signature,
propagated: value.propagated,
// This is safe because rec_id has values between 0-3.
rec_id: value.rec_id as i8,
}
}
}
Expand All @@ -61,6 +64,8 @@ impl From<Transaction> for types::Transaction {
.expect("invalid nonce in db"),
signature: value.signature,
propagated: value.propagated,
// This is safe because rec_id has values between 0-3.
rec_id: value.rec_id as u8,
}
}
}
1 change: 1 addition & 0 deletions crates/node/src/storage/database/postgres.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,7 @@ mod tests {
nonce: 64,
signature: Signature::default(),
propagated: false,
rec_id: 0,
};

database
Expand Down
105 changes: 102 additions & 3 deletions crates/node/src/types/transaction.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
use std::sync::Arc;
use std::{collections::HashSet, sync::Arc};

use super::hash::Hash;
use super::signature::Signature;
use libsecp256k1::{sign, verify, Message, PublicKey, SecretKey};
use eyre::Result;
use libsecp256k1::{recover, sign, verify, Message, PublicKey, RecoveryId, SecretKey};
use num_bigint::BigInt;
use serde::{Deserialize, Serialize};
use sha3::{Digest, Sha3_256};
use thiserror::Error;

#[derive(Clone, Debug, Deserialize, Serialize, PartialEq)]
pub enum TransactionTree {
Expand Down Expand Up @@ -223,12 +225,20 @@ impl Payload {
}
}

#[allow(clippy::enum_variant_names)]
#[derive(Error, Debug)]
pub enum TransactionError {
#[error("validation: {0}")]
Validation(String),
}

#[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.

#[serde(skip_serializing, skip_deserializing)]
pub propagated: bool,
}
Expand All @@ -238,13 +248,27 @@ impl Transaction {
// Refresh transaction hash before signing.
self.hash = self.compute_hash();
let msg: Message = self.hash.into();
let (sig, _) = sign(&msg, key);
let (sig, rec_id) = sign(&msg, key);
self.signature = sig.into();
self.rec_id = rec_id.serialize();
}

pub fn verify(&self, pub_key: &PublicKey) -> bool {
let hash = self.compute_hash();
let msg: Message = hash.into();

if let Ok(recovered_pub_key) = recover(
&msg,
&self.signature.into(),
&RecoveryId::parse(self.rec_id).expect("recovery_id"),
) {
if pub_key != &recovered_pub_key {
return false;
}
} else {
return false;
}

verify(&msg, &self.signature.into(), pub_key)
}

Expand All @@ -256,6 +280,42 @@ impl Transaction {
hasher.update(self.nonce.to_be_bytes());
(&hasher.finalize()[0..32]).into()
}

pub fn validate(&self) -> Result<()> {
if let Payload::Run { ref workflow } = self.payload {
let mut programs = HashSet::new();
for step in &workflow.steps {
if !programs.insert(step.program) {
return Err(TransactionError::Validation(format!(
"multiple programs in workflow: {}",
&step.program
))
.into());
}
}
}

let hash = self.compute_hash();
let msg: Message = hash.into();
if let Ok(recovered_pub_key) = recover(
&msg,
&self.signature.into(),
&RecoveryId::parse(self.rec_id).expect("recovery_id"),
) {
if !verify(&msg, &self.signature.into(), &recovered_pub_key) {
return Err(TransactionError::Validation(String::from(
"signature verification failed",
))
.into());
}
} else {
return Err(
TransactionError::Validation(String::from("public key recovery failed")).into(),
);
}

Ok(())
}
}

#[cfg(test)]
Expand Down Expand Up @@ -289,4 +349,43 @@ mod tests {
// Verify must return false.
assert!(!tx.verify(&pk));
}

#[test]
fn test_tx_validate_ensures_unique_programs() {
let prover = WorkflowStep::default();
let verifier = WorkflowStep::default();

let workflow = Workflow {
// Both steps are `Default::default()` -> same program hash -> invalid.
steps: vec![prover, verifier],
};

let mut tx = Transaction {
hash: Hash::default(),
payload: Payload::Run { workflow },
nonce: 0,
signature: Signature::default(),
propagated: false,
rec_id: 0,
};

let sk = SecretKey::random(&mut StdRng::from_entropy());
tx.sign(&sk);

assert!(tx.validate().is_err());
}

#[test]
fn test_tx_validations_verifies_signature() {
let tx = Transaction {
hash: Hash::default(),
payload: Payload::Empty,
nonce: 0,
signature: Signature::default(),
propagated: false,
rec_id: 0,
};

assert!(tx.validate().is_err());
}
}
4 changes: 4 additions & 0 deletions crates/node/src/workflow/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -452,6 +452,7 @@ mod tests {
nonce: 1,
signature: Signature::default(),
propagated: false,
rec_id: 0,
};

tx.sign(&key);
Expand All @@ -470,6 +471,7 @@ mod tests {
nonce: 1,
signature: Signature::default(),
propagated: false,
rec_id: 0,
};

tx.sign(&key);
Expand All @@ -487,6 +489,7 @@ mod tests {
nonce: 1,
signature: Signature::default(),
propagated: false,
rec_id: 0,
};

tx.sign(&key);
Expand All @@ -505,6 +508,7 @@ mod tests {
nonce: 1,
signature: Signature::default(),
propagated: false,
rec_id: 0,
};

tx.sign(&key);
Expand Down