Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 3 additions & 1 deletion crates/full-node/sov-sequencer/src/preferred/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -424,7 +424,9 @@ where
}

let (outer_res, nonce_to_mark_persisted) = match uniqueness {
UniquenessData::Generation(_) => (
UniquenessData::Generation(_)
| UniquenessData::Timestamp(_)
| UniquenessData::TimestampNonce(_) => (
self.synchronized_state_updator
.accept_tx_msg(
&baked_tx,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ serde = { workspace = true }

sov-modules-api = { workspace = true }
sov-state = { workspace = true }
sov-chain-state = { workspace = true }

[dev-dependencies]
alloy-consensus = { workspace = true }
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use sov_modules_api::capabilities::UniquenessData;
use sov_modules_api::ExecutionContext;
use sov_modules_api::{CredentialId, Spec, StateAccessor, StateReader, TxHash};
use sov_state::User;
use sov_modules_api::{CredentialId, Spec, TimeStateAccessor, TxHash};

use crate::Uniqueness;

Expand All @@ -19,7 +18,7 @@ impl<S: Spec> Uniqueness<S> {
transaction_uniqueness: UniquenessData,
transaction_hash: TxHash,
execution_context: &ExecutionContext,
state: &mut impl StateReader<User>,
state: &mut impl TimeStateAccessor,
) -> anyhow::Result<()> {
match transaction_uniqueness {
UniquenessData::Nonce(nonce) => match execution_context {
Expand All @@ -31,6 +30,9 @@ impl<S: Spec> Uniqueness<S> {
UniquenessData::Generation(generation) => {
self.check_generation_uniqueness(credential_id, generation, transaction_hash, state)
}
UniquenessData::Timestamp(ts) | UniquenessData::TimestampNonce(ts) => {
self.check_timestamp_uniqueness(credential_id, ts, transaction_hash, state)
}
}
}

Expand All @@ -43,7 +45,7 @@ impl<S: Spec> Uniqueness<S> {
credential_id: &CredentialId,
transaction_generation: UniquenessData,
transaction_hash: TxHash,
state: &mut impl StateAccessor,
state: &mut impl TimeStateAccessor,
) -> anyhow::Result<()> {
match transaction_generation {
UniquenessData::Nonce(_) => self.mark_nonce_tx_attempted(credential_id, state),
Expand All @@ -53,6 +55,12 @@ impl<S: Spec> Uniqueness<S> {
transaction_hash,
state,
),
UniquenessData::Timestamp(ts) => {
self.mark_timestamp_tx_attempted(None, ts, transaction_hash, state)
}
UniquenessData::TimestampNonce(ts) => {
self.mark_timestamp_tx_attempted(Some(credential_id), ts, transaction_hash, state)
}
Comment on lines +58 to +63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(non-blocking): flagging that this will be a chain hash breaking change

}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
mod capabilities;
mod generations;
mod nonces;
mod timestamp;
use std::collections::{BTreeMap, HashSet};

use sov_modules_api::{
Expand Down Expand Up @@ -34,6 +35,14 @@ pub struct Uniqueness<S: Spec> {
#[state]
pub(crate) nonces: StateMap<CredentialId, u64>,

/// Buckets of transactions with their expiry timestamp. The
/// buckets are taken from the first bytes of the TxHash.
#[state]
pub(crate) timestamps: StateMap<u16, Vec<(TxHash, u64)>>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(non-blocking): Noting for other reviewers that this looks non-breaking since it's the last #[state] item in the module


#[module]
pub(crate) chain_state: sov_chain_state::ChainState<S>,

#[phantom]
phantom: std::marker::PhantomData<S>,
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
use sov_modules_api::{CredentialId, Spec, TimeStateAccessor, TxHash};

use crate::Uniqueness;

impl<S: Spec> Uniqueness<S> {
pub(crate) fn check_timestamp_uniqueness(
&self,
credential_id: &CredentialId,
expires: u64,
transaction_hash: TxHash,
state: &mut impl TimeStateAccessor,
) -> anyhow::Result<()> {
let current_time = self.chain_state.get_oracle_time_nanos(state)? / 1000;
anyhow::ensure!(
expires as u128 > current_time,
"Time-expired transaction for credential_id {credential_id}: hash {transaction_hash:}"
);

anyhow::ensure!((expires as u128) < current_time + 60_000_000,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: let's extract this into a const

"Future transaction for credential_id {credential_id}: hash {transaction_hash:} needs to wait."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's probably helpful to include the two timestamps that lead to the rejection in this message

);
anyhow::ensure!(
expires > self.nonces.get(credential_id, state)?.unwrap_or_default(),
"Nonce-expired transaction for credential_id {credential_id}: hash {transaction_hash:} is invalidated"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: it's probably helpful to include the two timestamps that lead to the rejection in this message

);

let bucket = self
.timestamps
.get(&Self::hash_to_bucket(&transaction_hash), state)?
.unwrap_or_default();

if bucket.iter().any(|(tx, _)| transaction_hash == *tx) {
return Err(anyhow::anyhow!("Duplicate transaction for credential_id {credential_id}: hash {transaction_hash:} has already been seen"));
}
Ok(())
}

pub(crate) fn mark_timestamp_tx_attempted(
&mut self,
credential_id: Option<&CredentialId>,
expires: u64,
transaction_hash: TxHash,
state: &mut impl TimeStateAccessor,
) -> anyhow::Result<()> {
let bucket = Self::hash_to_bucket(&transaction_hash);
let current_time = self.chain_state.get_oracle_time_nanos(state)? / 1000;

if let Some(credential_id) = credential_id {
self.nonces.set(credential_id, &expires, state)?;
}
Comment on lines +48 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyone who uses this functionality is vulnerable to replay attacks. What's the design goal here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I see this is only intended to increment the nonce. this is probably fine then, but let's add documentation explaining why this is safe


let mut data = self.timestamps.get(&bucket, state)?.unwrap_or_default();
data.retain(|(_, ts)| *ts as u128 > current_time);
data.push((transaction_hash, expires));
Ok(self.timestamps.set(&bucket, &data, state)?)
}

/// Use the first two bytes as bucket value.
fn hash_to_bucket(transaction_hash: &TxHash) -> u16 {
let ptr: &[u8] = transaction_hash.as_ref();
unsafe { core::ptr::read_unaligned(ptr.as_ptr() as *const u16) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a strong norm of avoiding unsafe unless absolutely necessary. It looks to me like we can just copy_from_slice and from_le_bytes to get the same behavior but with no unsoundness on big-endian platforms (admittedly, those are very rare!) and no headaches around alignment

}
}
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use sov_modules_api::capabilities::UniquenessData;
use sov_modules_api::macros::config_value;
use sov_modules_api::{CredentialId, TxEffect};
use sov_test_utils::{BatchType, SlotInput, TransactionTestCase, TxProcessingError};
Expand Down Expand Up @@ -58,14 +59,18 @@ fn do_max_stored_tx_hashes_per_credential_test() {
// Generate txs to fill up our "bucket" of stored transaction hashes.
for i in 0..txs_per_generation {
for generation in 0..num_generations {
txs.push(generate_value_setter_tx(generation, i as u32, &admin));
txs.push(generate_value_setter_tx(
UniquenessData::Generation(generation),
i as u32,
&admin,
));
}
}
// We divided txs evenly across generations - if there was a remainder, account for it by putting the
// extra txs in the first bucket.
for i in 0..extra_txs_in_first_generation {
txs.push(generate_value_setter_tx(
0,
UniquenessData::Generation(0),
(i + txs_per_generation) as u32,
&admin,
))
Expand All @@ -87,7 +92,7 @@ fn do_max_stored_tx_hashes_per_credential_test() {
// Send one more transaction with a current generation number.
// This transaction should be skipped because it would cause the bucket to overflow.
runner.execute_transaction(TransactionTestCase {
input: generate_value_setter_tx(0, u32::MAX, &admin),
input: generate_value_setter_tx(UniquenessData::Generation(0), u32::MAX, &admin),
assert: Box::new(move |ctx, _| {
let TxEffect::Skipped(skipped) = ctx.tx_receipt else {
panic!("Transaction should be skipped");
Expand All @@ -106,7 +111,11 @@ fn do_max_stored_tx_hashes_per_credential_test() {
// Increment the generation number. Now the transaction should be accepted because it won't cause the bucket to overflow.
// Note that we need to add 1 to the number of generations because we have a strict inequality comparison for buckets.
runner.execute_transaction(TransactionTestCase {
input: generate_value_setter_tx(num_generations + 1, txs_per_generation as u32, &admin),
input: generate_value_setter_tx(
UniquenessData::Generation(num_generations + 1),
txs_per_generation as u32,
&admin,
),
assert: Box::new(move |ctx, _| {
assert!(
ctx.tx_receipt.is_successful(),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,12 @@ pub(crate) fn generate_default_tx(
create_contract_tx,
))
}
UniquenessData::Generation(generation) => generate_value_setter_tx(generation, 10, admin),
x => generate_value_setter_tx(x, 10, admin),
}
}

pub(crate) fn generate_value_setter_tx(
generation: u64,
nonce: UniquenessData,
value: u32,
admin: &TestUser<S>,
) -> TransactionType<RT, S> {
Expand All @@ -99,7 +99,7 @@ pub(crate) fn generate_value_setter_tx(
config_chain_id(),
TEST_DEFAULT_MAX_PRIORITY_FEE,
TEST_DEFAULT_MAX_FEE,
UniquenessData::Generation(generation),
nonce,
None,
);

Expand Down
7 changes: 4 additions & 3 deletions crates/module-system/sov-capabilities/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ use sov_modules_api::SequencerType;
use sov_modules_api::{
AggregatedProofPublicData, Amount, Context, DaSpec, Gas, GetGasPrice, InfallibleStateAccessor,
InvalidProofError, ModuleInfo, OperatingMode, Rewards, SovAttestation,
SovStateTransitionPublicData, Spec, StateAccessor, StateReader, StateWriter, Storage, TxState,
SovStateTransitionPublicData, Spec, StateAccessor, StateReader, StateWriter, Storage,
TimeStateAccessor, TxState,
};
use sov_rollup_interface::common::SlotNumber;
use sov_rollup_interface::zk::aggregated_proof::SerializedAggregatedProof;
Expand Down Expand Up @@ -255,7 +256,7 @@ impl<S: Spec, T> TransactionAuthorizer<S> for StandardProvenRollupCapabilities<'
auth_data: &AuthorizationData<S>,
_context: &Context<S>,
execution_context: &ExecutionContext,
state: &mut impl StateReader<User>,
state: &mut impl TimeStateAccessor,
) -> anyhow::Result<()> {
self.uniqueness.check_uniqueness(
&auth_data.credential_id,
Expand All @@ -271,7 +272,7 @@ impl<S: Spec, T> TransactionAuthorizer<S> for StandardProvenRollupCapabilities<'
&mut self,
auth_data: &AuthorizationData<S>,
_sequencer: &<S::Da as DaSpec>::Address,
state: &mut impl StateAccessor,
state: &mut impl TimeStateAccessor,
) -> anyhow::Result<()> {
self.uniqueness.mark_tx_attempted(
&auth_data.credential_id,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ use sov_rollup_interface::{Bytes, TxHash};
use sov_universal_wallet::UniversalWallet;

use crate::transaction::Credentials;
use crate::{Context, SequencerType, Spec, StateAccessor};
use crate::{Context, SequencerType, Spec, StateAccessor, TimeStateAccessor};

/// Authorizes transactions to be executed.
pub trait TransactionAuthorizer<S: Spec> {
Expand Down Expand Up @@ -42,15 +42,15 @@ pub trait TransactionAuthorizer<S: Spec> {
auth_data: &AuthorizationData<S>,
context: &Context<S>,
execution_context: &ExecutionContext,
state: &mut impl StateAccessor,
state: &mut impl TimeStateAccessor,
) -> anyhow::Result<()>;

/// Marks a transaction as having been executed, preventing it from executing again.
fn mark_tx_attempted(
&mut self,
auth_data: &AuthorizationData<S>,
sequencer: &<<S as Spec>::Da as DaSpec>::Address,
state: &mut impl StateAccessor,
state: &mut impl TimeStateAccessor,
) -> anyhow::Result<()>;
}

Expand All @@ -76,6 +76,12 @@ pub enum UniquenessData {
/// Transactions older than this buffer are invalid, transactions falling within it or with a
/// higher generation are valid but must have a unique hash within their generation
Generation(u64),
/// Timestamp-based uniqueness: a transaction expires at the point given in microseconds of
/// OracleTime.
Timestamp(u64),
/// Timestamp-based uniqueness: a transaction expires at the point given in microseconds of
/// OracleTime but it also updates the nonce so that older transactions get invalidated.
TimestampNonce(u64),
}

/// Data required to authorize a sov-transaction.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,7 @@ impl<S: Spec, I: StateProvider<S>> StateWriter<User> for TxScratchpad<S, I> {
}

impl<S: Spec, I: StateProvider<S>> ProvableStateReader<User> for PreExecWorkingSet<S, I> {}
impl<S: Spec, I: StateProvider<S>> ProvableStateReader<Kernel> for PreExecWorkingSet<S, I> {}
impl<S: Spec, I: StateProvider<S>> ProvableStateWriter<User> for PreExecWorkingSet<S, I> {}

impl<S: Spec, I: StateProvider<S>> ProvableStateReader<User> for WorkingSet<S, I> {}
Expand Down
2 changes: 1 addition & 1 deletion crates/module-system/sov-modules-api/src/state/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub use traits::{
GenesisState, InfallibleKernelStateAccessor, InfallibleStateAccessor,
InfallibleStateReaderAndWriter, PerBlockCache, PrivilegedKernelAccessor, ProvableStateReader,
ProvableStateWriter, StateAccessor, StateAccessorError, StateReader, StateReaderAndWriter,
StateWriter, TxState, VersionReader,
StateWriter, TimeStateAccessor, TxState, VersionReader,
};

#[cfg(feature = "native")]
Expand Down
18 changes: 18 additions & 0 deletions crates/module-system/sov-modules-api/src/state/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,24 @@ pub trait StateAccessor: StateReaderAndWriter<User> {
}
}

/// Trait to access time through sov_chain_state.
pub trait TimeStateAccessor:
StateReader<User, Error: Into<anyhow::Error>>
+ StateWriter<User, Error = <Self as StateReader<User>>::Error>
+ StateReader<Kernel, Error = <Self as StateReader<User>>::Error>
+ VersionReader
{
}

/// Auto implement trait.
impl<T> TimeStateAccessor for T where
T: StateReader<User, Error: Into<anyhow::Error>>
+ StateWriter<User, Error = <Self as StateReader<User>>::Error>
+ StateReader<Kernel, Error = <Self as StateReader<User>>::Error>
+ VersionReader
{
}

/// A trait that represents a [`StateAccessor`] that never fails on state accesses. Accessing the state with structs that implement
/// this trait will return [`Infallible`].
///
Expand Down
Loading