[accumulator] Fix bug for get txn info from chain (#2689)

(cherry picked from commit 8021ba3)

chain/src/chain.rs

@@ -271,13 +271,6 @@ impl BlockChain {
             .ok_or_else(|| format_err!("Can not find block hash by number {}", number))
     }
 
-    fn check_exist_transaction_info(&self, txn_info_id: HashValue) -> bool {
-        if let Ok(node) = self.txn_accumulator.get_node(txn_info_id) {
-            return node.is_some();
-        }
-        false
-    }
-
     fn check_exist_block(&self, block_id: HashValue, block_number: BlockNumber) -> Result<bool> {
         Ok(self
             .get_hash_by_number(block_number)?
@@ -504,6 +497,14 @@ impl BlockChain {
         storage.save_block_info(block_info)?;
         Ok(())
     }
+
+    pub fn get_txn_accumulator(&self) -> &MerkleAccumulator {
+        &self.txn_accumulator
+    }
+
+    pub fn get_block_accumulator(&self) -> &MerkleAccumulator {
+        &self.block_accumulator
+    }
 }
 
 impl ChainReader for BlockChain {
@@ -582,8 +583,11 @@ impl ChainReader for BlockChain {
     fn get_transaction_info(&self, txn_hash: HashValue) -> Result<Option<BlockTransactionInfo>> {
         let txn_info_ids = self.storage.get_transaction_info_ids_by_hash(txn_hash)?;
         for txn_info_id in txn_info_ids {
-            if self.check_exist_transaction_info(txn_info_id) {
-                return self.storage.get_transaction_info(txn_info_id);
+            let txn_info = self.storage.get_transaction_info(txn_info_id)?;
+            if let Some(txn_info) = txn_info {
+                if self.exist_block(txn_info.block_id())? {
+                    return Ok(Some(txn_info));
+                }
             }
         }
         Ok(None)

chain/tests/test_block_chain.rs

@@ -5,6 +5,7 @@ use anyhow::Result;
 use consensus::Consensus;
 use crypto::{ed25519::Ed25519PrivateKey, Genesis, PrivateKey};
 use starcoin_account_api::AccountInfo;
+use starcoin_accumulator::Accumulator;
 use starcoin_chain::BlockChain;
 use starcoin_chain::{ChainReader, ChainWriter};
 use starcoin_chain_mock::MockChain;
@@ -350,7 +351,7 @@ fn test_uncle_in_diff_epoch() {
 ///             ╭--> b3(t2)
 /// Genesis--> b1--> b2(t2)
 ///
-async fn test_block_chain_txn_info_fork_mapping() -> Result<()> {
+fn test_block_chain_txn_info_fork_mapping() -> Result<()> {
     let config = Arc::new(NodeConfig::random_for_test());
     let mut block_chain = test_helper::gen_blockchain_for_test(config.net())?;
     let header = block_chain.current_header();
@@ -385,7 +386,7 @@ async fn test_block_chain_txn_info_fork_mapping() -> Result<()> {
         );
         txn.as_signed_user_txn()?.clone()
     };
-    let tnx_hash = signed_txn_t2.id();
+    let txn_hash = signed_txn_t2.id();
     let (template_b2, excluded) = block_chain.create_block_template(
         *miner_account.address(),
         Some(miner_account.public_key.authentication_key()),
@@ -399,7 +400,7 @@ async fn test_block_chain_txn_info_fork_mapping() -> Result<()> {
         .consensus()
         .create_block(template_b2, config.net().time_service().as_ref())?;
 
-    block_chain.apply(block_b2)?;
+    block_chain.apply(block_b2.clone())?;
     let (template_b3, excluded) = block_chain2.create_block_template(
         *miner_account.address(),
         Some(miner_account.public_key.authentication_key()),
@@ -412,16 +413,46 @@ async fn test_block_chain_txn_info_fork_mapping() -> Result<()> {
     let block_b3 = block_chain2
         .consensus()
         .create_block(template_b3, config.net().time_service().as_ref())?;
-    block_chain2.apply(block_b3)?;
+    block_chain2.apply(block_b3.clone())?;
+
+    assert_ne!(
+        block_chain.get_txn_accumulator().root_hash(),
+        block_chain2.get_txn_accumulator().root_hash()
+    );
 
     let vec_txn = block_chain2
         .get_storage()
-        .get_transaction_info_ids_by_hash(tnx_hash)?;
+        .get_transaction_info_ids_by_hash(txn_hash)?;
 
     assert_eq!(vec_txn.len(), 2);
-    let txn_info = block_chain.get_transaction_info(tnx_hash)?;
-    assert!(txn_info.is_some());
-    assert_eq!(txn_info.unwrap().transaction_hash(), tnx_hash);
+    let txn_info1 = block_chain.get_transaction_info(txn_hash)?;
+    assert!(txn_info1.is_some());
+    let txn_info1 = txn_info1.unwrap();
+    assert!(vec_txn.contains(&txn_info1.id()));
+
+    let txn_info2 = block_chain2.get_transaction_info(txn_hash)?;
+    assert!(txn_info2.is_some());
+    let txn_info2 = txn_info2.unwrap();
+    assert!(vec_txn.contains(&txn_info2.id()));
+
+    assert_ne!(txn_info1, txn_info2);
+
+    assert_eq!(txn_info1.transaction_hash(), txn_hash);
+    assert_eq!(
+        txn_info1.block_id(),
+        block_b2.id(),
+        "txn_info's block id not as expect. {:?}",
+        txn_info1
+    );
+
+    assert_eq!(txn_info2.transaction_hash(), txn_hash);
+    assert_eq!(
+        txn_info2.block_id(),
+        block_b3.id(),
+        "txn_info's block id not as expect. {:?}",
+        txn_info2
+    );
+
     Ok(())
 }
 

commons/accumulator/src/accumulator_test.rs

@@ -3,7 +3,7 @@
 
 use crate::{
     node_index::NodeIndex, tree_store::mock::MockAccumulatorStore, Accumulator, AccumulatorNode,
-    LeafCount, MerkleAccumulator,
+    AccumulatorTreeStore, LeafCount, MerkleAccumulator,
 };
 use starcoin_crypto::{hash::ACCUMULATOR_PLACEHOLDER_HASH, HashValue};
 use std::time::SystemTime;
@@ -100,14 +100,14 @@ fn test_multiple_chain() {
     proof_verify(&accumulator, root_hash, &leaves, 0);
     let frozen_node = accumulator.get_frozen_subtree_roots();
     for node in frozen_node.clone() {
-        let acc = accumulator
+        let acc = mock_store
             .get_node(node)
             .expect("get accumulator node by hash should success")
             .unwrap();
         if let AccumulatorNode::Internal(internal) = acc {
-            let left = accumulator.get_node(internal.left()).unwrap().unwrap();
+            let left = mock_store.get_node(internal.left()).unwrap().unwrap();
             assert_eq!(left.is_frozen(), true);
-            let right = accumulator.get_node(internal.right()).unwrap().unwrap();
+            let right = mock_store.get_node(internal.right()).unwrap().unwrap();
             assert_eq!(right.is_frozen(), true);
         }
     }
@@ -243,19 +243,19 @@ fn test_update_right_leaf() {
 #[test]
 fn test_flush() {
     let leaves = create_leaves(1000..1020);
-    let mock_store = MockAccumulatorStore::new();
+    let mock_store = Arc::new(MockAccumulatorStore::new());
     let accumulator = MerkleAccumulator::new(
         *ACCUMULATOR_PLACEHOLDER_HASH,
         vec![],
         0,
         0,
-        Arc::new(mock_store),
+        mock_store.clone(),
     );
     let _root_hash = accumulator.append(&leaves).unwrap();
     accumulator.flush().unwrap();
     //get from storage
     for node_hash in leaves.clone() {
-        let node = accumulator.get_node(node_hash).unwrap();
+        let node = mock_store.get_node(node_hash).unwrap();
         assert!(node.is_some());
     }
 }

commons/accumulator/src/lib.rs

@@ -41,8 +41,6 @@ pub trait Accumulator {
     fn get_node_by_position(&self, position: u64) -> Result<Option<HashValue>>;
     /// Get proof by leaf index.
     fn get_proof(&self, leaf_index: u64) -> Result<Option<AccumulatorProof>>;
-    /// Get accumulator node by hash.
-    fn get_node(&self, hash: HashValue) -> Result<Option<AccumulatorNode>>;
     /// Flush node to storage.
     fn flush(&self) -> Result<()>;
     /// Get current accumulator tree root hash.
@@ -179,10 +177,6 @@ impl Accumulator for MerkleAccumulator {
         Ok(Some(AccumulatorProof::new(siblings)))
     }
 
-    fn get_node(&self, hash: HashValue) -> Result<Option<AccumulatorNode>> {
-        self.tree.lock().get_node(hash)
-    }
-
     fn flush(&self) -> Result<()> {
         self.tree.lock().flush()
     }

commons/accumulator/src/tree.rs

@@ -174,8 +174,8 @@ impl AccumulatorTree {
         Ok(hash)
     }
 
-    /// Get node for self package.
-    pub fn get_node(&self, hash: HashValue) -> Result<Option<AccumulatorNode>> {
+    /// Get node from store
+    fn get_node(&self, hash: HashValue) -> Result<Option<AccumulatorNode>> {
         let updates = &self.update_nodes;
         if !updates.is_empty() {
             if let Some(node) = updates.get(&hash) {