Refactor: Group a few structs together into a CommitBundleBuilder (#…

…864)

## Summary

I spotted a refactoring opportunity (a fairly old TODO) while working on
https://radixdlt.atlassian.net/browse/LZ-22.
It is useful to address it at this moment, since I am about to add
another piece of "data inserted on commit".

## Details

As in the PR title: I only move a few repeated structs into a single
re-used struct.
During the development here, I tried extending the scope a bit (e.g.
pulling the `proposer_timestamp` into the "tracked state" for
convenience), but the complexity suddenly exploded... and it started
touching on some conceptual questions (e.g. "is a `proposer_timestamp` a
part of transaction execution result?").

## Testing

This is a pure refactoring, so all existing tests just need to pass.

core-rust/state-manager/src/commit_bundle.rs

@@ -0,0 +1,152 @@
+/* Copyright 2021 Radix Publishing Ltd incorporated in Jersey (Channel Islands).
+ *
+ * Licensed under the Radix License, Version 1.0 (the "License"); you may not use this
+ * file except in compliance with the License. You may obtain a copy of the License at:
+ *
+ * radixfoundation.org/licenses/LICENSE-v1
+ *
+ * The Licensor hereby grants permission for the Canonical version of the Work to be
+ * published, distributed and used under or by reference to the Licensor’s trademark
+ * Radix ® and use of any unregistered trade names, logos or get-up.
+ *
+ * The Licensor provides the Work (and each Contributor provides its Contributions) on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied,
+ * including, without limitation, any warranties or conditions of TITLE, NON-INFRINGEMENT,
+ * MERCHANTABILITY, or FITNESS FOR A PARTICULAR PURPOSE.
+ *
+ * Whilst the Work is capable of being deployed, used and adopted (instantiated) to create
+ * a distributed ledger it is your responsibility to test and validate the code, together
+ * with all logic and performance of that code under all foreseeable scenarios.
+ *
+ * The Licensor does not make or purport to make and hereby excludes liability for all
+ * and any representation, warranty or undertaking in any form whatsoever, whether express
+ * or implied, to any entity or person, including any representation, warranty or
+ * undertaking, as to the functionality security use, value or other characteristics of
+ * any distributed ledger nor in respect the functioning or value of any tokens which may
+ * be created stored or transferred using the Work. The Licensor does not warrant that the
+ * Work or any use of the Work complies with any law or regulation in any territory where
+ * it may be implemented or used or that it will be appropriate for any specific purpose.
+ *
+ * Neither the licensor nor any current or former employees, officers, directors, partners,
+ * trustees, representatives, agents, advisors, contractors, or volunteers of the Licensor
+ * shall be liable for any direct or indirect, special, incidental, consequential or other
+ * losses of any kind, in tort, contract or otherwise (including but not limited to loss
+ * of revenue, income or profits, or loss of use or data, or loss of reputation, or loss
+ * of any economic or other opportunity of whatsoever nature or howsoever arising), arising
+ * out of or in connection with (without limitation of any use, misuse, of any ledger system
+ * or use made or its functionality or any performance or operation of any code or protocol
+ * caused by bugs or programming or logic errors or otherwise);
+ *
+ * A. any offer, purchase, holding, use, sale, exchange or transmission of any
+ * cryptographic keys, tokens or assets created, exchanged, stored or arising from any
+ * interaction with the Work;
+ *
+ * B. any failure in a transmission or loss of any token or assets keys or other digital
+ * artefacts due to errors in transmission;
+ *
+ * C. bugs, hacks, logic errors or faults in the Work or any communication;
+ *
+ * D. system software or apparatus including but not limited to losses caused by errors
+ * in holding or transmitting tokens by any third-party;
+ *
+ * E. breaches or failure of security including hacker attacks, loss or disclosure of
+ * password, loss of private key, unauthorised use or misuse of such passwords or keys;
+ *
+ * F. any losses including loss of anticipated savings or other benefits resulting from
+ * use of the Work or any changes to the Work (however implemented).
+ *
+ * You are solely responsible for; testing, validating and evaluation of all operation
+ * logic, functionality, security and appropriateness of using the Work for any commercial
+ * or non-commercial purpose and for any reproduction or redistribution by You of the
+ * Work. You assume all risks associated with Your use of the Work and the exercise of
+ * permissions under this License.
+ */
+
+use crate::staging::epoch_handling::EpochAwareAccuTreeFactory;
+
+use crate::store::traits::*;
+use crate::transaction::*;
+
+use crate::*;
+
+use crate::engine_prelude::*;
+
+use crate::accumulator_tree::slice_merger::AccuTreeSliceMerger;
+
+/// A builder of a [`CommitBundle`] from individual transactions and their commit results
+pub struct CommitBundleBuilder {
+    committed_transaction_bundles: Vec<CommittedTransactionBundle>,
+    substate_store_update: SubstateStoreUpdate,
+    state_hash_tree_update: HashTreeUpdate,
+    new_substate_node_ancestry_records: Vec<KeyedSubstateNodeAncestryRecord>,
+    transaction_tree_slice_merger: AccuTreeSliceMerger<TransactionTreeHash>,
+    receipt_tree_slice_merger: AccuTreeSliceMerger<ReceiptTreeHash>,
+}
+
+impl CommitBundleBuilder {
+    /// Starts a new build.
+    ///
+    /// The `epoch_state_version` vs `current_state_version` relationship is required only for
+    /// proper merging of hash accumulation trees (see [`AccuTreeSliceMerger`]).
+    pub fn new(epoch_state_version: StateVersion, current_state_version: StateVersion) -> Self {
+        let epoch_accu_trees =
+            EpochAwareAccuTreeFactory::new(epoch_state_version, current_state_version);
+        Self {
+            committed_transaction_bundles: Vec::new(),
+            substate_store_update: SubstateStoreUpdate::new(),
+            state_hash_tree_update: HashTreeUpdate::new(),
+            new_substate_node_ancestry_records: Vec::new(),
+            transaction_tree_slice_merger: epoch_accu_trees.create_merger(),
+            receipt_tree_slice_merger: epoch_accu_trees.create_merger(),
+        }
+    }
+
+    /// Adds another transaction execution to the bundle.
+    pub fn add_executed_transaction(
+        &mut self,
+        state_version: StateVersion,
+        proposer_timestamp_ms: i64,
+        raw: RawLedgerTransaction,
+        validated: ValidatedLedgerTransaction,
+        result: ProcessedCommitResult,
+    ) {
+        self.substate_store_update.apply(result.database_updates);
+        let hash_structures_diff = result.hash_structures_diff;
+        self.state_hash_tree_update
+            .add(state_version, hash_structures_diff.state_hash_tree_diff);
+        self.new_substate_node_ancestry_records
+            .extend(result.new_substate_node_ancestry_records);
+        self.transaction_tree_slice_merger
+            .append(hash_structures_diff.transaction_tree_diff.slice);
+        self.receipt_tree_slice_merger
+            .append(hash_structures_diff.receipt_tree_diff.slice);
+
+        self.committed_transaction_bundles
+            .push(CommittedTransactionBundle {
+                state_version,
+                raw,
+                receipt: result.local_receipt,
+                identifiers: CommittedTransactionIdentifiers {
+                    payload: validated.create_identifiers(),
+                    resultant_ledger_hashes: hash_structures_diff.ledger_hashes,
+                    proposer_timestamp_ms,
+                },
+            });
+    }
+
+    /// Finalizes the build with the given proof.
+    pub fn build(self, proof: LedgerProof, vertex_store: Option<Vec<u8>>) -> CommitBundle {
+        CommitBundle {
+            transactions: self.committed_transaction_bundles,
+            proof,
+            substate_store_update: self.substate_store_update,
+            vertex_store: vertex_store.map(VertexStoreBlobV1),
+            state_tree_update: self.state_hash_tree_update,
+            transaction_tree_slice: TransactionAccuTreeSliceV1(
+                self.transaction_tree_slice_merger.into_slice(),
+            ),
+            receipt_tree_slice: ReceiptAccuTreeSliceV1(self.receipt_tree_slice_merger.into_slice()),
+            new_substate_node_ancestry_records: self.new_substate_node_ancestry_records,
+        }
+    }
+}

core-rust/state-manager/src/jni/test_state_reader.rs

@@ -271,7 +271,7 @@ extern "system" fn Java_com_radixdlt_testutil_TestStateReader_leastStaleStateHas
     jni_sbor_coded_call(&env, request_payload, |_: ()| -> u64 {
         let database = JNINodeRustEnvironment::get_database(&env, j_rust_global_context);
         let least_stale_state_version = database
-            .access_direct() // the `get_stale_tree_parts_iter()` is inside a trait requiring writeability
+            .lock() // the `get_stale_tree_parts_iter()` is inside a trait requiring writeability
             .get_stale_tree_parts_iter()
             .next()
             .map(|(state_version, _)| state_version)

core-rust/state-manager/src/lib.rs

@@ -65,6 +65,7 @@
 extern crate core;
 
 mod accumulator_tree;
+mod commit_bundle;
 pub mod jni;
 mod limits;
 pub mod mempool;

core-rust/state-manager/src/protocol/protocol_updates/protocol_update_committer.rs

@@ -3,15 +3,12 @@
 use crate::engine_prelude::*;
 use node_common::locks::{LockFactory, RwLock};
 
-use crate::epoch_handling::EpochAwareAccuTreeFactory;
+use crate::commit_bundle::CommitBundleBuilder;
 use crate::protocol::*;
 use crate::query::TransactionIdentifierLoader;
 use crate::traits::*;
 use crate::transaction::*;
-use crate::{
-    CommittedTransactionIdentifiers, ExecutionCache, LedgerHeader, LedgerProof, LedgerProofOrigin,
-    ReadableStore,
-};
+use crate::{ExecutionCache, LedgerHeader, LedgerProof, LedgerProofOrigin, ReadableStore};
 
 #[derive(Debug, Clone, PartialEq, Eq, Sbor)]
 pub enum UpdateTransaction {
@@ -182,17 +179,10 @@ where
             dummy_protocol_state,
         );
 
-        // TODO: extract common code from here and StateComputer::commit (also see the comment there)
-        let mut committed_transaction_bundles = Vec::new();
-        let mut substate_store_update = SubstateStoreUpdate::new();
-        let mut state_tree_update = HashTreeUpdate::new();
-        let mut new_node_ancestry_records = Vec::new();
-        let epoch_accu_trees = EpochAwareAccuTreeFactory::new(
+        let mut commit_bundle_builder = CommitBundleBuilder::new(
             series_executor.epoch_identifiers().state_version,
             series_executor.latest_state_version(),
         );
-        let mut transaction_tree_slice_merger = epoch_accu_trees.create_merger();
-        let mut receipt_tree_slice_merger = epoch_accu_trees.create_merger();
 
         for transaction in transactions {
             let raw = transaction.to_raw().unwrap();
@@ -204,27 +194,13 @@ where
                 .expect("protocol update not committable")
                 .expect_success("protocol update");
 
-            substate_store_update.apply(commit.database_updates);
-            let hash_structures_diff = commit.hash_structures_diff;
-            state_tree_update.add(
+            commit_bundle_builder.add_executed_transaction(
                 series_executor.latest_state_version(),
-                hash_structures_diff.state_hash_tree_diff,
-            );
-            new_node_ancestry_records.extend(commit.new_substate_node_ancestry_records);
-            transaction_tree_slice_merger.append(hash_structures_diff.transaction_tree_diff.slice);
-            receipt_tree_slice_merger.append(hash_structures_diff.receipt_tree_diff.slice);
-
-            let proposer_timestamp_ms = latest_header.proposer_timestamp_ms;
-            committed_transaction_bundles.push(CommittedTransactionBundle {
-                state_version: series_executor.latest_state_version(),
+                latest_header.proposer_timestamp_ms,
                 raw,
-                receipt: commit.local_receipt,
-                identifiers: CommittedTransactionIdentifiers {
-                    payload: validated.create_identifiers(),
-                    resultant_ledger_hashes: *series_executor.latest_ledger_hashes(),
-                    proposer_timestamp_ms,
-                },
-            });
+                validated,
+                commit,
+            );
         }
 
         let resultant_state_version = series_executor.latest_state_version();
@@ -247,19 +223,7 @@ where
             },
         };
 
-        let commit_bundle = CommitBundle {
-            transactions: committed_transaction_bundles,
-            proof,
-            substate_store_update,
-            vertex_store: None,
-            state_tree_update,
-            transaction_tree_slice: TransactionAccuTreeSliceV1(
-                transaction_tree_slice_merger.into_slice(),
-            ),
-            receipt_tree_slice: ReceiptAccuTreeSliceV1(receipt_tree_slice_merger.into_slice()),
-            new_substate_node_ancestry_records: new_node_ancestry_records,
-        };
-
-        self.database.commit(commit_bundle);
+        self.database
+            .commit(commit_bundle_builder.build(proof, None));
     }
 }

core-rust/state-manager/src/state_computer.rs

@@ -91,6 +91,7 @@ use crate::store::traits::scenario::{
 };
 
 use crate::accumulator_tree::storage::ReadableAccuTreeStore;
+use crate::commit_bundle::CommitBundleBuilder;
 use std::ops::Deref;
 use std::sync::Arc;
 use std::time::{Instant, SystemTime};
@@ -1092,21 +1093,14 @@ impl StateComputer {
             return Err(InvalidCommitRequestError::TransactionRootMismatch);
         }
 
-        // TODO(after RCnet-v3): Refactor this group of fields into a "commit bundle builder"
-        let mut committed_transaction_bundles = Vec::new();
         let mut transactions_metrics_data = Vec::new();
-        let mut substate_store_update = SubstateStoreUpdate::new();
-        let mut state_tree_update = HashTreeUpdate::new();
-        let mut new_node_ancestry_records = Vec::new();
-        let epoch_accu_trees = EpochAwareAccuTreeFactory::new(
-            series_executor.epoch_identifiers().state_version,
-            series_executor.latest_state_version(),
-        );
-        let mut transaction_tree_slice_merger = epoch_accu_trees.create_merger();
-        let mut receipt_tree_slice_merger = epoch_accu_trees.create_merger();
         let mut committed_user_transactions = Vec::new();
 
         // Step 3.: Actually execute the transactions, collect their results into DB structures
+        let mut commit_bundle_builder = CommitBundleBuilder::new(
+            series_executor.epoch_identifiers().state_version,
+            series_executor.latest_state_version(),
+        );
         for ((raw, prepared), proposer_timestamp_ms) in commit_request
             .transactions
             .into_iter()
@@ -1132,31 +1126,18 @@ impl StateComputer {
                     notarized_transaction_hash: user_transaction.notarized_transaction_hash(),
                 });
             }
-
-            substate_store_update.apply(commit.database_updates);
-            let hash_structures_diff = commit.hash_structures_diff;
-            state_tree_update.add(
-                series_executor.latest_state_version(),
-                hash_structures_diff.state_hash_tree_diff,
-            );
-            new_node_ancestry_records.extend(commit.new_substate_node_ancestry_records);
-            transaction_tree_slice_merger.append(hash_structures_diff.transaction_tree_diff.slice);
-            receipt_tree_slice_merger.append(hash_structures_diff.receipt_tree_diff.slice);
-
             transactions_metrics_data.push(TransactionMetricsData::new(
                 raw.0.len(),
                 commit.local_receipt.local_execution.fee_summary.clone(),
             ));
-            committed_transaction_bundles.push(CommittedTransactionBundle {
-                state_version: series_executor.latest_state_version(),
+
+            commit_bundle_builder.add_executed_transaction(
+                series_executor.latest_state_version(),
+                proposer_timestamp_ms,
                 raw,
-                receipt: commit.local_receipt,
-                identifiers: CommittedTransactionIdentifiers {
-                    payload: validated.create_identifiers(),
-                    resultant_ledger_hashes: *series_executor.latest_ledger_hashes(),
-                    proposer_timestamp_ms,
-                },
-            });
+                validated,
+                commit,
+            );
         }
 
         let new_protocol_state = series_executor.protocol_state();
@@ -1191,42 +1172,31 @@ impl StateComputer {
             );
         }
 
-        self.execution_cache
-            .lock()
-            .progress_base(&final_ledger_hashes.transaction_root);
-
+        // capture these before we lose the appropriate borrows:
+        let proposer_timestamp_ms = commit_ledger_header.proposer_timestamp_ms;
         let round_counters = leader_round_counters_builder.build(series_executor.epoch_header());
-        let proposer_timestamp_ms = commit_ledger_header.proposer_timestamp_ms; // for metrics only
-        let next_epoch = commit_ledger_header
-            .next_epoch
-            .as_ref()
-            .map(|next_epoch| next_epoch.epoch);
+        let final_transaction_root = final_ledger_hashes.transaction_root;
 
-        database.commit(CommitBundle {
-            transactions: committed_transaction_bundles,
-            proof: commit_request.proof,
-            substate_store_update,
-            vertex_store: commit_request.vertex_store.map(VertexStoreBlobV1),
-            state_tree_update,
-            transaction_tree_slice: TransactionAccuTreeSliceV1(
-                transaction_tree_slice_merger.into_slice(),
-            ),
-            receipt_tree_slice: ReceiptAccuTreeSliceV1(receipt_tree_slice_merger.into_slice()),
-            new_substate_node_ancestry_records: new_node_ancestry_records,
-        });
+        database
+            .commit(commit_bundle_builder.build(commit_request.proof, commit_request.vertex_store));
         drop(database);
 
-        let num_user_transactions = committed_user_transactions.len() as u32;
+        self.execution_cache
+            .lock()
+            .progress_base(&final_transaction_root);
 
         self.mempool_manager.remove_committed(
             committed_user_transactions
                 .iter()
                 .map(|txn| &txn.intent_hash),
         );
-        if let Some(epoch) = next_epoch {
+
+        if let Some(next_epoch) = next_epoch {
             self.mempool_manager
-                .remove_txns_where_end_epoch_expired(epoch);
+                .remove_txns_where_end_epoch_expired(next_epoch.epoch);
         }
+
+        let num_user_transactions = committed_user_transactions.len() as u32;
         self.pending_transaction_result_cache
             .write()
             .track_committed_transactions(SystemTime::now(), committed_user_transactions);
@@ -1238,6 +1208,7 @@ impl StateComputer {
             proposer_timestamp_ms,
             commit_request.self_validator_id,
         );
+
         self.committed_transactions_metrics
             .update(transactions_metrics_data);