test: followups & integration tests optimizations

bin/integration-tests/README.md

@@ -239,8 +239,8 @@ for as long as it lives. A test that needs the funding to land in a particular t
 asserting on what a sync reports, say — should call `TestClient::take_funding` and consume the note
 itself.
 
-Funders must be **public** and carry their secret key: a public funder's state is re-read from the
-chain before every payment, which is what makes sharing one between test processes safe.
+Funders must be **public** and carry their secret key: a public funder's state is read from the
+chain by whichever process claims it, which is what makes sharing one between test processes safe.
 
 ### Environment variables
 

bin/integration-tests/build.rs

@@ -38,7 +38,9 @@ async fn {TEST_FUNCTION_NAME}() -> Result<()> {{
     let client_config = ClientConfig::default()
         .with_note_transport_endpoint(None)
         .with_funders(fee_funding::funders_path_from_env().as_deref())?;
-    {ORIGINAL_FUNCTION_NAME}(client_config).await
+    let result = {ORIGINAL_FUNCTION_NAME}(client_config.clone()).await;
+    let flushed = client_config.flush_funder().await;
+    result.and(flushed)
 }}"#;
 
 const TEST_REGISTRY_HEADER: &str = r#"// Auto-generated test cases module
@@ -299,9 +301,9 @@ fn parse_test_function_name(line: &str) -> Option<String> {
     let tokens: Vec<&str> = s.split_whitespace().collect();
     // Look for public function patterns
     let fn_pos = if tokens[0] == "pub" && tokens[1] == "async" && tokens[2] == "fn" {
-        2 // pub async fn 
+        2 // pub async fn
     } else if tokens[0] == "pub" && tokens[1] == "fn" {
-        1 // pub fn 
+        1 // pub fn
     } else {
         return None;
     };

bin/integration-tests/src/config.rs

@@ -125,12 +125,23 @@ impl ClientConfig {
         Ok(self.with_fee_funder(fee_funder))
     }
 
-    /// Creates a `TestClient` builder.
+    /// Waits until a block carries every payment the fee funder has submitted.
+    pub async fn flush_funder(&self) -> Result<()> {
+        match &self.fee_funder {
+            Some(funder) => funder.flush().await,
+            None => Ok(()),
+        }
+    }
+
+    /// Creates a `TestClient` without syncing it, for tests that have to wait for the node first.
     ///
-    /// The store is a `SQLite` database at a temporary location, and the keystore a temporary
-    /// directory, both created here rather than held on the config, so every client this is called
-    /// on gets its own.
-    pub fn into_client_builder(self) -> Result<ClientBuilder<FilesystemKeyStore>> {
+    /// The client gets its own store and keystore, the latter reachable through
+    /// `TestClient::keystore`. The store is a `SQLite` database at a temporary location, and the
+    /// keystore a temporary directory, both created here rather than held on the config, so every
+    /// client this is called on gets its own.
+    pub async fn into_unsynced_client(self) -> Result<TestClient> {
+        let fee_funder = self.fee_funder.clone();
+
         let store_config = create_test_store_path();
         let auth_path = create_test_auth_path();
 
@@ -170,17 +181,6 @@ impl ClientConfig {
             builder = builder.note_transport(nt_client);
         }
 
-        Ok(builder)
-    }
-
-    /// Creates a `TestClient` without syncing it, for tests that have to wait for the node first.
-    ///
-    /// The client gets its own store and keystore, the latter reachable through
-    /// `TestClient::keystore`.
-    pub async fn into_unsynced_client(self) -> Result<TestClient> {
-        let fee_funder = self.fee_funder.clone();
-        let builder = self.into_client_builder()?;
-
         let client = builder.build().await.with_context(|| "failed to build test client")?;
 
         Ok(TestClient::from(client).with_fee_funder(fee_funder))

bin/integration-tests/src/fee_funding.rs

@@ -1,29 +1,31 @@
 //! Supplies the native fee asset to the accounts the test helpers create, so the suite can run
 //! against a chain that charges transaction fees.
 //!
-//! Both runners give every test its own process, so a payment claims a wallet before using it,
-//! taking the first advisory lock in the pool that is free. Claiming rather than assigning by
-//! ordinal is what keeps a small pool useful under concurrency.
+//! Both runners give every test its own process, so a process claims a wallet before its first
+//! payment, taking the first advisory lock in the pool that is free, and holds it until it exits.
+//! Claiming rather than assigning by ordinal is what keeps a small pool useful under concurrency.
 
 use std::fmt;
 use std::fs::File;
 use std::path::{Path, PathBuf};
 use std::sync::Arc;
+use std::time::Duration;
 
 use anyhow::{Context, Result, bail};
-use miden_client::Deserializable;
 use miden_client::account::{AccountFile, AccountId};
 use miden_client::asset::FungibleAsset;
 use miden_client::block::BlockNumber;
 use miden_client::keystore::Keystore;
 use miden_client::note::{Note, NoteType, P2idNote};
 use miden_client::testing::common::TestClient;
 use miden_client::testing::fee::FeeFunder;
-use miden_client::transaction::TransactionRequestBuilder;
+use miden_client::transaction::{TransactionId, TransactionRequest, TransactionRequestBuilder};
+use miden_client::{ClientError, Deserializable};
 use rand::RngExt;
 use rustix::fs::{FlockOperation, flock};
 use rustix::io::Errno;
 use tokio::sync::Mutex;
+use tracing::warn;
 
 use crate::config::ClientConfig;
 
@@ -37,6 +39,9 @@ pub const FUNDER_ACCOUNTS_ENV: &str = "MIDEN_FUNDER_ACCOUNTS_DIR";
 /// tens of thousands of base units, so this covers far more than any one test spends.
 const FUNDING_AMOUNT: u64 = 10_000_000;
 
+/// How long to wait before a rejected payment is submitted again.
+const STALE_WALLET_RETRY_DELAY: Duration = Duration::from_secs(5);
+
 // LOADING
 // ================================================================================================
 
@@ -123,9 +128,20 @@ struct Funder {
     wallets: Vec<AccountFile>,
     /// Where this test starts scanning, so concurrent tests do not all try the same wallet first.
     scan_from: usize,
-    /// Built on the first funding request, holding every wallet's key. Separate from the clients
-    /// the test builds, so consecutive payments from one wallet chain off each other's nonce.
-    client: Mutex<Option<TestClient>>,
+    /// Built on the first funding request.
+    state: Mutex<Option<FunderState>>,
+}
+
+/// The wallet this process pays from.
+struct FunderState {
+    /// Holds every wallet's key. Separate from the clients the test builds, so consecutive payments
+    /// from one wallet chain off each other's nonce.
+    client: TestClient,
+    /// Claim on the wallet, released when the process exits. Whichever process takes the wallet
+    /// next reads its state from the chain, so [`Funder::flush`] runs before the release.
+    lock: AccountLock,
+    /// The last payment the node accepted, until a block carries it.
+    in_flight: Option<TransactionId>,
 }
 
 impl Funder {
@@ -137,10 +153,18 @@ impl Funder {
                 .with_note_transport_endpoint(None),
             wallets,
             scan_from: rand::rng().random::<u32>() as usize,
-            client: Mutex::new(None),
+            state: Mutex::new(None),
         }
     }
 
+    /// Claims a wallet to pay from and builds the client that pays with it.
+    async fn claim_state(&self) -> Result<FunderState> {
+        let lock = self.claim()?;
+        let client = self.build_client().await?;
+
+        Ok(FunderState { client, lock, in_flight: None })
+    }
+
     /// Claims a wallet to pay from, waiting only if every wallet in the pool is busy.
     fn claim(&self) -> Result<AccountLock> {
         for offset in 0..self.wallets.len() {
@@ -150,8 +174,16 @@ impl Funder {
             }
         }
 
-        // Everything is busy. Waiting on this test's own starting wallet spreads the waiters.
+        // Everything is busy. Waiting on this test's own starting wallet spreads the waiters. A
+        // wallet is held for as long as the process that claimed it runs, so this waits for one of
+        // them to exit. Raise `MIDEN_NUM_FUNDER_WALLETS` if a run reaches here often.
         let wallet = &self.wallets[self.scan_from % self.wallets.len()];
+        warn!(
+            wallets = self.wallets.len(),
+            funder_id = %wallet.account.id(),
+            "Every funder wallet is claimed, waiting for one to be released",
+        );
+
         AccountLock::acquire(wallet.account.id())
     }
 
@@ -177,16 +209,18 @@ impl Funder {
         Ok(client)
     }
 
-    /// Pays every account in `targets` from `wallet_id` in a single transaction, returning each
-    /// target paired with the note carrying its funds.
+    /// Pays every account in `targets` from the claimed wallet in a single transaction, returning
+    /// each target paired with the note carrying its funds.
     ///
     /// One transaction rather than one per target: each costs a fee and a proof.
     async fn pay(
         &self,
-        client: &mut TestClient,
-        wallet_id: AccountId,
+        state: &mut FunderState,
         targets: &[AccountId],
     ) -> Result<Vec<(AccountId, Note)>> {
+        let wallet_id = state.lock.account_id();
+        let client = &mut state.client;
+
         // Imported once per client. A re-import of a wallet this client has already paid from
         // fails, because its local nonce is ahead of the chain's until that payment commits.
         if client.account_reader(wallet_id).nonce().await.is_err() {
@@ -231,40 +265,95 @@ impl Funder {
             .build()
             .context("failed to build the funding transaction request")?;
 
-        let tx_id =
-            Box::pin(client.submit_new_transaction(wallet_id, request)).await.with_context(
-                || format!("funder {wallet_id} failed to pay {} accounts", targets.len()),
-            )?;
+        let tx_id = Self::submit_payment(client, wallet_id, request).await.with_context(|| {
+            format!("funder {wallet_id} failed to pay {} accounts", targets.len())
+        })?;
+        state.in_flight = Some(tx_id);
+
+        Ok(funded)
+    }
+
+    /// Submits `request` from `wallet_id`, trying a second time after a sync if the node rejects
+    /// the first attempt.
+    async fn submit_payment(
+        client: &mut TestClient,
+        wallet_id: AccountId,
+        request: TransactionRequest,
+    ) -> Result<TransactionId> {
+        let rejection =
+            match Box::pin(client.submit_new_transaction(wallet_id, request.clone())).await {
+                Ok(tx_id) => return Ok(tx_id),
+                Err(err) => err,
+            };
+        if !is_safe_to_retry(&rejection) {
+            return Err(anyhow::Error::new(rejection));
+        }
 
-        // Waited on before the wallet is released: another process claiming it reads its state from
-        // the chain, which does not carry this payment until it commits.
+        warn!(
+            funder_id = %wallet_id,
+            error = %rejection,
+            "The funder payment was rejected, retrying after a sync",
+        );
+        tokio::time::sleep(STALE_WALLET_RETRY_DELAY).await;
         client
-            .wait_for_tx(tx_id)
+            .sync_state()
             .await
-            .with_context(|| format!("the payment from funder {wallet_id} never committed"))?;
+            .context("failed to sync the funder client before a retry")?;
 
-        Ok(funded)
+        Box::pin(client.submit_new_transaction(wallet_id, request))
+            .await
+            .map_err(|err| {
+                anyhow::Error::new(err).context(format!(
+                    "the retry was rejected too, after the first attempt failed with: {rejection}"
+                ))
+            })
     }
 }
 
+/// Returns whether a failed submission definitely left the node's state untouched, so the same
+/// payment can be built and sent again.
+///
+/// A transaction the node accepted, or may have accepted, has already moved the wallet's nonce. A
+/// second copy of it would be rejected, and would hide the first.
+fn is_safe_to_retry(err: &ClientError) -> bool {
+    !matches!(
+        err,
+        ClientError::ApplyTransactionAfterSubmitFailed { .. }
+            | ClientError::SubmissionOutcomeUnknown { .. }
+    )
+}
+
 #[async_trait::async_trait(?Send)]
 impl FeeFunder for Funder {
     async fn fund(&self, account_ids: &[AccountId]) -> Result<Vec<(AccountId, Note)>> {
         if account_ids.is_empty() {
             return Ok(Vec::new());
         }
 
-        // Held across the payment only. The notes are spent later, by the funded accounts
-        // themselves on their own client, which never touches this wallet.
-        let _lock = self.claim()?;
+        let mut guard = self.state.lock().await;
+        let state = match guard.as_mut() {
+            Some(state) => state,
+            None => guard.insert(self.claim_state().await?),
+        };
 
-        let mut guard = self.client.lock().await;
-        if guard.is_none() {
-            *guard = Some(self.build_client().await?);
-        }
-        let funder_client = guard.as_mut().expect("the funder client was just built");
+        self.pay(state, account_ids).await
+    }
 
-        self.pay(funder_client, _lock.account_id(), account_ids).await
+    async fn flush(&self) -> Result<()> {
+        let mut guard = self.state.lock().await;
+        let Some(state) = guard.as_mut() else {
+            return Ok(());
+        };
+        let Some(tx_id) = state.in_flight.take() else {
+            return Ok(());
+        };
+        let wallet_id = state.lock.account_id();
+
+        state
+            .client
+            .wait_for_tx(tx_id)
+            .await
+            .with_context(|| format!("the payment from funder {wallet_id} never committed"))
     }
 }
 
@@ -282,9 +371,9 @@ impl fmt::Debug for Funder {
 
 /// An advisory lock over one account, shared across the test processes on this machine.
 ///
-/// Held by the funder pool above and by the agglayer tests over the accounts they share. The lock
-/// file lives in the temp directory, so a read-only account file is never written to, and releases
-/// on drop or when the holding process dies.
+/// Held by the funder above over the wallet it claimed, and by the agglayer tests over the accounts
+/// they share. The lock file lives in the temp directory, so a read-only account file is never
+/// written to, and releases on drop or when the holding process dies.
 pub struct AccountLock {
     file: File,
     account_id: AccountId,

bin/integration-tests/src/main.rs

@@ -468,7 +468,12 @@ fn run_single_test_subprocess(args: &Args, test_name: &str) {
                 .with_prover_endpoint(base_config.prover_endpoint.clone())
                 .with_note_transport_endpoint(base_config.note_transport_endpoint.clone())
                 .with_funders(base_config.funders.as_deref())?;
-            (test.function)(config).await
+            // The funder answers a payment as soon as the node accepts it, so the wallet it paid
+            // from is left at a state the chain agrees with here, once the test no longer needs it.
+            // Reports the test's own error first, since that is the one worth reading.
+            let result = (test.function)(config.clone()).await;
+            let flushed = config.flush_funder().await;
+            result.and(flushed)
         })
     }));
 

bin/integration-tests/src/tests/client.rs

@@ -23,7 +23,7 @@ use miden_client::account::{
 };
 use miden_client::assembly::CodeBuilder;
 use miden_client::asset::{Asset, AssetAmount, FungibleAsset};
-use miden_client::auth::{AuthSchemeId, AuthSecretKey, AuthSingleSig};
+use miden_client::auth::{AuthSchemeId, AuthSecretKey, AuthSingleSig, ECDSA_K256_KECCAK_SCHEME_ID};
 use miden_client::builder::ClientBuilder;
 use miden_client::keystore::FilesystemKeyStore;
 use miden_client::note::standards::NoteSyncHint;
@@ -57,6 +57,7 @@ use miden_client::transaction::{
 };
 use miden_client::{ClientError, Felt, Word};
 use miden_client_sqlite_store::ClientBuilderSqliteExt;
+use rand::Rng;
 use tracing::info;
 
 use crate::{ClientConfig, create_test_auth_path};
@@ -1368,9 +1369,20 @@ pub async fn test_unused_rpc_api(client_config: ClientConfig) -> Result<()> {
         AccountComponentMetadata::new("miden::testing::custom_component"),
     )
     .map_err(|err| anyhow::anyhow!(err))?;
-    let (account_with_map_item, _) = client
-        .insert_account(AccountSetup::wallet(AccountType::Public).component(custom_component))
-        .await?;
+    // The account is built here rather than through a standard setup because it carries the custom
+    // component above on top of a basic wallet.
+    let (auth, key) = auth_component(ECDSA_K256_KECCAK_SCHEME_ID)?;
+    let mut init_seed = [0u8; 32];
+    client.rng().fill_bytes(&mut init_seed);
+    let wallet = AccountBuilder::new(init_seed)
+        .account_type(AccountType::Public)
+        .with_component(auth)
+        .with_component(BasicWallet)
+        .with_component(custom_component)
+        .build_with_schema_commitment()?;
+
+    let (account_with_map_item, _) =
+        client.insert_account(AccountSetup::prebuilt(wallet, key)).await?;
 
     client.sync_state().await.unwrap();