refactor(ethexe-consensus): fix some bad places in batch aggregation logic

ethexe/consensus/src/validator/batch/filler.rs

@@ -16,16 +16,23 @@
 // You should have received a copy of the GNU General Public License
 // along with this program. If not, see <https://www.gnu.org/licenses/>.
 
-use super::types::{BatchLimits, BatchParts, BatchSizeCounter, ValidationRejectReason};
-
+use super::types::{BatchParts, BatchSizeCounter};
 use ethexe_common::gear::{
     ChainCommitment, CodeCommitment, RewardsCommitment, ValidatorsCommitment,
 };
 
+#[derive(Debug, derive_more::Display, Clone, Copy, PartialEq, Eq)]
+pub enum BatchIncludeError {
+    #[display("batch size limit exceeded")]
+    SizeLimitExceeded,
+}
+
+type FillerResult = Result<(), BatchIncludeError>;
+
 // TODO #5356: squash transitions before charging size so repeated actors are
 // counted against the actual committed payload rather than the pre-squash input.
 /// Stateful helper used by [`BatchCommitmentManager`](super::manager::BatchCommitmentManager)
-/// to assemble a candidate batch commitment under protocol size and deepness limits.
+/// to assemble a candidate batch commitment under protocol size limits.
 ///
 /// The manager decides which commitments are eligible, while `BatchFiller`
 /// tracks the accumulated parts and rejects additions that would exceed the
@@ -34,69 +41,39 @@ use ethexe_common::gear::{
 pub struct BatchFiller {
     /// Parts accumulated for the candidate batch being assembled.
     parts: BatchParts,
-    /// Protocol limits that decide whether candidate parts may be included.
-    limits: BatchLimits,
     /// Running payload budget for the ABI-encoded batch commitment.
     size_counter: BatchSizeCounter,
 }
 
-#[derive(Debug, derive_more::Display, Clone, Copy, PartialEq, Eq)]
-pub enum BatchIncludeError {
-    #[display("batch size limit exceeded")]
-    SizeLimitExceeded,
-}
-
-impl From<BatchIncludeError> for ValidationRejectReason {
-    fn from(value: BatchIncludeError) -> Self {
-        match value {
-            BatchIncludeError::SizeLimitExceeded => Self::BatchSizeLimitExceeded,
-        }
-    }
-}
-
-type FillerResult = Result<(), BatchIncludeError>;
-
 impl BatchFiller {
-    pub fn new(limits: BatchLimits) -> Self {
+    pub fn new(batch_size_limit: u64) -> Self {
         Self {
             parts: BatchParts::default(),
-            size_counter: BatchSizeCounter::new(limits.batch_size_limit),
-            limits,
-        }
-    }
-
-    pub fn into_parts(mut self) -> BatchParts {
-        if let Some(chain) = &mut self.parts.chain_commitment {
-            chain.transitions =
-                super::utils::squash_transitions_by_actor(std::mem::take(&mut chain.transitions));
-            super::utils::sort_transitions_by_value_to_receive(&mut chain.transitions);
+            size_counter: BatchSizeCounter::new(batch_size_limit),
         }
-        self.parts
     }
 
     pub fn include_validators_commitment(
         &mut self,
         commitment: ValidatorsCommitment,
     ) -> FillerResult {
-        let commitment = Some(commitment);
         if !self
             .size_counter
             .charge_for_validators_commitment(&commitment)
         {
             return Err(BatchIncludeError::SizeLimitExceeded);
         }
 
-        self.parts.validators_commitment = commitment;
+        self.parts.validators_commitment = Some(commitment);
         Ok(())
     }
 
     pub fn include_rewards_commitment(&mut self, commitment: RewardsCommitment) -> FillerResult {
-        let commitment = Some(commitment);
         if !self.size_counter.charge_for_rewards_commitment(&commitment) {
             return Err(BatchIncludeError::SizeLimitExceeded);
         }
 
-        self.parts.rewards_commitment = commitment;
+        self.parts.rewards_commitment = Some(commitment);
         Ok(())
     }
 
@@ -109,11 +86,7 @@ impl BatchFiller {
         Ok(())
     }
 
-    pub fn include_chain_commitment(
-        &mut self,
-        commitment: ChainCommitment,
-        deepness: u32,
-    ) -> FillerResult {
+    pub fn include_chain_commitment(&mut self, commitment: ChainCommitment) -> FillerResult {
         match self.parts.chain_commitment.as_mut() {
             Some(chain_commitment) => {
                 // Once the chain header is present, only appended transitions consume extra space.
@@ -123,27 +96,82 @@ impl BatchFiller {
                 {
                     return Err(BatchIncludeError::SizeLimitExceeded);
                 }
+
                 chain_commitment.head_announce = commitment.head_announce;
                 chain_commitment.transitions.extend(commitment.transitions);
             }
             None => {
-                // NOTE: Empty transition chains are skipped until they become old enough to force inclusion.
-                if !self.should_include_chain_commitment(&commitment, deepness) {
-                    return Ok(());
-                }
-
-                let commitment = Some(commitment);
                 if !self.size_counter.charge_for_chain_commitment(&commitment) {
                     return Err(BatchIncludeError::SizeLimitExceeded);
                 }
-                self.parts.chain_commitment = commitment;
+
+                self.parts.chain_commitment = Some(commitment);
             }
         }
+
         Ok(())
     }
 
-    fn should_include_chain_commitment(&self, commitment: &ChainCommitment, deepness: u32) -> bool {
-        // A deep enough chain must eventually be committed even if it carries no transitions.
-        !commitment.transitions.is_empty() || deepness + 1 > self.limits.chain_deepness_threshold
+    pub fn into_parts(self) -> BatchParts {
+        self.parts
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    use crate::{
+        mock::{test_chain_commitment, test_code_commitment, test_state_transition},
+        validator::batch::BatchManagerConfig,
+    };
+    use alloy::sol_types::SolValue;
+    use ethexe_common::{Announce, HashOf};
+    use ethexe_ethereum::abi::Gear;
+
+    #[test]
+    fn size_limit_rejects_once_budget_exhausted() {
+        let first = test_code_commitment(1);
+        let one_encoded: Gear::CodeCommitment = first.clone().into();
+        // Budget fits exactly one commitment, so the second include must be rejected.
+        let mut filler = BatchFiller::new(one_encoded.abi_encoded_size() as u64);
+
+        filler.include_code_commitment(first.clone()).unwrap();
+        assert_eq!(
+            filler.include_code_commitment(test_code_commitment(2)),
+            Err(BatchIncludeError::SizeLimitExceeded),
+        );
+
+        let parts = filler.into_parts();
+        assert_eq!(
+            parts.code_commitments,
+            vec![first],
+            "rejected commitment must not leak into parts",
+        );
+    }
+
+    #[test]
+    fn include_chain_commitment_merges_transitions() {
+        let mut filler = BatchFiller::new(BatchManagerConfig::default().batch_size_limit);
+
+        let head_1 = HashOf::<Announce>::random();
+        let head_2 = HashOf::<Announce>::random();
+
+        let first = test_chain_commitment(head_1, 1);
+        filler.include_chain_commitment(first.clone()).unwrap();
+
+        let second = ChainCommitment {
+            head_announce: head_2,
+            transitions: vec![test_state_transition(999)],
+        };
+        filler.include_chain_commitment(second.clone()).unwrap();
+
+        let chain = filler.into_parts().chain_commitment.unwrap();
+        assert_eq!(
+            chain.head_announce, head_2,
+            "second include must advance head_announce to the new tip",
+        );
+        let mut expected_transitions = first.transitions;
+        expected_transitions.extend(second.transitions);
+        assert_eq!(chain.transitions, expected_transitions);
     }
 }