Add changes from PR #976

BSA: Allocate encrypted txs first

Update PrepareProposal logic

Fix unit tests in PrepareProposal

Update ProcessProposal logic

apps/src/lib/node/ledger/shell/block_space_alloc.rs

@@ -16,14 +16,12 @@
 //! In the current implementation, we allocate space for transactions
 //! in the following order of preference:
 //!
-//! - First, we allot space for DKG decrypted txs. Decrypted txs take up as much
+//! - First, we allot space for DKG encrypted txs. We allow DKG encrypted txs to
+//!   take up at most 1/3 of the total block space.
+//! - Next, we allot space for DKG decrypted txs. Decrypted txs take up as much
 //!   space as needed. We will see, shortly, why in practice this is fine.
-//! - Next, we allot space for protocol txs. Protocol txs get half of the
+//! - Finally, we allot space for protocol txs. Protocol txs get half of the
 //!   remaining block space allotted to them.
-//! - Finally, we allot space for DKG encrypted txs. We allow DKG encrypted txs
-//!   to take up at most 1/3 of the total block space.
-//! - If any space remains, we try to fit any leftover protocol txs in the
-//!   block.
 //!
 //! Since at some fixed height `H` decrypted txs only take up as
 //! much space as the encrypted txs from height `H - 1`, and we
@@ -56,7 +54,7 @@ pub mod states;
 use std::marker::PhantomData;
 
 use namada::core::ledger::storage::{self, WlStorage};
-use namada::ledger::pos::PosQueries;
+use namada::proof_of_stake::pos_queries::PosQueries;
 
 #[allow(unused_imports)]
 use crate::facade::tendermint_proto::abci::RequestPrepareProposal;
@@ -80,9 +78,9 @@ pub enum AllocFailure {
 ///
 /// We keep track of the current space utilized by:
 ///
-///   - Protocol transactions.
-///   - DKG decrypted transactions.
 ///   - DKG encrypted transactions.
+///   - DKG decrypted transactions.
+///   - Protocol transactions.
 #[derive(Debug, Default)]
 pub struct BlockSpaceAllocator<State> {
     /// The current state of the [`BlockSpaceAllocator`] state machine.
@@ -98,19 +96,19 @@ pub struct BlockSpaceAllocator<State> {
     decrypted_txs: TxBin,
 }
 
-impl<D, H> From<&WlStorage<D, H>>
-    for BlockSpaceAllocator<states::BuildingDecryptedTxBatch>
+impl<D, H, M> From<&WlStorage<D, H>>
+    for BlockSpaceAllocator<states::BuildingEncryptedTxBatch<M>>
 where
     D: storage::DB + for<'iter> storage::DBIter<'iter>,
     H: storage::StorageHasher,
 {
     #[inline]
-    fn from(wl_storage: &WlStorage<D, H>) -> Self {
-        Self::init(wl_storage.pos_queries().get_max_proposal_bytes().get())
+    fn from(storage: &WlStorage<D, H>) -> Self {
+        Self::init(storage.pos_queries().get_max_proposal_bytes().get())
     }
 }
 
-impl BlockSpaceAllocator<states::BuildingDecryptedTxBatch> {
+impl<M> BlockSpaceAllocator<states::BuildingEncryptedTxBatch<M>> {
     /// Construct a new [`BlockSpaceAllocator`], with an upper bound
     /// on the max size of all txs in a block defined by Tendermint.
     #[inline]
@@ -120,11 +118,8 @@ impl BlockSpaceAllocator<states::BuildingDecryptedTxBatch> {
             _state: PhantomData,
             block: TxBin::init(max),
             protocol_txs: TxBin::default(),
-            encrypted_txs: TxBin::default(),
-            // decrypted txs can use as much space as needed; in practice,
-            // we'll only need, at most, the amount of space reserved for
-            // encrypted txs at the prev block height
-            decrypted_txs: TxBin::init(max),
+            encrypted_txs: TxBin::init_over_ratio(max, threshold::ONE_THIRD),
+            decrypted_txs: TxBin::default(),
         }
     }
 }
@@ -143,21 +138,6 @@ impl<State> BlockSpaceAllocator<State> {
             + self.decrypted_txs.allotted_space_in_bytes;
         self.block.allotted_space_in_bytes - total_bin_space
     }
-
-    /// Claim all the space used by the [`TxBin`] instances
-    /// as block space.
-    #[inline]
-    fn claim_block_space(&mut self) {
-        let used_space = self.protocol_txs.occupied_space_in_bytes
-            + self.encrypted_txs.occupied_space_in_bytes
-            + self.decrypted_txs.occupied_space_in_bytes;
-
-        self.block.occupied_space_in_bytes = used_space;
-
-        self.decrypted_txs = TxBin::default();
-        self.protocol_txs = TxBin::default();
-        self.encrypted_txs = TxBin::default();
-    }
 }
 
 /// Allotted space for a batch of transactions of the same kind in some
@@ -250,9 +230,6 @@ pub mod threshold {
 
     /// Divide free space in three.
     pub const ONE_THIRD: Threshold = Threshold::new(1, 3);
-
-    /// Divide free space in two.
-    pub const ONE_HALF: Threshold = Threshold::new(1, 2);
 }
 
 #[cfg(test)]
@@ -263,12 +240,22 @@ mod tests {
     use proptest::prelude::*;
 
     use super::states::{
-        NextState, NextStateWithEncryptedTxs, NextStateWithoutEncryptedTxs,
-        TryAlloc,
+        BuildingEncryptedTxBatch, NextState, TryAlloc, WithEncryptedTxs,
+        WithoutEncryptedTxs,
     };
     use super::*;
     use crate::node::ledger::shims::abcipp_shim_types::shim::TxBytes;
 
+    /// Convenience alias for a block space allocator at a state with encrypted
+    /// txs.
+    type BsaWrapperTxs =
+        BlockSpaceAllocator<BuildingEncryptedTxBatch<WithEncryptedTxs>>;
+
+    /// Convenience alias for a block space allocator at a state without
+    /// encrypted txs.
+    type BsaNoWrapperTxs =
+        BlockSpaceAllocator<BuildingEncryptedTxBatch<WithoutEncryptedTxs>>;
+
     /// Proptest generated txs.
     #[derive(Debug)]
     struct PropTx {
@@ -285,57 +272,41 @@ mod tests {
     fn test_txs_are_evenly_split_across_block() {
         const BLOCK_SIZE: u64 = 60;
 
-        // reserve block space for decrypted txs
-        let mut alloc = BlockSpaceAllocator::init(BLOCK_SIZE);
+        // reserve block space for encrypted txs
+        let mut alloc = BsaWrapperTxs::init(BLOCK_SIZE);
 
-        // assume we got ~1/3 encrypted txs at the prev block
+        // allocate ~1/3 of the block space to encrypted txs
         assert!(alloc.try_alloc(&[0; 18]).is_ok());
 
-        // reserve block space for protocol txs
+        // reserve block space for decrypted txs
         let mut alloc = alloc.next_state();
 
-        // the space we allotted to decrypted txs was shrunk to
+        // the space we allotted to encrypted txs was shrunk to
         // the total space we actually used up
-        assert_eq!(alloc.decrypted_txs.allotted_space_in_bytes, 18);
+        assert_eq!(alloc.encrypted_txs.allotted_space_in_bytes, 18);
 
-        // check that the allotted space for protocol txs is correct
-        assert_eq!(21, (BLOCK_SIZE - 18) / 2);
-        assert_eq!(alloc.protocol_txs.allotted_space_in_bytes, 21);
+        // check that the allotted space for decrypted txs is correct
+        assert_eq!(
+            alloc.decrypted_txs.allotted_space_in_bytes,
+            BLOCK_SIZE - 18
+        );
 
-        // fill up the block space with protocol txs
+        // add about ~1/3 worth of decrypted txs
         assert!(alloc.try_alloc(&[0; 17]).is_ok());
-        assert_matches!(
-            alloc.try_alloc(&[0; (21 - 17) + 1]),
-            Err(AllocFailure::Rejected { .. })
-        );
 
-        // reserve block space for encrypted txs
-        let mut alloc = alloc.next_state_with_encrypted_txs();
+        // reserve block space for protocol txs
+        let mut alloc = alloc.next_state();
 
         // check that space was shrunk
-        assert_eq!(alloc.protocol_txs.allotted_space_in_bytes, 17);
-
-        // check that we reserve at most 1/3 of the block space to
-        // encrypted txs
-        assert_eq!(25, BLOCK_SIZE - 17 - 18);
-        assert_eq!(20, BLOCK_SIZE / 3);
-        assert_eq!(alloc.encrypted_txs.allotted_space_in_bytes, 20);
-
-        // fill up the block space with encrypted txs
-        assert!(alloc.try_alloc(&[0; 20]).is_ok());
-        assert_matches!(
-            alloc.try_alloc(&[0; 1]),
-            Err(AllocFailure::Rejected { .. })
+        assert_eq!(
+            alloc.protocol_txs.allotted_space_in_bytes,
+            BLOCK_SIZE - (18 + 17)
         );
 
-        // check that there is still remaining space left at the end
-        let mut alloc = alloc.next_state();
-        let remaining_space = alloc.block.allotted_space_in_bytes
-            - alloc.block.occupied_space_in_bytes;
-        assert_eq!(remaining_space, 5);
+        // add protocol txs to the block space allocator
+        assert!(alloc.try_alloc(&[0; 25]).is_ok());
 
-        // fill up the remaining space
-        assert!(alloc.try_alloc(&[0; 5]).is_ok());
+        // the block should be full at this point
         assert_matches!(
             alloc.try_alloc(&[0; 1]),
             Err(AllocFailure::Rejected { .. })
@@ -346,9 +317,7 @@ mod tests {
     // when the state invariants banish them from inclusion.
     #[test]
     fn test_encrypted_txs_are_rejected() {
-        let alloc = BlockSpaceAllocator::init(1234);
-        let alloc = alloc.next_state();
-        let mut alloc = alloc.next_state_without_encrypted_txs();
+        let mut alloc = BsaNoWrapperTxs::init(1234);
         assert_matches!(
             alloc.try_alloc(&[0; 1]),
             Err(AllocFailure::Rejected { .. })
@@ -363,12 +332,11 @@ mod tests {
             proptest_reject_tx_on_bin_cap_reached(max)
         }
 
-        /// Check if the sum of all individual bin allotments for a
-        /// [`BlockSpaceAllocator`] corresponds to the total space ceded
-        /// by Tendermint.
+        /// Check if the initial bin capcity of the [`BlockSpaceAllocator`]
+        /// is correct.
         #[test]
-        fn test_bin_capacity_eq_provided_space(max in prop::num::u64::ANY) {
-            proptest_bin_capacity_eq_provided_space(max)
+        fn test_initial_bin_capacity(max in prop::num::u64::ANY) {
+            proptest_initial_bin_capacity(max)
         }
 
         /// Test that dumping txs whose total combined size
@@ -383,27 +351,25 @@ mod tests {
     fn proptest_reject_tx_on_bin_cap_reached(
         tendermint_max_block_space_in_bytes: u64,
     ) {
-        let mut bins =
-            BlockSpaceAllocator::init(tendermint_max_block_space_in_bytes);
+        let mut bins = BsaWrapperTxs::init(tendermint_max_block_space_in_bytes);
 
-        // fill the entire bin of decrypted txs
-        bins.decrypted_txs.occupied_space_in_bytes =
-            bins.decrypted_txs.allotted_space_in_bytes;
+        // fill the entire bin of encrypted txs
+        bins.encrypted_txs.occupied_space_in_bytes =
+            bins.encrypted_txs.allotted_space_in_bytes;
 
-        // make sure we can't dump any new decrypted txs in the bin
+        // make sure we can't dump any new encrypted txs in the bin
         assert_matches!(
             bins.try_alloc(b"arbitrary tx bytes"),
             Err(AllocFailure::Rejected { .. })
         );
     }
 
-    /// Implementation of [`test_bin_capacity_eq_provided_space`].
-    fn proptest_bin_capacity_eq_provided_space(
-        tendermint_max_block_space_in_bytes: u64,
-    ) {
-        let bins =
-            BlockSpaceAllocator::init(tendermint_max_block_space_in_bytes);
-        assert_eq!(0, bins.uninitialized_space_in_bytes());
+    /// Implementation of [`test_initial_bin_capacity`].
+    fn proptest_initial_bin_capacity(tendermint_max_block_space_in_bytes: u64) {
+        let bins = BsaWrapperTxs::init(tendermint_max_block_space_in_bytes);
+        let expected = tendermint_max_block_space_in_bytes
+            - threshold::ONE_THIRD.over(tendermint_max_block_space_in_bytes);
+        assert_eq!(expected, bins.uninitialized_space_in_bytes());
     }
 
     /// Implementation of [`test_tx_dump_doesnt_fill_up_bin`].
@@ -421,36 +387,35 @@ mod tests {
         // iterate over the produced txs to make sure we can keep
         // dumping new txs without filling up the bins
 
-        let bins = RefCell::new(BlockSpaceAllocator::init(
+        let bins = RefCell::new(BsaWrapperTxs::init(
             tendermint_max_block_space_in_bytes,
         ));
-        let decrypted_txs = decrypted_txs.into_iter().take_while(|tx| {
-            let bin = bins.borrow().decrypted_txs;
+        let encrypted_txs = encrypted_txs.into_iter().take_while(|tx| {
+            let bin = bins.borrow().encrypted_txs;
             let new_size = bin.occupied_space_in_bytes + tx.len() as u64;
             new_size < bin.allotted_space_in_bytes
         });
-        for tx in decrypted_txs {
+        for tx in encrypted_txs {
             assert!(bins.borrow_mut().try_alloc(&tx).is_ok());
         }
 
         let bins = RefCell::new(bins.into_inner().next_state());
-        let protocol_txs = protocol_txs.into_iter().take_while(|tx| {
-            let bin = bins.borrow().protocol_txs;
+        let decrypted_txs = decrypted_txs.into_iter().take_while(|tx| {
+            let bin = bins.borrow().decrypted_txs;
             let new_size = bin.occupied_space_in_bytes + tx.len() as u64;
             new_size < bin.allotted_space_in_bytes
         });
-        for tx in protocol_txs {
+        for tx in decrypted_txs {
             assert!(bins.borrow_mut().try_alloc(&tx).is_ok());
         }
 
-        let bins =
-            RefCell::new(bins.into_inner().next_state_with_encrypted_txs());
-        let encrypted_txs = encrypted_txs.into_iter().take_while(|tx| {
-            let bin = bins.borrow().encrypted_txs;
+        let bins = RefCell::new(bins.into_inner().next_state());
+        let protocol_txs = protocol_txs.into_iter().take_while(|tx| {
+            let bin = bins.borrow().protocol_txs;
             let new_size = bin.occupied_space_in_bytes + tx.len() as u64;
             new_size < bin.allotted_space_in_bytes
         });
-        for tx in encrypted_txs {
+        for tx in protocol_txs {
             assert!(bins.borrow_mut().try_alloc(&tx).is_ok());
         }
     }