Merge remote-tracking branch 'namada/bat/hotfix/dont-persist-gensis-a…

…t-init-chain' (#1182) into maint-0.14

.changelog/unreleased/bug-fixes/1182-dont-persist-genesis-on-init-chain.md

@@ -0,0 +1,4 @@
+- Fixed the init-chain handler to stop committing state to the DB
+  as it may be re-applied when the node is shut-down before the
+  first block is committed, leading to an invalid genesis state.
+  ([#1182](https://github.com/anoma/namada/pull/1182))

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

@@ -402,7 +402,6 @@ where
 
         let new_epoch = self
             .wl_storage
-            .storage
             .update_epoch(height, header_time)
             .expect("Must be able to update epoch");
 
@@ -837,7 +836,7 @@ mod test_finalize_block {
             min_duration: DurationSecs(0),
         };
         namada::ledger::parameters::update_epoch_parameter(
-            &mut shell.wl_storage.storage,
+            &mut shell.wl_storage,
             &epoch_duration,
         )
         .unwrap();
@@ -878,7 +877,7 @@ mod test_finalize_block {
         add_proposal(1, ProposalVote::Nay);
 
         // Commit the genesis state
-        shell.wl_storage.commit_genesis().unwrap();
+        shell.wl_storage.commit_block().unwrap();
         shell.commit();
 
         // Collect all storage key-vals into a sorted map

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

@@ -25,6 +25,8 @@ where
     /// Create a new genesis for the chain with specified id. This includes
     /// 1. A set of initial users and tokens
     /// 2. Setting up the validity predicates for both users and tokens
+    ///
+    /// INVARIANT: This method must not commit the state changes to DB.
     pub fn init_chain(
         &mut self,
         init: request::InitChain,
@@ -138,12 +140,15 @@ where
             #[cfg(not(feature = "mainnet"))]
             wrapper_tx_fees,
         };
-        parameters.init_storage(&mut self.wl_storage.storage);
+        parameters
+            .init_storage(&mut self.wl_storage)
+            .expect("Initializing chain parameters must not fail");
 
         // Initialize governance parameters
         genesis
             .gov_params
-            .init_storage(&mut self.wl_storage.storage);
+            .init_storage(&mut self.wl_storage)
+            .expect("Initializing governance parameters must not fail");
 
         // Depends on parameters being initialized
         self.wl_storage
@@ -342,7 +347,7 @@ where
                 .map(|validator| validator.pos_data),
             current_epoch,
         );
-        ibc::init_genesis_storage(&mut self.wl_storage.storage);
+        ibc::init_genesis_storage(&mut self.wl_storage);
 
         // Set the initial validator set
         for validator in genesis.validators {
@@ -360,10 +365,6 @@ where
             response.validators.push(abci_validator);
         }
 
-        self.wl_storage
-            .commit_genesis()
-            .expect("Must be able to commit genesis state");
-
         Ok(response)
     }
 }
@@ -391,3 +392,59 @@ where
         }
     }
 }
+
+#[cfg(test)]
+mod test {
+    use std::collections::BTreeMap;
+    use std::str::FromStr;
+
+    use namada::ledger::storage::DBIter;
+    use namada::types::chain::ChainId;
+    use namada::types::storage;
+
+    use crate::facade::tendermint_proto::abci::RequestInitChain;
+    use crate::facade::tendermint_proto::google::protobuf::Timestamp;
+    use crate::node::ledger::shell::test_utils::TestShell;
+
+    /// Test that the init-chain handler never commits changes directly to the
+    /// DB.
+    #[test]
+    fn test_init_chain_doesnt_commit_db() {
+        let (mut shell, _receiver) = TestShell::new();
+
+        // Collect all storage key-vals into a sorted map
+        let store_block_state = |shell: &TestShell| -> BTreeMap<_, _> {
+            let prefix: storage::Key = FromStr::from_str("").unwrap();
+            shell
+                .wl_storage
+                .storage
+                .db
+                .iter_prefix(&prefix)
+                .map(|(key, val, _gas)| (key, val))
+                .collect()
+        };
+
+        // Store the full state in sorted map
+        let initial_storage_state: std::collections::BTreeMap<String, Vec<u8>> =
+            store_block_state(&shell);
+
+        shell.init_chain(RequestInitChain {
+            time: Some(Timestamp {
+                seconds: 0,
+                nanos: 0,
+            }),
+            chain_id: ChainId::default().to_string(),
+            ..Default::default()
+        });
+
+        // Store the full state again
+        let storage_state: std::collections::BTreeMap<String, Vec<u8>> =
+            store_block_state(&shell);
+
+        // The storage state must be unchanged
+        itertools::assert_equal(
+            initial_storage_state.iter(),
+            storage_state.iter(),
+        );
+    }
+}

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

@@ -709,9 +709,9 @@ where
         tx: &namada::types::transaction::WrapperTx,
     ) -> bool {
         if let Some(solution) = &tx.pow_solution {
-            if let (Some(faucet_address), _gas) =
+            if let Some(faucet_address) =
                 namada::ledger::parameters::read_faucet_account_parameter(
-                    &self.wl_storage.storage,
+                    &self.wl_storage,
                 )
                 .expect("Must be able to read faucet account parameter")
             {
@@ -727,11 +727,10 @@ where
     #[cfg(not(feature = "mainnet"))]
     /// Get fixed amount of fees for wrapper tx
     fn get_wrapper_tx_fees(&self) -> token::Amount {
-        let (fees, _gas) =
-            namada::ledger::parameters::read_wrapper_tx_fees_parameter(
-                &self.wl_storage.storage,
-            )
-            .expect("Must be able to read wrapper tx fees parameter");
+        let fees = namada::ledger::parameters::read_wrapper_tx_fees_parameter(
+            &self.wl_storage,
+        )
+        .expect("Must be able to read wrapper tx fees parameter");
         fees.unwrap_or(token::Amount::whole(MIN_FEE))
     }
 
@@ -743,9 +742,9 @@ where
         tx: &namada::types::transaction::WrapperTx,
     ) -> bool {
         if let Some(solution) = &tx.pow_solution {
-            if let (Some(faucet_address), _gas) =
+            if let Some(faucet_address) =
                 namada::ledger::parameters::read_faucet_account_parameter(
-                    &self.wl_storage.storage,
+                    &self.wl_storage,
                 )
                 .expect("Must be able to read faucet account parameter")
             {

apps/src/lib/node/ledger/storage/mod.rs

@@ -389,7 +389,7 @@ mod tests {
         itertools::assert_equal(iter, expected.clone());
 
         // Commit genesis state
-        storage.commit_genesis().unwrap();
+        storage.commit_block().unwrap();
 
         // Again, try to iterate over their prefix
         let iter = storage_api::iter_prefix(&storage, &prefix)
@@ -440,7 +440,7 @@ mod tests {
         itertools::assert_equal(iter, expected.clone());
 
         // Commit genesis state
-        storage.commit_genesis().unwrap();
+        storage.commit_block().unwrap();
 
         // And check again
         let iter = storage_api::iter_prefix(&storage, &prefix)

core/src/ledger/governance/parameters.rs

@@ -3,8 +3,7 @@ use std::fmt::Display;
 use borsh::{BorshDeserialize, BorshSerialize};
 
 use super::storage as gov_storage;
-use crate::ledger::storage::types::encode;
-use crate::ledger::storage::{self, Storage};
+use crate::ledger::storage_api::{self, StorageRead, StorageWrite};
 use crate::types::token::Amount;
 
 #[derive(
@@ -66,10 +65,9 @@ impl Default for GovParams {
 
 impl GovParams {
     /// Initialize governance parameters into storage
-    pub fn init_storage<DB, H>(&self, storage: &mut Storage<DB, H>)
+    pub fn init_storage<S>(&self, storage: &mut S) -> storage_api::Result<()>
     where
-        DB: storage::DB + for<'iter> storage::DBIter<'iter>,
-        H: storage::StorageHasher,
+        S: StorageRead + StorageWrite,
     {
         let Self {
             min_proposal_fund,
@@ -82,49 +80,31 @@ impl GovParams {
 
         let min_proposal_fund_key = gov_storage::get_min_proposal_fund_key();
         let amount = Amount::whole(*min_proposal_fund);
-        storage
-            .write(&min_proposal_fund_key, encode(&amount))
-            .unwrap();
+        storage.write(&min_proposal_fund_key, amount)?;
 
         let max_proposal_code_size_key =
             gov_storage::get_max_proposal_code_size_key();
-        storage
-            .write(&max_proposal_code_size_key, encode(max_proposal_code_size))
-            .unwrap();
+        storage.write(&max_proposal_code_size_key, max_proposal_code_size)?;
 
         let min_proposal_period_key =
             gov_storage::get_min_proposal_period_key();
-        storage
-            .write(&min_proposal_period_key, encode(min_proposal_period))
-            .unwrap();
+        storage.write(&min_proposal_period_key, min_proposal_period)?;
 
         let max_proposal_period_key =
             gov_storage::get_max_proposal_period_key();
-        storage
-            .write(&max_proposal_period_key, encode(max_proposal_period))
-            .unwrap();
+        storage.write(&max_proposal_period_key, max_proposal_period)?;
 
         let max_proposal_content_size_key =
             gov_storage::get_max_proposal_content_key();
         storage
-            .write(
-                &max_proposal_content_size_key,
-                encode(max_proposal_content_size),
-            )
-            .expect("Should be able to write to storage");
+            .write(&max_proposal_content_size_key, max_proposal_content_size)?;
 
         let min_proposal_grace_epoch_key =
             gov_storage::get_min_proposal_grace_epoch_key();
         storage
-            .write(
-                &min_proposal_grace_epoch_key,
-                encode(min_proposal_grace_epochs),
-            )
-            .expect("Should be able to write to storage");
+            .write(&min_proposal_grace_epoch_key, min_proposal_grace_epochs)?;
 
         let counter_key = gov_storage::get_counter_key();
-        storage
-            .write(&counter_key, encode(&u64::MIN))
-            .expect("Should be able to write to storage");
+        storage.write(&counter_key, u64::MIN)
     }
 }