fix: cleanups and more tests

prajwolrg · prajwolrg · commit 1400d32ab8e3 · 2025-09-29T21:57:41.000+05:45
diff --git a/crates/asm/subprotocols/bridge-v1/src/errors.rs b/crates/asm/subprotocols/bridge-v1/src/errors.rs
@@ -11,6 +11,24 @@ use strata_primitives::{
 };
 use thiserror::Error;
 
+#[derive(Debug, Error)]
+pub enum BridgeSubprotocolError {
+    #[error("failed to parse deposit tx")]
+    DepositTxParse(#[from] DepositTxParseError),
+
+    #[error("failed to process deposit tx")]
+    DepositTxProcess(#[from] DepositValidationError),
+
+    #[error("failed to parse withdrawal fulfillment tx")]
+    WithdrawalTxParse(#[from] WithdrawalParseError),
+
+    #[error("failed to parse withdrawal fulfillment tx")]
+    WithdrawalTxProcess(#[from] WithdrawalValidationError),
+
+    #[error("unsupported tx type {0}")]
+    UnsupportedTxType(TxType),
+}
+
 /// Errors that can occur when validating deposit transactions at the subprotocol level.
 ///
 /// These errors represent state-level validation failures that occur after successful
@@ -40,24 +58,6 @@ pub enum DepositValidationError {
     EmptyOperators,
 }
 
-#[derive(Debug, Error)]
-pub enum BridgeSubprotocolError {
-    #[error("failed to parse deposit tx")]
-    DepositTxParse(#[from] DepositTxParseError),
-
-    #[error("failed to process deposit tx")]
-    DepositTxProcess(#[from] DepositValidationError),
-
-    #[error("failed to parse withdrawal fulfillment tx")]
-    WithdrawalTxParse(#[from] WithdrawalParseError),
-
-    #[error("failed to parse withdrawal fulfillment tx")]
-    WithdrawalTxProcess(#[from] WithdrawalValidationError),
-
-    #[error("unsupported tx type {0}")]
-    UnsupportedTxType(TxType),
-}
-
 /// Errors that can occur when validating withdrawal fulfillment transactions.
 ///
 /// When these validation errors occur, they are logged and the transaction is skipped.
@@ -105,3 +105,23 @@ pub enum WithdrawalCommandError {
     #[error("Deposit amount mismatch {0}")]
     DepositWithdrawalAmountMismatch(Mismatch<u64>),
 }
+
+/// Error type for OperatorBitmap operations.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub enum BitmapError {
+    /// Attempted to set a bit at an index that would create a gap in the bitmap.
+    /// Only sequential indices are allowed.
+    IndexOutOfBounds(OperatorIdx),
+}
+
+impl std::fmt::Display for BitmapError {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        match self {
+            BitmapError::IndexOutOfBounds(idx) => {
+                write!(f, "Index {} is out of bounds for sequential bitmap", idx)
+            }
+        }
+    }
+}
+
+impl std::error::Error for BitmapError {}
diff --git a/crates/asm/subprotocols/bridge-v1/src/state/assignment.rs b/crates/asm/subprotocols/bridge-v1/src/state/assignment.rs
@@ -142,11 +142,13 @@ impl AssignmentEntry {
         let seed_bytes: [u8; 32] = Buf32::from(seed).into();
         let mut rng = ChaChaRng::from_seed(seed_bytes);
 
-        let empty_bitmap = OperatorBitmap::new_empty(); // No previous assignees at creation
+        // No previous assignees at creation
+        let previous_assignees =
+            OperatorBitmap::new_with_size(deposit_entry.notary_operators_bitmap().len(), false);
 
         let eligible_operators = filter_eligible_operators(
-            deposit_entry.notary_operators(),
-            &empty_bitmap,
+            deposit_entry.notary_operators_bitmap(),
+            &previous_assignees,
             current_active_operators,
         );
 
@@ -162,10 +164,10 @@ impl AssignmentEntry {
         let current_assignee = eligible_indices[random_index];
 
         Ok(Self {
-            deposit_entry,
+            deposit_entry: deposit_entry.clone(),
             withdrawal_cmd,
             current_assignee,
-            previous_assignees: OperatorBitmap::new_empty(),
+            previous_assignees,
             exec_deadline,
         })
     }
@@ -230,16 +232,19 @@ impl AssignmentEntry {
 
         // Use the already cached bitmap from DepositEntry instead of converting from Vec
         let mut eligible_operators = filter_eligible_operators(
-            self.deposit_entry.notary_operators(),
+            self.deposit_entry.notary_operators_bitmap(),
             &self.previous_assignees,
             current_active_operators,
         );
 
         if eligible_operators.not_any() {
             // If no eligible operators left, clear previous assignees
-            self.previous_assignees = OperatorBitmap::new_empty();
+            self.previous_assignees = OperatorBitmap::new_with_size(
+                self.deposit_entry.notary_operators_bitmap().len(),
+                false,
+            );
             eligible_operators = filter_eligible_operators(
-                self.deposit_entry.notary_operators(),
+                self.deposit_entry.notary_operators_bitmap(),
                 &self.previous_assignees,
                 current_active_operators,
             );
@@ -388,7 +393,7 @@ impl AssignmentTable {
     ///
     /// ```rust,ignore
     /// let current_height = BitcoinBlockHeight::from(1000);
-    /// let active_operators = OperatorBitmap::new_sequential_active(3);
+    /// let active_operators = OperatorBitmap::new_with_size(3, true);
     /// let seed = L1BlockId::from([0u8; 32]);
     ///
     /// table.reassign_expired_assignments(current_height, &active_operators, seed)?;
@@ -432,7 +437,7 @@ mod tests {
         let seed: L1BlockId = arb.generate();
 
         // Use the deposit's notary operators as active operators
-        let current_active_operators = deposit_entry.notary_operators().clone();
+        let current_active_operators = deposit_entry.notary_operators_bitmap().clone();
 
         let result = AssignmentEntry::create_with_random_assignment(
             deposit_entry.clone(),
@@ -482,21 +487,24 @@ mod tests {
     #[test]
     fn test_reassign_success() {
         let mut arb = ArbitraryGenerator::new();
-        let deposit_entry: DepositEntry = arb.generate();
+
+        // Keep generating deposit entries until we have at least 2 active operators
+        let deposit_entry: DepositEntry = loop {
+            let candidate: DepositEntry = arb.generate();
+            if candidate.notary_operators_bitmap().active_count() >= 2 {
+                break candidate;
+            }
+        };
+
         let withdrawal_cmd: WithdrawalCommand = arb.generate();
         let exec_deadline: BitcoinBlockHeight = 100;
         let seed1: L1BlockId = arb.generate();
         let seed2: L1BlockId = arb.generate();
 
         // Use the deposit's notary operators as active operators
-        let current_active_operators = deposit_entry.notary_operators().clone();
+        let current_active_operators = deposit_entry.notary_operators_bitmap().clone();
         let new_fee = BitcoinAmount::from_sat(20_000);
 
-        // Ensure we have at least 2 operators for reassignment
-        if current_active_operators.active_count() < 2 {
-            return; // Skip test if not enough operators
-        }
-
         let mut assignment = AssignmentEntry::create_with_random_assignment(
             deposit_entry,
             withdrawal_cmd,
@@ -513,18 +521,10 @@ mod tests {
         let result = assignment.reassign(new_fee, seed2, &current_active_operators);
         assert!(result.is_ok());
 
-        // Verify reassignment - the behavior depends on how many operators are available
-        if assignment.previous_assignees().len() == 1 {
-            // Normal case: different operator selected and previous assignee tracked
-            assert_eq!(assignment.previous_assignees()[0], original_assignee);
-            assert_ne!(assignment.current_assignee(), original_assignee);
-        } else {
-            // Edge case: previous assignees were cleared during reassignment
-            // This happens when no eligible operators are found initially, forcing
-            // the reassignment logic to clear previous assignees and retry
-            assert_eq!(assignment.previous_assignees().len(), 0);
-        }
-        assert!(current_active_operators.is_active(assignment.current_assignee()));
+        // Verify reassignment
+        assert_eq!(assignment.previous_assignees().len(), 1);
+        assert_eq!(assignment.previous_assignees()[0], original_assignee);
+        assert_ne!(assignment.current_assignee(), original_assignee);
     }
 
     #[test]
@@ -533,7 +533,7 @@ mod tests {
         let mut deposit_entry: DepositEntry = arb.generate();
 
         // Force single operator for this test
-        let operators = OperatorBitmap::new_sequential_active(1);
+        let operators = OperatorBitmap::new_with_size(1, true);
         deposit_entry = DepositEntry::new(
             deposit_entry.idx(),
             *deposit_entry.output(),
@@ -548,7 +548,7 @@ mod tests {
         let seed1: L1BlockId = arb.generate();
         let seed2: L1BlockId = arb.generate();
 
-        let current_active_operators = OperatorBitmap::new_sequential_active(1); // Single operator with index 0
+        let current_active_operators = OperatorBitmap::new_with_size(1, true); // Single operator with index 0
 
         let mut assignment = AssignmentEntry::create_with_random_assignment(
             deposit_entry,
@@ -579,7 +579,7 @@ mod tests {
         let withdrawal_cmd: WithdrawalCommand = arb.generate();
         let exec_deadline: BitcoinBlockHeight = 100;
         let seed: L1BlockId = arb.generate();
-        let current_active_operators = deposit_entry.notary_operators().clone();
+        let current_active_operators = deposit_entry.notary_operators_bitmap().clone();
 
         let assignment = AssignmentEntry::create_with_random_assignment(
             deposit_entry.clone(),
@@ -623,7 +623,7 @@ mod tests {
         let deposit_entry1: DepositEntry = arb.generate();
         let withdrawal_cmd1: WithdrawalCommand = arb.generate();
         let expired_deadline: BitcoinBlockHeight = 100; // Less than current_height
-        let current_active_operators1 = deposit_entry1.notary_operators().clone();
+        let current_active_operators1 = deposit_entry1.notary_operators_bitmap().clone();
 
         let expired_assignment = AssignmentEntry::create_with_random_assignment(
             deposit_entry1.clone(),
@@ -642,7 +642,7 @@ mod tests {
         let deposit_entry2: DepositEntry = arb.generate();
         let withdrawal_cmd2: WithdrawalCommand = arb.generate();
         let future_deadline: BitcoinBlockHeight = 200; // Greater than current_height
-        let current_active_operators2 = deposit_entry2.notary_operators().clone();
+        let current_active_operators2 = deposit_entry2.notary_operators_bitmap().clone();
 
         let future_assignment = AssignmentEntry::create_with_random_assignment(
             deposit_entry2.clone(),
@@ -680,7 +680,7 @@ mod tests {
         let expired_assignment_after = table.get_assignment(expired_deposit_idx).unwrap();
 
         // The behavior depends on how many eligible operators are available
-        let deposit1_notary_count = deposit_entry1.notary_operators().active_count();
+        let deposit1_notary_count = deposit_entry1.notary_operators_bitmap().active_count();
         if deposit1_notary_count > 1
             && expired_assignment_after.current_assignee() != original_assignee
         {
diff --git a/crates/asm/subprotocols/bridge-v1/src/state/bridge.rs b/crates/asm/subprotocols/bridge-v1/src/state/bridge.rs
@@ -192,8 +192,8 @@ impl BridgeV1State {
     ) -> Result<(), DepositValidationError> {
         // Validate the deposit first
         self.validate_deposit(tx, info)?;
-        let operators = self.operators.current_multisig_bitmap().clone();
-        let entry = DepositEntry::new(info.deposit_idx, info.outpoint, operators, info.amt)?;
+        let notary_operators = self.operators.current_multisig_bitmap().clone();
+        let entry = DepositEntry::new(info.deposit_idx, info.outpoint, notary_operators, info.amt)?;
         self.deposits.insert_deposit(entry)?;
 
         Ok(())
diff --git a/crates/asm/subprotocols/bridge-v1/src/state/deposit.rs b/crates/asm/subprotocols/bridge-v1/src/state/deposit.rs
@@ -8,6 +8,7 @@
 use arbitrary::Arbitrary;
 use borsh::{BorshDeserialize, BorshSerialize};
 use strata_primitives::{
+    bridge::OperatorIdx,
     l1::{BitcoinAmount, OutputRef},
     sorted_vec::SortedVec,
 };
@@ -55,7 +56,7 @@ pub struct DepositEntry {
     /// Any one honest operator from this set can process user withdrawals.
     ///
     /// Uses a memory-efficient bitmap representation instead of storing operator indices.
-    operators: OperatorBitmap,
+    notary_operators: OperatorBitmap,
 
     /// Amount of Bitcoin locked in this deposit (in satoshis).
     amt: BitcoinAmount,
@@ -95,17 +96,17 @@ impl DepositEntry {
     pub fn new(
         idx: u32,
         output: OutputRef,
-        operators: OperatorBitmap,
+        notary_operators: OperatorBitmap,
         amt: BitcoinAmount,
     ) -> Result<Self, DepositValidationError> {
-        if operators.active_count() == 0 {
+        if notary_operators.active_count() == 0 {
             return Err(DepositValidationError::EmptyOperators);
         }
 
         Ok(Self {
             deposit_idx: idx,
             output,
-            operators,
+            notary_operators,
             amt,
         })
     }
@@ -125,8 +126,17 @@ impl DepositEntry {
     /// # Returns
     ///
     /// A reference to the operator bitmap that contains the operators active for this deposit.
-    pub fn notary_operators(&self) -> &OperatorBitmap {
-        &self.operators
+    pub fn notary_operators_bitmap(&self) -> &OperatorBitmap {
+        &self.notary_operators
+    }
+
+    /// Returns the indices of operators that formed the N/N multisig for this deposit.
+    ///
+    /// # Returns
+    ///
+    /// Vector containing [`OperatorIdx`] for operators that were active for this deposit.
+    pub fn notary_operators_indices(&self) -> Vec<OperatorIdx> {
+        self.notary_operators.to_indices()
     }
 
     /// Returns the amount of Bitcoin locked in this deposit.
@@ -143,17 +153,14 @@ impl<'a> Arbitrary<'a> for DepositEntry {
         // Generate a random Bitcoin UTXO reference
         let output: OutputRef = u.arbitrary()?;
 
-        // Generate a random number of notary operators between 1 and 20
-        let num_operators = u.int_in_range(1..=20)?;
-
         // Create OperatorBitmap directly by setting sequential operators as active
-        let operators = OperatorBitmap::new_sequential_active(num_operators);
+        let notary_operators = u.arbitrary()?;
 
         // Generate a random Bitcoin amount (between 1 satoshi and 21 million BTC)
         let amount: BitcoinAmount = u.arbitrary()?;
 
         // Create the DepositEntry - this should not fail since we ensure operators is non-empty
-        Self::new(deposit_idx, output, operators, amount)
+        Self::new(deposit_idx, output, notary_operators, amount)
             .map_err(|_| arbitrary::Error::IncorrectFormat)
     }
 }
diff --git a/crates/asm/subprotocols/bridge-v1/src/state/operator.rs b/crates/asm/subprotocols/bridge-v1/src/state/operator.rs
