Complete test implementation (#1124)

* Complete test implementation

* Add comment

* Wait for proof

* Sort before checking range

* Send shutdown message to da queue

* Cleanup

* Remove docker from prover test config

* Restore prefixes

* Update bin/citrea/tests/bitcoin_e2e/tests/prover_test.rs

Co-authored-by: jfldde <[email protected]>

* Restore inscribes_queue

* Use finality depth to generate 1 finalized DA height

* Unwrap or else

* Update crates/prover/src/da_block_handler.rs

* Mention L1 height in log

* Remove unnecessary sort

---------

Co-authored-by: jfldde <[email protected]>

bin/citrea/src/rollup/bitcoin.rs

@@ -119,7 +119,7 @@ impl RollupBlueprint for BitcoinRollup {
         rollup_config: &FullNodeConfig<Self::DaConfig>,
         require_wallet_check: bool,
     ) -> Result<Arc<Self::DaService>, anyhow::Error> {
-        let (tx, rx) = unbounded_channel::<SenderWithNotifier<TxidWrapper>>();
+        let (tx, rx) = unbounded_channel::<Option<SenderWithNotifier<TxidWrapper>>>();
 
         let bitcoin_service = if require_wallet_check {
             BitcoinService::new_with_wallet_check(

bin/citrea/tests/bitcoin_e2e/test_case.rs

@@ -3,16 +3,15 @@
 
 use std::panic::{self};
 use std::path::{Path, PathBuf};
-use std::sync::Arc;
 use std::time::Duration;
 
 use anyhow::{bail, Context};
 use async_trait::async_trait;
 use bitcoin_da::service::BitcoinServiceConfig;
 use bitcoincore_rpc::RpcApi;
 use citrea_sequencer::SequencerConfig;
+use futures::FutureExt;
 use sov_stf_runner::{ProverConfig, RpcConfig, RunnerConfig, StorageConfig};
-use tokio::task;
 
 use super::config::{
     default_rollup_config, BitcoinConfig, FullFullNodeConfig, FullProverConfig,
@@ -29,12 +28,12 @@ use crate::bitcoin_e2e::utils::{get_default_genesis_path, get_workspace_root};
 /// It creates a test framework with the associated configs, spawns required nodes, connects them,
 /// runs the test case, and performs cleanup afterwards. The `run` method handles any panics that
 /// might occur during test execution and takes care of cleaning up and stopping the child processes.
-pub struct TestCaseRunner<T: TestCase>(Arc<T>);
+pub struct TestCaseRunner<T: TestCase>(T);
 
 impl<T: TestCase> TestCaseRunner<T> {
     /// Creates a new TestCaseRunner with the given test case.
     pub fn new(test_case: T) -> Self {
-        Self(Arc::new(test_case))
+        Self(test_case)
     }
 
     pub async fn setup(&self, f: &mut TestFramework) -> Result<()> {
@@ -60,7 +59,7 @@ impl<T: TestCase> TestCaseRunner<T> {
     }
 
     /// Internal method to set up connect the nodes, wait for the nodes to be ready and run the test.
-    async fn run_test_case(&self, f: &mut TestFramework) -> Result<()> {
+    async fn run_test_case(&mut self, f: &mut TestFramework) -> Result<()> {
         f.bitcoin_nodes.connect_nodes().await?;
 
         if let Some(sequencer) = &f.sequencer {
@@ -72,34 +71,29 @@ impl<T: TestCase> TestCaseRunner<T> {
 
     /// Executes the test case, handling any panics and performing cleanup.
     ///
-    /// This method spawns a blocking task to run the test, sets up the framework,
-    /// executes the test, and ensures cleanup is performed even if a panic occurs.
-    pub async fn run(self) -> Result<()> {
-        let result = task::spawn_blocking(move || {
-            let mut framework = None;
-            let result = panic::catch_unwind(panic::AssertUnwindSafe(|| {
-                futures::executor::block_on(async {
-                    framework = Some(TestFramework::new(Self::generate_test_config()?).await?);
-                    self.setup(framework.as_mut().unwrap()).await?;
-                    self.run_test_case(framework.as_mut().unwrap()).await
-                })
-            }));
-
-            // Always attempt to stop the framework, even if a panic occurred
-            if let Some(mut f) = framework {
-                let _ = futures::executor::block_on(f.stop());
-
-                if result.is_err() {
-                    if let Err(e) = f.dump_log() {
-                        eprintln!("{e}")
-                    }
+    /// This sets up the framework, executes the test, and ensures cleanup is performed even if a panic occurs.
+    pub async fn run(mut self) -> Result<()> {
+        let result = panic::AssertUnwindSafe(async {
+            let mut framework = TestFramework::new(Self::generate_test_config()?).await?;
+            self.setup(&mut framework).await?;
+
+            let test_result = self.run_test_case(&mut framework).await;
+
+            if test_result.is_err() {
+                if let Err(e) = framework.dump_log() {
+                    eprintln!("Error dumping log: {}", e);
                 }
             }
 
-            result
+            framework.stop().await?;
+
+            test_result
         })
-        .await
-        .expect("Task panicked");
+        .catch_unwind()
+        .await;
+
+        // Additional test cleanup
+        self.0.cleanup().await?;
 
         match result {
             Ok(Ok(())) => Ok(()),
@@ -309,7 +303,11 @@ pub trait TestCase: Send + Sync + 'static {
     ///
     /// # Arguments
     /// * `framework` - A reference to the TestFramework instance
-    async fn run_test(&self, framework: &TestFramework) -> Result<()>;
+    async fn run_test(&mut self, framework: &TestFramework) -> Result<()>;
+
+    async fn cleanup(&self) -> Result<()> {
+        Ok(())
+    }
 }
 
 fn create_dirs(base_dir: &Path) -> Result<[PathBuf; 6]> {

bin/citrea/tests/bitcoin_e2e/tests/mempool_accept.rs

@@ -23,7 +23,7 @@ impl TestCase for MempoolAcceptTest {
         }
     }
 
-    async fn run_test(&self, f: &TestFramework) -> Result<()> {
+    async fn run_test(&mut self, f: &TestFramework) -> Result<()> {
         let sequencer = f.sequencer.as_ref().unwrap();
         let da = f.bitcoin_nodes.get(0).expect("DA not running.");
 

bin/citrea/tests/bitcoin_e2e/tests/prover_test.rs

@@ -3,13 +3,13 @@ use std::time::Duration;
 
 use anyhow::bail;
 use async_trait::async_trait;
-use bitcoin::secp256k1::generate_keypair;
-use bitcoin_da::service::{BitcoinService, BitcoinServiceConfig, FINALITY_DEPTH};
+use bitcoin_da::service::{BitcoinService, BitcoinServiceConfig, TxidWrapper, FINALITY_DEPTH};
 use bitcoin_da::spec::RollupParams;
 use bitcoincore_rpc::RpcApi;
 use citrea_primitives::{REVEAL_BATCH_PROOF_PREFIX, REVEAL_LIGHT_CLIENT_PREFIX};
-use hex::ToHex;
 use sov_rollup_interface::da::{DaData, SequencerCommitment};
+use sov_rollup_interface::services::da::SenderWithNotifier;
+use tokio::sync::mpsc::UnboundedSender;
 
 use crate::bitcoin_e2e::config::{SequencerConfig, TestCaseConfig};
 use crate::bitcoin_e2e::framework::TestFramework;
@@ -42,7 +42,7 @@ impl TestCase for BasicProverTest {
         }
     }
 
-    async fn run_test(&self, f: &TestFramework) -> Result<()> {
+    async fn run_test(&mut self, f: &TestFramework) -> Result<()> {
         let Some(sequencer) = &f.sequencer else {
             bail!("Sequencer not running. Set TestCaseConfig with_sequencer to true")
         };
@@ -111,15 +111,17 @@ impl TestCase for BasicProverTest {
     }
 }
 
-struct SkipPreprovenCommitmentsTest;
+#[derive(Default)]
+struct SkipPreprovenCommitmentsTest {
+    tx: Option<UnboundedSender<Option<SenderWithNotifier<TxidWrapper>>>>,
+}
 
 #[async_trait]
 impl TestCase for SkipPreprovenCommitmentsTest {
     fn test_config() -> TestCaseConfig {
         TestCaseConfig {
             with_prover: true,
             with_full_node: true,
-            docker: true,
             ..Default::default()
         }
     }
@@ -132,7 +134,7 @@ impl TestCase for SkipPreprovenCommitmentsTest {
         }
     }
 
-    async fn run_test(&self, f: &TestFramework) -> Result<()> {
+    async fn run_test(&mut self, f: &TestFramework) -> Result<()> {
         let Some(sequencer) = &f.sequencer else {
             bail!("Sequencer not running. Set TestCaseConfig with_sequencer to true")
         };
@@ -151,11 +153,6 @@ impl TestCase for SkipPreprovenCommitmentsTest {
 
         let _initial_height = f.initial_da_height;
 
-        let (secret_key, _public_key) = generate_keypair(&mut rand::thread_rng());
-        let secret_key = secret_key.secret_bytes().encode_hex();
-        // let key_pair = Keypair::from_secret_key(&secp, &secret_key);
-        // let mut buf = [0u8; constants::SECRET_KEY_SIZE * 2];
-        // to_hex(&self.secret_bytes(), &mut buf).expect("fixed-size hex serialization");
         let da_config = &f.bitcoin_nodes.get(0).unwrap().config;
         let bitcoin_da_service_config = BitcoinServiceConfig {
             node_url: format!(
@@ -166,18 +163,29 @@ impl TestCase for SkipPreprovenCommitmentsTest {
             node_username: da_config.rpc_user.clone(),
             node_password: da_config.rpc_password.clone(),
             network: bitcoin::Network::Regtest,
-            da_private_key: Some(secret_key),
+            da_private_key: Some(
+                // This is the private key used by the sequencer.
+                // This is because the prover has a check to make sure that the commitment was
+                // submitted by the sequencer and NOT any other key. Which means that arbitrary keys
+                // CANNOT submit preproven commitments.
+                // Using the sequencer DA private key means that we simulate the fact that the sequencer
+                // somehow resubmitted the same commitment.
+                "045FFC81A3C1FDB3AF1359DBF2D114B0B3EFBF7F29CC9C5DA01267AA39D2C78D".to_owned(),
+            ),
             tx_backup_dir: get_tx_backup_dir(),
         };
         let (tx, rx) = tokio::sync::mpsc::unbounded_channel();
+        // Keep sender for cleanup
+        self.tx = Some(tx.clone());
+
         let bitcoin_da_service = Arc::new(
             BitcoinService::new_with_wallet_check(
                 bitcoin_da_service_config,
                 RollupParams {
                     reveal_light_client_prefix: REVEAL_LIGHT_CLIENT_PREFIX.to_vec(),
                     reveal_batch_prover_prefix: REVEAL_BATCH_PROOF_PREFIX.to_vec(),
                 },
-                tx,
+                tx.clone(),
             )
             .await
             .unwrap(),
@@ -190,26 +198,31 @@ impl TestCase for SkipPreprovenCommitmentsTest {
         let seq_height0 = sequencer.client.eth_block_number().await;
         assert_eq!(seq_height0, 0);
 
-        for _ in 0..10 {
+        let min_soft_confirmations_per_commitment =
+            sequencer.min_soft_confirmations_per_commitment();
+
+        for _ in 0..min_soft_confirmations_per_commitment {
             sequencer.client.send_publish_batch_request().await;
         }
 
-        da.generate(1 + FINALITY_DEPTH, None).await?;
+        da.generate(FINALITY_DEPTH, None).await?;
 
         // Wait for blob inscribe tx to be in mempool
         da.wait_mempool_len(1, None).await?;
 
-        da.generate(1 + FINALITY_DEPTH, None).await?;
+        da.generate(FINALITY_DEPTH, None).await?;
 
         let finalized_height = da.get_finalized_height().await?;
-        println!("FINALIZED HEIGHT: {}", finalized_height);
         prover
             .wait_for_l1_height(finalized_height, Some(Duration::from_secs(300)))
             .await;
 
-        da.generate(5, None).await?;
+        da.generate(FINALITY_DEPTH, None).await?;
         let proofs = full_node
-            .wait_for_zkproofs(finalized_height + 5, Some(Duration::from_secs(120)))
+            .wait_for_zkproofs(
+                finalized_height + FINALITY_DEPTH,
+                Some(Duration::from_secs(120)),
+            )
             .await
             .unwrap();
 
@@ -220,13 +233,21 @@ impl TestCase for SkipPreprovenCommitmentsTest {
             .preproven_commitments
             .is_empty());
 
+        // Make sure the mempool is mined.
+        da.wait_mempool_len(0, None).await?;
+
         // Fetch the commitment created from the previous L1 range
-        let commitments: Vec<SequencerCommitment> = sequencer
+        let commitments: Vec<SequencerCommitment> = full_node
             .client
             .ledger_get_sequencer_commitments_on_slot_by_number(finalized_height)
             .await
-            .unwrap()
-            .unwrap()
+            .unwrap_or_else(|_| {
+                panic!(
+                    "Failed to get sequencer commitments at {}",
+                    finalized_height
+                )
+            })
+            .unwrap_or_else(|| panic!("No sequencer commitments found at {}", finalized_height))
             .into_iter()
             .map(|response| SequencerCommitment {
                 merkle_root: response.merkle_root,
@@ -235,6 +256,7 @@ impl TestCase for SkipPreprovenCommitmentsTest {
             })
             .collect();
 
+        // Send the same commitment that was already proven.
         let fee_sat_per_vbyte = bitcoin_da_service.get_fee_rate().await.unwrap();
         bitcoin_da_service
             .send_transaction_with_fee_rate(
@@ -245,12 +267,35 @@ impl TestCase for SkipPreprovenCommitmentsTest {
             .await
             .unwrap();
 
-        da.generate(5, None).await?;
+        // Wait for the duplicate commitment transaction to be accepted.
+        da.wait_mempool_len(2, None).await?;
+
+        // Trigger a new commitment.
+        for _ in 0..min_soft_confirmations_per_commitment {
+            sequencer.client.send_publish_batch_request().await;
+        }
+
+        // Wait for the sequencer commitment to be submitted & accepted.
+        da.wait_mempool_len(4, None).await?;
+
+        da.generate(FINALITY_DEPTH, None).await?;
+
+        let finalized_height = da.get_finalized_height().await?;
 
         prover
-            .wait_for_l1_height(FINALITY_DEPTH, Some(Duration::from_secs(300)))
+            .wait_for_l1_height(finalized_height, Some(Duration::from_secs(300)))
             .await;
 
+        da.generate(FINALITY_DEPTH, None).await?;
+
+        let proofs = full_node
+            .wait_for_zkproofs(
+                finalized_height + FINALITY_DEPTH,
+                Some(Duration::from_secs(120)),
+            )
+            .await
+            .unwrap();
+
         assert_eq!(
             proofs
                 .first()
@@ -263,17 +308,24 @@ impl TestCase for SkipPreprovenCommitmentsTest {
 
         Ok(())
     }
+
+    async fn cleanup(&self) -> Result<()> {
+        // Send shutdown message to da queue
+        if let Some(tx) = &self.tx {
+            tx.send(None).unwrap();
+        }
+        Ok(())
+    }
 }
 
 #[tokio::test]
 async fn basic_prover_test() -> Result<()> {
     TestCaseRunner::new(BasicProverTest).run().await
 }
 
-#[ignore]
 #[tokio::test]
 async fn prover_skips_preproven_commitments_test() -> Result<()> {
-    TestCaseRunner::new(SkipPreprovenCommitmentsTest)
+    TestCaseRunner::new(SkipPreprovenCommitmentsTest::default())
         .run()
         .await
 }

bin/citrea/tests/bitcoin_e2e/tests/sequencer_commitments.rs

@@ -35,7 +35,7 @@ impl TestCase for LedgerGetCommitmentsProverTest {
         }
     }
 
-    async fn run_test(&self, f: &TestFramework) -> Result<()> {
+    async fn run_test(&mut self, f: &TestFramework) -> Result<()> {
         let sequencer = f.sequencer.as_ref().unwrap();
         let da = f.bitcoin_nodes.get(0).expect("DA not running.");
         let prover = f.prover.as_ref().unwrap();
@@ -113,7 +113,7 @@ impl TestCase for LedgerGetCommitmentsTest {
         }
     }
 
-    async fn run_test(&self, f: &TestFramework) -> Result<()> {
+    async fn run_test(&mut self, f: &TestFramework) -> Result<()> {
         let sequencer = f.sequencer.as_ref().unwrap();
         let da = f.bitcoin_nodes.get(0).expect("DA not running.");
         let full_node = f.full_node.as_ref().unwrap();
@@ -181,7 +181,7 @@ impl TestCase for SequencerSendCommitmentsToDaTest {
         }
     }
 
-    async fn run_test(&self, f: &TestFramework) -> Result<()> {
+    async fn run_test(&mut self, f: &TestFramework) -> Result<()> {
         let sequencer = f.sequencer.as_ref().unwrap();
         let da = f.bitcoin_nodes.get(0).expect("DA not running.");
 

bin/citrea/tests/bitcoin_e2e/tests/sequencer_test.rs

@@ -11,7 +11,7 @@ struct BasicSequencerTest;
 
 #[async_trait]
 impl TestCase for BasicSequencerTest {
-    async fn run_test(&self, f: &TestFramework) -> Result<()> {
+    async fn run_test(&mut self, f: &TestFramework) -> Result<()> {
         let Some(sequencer) = &f.sequencer else {
             anyhow::bail!("Sequencer not running. Set TestCaseConfig with_sequencer to true")
         };