[address-balance] support multi-epoch ValidDuring

Author: alex-mysten

crates/sui-core/src/authority.rs

@@ -10,6 +10,7 @@ use crate::congestion_tracker::CongestionTracker;
 use crate::consensus_adapter::ConsensusOverloadChecker;
 use crate::execution_cache::ExecutionCacheTraitPointers;
 use crate::execution_cache::TransactionCacheRead;
+use crate::execution_cache::writeback_cache::WritebackCache;
 use crate::execution_scheduler::ExecutionScheduler;
 use crate::execution_scheduler::SchedulingSource;
 use crate::jsonrpc_index::CoinIndexKey2;
@@ -1260,6 +1261,16 @@ impl AuthorityState {
             return Ok(());
         }
 
+        if let Ok(true) = self
+            .get_transaction_cache_reader()
+            .transaction_executed_in_last_epoch(transaction.digest(), epoch_store.epoch())
+        {
+            return Err(SuiErrorKind::TransactionAlreadyExecuted {
+                digest: (*transaction.digest()),
+            }
+            .into());
+        }
+
         let result =
             self.handle_transaction_impl(transaction, false /* sign */, epoch_store)?;
         assert!(
@@ -3721,6 +3732,12 @@ impl AuthorityState {
             .database_for_testing()
     }
 
+    pub fn cache_for_testing(&self) -> &WritebackCache {
+        self.execution_cache_trait_pointers
+            .testing_api
+            .cache_for_testing()
+    }
+
     pub async fn prune_checkpoints_for_eligible_epochs_for_testing(
         &self,
         config: NodeConfig,

crates/sui-core/src/authority/authority_store.rs

@@ -1597,6 +1597,18 @@ impl AuthorityStore {
         let _ = AuthorityStorePruner::compact(&self.perpetual_tables);
     }
 
+    pub fn remove_executed_effects_for_testing(
+        &self,
+        tx_digest: &TransactionDigest,
+    ) -> anyhow::Result<()> {
+        let effects_digest = self.perpetual_tables.executed_effects.get(tx_digest)?;
+        if let Some(effects_digest) = effects_digest {
+            self.perpetual_tables.executed_effects.remove(tx_digest)?;
+            self.perpetual_tables.effects.remove(&effects_digest)?;
+        }
+        Ok(())
+    }
+
     #[cfg(test)]
     pub async fn prune_objects_immediately_for_testing(
         &self,

crates/sui-core/src/authority/authority_store_tables.rs

@@ -633,6 +633,19 @@ impl AuthorityPerpetualTables {
         Ok(self.effects.get(&effect_digest)?)
     }
 
+    pub(crate) fn was_transaction_executed_in_last_epoch(
+        &self,
+        digest: &TransactionDigest,
+        current_epoch: EpochId,
+    ) -> SuiResult<bool> {
+        if current_epoch == 0 {
+            return Ok(false);
+        }
+        Ok(self
+            .executed_transaction_digests
+            .contains_key(&(current_epoch - 1, *digest))?)
+    }
+
     // DEPRECATED as the backing table has been moved to authority_per_epoch_store.
     // Please do not add new accessors/callsites.
     pub fn get_checkpoint_sequence_number(

crates/sui-core/src/authority_server.rs

@@ -757,6 +757,21 @@ impl ValidatorService {
                 }
             }
 
+            if self
+                .state
+                .get_transaction_cache_reader()
+                .transaction_executed_in_last_epoch(tx_digest, epoch_store.epoch())?
+            {
+                results[idx] = Some(SubmitTxResult::Rejected {
+                    error: UserInputError::TransactionAlreadyExecuted { digest: *tx_digest }.into(),
+                });
+                debug!(
+                    ?tx_digest,
+                    "handle_submit_transaction: transaction already executed in previous epoch"
+                );
+                continue;
+            }
+
             debug!(
                 ?tx_digest,
                 "handle_submit_transaction: waiting for fastpath dependency objects"

crates/sui-core/src/checkpoints/mod.rs

@@ -3757,6 +3757,14 @@ mod tests {
         ) -> Option<Vec<sui_types::storage::ObjectKey>> {
             unimplemented!()
         }
+
+        fn transaction_executed_in_last_epoch(
+            &self,
+            _: &TransactionDigest,
+            _: EpochId,
+        ) -> SuiResult<bool> {
+            unimplemented!()
+        }
     }
 
     #[async_trait::async_trait]

crates/sui-core/src/execution_cache.rs

@@ -500,6 +500,12 @@ pub trait TransactionCacheRead: Send + Sync {
             .expect("multi-get must return correct number of items")
     }
 
+    fn transaction_executed_in_last_epoch(
+        &self,
+        digest: &TransactionDigest,
+        current_epoch: EpochId,
+    ) -> SuiResult<bool>;
+
     fn multi_get_effects(
         &self,
         digests: &[TransactionEffectsDigest],
@@ -689,6 +695,8 @@ pub trait StateSyncAPI: Send + Sync {
 
 pub trait TestingAPI: Send + Sync {
     fn database_for_testing(&self) -> Arc<AuthorityStore>;
+
+    fn cache_for_testing(&self) -> &WritebackCache;
 }
 
 macro_rules! implement_storage_traits {
@@ -882,6 +890,10 @@ macro_rules! implement_passthrough_traits {
             fn database_for_testing(&self) -> Arc<AuthorityStore> {
                 self.store.clone()
             }
+
+            fn cache_for_testing(&self) -> &WritebackCache {
+                self
+            }
         }
     };
 }

crates/sui-core/src/execution_cache/writeback_cache.rs

@@ -549,6 +549,12 @@ impl WritebackCache {
         std::mem::swap(self, &mut new);
     }
 
+    pub fn evict_executed_effects_from_cache_for_testing(&self, tx_digest: &TransactionDigest) {
+        self.cached.executed_effects_digests.invalidate(tx_digest);
+        self.cached.transaction_events.invalidate(tx_digest);
+        self.cached.transactions.invalidate(tx_digest);
+    }
+
     fn write_object_entry(
         &self,
         object_id: &ObjectID,
@@ -2082,6 +2088,16 @@ impl TransactionCacheRead for WritebackCache {
         )
     }
 
+    fn transaction_executed_in_last_epoch(
+        &self,
+        digest: &TransactionDigest,
+        current_epoch: EpochId,
+    ) -> SuiResult<bool> {
+        self.store
+            .perpetual_tables
+            .was_transaction_executed_in_last_epoch(digest, current_epoch)
+    }
+
     fn notify_read_executed_effects_digests<'a>(
         &'a self,
         task_name: &'static str,

crates/sui-e2e-tests/tests/address_balance_tests.rs

@@ -1248,6 +1248,48 @@ fn create_withdraw_balance_transaction(
     })
 }
 
+fn create_address_balance_transaction_with_expiration(
+    sender: SuiAddress,
+    rgp: u64,
+    min_epoch: Option<u64>,
+    max_epoch: Option<u64>,
+    chain_id: ChainIdentifier,
+    nonce: u32,
+    gas_test_package_id: ObjectID,
+) -> TransactionData {
+    use sui_types::transaction::{GasData, TransactionDataV1, TransactionExpiration};
+
+    let mut builder = ProgrammableTransactionBuilder::new();
+    let value = builder.pure(42u64).unwrap();
+    builder.programmable_move_call(
+        gas_test_package_id,
+        Identifier::new("gas_test").unwrap(),
+        Identifier::new("create_object_with_storage").unwrap(),
+        vec![],
+        vec![value],
+    );
+    let tx = TransactionKind::ProgrammableTransaction(builder.finish());
+
+    TransactionData::V1(TransactionDataV1 {
+        kind: tx,
+        sender,
+        gas_data: GasData {
+            payment: vec![],
+            owner: sender,
+            price: rgp,
+            budget: 10000000,
+        },
+        expiration: TransactionExpiration::ValidDuring {
+            min_epoch,
+            max_epoch,
+            min_timestamp_seconds: None,
+            max_timestamp_seconds: None,
+            chain: chain_id,
+            nonce,
+        },
+    })
+}
+
 fn create_regular_gas_transaction_with_current_epoch(
     sender: SuiAddress,
     gas_coin: ObjectRef,
@@ -1471,8 +1513,8 @@ async fn test_transaction_expiration_min_none_max_some() {
     let err = result.expect_err("Transaction should be rejected when only max_epoch is specified");
     let err_str = format!("{:?}", err);
     assert!(
-        err_str.contains("Both min_epoch and max_epoch must be specified and equal"),
-        "Expected validation error 'Both min_epoch and max_epoch must be specified and equal', got: {:?}",
+        err_str.contains("Both min_epoch and max_epoch must be specified"),
+        "Expected validation error 'Both min_epoch and max_epoch must be specified', got: {:?}",
         err
     );
 }
@@ -2422,3 +2464,161 @@ async fn test_multiple_deposits_merged_in_effects() {
 
     test_cluster.trigger_reconfiguration().await;
 }
+
+#[sim_test]
+async fn test_reject_transaction_executed_in_previous_epoch() {
+    let _guard = ProtocolConfig::apply_overrides_for_testing(|_, mut cfg| {
+        cfg.enable_address_balance_gas_payments_for_testing();
+        cfg.enable_accumulators_for_testing();
+        cfg
+    });
+
+    let mut test_cluster = TestClusterBuilder::new()
+        .with_num_validators(1)
+        .build()
+        .await;
+
+    let (sender, gas_objects) = get_sender_and_all_gas(&mut test_cluster.wallet).await;
+    let rgp = test_cluster.get_reference_gas_price().await;
+    let chain_id = test_cluster
+        .fullnode_handle
+        .sui_node
+        .with_async(|node| async { node.state().get_chain_identifier() })
+        .await;
+
+    let current_epoch = test_cluster
+        .fullnode_handle
+        .sui_node
+        .with(|node| node.state().epoch_store_for_testing().epoch());
+
+    let gas_test_package_id = setup_test_package(&mut test_cluster.wallet).await;
+
+    let fund_tx = make_send_to_account_tx(100_000_000, sender, sender, gas_objects[1], rgp);
+    test_cluster.sign_and_execute_transaction(&fund_tx).await;
+
+    let tx = create_address_balance_transaction_with_expiration(
+        sender,
+        rgp,
+        Some(current_epoch),
+        Some(current_epoch + 1),
+        chain_id,
+        100,
+        gas_test_package_id,
+    );
+
+    let signed_tx = test_cluster.sign_transaction(&tx).await;
+
+    let response = test_cluster
+        .wallet
+        .execute_transaction_may_fail(signed_tx.clone())
+        .await
+        .unwrap();
+
+    assert!(
+        response.effects.unwrap().status().is_ok(),
+        "First execution should succeed"
+    );
+
+    let tx_digest = *signed_tx.digest();
+
+    test_cluster.trigger_reconfiguration().await;
+
+    for handle in test_cluster.swarm.validator_node_handles() {
+        handle.with(|node| {
+            node.state()
+                .database_for_testing()
+                .remove_executed_effects_for_testing(&tx_digest)
+                .unwrap();
+            node.state()
+                .cache_for_testing()
+                .evict_executed_effects_from_cache_for_testing(&tx_digest);
+        });
+    }
+
+    test_cluster.fullnode_handle.sui_node.with(|node| {
+        node.state()
+            .database_for_testing()
+            .remove_executed_effects_for_testing(&tx_digest)
+            .unwrap();
+        node.state()
+            .cache_for_testing()
+            .evict_executed_effects_from_cache_for_testing(&tx_digest);
+    });
+
+    let result = test_cluster
+        .execute_signed_txns_in_soft_bundle(std::slice::from_ref(&signed_tx))
+        .await;
+
+    match result {
+        Err(e) => {
+            let err_str = e.to_string();
+            assert!(
+                err_str.contains("was already executed"),
+                "Expected 'was already executed' error, got: {}",
+                err_str
+            );
+        }
+        Ok(_) => {
+            panic!(
+                "Transaction should be rejected as already executed in previous epoch, but submission succeeded"
+            );
+        }
+    }
+}
+
+#[sim_test]
+async fn test_transaction_executes_in_next_epoch_with_one_epoch_range() {
+    let _guard = ProtocolConfig::apply_overrides_for_testing(|_, mut cfg| {
+        cfg.enable_address_balance_gas_payments_for_testing();
+        cfg.enable_accumulators_for_testing();
+        cfg
+    });
+
+    let mut test_cluster = TestClusterBuilder::new()
+        .with_num_validators(1)
+        .build()
+        .await;
+
+    let (sender, gas_objects) = get_sender_and_all_gas(&mut test_cluster.wallet).await;
+    let rgp = test_cluster.get_reference_gas_price().await;
+    let chain_id = test_cluster
+        .fullnode_handle
+        .sui_node
+        .with_async(|node| async { node.state().get_chain_identifier() })
+        .await;
+
+    let current_epoch = test_cluster
+        .fullnode_handle
+        .sui_node
+        .with(|node| node.state().epoch_store_for_testing().epoch());
+
+    let gas_test_package_id = setup_test_package(&mut test_cluster.wallet).await;
+
+    let fund_tx = make_send_to_account_tx(100_000_000, sender, sender, gas_objects[1], rgp);
+    test_cluster.sign_and_execute_transaction(&fund_tx).await;
+
+    let tx = create_address_balance_transaction_with_expiration(
+        sender,
+        rgp,
+        Some(current_epoch),
+        Some(current_epoch + 1),
+        chain_id,
+        100,
+        gas_test_package_id,
+    );
+
+    let signed_tx = test_cluster.sign_transaction(&tx).await;
+
+    test_cluster.trigger_reconfiguration().await;
+
+    let response = test_cluster
+        .wallet
+        .execute_transaction_may_fail(signed_tx.clone())
+        .await
+        .unwrap();
+
+    assert!(
+        response.effects.unwrap().status().is_ok(),
+        "Transaction with 1-epoch range should execute successfully in next epoch"
+    );
+}