Merge pull request #4272 from Fraser999/4268-fix-storage

Fix issues in storage

Makefile

@@ -134,7 +134,7 @@ lint-smart-contracts:
 
 .PHONY: audit-rs
 audit-rs:
-	$(CARGO) audit
+	$(CARGO) audit --ignore RUSTSEC-2022-0093
 
 .PHONY: audit-as
 audit-as:

node/CHANGELOG.md

@@ -11,6 +11,14 @@ All notable changes to this project will be documented in this file.  The format
 
 
 
+## 1.5.2-alt
+
+### Fixed
+* Fix issue in `chain_get_block_transfers` JSON-RPC where blocks with no deploys could be reported as having `null` transfers rather than `[]`.
+* Fix issue in `chain_get_block_transfers` JSON-RPC where blocks containing successful transfers could erroneously be reported as having none.
+
+
+
 ## 1.5.2
 
 ### Added

node/src/components/storage.rs

@@ -42,7 +42,7 @@ mod tests;
 use std::collections::BTreeSet;
 use std::{
     borrow::Cow,
-    collections::{btree_map::Entry, BTreeMap, HashMap, HashSet},
+    collections::{btree_map, hash_map, BTreeMap, HashMap, HashSet},
     convert::{TryFrom, TryInto},
     fmt::{self, Display, Formatter},
     fs::{self, OpenOptions},
@@ -71,7 +71,7 @@ use tracing::{debug, error, info, trace, warn};
 use casper_hashing::Digest;
 use casper_types::{
     bytesrepr::{FromBytes, ToBytes},
-    EraId, ExecutionResult, ProtocolVersion, PublicKey, TimeDiff, Timestamp, Transfer, Transform,
+    EraId, ExecutionResult, ProtocolVersion, PublicKey, TimeDiff, Timestamp, Transfer,
 };
 
 use crate::{
@@ -853,10 +853,8 @@ impl Storage {
                 block_hash,
                 responder,
             } => {
-                let mut txn = self.env.begin_ro_txn()?;
-                responder
-                    .respond(self.get_transfers(&mut txn, &block_hash)?)
-                    .ignore()
+                let maybe_transfers = self.get_transfers(&block_hash)?;
+                responder.respond(maybe_transfers).ignore()
             }
             StorageRequest::PutDeploy { deploy, responder } => {
                 responder.respond(self.put_deploy(&deploy)?).ignore()
@@ -1466,37 +1464,25 @@ impl Storage {
     ) -> Result<bool, FatalStorageError> {
         let mut transfers: Vec<Transfer> = vec![];
         for (deploy_hash, execution_result) in execution_results {
+            transfers.extend(execution_result.successful_transfers());
+
             let mut metadata = self
                 .get_deploy_metadata(txn, &deploy_hash)?
                 .unwrap_or_default();
 
             // If we have a previous execution result, we can continue if it is the same.
-            if let Some(prev) = metadata.execution_results.get(block_hash) {
-                if prev == &execution_result {
-                    continue;
-                } else {
-                    debug!(%deploy_hash, %block_hash, "different execution result");
-                }
-            }
-
-            if let ExecutionResult::Success { effect, .. } = execution_result.clone() {
-                for transform_entry in effect.transforms {
-                    if let Transform::WriteTransfer(transfer) = transform_entry.transform {
-                        transfers.push(transfer);
+            match metadata.execution_results.entry(*block_hash) {
+                hash_map::Entry::Occupied(entry) => {
+                    if *entry.get() == execution_result {
+                        continue;
                     }
+                    *entry.into_mut() = execution_result;
+                }
+                hash_map::Entry::Vacant(vacant) => {
+                    vacant.insert(execution_result);
                 }
             }
 
-            // TODO: this is currently done like this because rpc get_deploy returns the
-            // data, but the organization of deploy, block_hash, and
-            // execution_result is incorrectly represented. it should be
-            // inverted; for a given block_hash 0n deploys and each deploy has exactly 1
-            // result (aka deploy_metadata in this context).
-
-            // Update metadata and write back to db.
-            metadata
-                .execution_results
-                .insert(*block_hash, execution_result);
             let was_written =
                 txn.put_value(self.deploy_metadata_db, &deploy_hash, &metadata, true)?;
             if !was_written {
@@ -2175,16 +2161,40 @@ impl Storage {
         Ok(txn.get_value(self.deploy_metadata_db, deploy_hash)?)
     }
 
-    /// Retrieves transfers associated with block.
+    /// Retrieves successful transfers associated with block.
     ///
-    /// If no transfers are stored for the block, an empty transfers instance will be
-    /// created, but not stored.
-    fn get_transfers<Tx: Transaction>(
+    /// If there is no record of successful transfers for this block, then the list will be built
+    /// from the execution results and stored to `transfer_db`.  The record could have been missing
+    /// or incorrectly set to an empty collection due to previous synchronization and storage
+    /// issues.  See https://github.com/casper-network/casper-node/issues/4255 and
+    /// https://github.com/casper-network/casper-node/issues/4268 for further info.
+    fn get_transfers(
         &self,
-        txn: &mut Tx,
         block_hash: &BlockHash,
     ) -> Result<Option<Vec<Transfer>>, FatalStorageError> {
-        Ok(txn.get_value(self.transfer_db, block_hash)?)
+        let mut txn = self.env.begin_rw_txn()?;
+        if let Some(transfers) = txn.get_value::<_, Vec<Transfer>>(self.transfer_db, block_hash)? {
+            if !transfers.is_empty() {
+                return Ok(Some(transfers));
+            }
+        }
+
+        let block = match self.get_single_block(&mut txn, block_hash)? {
+            Some(block) => block,
+            None => return Ok(None),
+        };
+
+        let mut transfers: Vec<Transfer> = vec![];
+        for deploy_hash in block.deploy_and_transfer_hashes() {
+            let metadata = self
+                .get_deploy_metadata(&mut txn, deploy_hash)?
+                .unwrap_or_default();
+
+            transfers.extend(metadata.successful_transfers(block_hash));
+        }
+        txn.put_value(self.transfer_db, block_hash, &transfers, true)?;
+        txn.commit()?;
+        Ok(Some(transfers))
     }
 
     /// Retrieves block signatures for a block with a given block hash.
@@ -2614,10 +2624,10 @@ fn insert_to_block_header_indices(
 
     if block_header.is_switch_block() {
         match switch_block_era_id_index.entry(block_header.era_id()) {
-            Entry::Vacant(entry) => {
+            btree_map::Entry::Vacant(entry) => {
                 let _ = entry.insert(block_hash);
             }
-            Entry::Occupied(entry) => {
+            btree_map::Entry::Occupied(entry) => {
                 if *entry.get() != block_hash {
                     return Err(FatalStorageError::DuplicateEraIdIndex {
                         era_id: block_header.era_id(),

node/src/components/storage/tests.rs

@@ -8,35 +8,35 @@ use std::{
     sync::Arc,
 };
 
+use lmdb::Transaction;
 use rand::{prelude::SliceRandom, Rng};
 use serde::{Deserialize, Serialize};
 use smallvec::smallvec;
 
 use casper_types::{
     generate_ed25519_keypair, system::auction::UnbondingPurse, testing::TestRng, AccessRights,
-    EraId, ExecutionResult, ProtocolVersion, PublicKey, SecretKey, TimeDiff, URef, U512,
+    EraId, ExecutionEffect, ExecutionResult, Key, ProtocolVersion, PublicKey, SecretKey, TimeDiff,
+    Transfer, Transform, TransformEntry, URef, U512,
 };
 
 use super::{
+    lmdb_ext::{deserialize_internal, serialize_internal, TransactionExt, WriteTransactionExt},
     move_storage_files_to_network_subdir, should_move_storage_files_to_network_subdir, Config,
-    Storage,
+    Storage, FORCE_RESYNC_FILE_NAME,
 };
 use crate::{
     components::fetcher::{FetchItem, FetchResponse},
     effect::{
         requests::{MarkBlockCompletedRequest, StorageRequest},
         Multiple,
     },
-    storage::{
-        lmdb_ext::{deserialize_internal, serialize_internal},
-        FORCE_RESYNC_FILE_NAME,
-    },
     testing::{ComponentHarness, UnitTestEvent},
     types::{
         sync_leap_validation_metadata::SyncLeapValidationMetaData, AvailableBlockRange, Block,
         BlockHash, BlockHashAndHeight, BlockHeader, BlockHeaderWithMetadata, BlockSignatures,
         Chainspec, ChainspecRawBytes, Deploy, DeployHash, DeployMetadata, DeployMetadataExt,
         DeployWithFinalizedApprovals, FinalitySignature, LegacyDeploy, SyncLeapIdentifier,
+        TestBlockBuilder,
     },
     utils::{Loadable, WithDir},
 };
@@ -1056,21 +1056,168 @@ fn store_execution_results_twice_for_same_block_deploy_pair() {
     put_execution_results(&mut harness, &mut storage, block_hash, exec_result_2);
 }
 
+fn prepare_exec_result_with_transfer(
+    rng: &mut TestRng,
+    deploy_hash: &DeployHash,
+) -> (ExecutionResult, Transfer) {
+    let transfer = Transfer::new(
+        (*deploy_hash).into(),
+        rng.gen(),
+        Some(rng.gen()),
+        rng.gen(),
+        rng.gen(),
+        rng.gen(),
+        rng.gen(),
+        Some(rng.gen()),
+    );
+    let transform = TransformEntry {
+        key: Key::DeployInfo((*deploy_hash).into()).to_formatted_string(),
+        transform: Transform::WriteTransfer(transfer),
+    };
+    let effect = ExecutionEffect::new(vec![transform]);
+    let exec_result = ExecutionResult::Success {
+        effect,
+        transfers: vec![],
+        cost: rng.gen(),
+    };
+    (exec_result, transfer)
+}
+
 #[test]
 fn store_identical_execution_results() {
     let mut harness = ComponentHarness::default();
     let mut storage = storage_fixture(&harness);
 
-    let block_hash = BlockHash::random(&mut harness.rng);
-    let deploy_hash = DeployHash::random(&mut harness.rng);
+    let deploy = Deploy::random_valid_native_transfer(&mut harness.rng);
+    let deploy_hash = *deploy.hash();
+    let block = Block::random_with_deploys(&mut harness.rng, Some(&deploy));
+    storage.write_block(&block).unwrap();
+    let block_hash = *block.hash();
 
-    let mut exec_result = HashMap::new();
-    exec_result.insert(deploy_hash, harness.rng.gen());
+    let (exec_result, transfer) = prepare_exec_result_with_transfer(&mut harness.rng, &deploy_hash);
+    let mut exec_results = HashMap::new();
+    exec_results.insert(deploy_hash, exec_result.clone());
 
-    put_execution_results(&mut harness, &mut storage, block_hash, exec_result.clone());
+    put_execution_results(&mut harness, &mut storage, block_hash, exec_results.clone());
+    {
+        let mut txn = storage.env.begin_ro_txn().unwrap();
+        let retrieved_results = storage
+            .get_execution_results(&mut txn, &block_hash)
+            .expect("should execute get")
+            .expect("should return Some");
+        assert_eq!(retrieved_results.len(), 1);
+        assert_eq!(retrieved_results[0].0, deploy_hash);
+        assert_eq!(retrieved_results[0].1, exec_result);
+    }
+    let retrieved_transfers = storage
+        .get_transfers(&block_hash)
+        .expect("should execute get")
+        .expect("should return Some");
+    assert_eq!(retrieved_transfers.len(), 1);
+    assert_eq!(retrieved_transfers[0], transfer);
 
     // We should be fine storing the exact same result twice.
-    put_execution_results(&mut harness, &mut storage, block_hash, exec_result);
+    put_execution_results(&mut harness, &mut storage, block_hash, exec_results);
+    {
+        let mut txn = storage.env.begin_ro_txn().unwrap();
+        let retrieved_results = storage
+            .get_execution_results(&mut txn, &block_hash)
+            .expect("should execute get")
+            .expect("should return Some");
+        assert_eq!(retrieved_results.len(), 1);
+        assert_eq!(retrieved_results[0].0, deploy_hash);
+        assert_eq!(retrieved_results[0].1, exec_result);
+    }
+    let retrieved_transfers = storage
+        .get_transfers(&block_hash)
+        .expect("should execute get")
+        .expect("should return Some");
+    assert_eq!(retrieved_transfers.len(), 1);
+    assert_eq!(retrieved_transfers[0], transfer);
+}
+
+/// This is a regression test for the issue where `Transfer`s under a block with no deploys could be
+/// returned as `None` rather than the expected `Some(vec![])`.  The fix should ensure that if no
+/// Transfers are found, storage will respond with an empty collection and store the correct value
+/// for future requests.
+///
+/// See https://github.com/casper-network/casper-node/issues/4255 for further info.
+#[test]
+fn should_provide_transfers_if_not_stored() {
+    let mut harness = ComponentHarness::default();
+    let mut storage = storage_fixture(&harness);
+
+    let block = TestBlockBuilder::new()
+        .deploys(None)
+        .build(&mut harness.rng);
+    assert_eq!(block.body().deploy_and_transfer_hashes().count(), 0);
+    storage.write_block(&block).unwrap();
+    let block_hash = *block.hash();
+
+    // Check an empty collection is returned.
+    let retrieved_transfers = storage
+        .get_transfers(&block_hash)
+        .expect("should execute get")
+        .expect("should return Some");
+    assert!(retrieved_transfers.is_empty());
+
+    // Check the empty collection has been stored.
+    let mut txn = storage.env.begin_ro_txn().unwrap();
+    let maybe_transfers = txn
+        .get_value::<_, Vec<Transfer>>(storage.transfer_db, &block_hash)
+        .unwrap();
+    assert_eq!(Some(vec![]), maybe_transfers);
+}
+
+/// This is a regression test for the issue where a valid collection of `Transfer`s under a given
+/// block could be erroneously replaced with an empty collection.  The fix should ensure that if an
+/// empty collection of Transfers is found, storage will replace it with the correct collection and
+/// store the correct value for future requests.
+///
+/// See https://github.com/casper-network/casper-node/issues/4268 for further info.
+#[test]
+fn should_provide_transfers_after_emptied() {
+    let mut harness = ComponentHarness::default();
+    let mut storage = storage_fixture(&harness);
+
+    let deploy = Deploy::random_valid_native_transfer(&mut harness.rng);
+    let deploy_hash = *deploy.hash();
+    let block = Block::random_with_deploys(&mut harness.rng, Some(&deploy));
+    storage.write_block(&block).unwrap();
+    let block_hash = *block.hash();
+
+    let (exec_result, transfer) = prepare_exec_result_with_transfer(&mut harness.rng, &deploy_hash);
+    let mut exec_results = HashMap::new();
+    exec_results.insert(deploy_hash, exec_result);
+
+    put_execution_results(&mut harness, &mut storage, block_hash, exec_results.clone());
+    // Replace the valid collection with an empty one.
+    {
+        let mut txn = storage.env.begin_rw_txn().unwrap();
+        txn.put_value(
+            storage.transfer_db,
+            &block_hash,
+            &Vec::<Transfer>::new(),
+            true,
+        )
+        .unwrap();
+        txn.commit().unwrap();
+    }
+
+    // Check the correct value is returned.
+    let retrieved_transfers = storage
+        .get_transfers(&block_hash)
+        .expect("should execute get")
+        .expect("should return Some");
+    assert_eq!(retrieved_transfers.len(), 1);
+    assert_eq!(retrieved_transfers[0], transfer);
+
+    // Check the correct value has been stored.
+    let mut txn = storage.env.begin_ro_txn().unwrap();
+    let maybe_transfers = txn
+        .get_value::<_, Vec<Transfer>>(storage.transfer_db, &block_hash)
+        .unwrap();
+    assert_eq!(Some(vec![transfer]), maybe_transfers);
 }
 
 /// Example state used in storage.