Skip to content

Commit d16aa9e

Browse files
authored
fix!(da): validate transactions also if no signer whitelist is given (#1208)
1 parent 236c2be commit d16aa9e

File tree

9 files changed

+221
-167
lines changed

9 files changed

+221
-167
lines changed

Cargo.lock

Lines changed: 2 additions & 3 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

protocol-units/da/movement/protocol/light-node/src/sequencer.rs

Lines changed: 30 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,8 @@
1-
use movement_da_light_node_da::DaOperations;
2-
use movement_da_light_node_prevalidator::{aptos::whitelist::Validator, PrevalidatorOperations};
3-
use movement_signer::cryptography::secp256k1::Secp256k1;
4-
use movement_signer_loader::LoadedSigner;
5-
use std::boxed::Box;
6-
use std::fmt::Debug;
7-
use std::path::PathBuf;
8-
use std::pin::Pin;
9-
use std::sync::{atomic::AtomicU64, Arc};
10-
use std::time::Duration;
11-
121
use memseq::{Sequencer, Transaction};
132
use movement_da_light_node_celestia::da::Da as CelestiaDa;
3+
use movement_da_light_node_da::DaOperations;
144
use movement_da_light_node_digest_store::da::Da as DigestStoreDa;
5+
use movement_da_light_node_prevalidator::{aptos::Validator, Prevalidated};
156
use movement_da_light_node_proto as grpc;
167
use movement_da_light_node_proto::blob_response::BlobType;
178
use movement_da_light_node_proto::light_node_service_server::LightNodeService;
@@ -20,8 +11,11 @@ use movement_da_util::{
2011
blob::ir::{blob::DaBlob, data::InnerSignedBlobV1Data},
2112
config::Config,
2213
};
14+
use movement_signer::cryptography::secp256k1::Secp256k1;
2315
use movement_signer::{cryptography::Curve, Digester, Signing, Verify};
16+
use movement_signer_loader::LoadedSigner;
2417
use movement_types::block::Block;
18+
2519
use serde::{Deserialize, Serialize};
2620
use tokio::{
2721
sync::mpsc::{Receiver, Sender},
@@ -30,6 +24,13 @@ use tokio::{
3024
use tokio_stream::Stream;
3125
use tracing::{debug, info};
3226

27+
use std::boxed::Box;
28+
use std::fmt::Debug;
29+
use std::path::PathBuf;
30+
use std::pin::Pin;
31+
use std::sync::{atomic::AtomicU64, Arc};
32+
use std::time::Duration;
33+
3334
use crate::{passthrough::LightNode as LightNodePassThrough, LightNodeRuntime};
3435

3536
const LOGGING_UID: AtomicU64 = AtomicU64::new(0);
@@ -53,7 +54,7 @@ where
5354
{
5455
pub pass_through: LightNodePassThrough<O, C, Da, V>,
5556
pub memseq: Arc<memseq::Memseq<memseq::RocksdbMempool>>,
56-
pub prevalidator: Option<Arc<Validator>>,
57+
pub prevalidator: Arc<Validator>,
5758
}
5859

5960
impl<O, C, Da, V> Debug for LightNode<O, C, Da, V>
@@ -104,10 +105,10 @@ impl LightNodeRuntime
104105

105106
// prevalidator
106107
let whitelisted_accounts = config.whitelisted_accounts()?;
107-
let prevalidator = match whitelisted_accounts {
108-
Some(whitelisted_accounts) => Some(Arc::new(Validator::new(whitelisted_accounts))),
109-
None => None,
110-
};
108+
let prevalidator = Arc::new(match whitelisted_accounts {
109+
Some(whitelisted_accounts) => Validator::with_whitelist(whitelisted_accounts),
110+
None => Validator::new(),
111+
});
111112

112113
Ok(Self { pass_through, memseq, prevalidator })
113114
}
@@ -401,6 +402,8 @@ where
401402
&self,
402403
request: tonic::Request<grpc::BatchWriteRequest>,
403404
) -> std::result::Result<tonic::Response<grpc::BatchWriteResponse>, tonic::Status> {
405+
use movement_da_light_node_prevalidator::Error;
406+
404407
let blobs_for_submission = request.into_inner().blobs;
405408

406409
// make transactions from the blobs
@@ -409,30 +412,18 @@ where
409412
let transaction: Transaction = serde_json::from_slice(&blob.data)
410413
.map_err(|e| tonic::Status::internal(e.to_string()))?;
411414

412-
match &self.prevalidator {
413-
Some(prevalidator) => {
414-
// match the prevalidated status, if validation error discard if internal error raise internal error
415-
match prevalidator.prevalidate(transaction).await {
416-
Ok(prevalidated) => {
417-
transactions.push(prevalidated.into_inner());
418-
}
419-
Err(e) => {
420-
match e {
421-
movement_da_light_node_prevalidator::Error::Validation(_) => {
422-
// discard the transaction
423-
info!(
424-
"discarding transaction due to prevalidation error {:?}",
425-
e
426-
);
427-
}
428-
movement_da_light_node_prevalidator::Error::Internal(e) => {
429-
return Err(tonic::Status::internal(e.to_string()));
430-
}
431-
}
432-
}
433-
}
415+
// match the prevalidated status, if validation error discard if internal error raise internal error
416+
match self.prevalidator.prevalidate(transaction) {
417+
Ok(Prevalidated(transaction)) => {
418+
transactions.push(transaction);
419+
}
420+
Err(Error::Validation(e)) => {
421+
// discard the transaction
422+
info!("discarding transaction due to prevalidation error: {}", e);
423+
}
424+
Err(Error::Internal(e)) => {
425+
return Err(tonic::Status::internal(e.to_string()));
434426
}
435-
None => transactions.push(transaction),
436427
}
437428
}
438429

protocol-units/da/movement/protocol/prevalidator/Cargo.toml

Lines changed: 4 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -28,14 +28,13 @@ thiserror = { workspace = true }
2828
serde_json = { workspace = true }
2929
ecdsa = { workspace = true, features = ["signing", "verifying", "der"] }
3030
tracing = { workspace = true }
31-
movement-types = { workspace = true}
31+
movement-types = { workspace = true }
3232
bcs = { workspace = true }
33-
aptos-types = { workspace = true, optional = true}
33+
aptos-types = { workspace = true, optional = true }
3434

3535
[dev-dependencies]
36-
movement-da-light-node-setup = { workspace = true }
37-
dot-movement = { workspace = true }
38-
k256 = { workspace = true }
36+
aptos-crypto = { workspace = true }
37+
aptos-sdk = { workspace = true }
3938
rand = { workspace = true }
4039

4140

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,183 @@
1+
//! Prevalidation of Aptos transactions.
2+
3+
use crate::{Error, Prevalidated};
4+
5+
use aptos_types::account_address::AccountAddress;
6+
use aptos_types::transaction::SignedTransaction as AptosTransaction;
7+
use movement_types::transaction::Transaction;
8+
9+
use std::collections::HashSet;
10+
11+
/// Prevalidates a Transaction as a correctly encoded and signed AptosTransaction,
12+
/// optionally vetted against a whitelist of sender addresses.
13+
pub struct Validator {
14+
whitelist: Option<HashSet<AccountAddress>>,
15+
}
16+
17+
impl Validator {
18+
/// Creates a Validator with no whitelist. All well-formed signed transactions
19+
/// are validated.
20+
pub fn new() -> Self {
21+
Validator { whitelist: None }
22+
}
23+
24+
/// Creates a Validator configured with a whitelist. Transactions are checked
25+
/// against the addresses in the whitelist, in addition to being well-formed
26+
/// and signed.
27+
pub fn with_whitelist<I>(whitelist: I) -> Self
28+
where
29+
I: IntoIterator<Item = AccountAddress>,
30+
{
31+
Validator { whitelist: Some(whitelist.into_iter().collect()) }
32+
}
33+
34+
/// Returns `Ok` if the transaction is valid accordingly to this instance's
35+
/// configuration. `Err` is returned for validation errors.
36+
pub fn prevalidate(
37+
&self,
38+
transaction: Transaction,
39+
) -> Result<Prevalidated<Transaction>, Error> {
40+
// Deserialize data as Aptos transaction, fail if invalid.
41+
let aptos_transaction: AptosTransaction =
42+
bcs::from_bytes(&transaction.data()).map_err(|e| {
43+
Error::Validation(format!("failed to deserialize Aptos transaction: {}", e))
44+
})?;
45+
46+
// Verify that the signature is valid
47+
aptos_transaction
48+
.verify_signature()
49+
.map_err(|e| Error::Validation(format!("signature verification failed: {}", e)))?;
50+
51+
// Check the sender against the whitelist, if provided.
52+
if let Some(whitelist) = &self.whitelist {
53+
if !whitelist.contains(&aptos_transaction.sender()) {
54+
return Err(Error::Validation("transaction sender not in whitelist".into()));
55+
}
56+
}
57+
58+
Ok(Prevalidated(transaction))
59+
}
60+
}
61+
62+
#[cfg(test)]
63+
mod tests {
64+
use super::*;
65+
use aptos_crypto::{ed25519::Ed25519PrivateKey, Uniform as _};
66+
use aptos_sdk::{
67+
transaction_builder::TransactionFactory,
68+
types::{chain_id::ChainId, LocalAccount},
69+
};
70+
use aptos_types::account_config::aptos_test_root_address;
71+
use aptos_types::transaction::{RawTransaction, SignedTransaction};
72+
use movement_types::transaction::Transaction;
73+
74+
use rand::rngs::OsRng;
75+
76+
fn create_test_transaction(account: &LocalAccount) -> Result<Transaction, anyhow::Error> {
77+
let tx_factory = TransactionFactory::new(ChainId::test())
78+
.with_gas_unit_price(100)
79+
.with_max_gas_amount(100_000);
80+
81+
let aptos_transaction = account
82+
.sign_with_transaction_builder(tx_factory.create_user_account(account.public_key()));
83+
84+
let serialized_aptos_transaction = bcs::to_bytes(&aptos_transaction)?;
85+
86+
Ok(Transaction::new(serialized_aptos_transaction, 0, aptos_transaction.sequence_number()))
87+
}
88+
89+
fn create_test_transaction_with_invalid_signature(
90+
account: &LocalAccount,
91+
) -> Result<Transaction, anyhow::Error> {
92+
// Create a raw transaction
93+
let factory = TransactionFactory::new(ChainId::test());
94+
let raw_txn: RawTransaction = factory
95+
.transfer(aptos_test_root_address(), 1000)
96+
.sender(account.address())
97+
.sequence_number(0)
98+
.build();
99+
100+
// Now generate a DIFFERENT key to sign (invalid signer)
101+
let bad_key = Ed25519PrivateKey::generate(&mut OsRng);
102+
103+
// Manually create a SignedTransaction with the wrong signature
104+
let aptos_transaction = raw_txn.sign(
105+
&bad_key,
106+
account.public_key().clone(), // <- NOTE: still using the correct pubkey
107+
)?;
108+
let aptos_transaction: SignedTransaction = aptos_transaction.into_inner();
109+
110+
let serialized_aptos_transaction = bcs::to_bytes(&aptos_transaction)?;
111+
112+
Ok(Transaction::new(serialized_aptos_transaction, 0, aptos_transaction.sequence_number()))
113+
}
114+
115+
#[test]
116+
fn invalid_transaction() -> Result<(), anyhow::Error> {
117+
let tx = Transaction::new(vec![42; 42], 0, 0);
118+
119+
let validator = Validator::new();
120+
121+
match validator.prevalidate(tx) {
122+
Err(Error::Validation(_)) => Ok(()),
123+
Err(e) => panic!("unexpected error: {e:?}"),
124+
Ok(_) => panic!("should not prevalidate an invalid payload"),
125+
}
126+
}
127+
128+
#[test]
129+
fn incorrectly_signed_transaction() -> Result<(), anyhow::Error> {
130+
let account = LocalAccount::generate(&mut OsRng);
131+
let tx = create_test_transaction_with_invalid_signature(&account)?;
132+
133+
let validator = Validator::new();
134+
135+
match validator.prevalidate(tx) {
136+
Err(Error::Validation(_)) => Ok(()),
137+
Err(e) => panic!("unexpected error: {e:?}"),
138+
Ok(_) => panic!("should not prevalidate an invalid payload"),
139+
}
140+
}
141+
142+
#[test]
143+
fn valid_transaction_no_whitelist() -> Result<(), anyhow::Error> {
144+
let account = LocalAccount::generate(&mut OsRng);
145+
let tx = create_test_transaction(&account)?;
146+
let tx_hash = tx.id();
147+
148+
let validator = Validator::new();
149+
let Prevalidated(tx) = validator.prevalidate(tx)?;
150+
151+
assert_eq!(tx.id(), tx_hash);
152+
Ok(())
153+
}
154+
155+
#[test]
156+
fn valid_transaction_sender_whitelisted() -> Result<(), anyhow::Error> {
157+
let account = LocalAccount::generate(&mut OsRng);
158+
let tx = create_test_transaction(&account)?;
159+
let tx_hash = tx.id();
160+
161+
let validator = Validator::with_whitelist([account.address()]);
162+
let Prevalidated(tx) = validator.prevalidate(tx)?;
163+
164+
assert_eq!(tx.id(), tx_hash);
165+
Ok(())
166+
}
167+
168+
#[test]
169+
fn valid_transaction_sender_not_in_whitelist() -> Result<(), anyhow::Error> {
170+
let mut rng = OsRng;
171+
let account = LocalAccount::generate(&mut rng);
172+
let tx = create_test_transaction(&account)?;
173+
let whitelisted_account = LocalAccount::generate(&mut rng);
174+
175+
let validator = Validator::with_whitelist([whitelisted_account.address()]);
176+
177+
match validator.prevalidate(tx) {
178+
Err(Error::Validation(_)) => Ok(()),
179+
Err(e) => panic!("unexpected error: {e:?}"),
180+
Ok(_) => panic!("should not prevalidate an invalid payload"),
181+
}
182+
}
183+
}

protocol-units/da/movement/protocol/prevalidator/src/aptos/mod.rs

Lines changed: 0 additions & 2 deletions
This file was deleted.

protocol-units/da/movement/protocol/prevalidator/src/aptos/transaction.rs

Lines changed: 0 additions & 26 deletions
This file was deleted.

0 commit comments

Comments
 (0)