chore: merge feature dan2 into development

[sdbondi]

Description

chore!: merge feature dan2 into development
fix: base node gRPC get version method returns response object containing version and network
Validator node Merkle tree uses jmt
Define empty Merkle root for the validator node MR so that it matches the hardcoded Merkle root in genesis blocks

Motivation and Context

Merge feature branch

How Has This Been Tested?

Existing tests, manually.
Tested base node mainnet sync and wallet recovery

What process can a PR reviewer use to test or verify this change?

Breaking Changes

• None
• Requires data directory on base node to be deleted
• Requires hard fork
• Other - Please specify

Summary by CodeRabbit

• New Features

  • Introduced comprehensive sidechain support, including validator node registration, exit, and eviction mechanisms.
  • Added sidechain ID signing and verification for enhanced security and identity management.
  • Enabled support for sidechain commit and eviction proofs, including quorum certificate validation.
  • Expanded gRPC and wallet interfaces to manage validator nodes and sidechain-related transactions.
  • Added UI fields and CLI options for sidechain deployment keys and validator node claim public keys.

• Enhancements

  • Improved validator node lifecycle management with epoch-based activation and exit queues.
  • Enhanced validation logic for sidechain outputs, validator node states, and output spend rules.
  • Extended consensus constants and output types to support new sidechain and validator node features.
  • Simplified and improved conversion and serialization logic for sidechain-related types.
  • Refined error handling and logging for validator node and sidechain operations.
  • Updated UI and CLI to integrate sidechain deployment key inputs seamlessly.

• Bug Fixes

  • Fixed and clarified error handling and validation for validator node registration, exit, and eviction.
  • Corrected validation logic to prevent spending active validator node registrations.
  • Improved signature verification for validator node registrations and exits.

• Documentation

  • Added and updated code comments and documentation for new sidechain structures and validation flows.

• Chores

  • Updated dependencies and workspace configuration to include new sidechain components.
  • Refactored and cleaned up imports, trait bounds, and test utilities to align with new features.
  • Replaced deprecated or redundant code with streamlined implementations across modules.
  • Added new test fixtures and expanded test coverage for sidechain proofs and validator node logic.

End-users will notice enhanced sidechain capabilities, new validator node management options, and improved security for sidechain operations.

[coderabbitai]

Actionable comments posted: 20

🔭 Outside diff range comments (2)

applications/minotari_console_wallet/src/ui/components/register_template_tab.rs (1)

563-657: Missing input handling for SidechainIdKey mode.

The SidechainIdKey input mode doesn't handle character input, preventing users from typing in the field.

 fn on_key_send_input(&mut self, c: char) -> KeyHandled {
     if self.input_mode != InputMode::None {
         match self.input_mode {
-            InputMode::None | InputMode::SidechainIdKey => (),
+            InputMode::None => (),
+            InputMode::SidechainIdKey => match c {
+                '\n' => self.input_mode = InputMode::None,
+                c => {
+                    if c.is_alphanumeric() {
+                        self.sidechain_id_key_field.push(c);
+                    }
+                    return KeyHandled::Handled;
+                },
+            },
             InputMode::BinaryUrl => match c {

base_layer/core/src/chain_storage/blockchain_database.rs (1)

1509-1520: Incorrect validator node size for non-epoch blocks.

The validator_node_size is set to 0 for non-epoch blocks, but it should maintain the count from the previous block since the active validator set doesn't change within an epoch. Consider storing and reusing the previous block's validator count.

 } else {
     // MR is unchanged except for epoch boundary
     // Active validator set never changes within epochs, so we can reuse the previous block VN MR
-    // TODO: fetch a count for the active validator set
-    (tip_header.validator_node_mr, 0)
+    (tip_header.validator_node_mr, tip_header.validator_node_size as usize)
 };

♻️ Duplicate comments (1)

applications/minotari_app_grpc/src/conversions/sidechain_feature.rs (1)

414-415: Same borsh serialization concern as in proto file.

This has the same pattern of using expect() in the From implementation.

Also applies to: 427-428

🧹 Nitpick comments (26)

base_layer/sidechain/tests/support/mod.rs (1)

6-10: Consider improving error handling for better debugging.

While the current implementation works for tests, consider providing more descriptive error messages to help debug fixture loading issues.

 pub fn load_fixture<T: DeserializeOwned>(name: &str) -> T {
     let path = format!("tests/fixtures/{name}");
-    let file = std::fs::File::open(path).unwrap();
-    serde_json::from_reader(file).unwrap()
+    let file = std::fs::File::open(&path)
+        .unwrap_or_else(|e| panic!("Failed to open fixture file '{}': {}", path, e));
+    serde_json::from_reader(file)
+        .unwrap_or_else(|e| panic!("Failed to parse JSON from fixture '{}': {}", path, e))
 }

base_layer/core/src/transactions/transaction_components/range_proof_type.rs (1)

96-115: Improve test robustness and maintainability.

The test logic is correct but has maintainability concerns:

1. The hardcoded expected value 0b11 creates maintenance burden if enum variants are added or removed
2. The panic message "Duplicate variant" lacks context about which variant is duplicated

Consider this more maintainable approach:

 #[test]
 fn it_contains_all_enum_variants() {
     let mut variant_bits = 0u8;
+    let all_variants = RangeProofType::all();
 
-    fn check_duplicate(variant_bits: u8, mask: u8) {
+    fn check_duplicate(variant_bits: u8, mask: u8, variant: RangeProofType) {
         if variant_bits & mask != 0 {
-            panic!("Duplicate variant");
+            panic!("Duplicate variant: {:?}", variant);
         }
     }
 
-    for variant in RangeProofType::all() {
+    for variant in all_variants {
         let mask = 1 << *variant as u8;
-        check_duplicate(variant_bits, mask);
+        check_duplicate(variant_bits, mask, *variant);
         match variant {
             RangeProofType::BulletProofPlus => variant_bits |= mask,
             RangeProofType::RevealedValue => variant_bits |= mask,
         }
     }
-    assert_eq!(variant_bits, 0b11);
+    // Check that we have exactly 2^n - 1 where n is the number of variants
+    let expected_bits = (1 << all_variants.len()) - 1;
+    assert_eq!(variant_bits, expected_bits as u8);
 }

base_layer/sidechain/src/shard_group.rs (1)

7-11: Consider adding validation for shard range integrity.

The ShardGroup struct looks well-designed, but lacks validation to ensure start <= end_inclusive.

Consider adding validation methods:

 #[derive(Debug, Clone, Copy, PartialEq, Eq, Deserialize, Serialize, BorshSerialize, BorshDeserialize)]
 pub struct ShardGroup {
     pub start: u32,
     pub end_inclusive: u32,
 }
+
+impl ShardGroup {
+    /// Creates a new ShardGroup, validating that start <= end_inclusive
+    pub fn new(start: u32, end_inclusive: u32) -> Result<Self, &'static str> {
+        if start <= end_inclusive {
+            Ok(Self { start, end_inclusive })
+        } else {
+            Err("start must be <= end_inclusive")
+        }
+    }
+    
+    /// Returns the size of the shard group
+    pub fn size(&self) -> u32 {
+        self.end_inclusive - self.start + 1
+    }
+    
+    /// Returns true if the shard is contained in this group
+    pub fn contains(&self, shard: u32) -> bool {
+        shard >= self.start && shard <= self.end_inclusive
+    }
+}

base_layer/sidechain/tests/eviction_proof.rs (1)

8-21: Add documentation and improve test clarity.

While the test logic is correct, the tests could be more maintainable and self-documenting:

1. The magic number 4 should be explained or made a named constant
2. Add docstrings explaining what these tests verify
3. Consider making the validator closure logic more explicit

Consider applying these improvements:

mod validate {
    use super::*;
+   
+   const COMMITTEE_SIZE: u32 = 4;
+   
+   /// Tests that a valid eviction proof passes validation when all validators are known
    #[test]
    fn it_validates_a_valid_proof() {
        let proof = support::load_fixture::<EvictionProof>("eviction_proof1.json");
-       proof.validate(4, &|_| Ok(true)).unwrap();
+       proof.validate(COMMITTEE_SIZE, &|_validator_public_key| {
+           // Simulate that all validators are known/registered
+           Ok(true)
+       }).unwrap();
    }

+   /// Tests that eviction proof validation fails when the QC signs for unknown validators
    #[test]
    fn it_rejects_if_qc_signs_for_unknown_validator() {
        let proof = support::load_fixture::<EvictionProof>("eviction_proof1.json");
-       proof.validate(4, &|_| Ok(false)).unwrap_err();
+       proof.validate(COMMITTEE_SIZE, &|_validator_public_key| {
+           // Simulate that validators are unknown/unregistered
+           Ok(false)
+       }).unwrap_err();
    }
}

base_layer/core/src/chain_storage/mod.rs (1)

98-106: Consider adding additional derive traits for better usability.

The struct only derives Debug and Clone. Consider adding PartialEq and Eq for comparison operations, which would be useful for testing and validation logic.

-#[derive(Debug, Clone)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub struct ValidatorNodeRegistrationInfo {

base_layer/sidechain/src/serde.rs (1)

23-23: Fix documentation comment.

The comment says "serialize" but the function is actually for deserialization.

-    /// Use a serde deserializer to serialize the hex string of the given object.
+    /// Use a serde deserializer to deserialize the hex string into the given object type.

base_layer/core/src/base_node/comms_interface/comms_request.rs (1)

146-146: Minor: Display message mentions "GetShardKey" instead of "GetValidatorNode".

The display message for GetValidatorNode still says "GetShardKey" which appears to be a copy-paste error from the old variant name.

Update the display message to match the variant name:

-               write!(f, "GetShardKey ({:?}), public key ({:?})", sidechain_id, public_key)
+               write!(f, "GetValidatorNode (sidechain_id: {:?}, public_key: {:?})", sidechain_id, public_key)

base_layer/core/src/transactions/transaction_components/side_chain/mod.rs (1)

24-24: Consider the implications of switching to wildcard exports.

Switching from selective exports to wildcard exports (pub use module::*) exposes all public items from these submodules. This could:

1. Expose internal types that weren't meant to be part of the public API
2. Make it harder to track what's being exported
3. Lead to namespace pollution
4. Potentially cause breaking changes if consumers import conflicting names

Consider documenting which specific types are intended for public use, or reverting to selective exports if not all items need to be exposed.

Also applies to: 33-33, 36-39

base_layer/core/src/validation/helpers.rs (1)

382-385: Simplify the quorum threshold calculation

The current calculation is correct but convoluted. This calculates a 2/3 majority threshold more clearly.

-    let quorum_threshold =
-        db.validator_nodes_count_for_shard_group(sidechain_features.sidechain_public_key(), tip_epoch, shard_group)?;
-    let quorum_threshold = committee_size - (committee_size - 1) / 3;
+    let committee_size =
+        db.validator_nodes_count_for_shard_group(sidechain_features.sidechain_public_key(), tip_epoch, shard_group)?;
+    let quorum_threshold = (2 * committee_size + 2) / 3;

applications/minotari_console_wallet/src/ui/state/app_state.rs (1)

356-411: LGTM! Consider extracting transaction parameters

The sidechain deployment key integration is well implemented with proper error handling. However, the method now has many parameters.

Consider creating a parameter struct to reduce the number of arguments:

pub struct BurnTransactionParams {
    pub burn_proof_filepath: Option<String>,
    pub claim_public_key: Option<String>,
    pub amount: u64,
    pub selection_criteria: UtxoSelectionCriteria,
    pub fee_per_gram: u64,
    pub payment_id: PaymentId,
    pub sidechain_deployment_key: Option<String>,
}

base_layer/core/src/chain_storage/lmdb_db/composite_key.rs (1)

115-122: Consider defining a constant for the stack allocation threshold.

The magic number 64 should be defined as a named constant to improve code clarity and maintainability.

+/// Maximum size in bytes for stack allocation in SmallBytes
+const STACK_ALLOCATION_THRESHOLD: usize = 64;
+
 /// Returns a fixed 0-filled byte array.
 fn new_buf() -> SmallBytes<L> {
-    if L <= 64 {
+    if L <= STACK_ALLOCATION_THRESHOLD {
         return SmallBytes::Stack([0u8; L]);
     }
     SmallBytes::Heap(Box::new([0x0u8; L]))
 }

base_layer/core/src/transactions/transaction_components/side_chain/sidechain_feature.rs (1)

138-147: Consider more accurate error message for signing operation.

While RistrettoSchnorr::sign failures are extremely rare, the message "completely infallible" is technically incorrect since OsRng could fail in edge cases (e.g., system entropy exhaustion).

-            knowledge_proof: CompressedRistrettoSchnorr::new_from_schnorr(
-                RistrettoSchnorr::sign(private_key, message, &mut OsRng)
-                    .expect("RistrettoSchnorr::sign is completely infallible"),
-            ),
+            knowledge_proof: CompressedRistrettoSchnorr::new_from_schnorr(
+                RistrettoSchnorr::sign(private_key, message, &mut OsRng)
+                    .expect("RistrettoSchnorr::sign should not fail with valid inputs"),
+            ),

applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (1)

1733-1737: Address TODO comment about transaction type.

The TODO comment suggests uncertainty about whether this should be its own TxType. Consider creating a dedicated transaction type for validator node exits to improve transaction categorization and tracking.

Would you like me to help create a new TxType::ValidatorNodeExit variant to properly categorize these transactions?

applications/minotari_node/src/grpc/base_node_grpc_server.rs (2)

2753-2753: Consider using empty vector for None sidechain_id.

Using vec![0u8; 32] as the default for None sidechain_id could be confusing as it resembles a valid but zeroed public key. Consider using an empty vector instead to clearly indicate the absence of a sidechain_id:

-                    sidechain_id: sidechain_id.as_ref().map(|n| n.to_vec()).unwrap_or(vec![0u8; 32]),
+                    sidechain_id: sidechain_id.as_ref().map(|n| n.to_vec()).unwrap_or_default(),

---

2869-2883: Error handling pattern change.

The removal of obscure_error_if_true wrapper makes these errors more transparent, which is good for debugging. However, this creates inconsistency with the error handling pattern used throughout the rest of the file. If this is intentional, consider applying this pattern consistently across all methods.

applications/minotari_app_grpc/proto/sidechain_types.proto (1)

143-144: Minor: Unusual field ordering in SidechainBlockHeader.

The signature field (10) appears before metadata_hash (11) in the field list, but metadata_hash is declared after. While this doesn't affect the protocol, it's more conventional to order fields by their numbers.

Consider reordering the fields to match their field numbers:

-  Signature signature = 11;
   bytes metadata_hash = 10;
+  Signature signature = 11;

applications/minotari_app_grpc/proto/wallet.proto (1)

116-117: Fix comment formatting.

-  // // Call the Identify RPC method
+  // Call the Identify RPC method

base_layer/sidechain/src/commit_proof.rs (1)

362-362: Clarify comment about public key encoding.

The comment states "variable length bytes" but CompressedPublicKey should always be exactly 32 bytes when compressed. This might be confusing.

-    // NOTE this is borsh encoded as variable length bytes - technically should always be 32
+    // NOTE: While borsh encodes this as variable length bytes, CompressedPublicKey is always 32 bytes

base_layer/core/src/proto/sidechain_feature.rs (1)

413-415: Consider safer error handling for borsh serialization.

While the TryFrom implementation properly handles errors, the From implementation uses expect() which could panic if serialization fails. Consider documenting this invariant or using a different approach.

Consider either:

1. Document that this conversion assumes the data is always serializable
2. Use a custom method instead of the From trait to allow for error handling
3. Add validation in the type constructor to ensure data is always serializable

Also applies to: 427-428

base_layer/core/src/validation/aggregate_body/aggregate_body_chain_validator.rs (1)

122-125: Address the performance issue mentioned in the TODO comment.

The comment correctly identifies unnecessary cloning. This should be tracked as technical debt.

Would you like me to create an issue to track this performance optimization?

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

3032-3033: Track shard group implementation requirement.

The shard group parameter is currently ignored. This functionality needs to be implemented to support sharded validator networks.

Would you like me to create an issue to track the implementation of shard group support for validator node queries?

base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs (3)

1200-1206: Incomplete functionality: Re-registration with same public key

The TODO comment and commented-out test code indicate that re-registration of validators with the same public key is not properly supported. This could be a limitation if validators need to rejoin the network after exiting.

Would you like me to help implement proper support for validator re-registration or create an issue to track this enhancement?

---

436-557: Consider adding documentation for complex logic

This method implements complex logic to gather the entire validator set, handling both active validators and those in the exit queue. While the implementation appears correct, the complexity would benefit from additional inline documentation.

Add inline comments explaining:

• Why we first gather from activation queue
• The logic for checking exit queue when node not found in main store
• The significance of exit_epoch > end_epoch check

---

98-118: Consider validating that node hasn't exited before deletion

The delete method doesn't check if a node has already exited before allowing deletion. This could lead to inconsistencies if attempting to delete a node that's already in the exit queue.

Add a check to ensure the node hasn't already exited:

 pub fn delete(
     &self,
     sidechain_pk: Option<&CompressedPublicKey>,
     public_key: &CompressedPublicKey,
 ) -> Result<(), ChainStorageError> {
     let key = create_vn_key(sidechain_pk, public_key);
+    
+    // Check if node has already exited
+    let exit_key_prefix = sid_as_slice(sidechain_pk);
+    let mut cursor = self.exit_queue_read_cursor()?;
+    if cursor.seek_range(exit_key_prefix)? {
+        // Search for the node in exit queue
+        // ... implement check logic
+    }
+    
     let vn = lmdb_get::<_, ValidatorNodeEntry>(self.txn, &self.db_validator_nodes, &key)?.ok_or_else(|| {

base_layer/sidechain/src/validations.rs (1)

97-110: Consider using unreachable!() macro for clarity

The "BUG:" comments indicate unreachable code. Consider using the unreachable!() macro for better clarity and panic behavior in debug builds.

For example, replace the error return with:

-        // Not reachable because we check length of proof elements above
-        return Err(SidechainProofValidationError::InvalidProof {
-            details: "BUG: Proof must contain at least one proof element".to_string(),
-        });
+        // Not reachable because we check length of proof elements above
+        unreachable!("Proof must contain at least one proof element");

base_layer/core/src/transactions/transaction_components/side_chain/validator_node_exit.rs (1)

1-1: Consider updating copyright year for consistency

The copyright year is 2022 while other files in this PR show 2019 or 2024. Consider updating for consistency with the project's copyright practices.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 678bc68 and ff37627.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (107)

• Cargo.toml (4 hunks)
• applications/minotari_app_grpc/Cargo.toml (1 hunks)
• applications/minotari_app_grpc/build.rs (1 hunks)
• applications/minotari_app_grpc/proto/base_node.proto (4 hunks)
• applications/minotari_app_grpc/proto/sidechain_types.proto (2 hunks)
• applications/minotari_app_grpc/proto/types.proto (1 hunks)
• applications/minotari_app_grpc/proto/validator_node.proto (0 hunks)
• applications/minotari_app_grpc/proto/wallet.proto (7 hunks)
• applications/minotari_app_grpc/src/conversions/consensus_constants.rs (1 hunks)
• applications/minotari_app_grpc/src/conversions/mod.rs (0 hunks)
• applications/minotari_app_grpc/src/conversions/output_features.rs (2 hunks)
• applications/minotari_app_grpc/src/conversions/sidechain_feature.rs (5 hunks)
• applications/minotari_app_grpc/src/conversions/signature.rs (1 hunks)
• applications/minotari_app_utilities/src/identity_management.rs (1 hunks)
• applications/minotari_console_wallet/src/automation/commands.rs (8 hunks)
• applications/minotari_console_wallet/src/cli.rs (2 hunks)
• applications/minotari_console_wallet/src/config.rs (0 hunks)
• applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (7 hunks)
• applications/minotari_console_wallet/src/ui/components/burn_tab.rs (8 hunks)
• applications/minotari_console_wallet/src/ui/components/menu.rs (1 hunks)
• applications/minotari_console_wallet/src/ui/components/register_template_tab.rs (14 hunks)
• applications/minotari_console_wallet/src/ui/components/transactions_tab.rs (1 hunks)
• applications/minotari_console_wallet/src/ui/mod.rs (1 hunks)
• applications/minotari_console_wallet/src/ui/state/app_state.rs (7 hunks)
• applications/minotari_console_wallet/src/ui/state/tasks.rs (6 hunks)
• applications/minotari_ledger_wallet/common/src/common_types.rs (4 hunks)
• applications/minotari_node/src/commands/cli.rs (2 hunks)
• applications/minotari_node/src/commands/command/check_for_updates.rs (1 hunks)
• applications/minotari_node/src/commands/command/list_validator_nodes.rs (3 hunks)
• applications/minotari_node/src/grpc/base_node_grpc_server.rs (11 hunks)
• applications/minotari_node/src/grpc/readiness_grpc_server.rs (1 hunks)
• applications/minotari_node/src/grpc_method.rs (6 hunks)
• base_layer/common_types/src/epoch.rs (2 hunks)
• base_layer/common_types/src/key_branches.rs (6 hunks)
• base_layer/common_types/src/types/fixed_hash.rs (3 hunks)
• base_layer/core/Cargo.toml (2 hunks)
• base_layer/core/src/base_node/comms_interface/comms_request.rs (4 hunks)
• base_layer/core/src/base_node/comms_interface/comms_response.rs (4 hunks)
• base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (4 hunks)
• base_layer/core/src/base_node/comms_interface/local_interface.rs (3 hunks)
• base_layer/core/src/base_node/comms_interface/mod.rs (1 hunks)
• base_layer/core/src/base_node/rpc/service.rs (1 hunks)
• base_layer/core/src/blocks/genesis_block.rs (12 hunks)
• base_layer/core/src/chain_storage/active_validator_node.rs (1 hunks)
• base_layer/core/src/chain_storage/async_db.rs (2 hunks)
• base_layer/core/src/chain_storage/blockchain_backend.rs (2 hunks)
• base_layer/core/src/chain_storage/blockchain_database.rs (11 hunks)
• base_layer/core/src/chain_storage/lmdb_db/composite_key.rs (4 hunks)
• base_layer/core/src/chain_storage/lmdb_db/cursors.rs (4 hunks)
• base_layer/core/src/chain_storage/lmdb_db/helpers.rs (1 hunks)
• base_layer/core/src/chain_storage/lmdb_db/lmdb.rs (1 hunks)
• base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (23 hunks)
• base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs (4 hunks)
• base_layer/core/src/chain_storage/mod.rs (1 hunks)
• base_layer/core/src/chain_storage/smt_hasher.rs (2 hunks)
• base_layer/core/src/chain_storage/template_registation.rs (1 hunks)
• base_layer/core/src/chain_storage/tests/blockchain_database.rs (3 hunks)
• base_layer/core/src/chain_storage/tests/temp_db.rs (3 hunks)
• base_layer/core/src/consensus/consensus_constants.rs (14 hunks)
• base_layer/core/src/consensus/consensus_manager.rs (1 hunks)
• base_layer/core/src/covenants/filters/output_hash_eq.rs (2 hunks)
• base_layer/core/src/covenants/test.rs (2 hunks)
• base_layer/core/src/lib.rs (2 hunks)
• base_layer/core/src/proto/sidechain_feature.proto (1 hunks)
• base_layer/core/src/proto/sidechain_feature.rs (7 hunks)
• base_layer/core/src/proto/transaction.rs (1 hunks)
• base_layer/core/src/proto/types_impls.rs (3 hunks)
• base_layer/core/src/test_helpers/blockchain.rs (3 hunks)
• base_layer/core/src/test_helpers/mod.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/mod.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/output_features.rs (4 hunks)
• base_layer/core/src/transactions/transaction_components/output_type.rs (3 hunks)
• base_layer/core/src/transactions/transaction_components/range_proof_type.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/side_chain/confidential_output.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/side_chain/mod.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/side_chain/sidechain_feature.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/side_chain/template_registration.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/side_chain/validator_node_exit.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/side_chain/validator_node_registration.rs (4 hunks)
• base_layer/core/src/transactions/transaction_components/side_chain/validator_node_signature.rs (2 hunks)
• base_layer/core/src/transactions/transaction_components/transaction_input.rs (1 hunks)
• base_layer/core/src/transactions/transaction_components/transaction_output.rs (1 hunks)
• base_layer/core/src/validation/aggregate_body/aggregate_body_chain_validator.rs (8 hunks)
• base_layer/core/src/validation/aggregate_body/aggregate_body_internal_validator.rs (6 hunks)
• base_layer/core/src/validation/block_body/block_body_full_validator.rs (1 hunks)
• base_layer/core/src/validation/block_body/test.rs (3 hunks)
• base_layer/core/src/validation/error.rs (4 hunks)
• base_layer/core/src/validation/helpers.rs (2 hunks)
• base_layer/core/src/validation/transaction/transaction_chain_validator.rs (1 hunks)
• base_layer/mmr/src/balanced_binary_merkle_tree.rs (1 hunks)
• base_layer/mmr/src/sparse_merkle_tree/tree.rs (3 hunks)
• base_layer/sidechain/Cargo.toml (1 hunks)
• base_layer/sidechain/src/command.rs (1 hunks)
• base_layer/sidechain/src/commit_proof.rs (1 hunks)
• base_layer/sidechain/src/error.rs (1 hunks)
• base_layer/sidechain/src/eviction_proof.rs (1 hunks)
• base_layer/sidechain/src/lib.rs (1 hunks)
• base_layer/sidechain/src/serde.rs (1 hunks)
• base_layer/sidechain/src/shard_group.rs (1 hunks)
• base_layer/sidechain/src/validations.rs (1 hunks)
• base_layer/sidechain/tests/commit_proof.rs (1 hunks)
• base_layer/sidechain/tests/eviction_proof.rs (1 hunks)
• base_layer/sidechain/tests/fixtures/commit_proof.json (1 hunks)
• base_layer/sidechain/tests/fixtures/eviction_proof1.json (1 hunks)
• base_layer/sidechain/tests/support/mod.rs (1 hunks)
• base_layer/wallet/Cargo.toml (1 hunks)
• base_layer/wallet/build.rs (1 hunks)

⛔ Files not processed due to max files limit (31)

• base_layer/wallet/src/output_manager_service/recovery/standard_outputs_recoverer.rs
• base_layer/wallet/src/output_manager_service/storage/output_source.rs
• base_layer/wallet/src/transaction_service/error.rs
• base_layer/wallet/src/transaction_service/handle.rs
• base_layer/wallet/src/transaction_service/service.rs
• base_layer/wallet/tests/transaction_service_tests/service.rs
• ci_all.bat
• clients/rust/base_node_grpc_client/Cargo.toml
• common/config/presets/c_base_node_b_mining_allow_methods.toml
• common/config/presets/c_base_node_b_mining_deny_methods.toml
• common/config/presets/c_base_node_b_non_mining_allow_methods.toml
• common/config/presets/c_base_node_b_non_mining_deny_methods.toml
• comms/dht/src/peer_validator.rs
• hashing/Cargo.toml
• hashing/src/borsh_hasher.rs
• hashing/src/domains.rs
• hashing/src/layer2.rs
• hashing/src/lib.rs
• infrastructure/jellyfish/Cargo.toml
• infrastructure/jellyfish/src/bit_iter.rs
• infrastructure/jellyfish/src/error.rs
• infrastructure/jellyfish/src/hash.rs
• infrastructure/jellyfish/src/lib.rs
• infrastructure/jellyfish/src/memory_store.rs
• infrastructure/jellyfish/src/store.rs
• infrastructure/jellyfish/src/tree.rs
• infrastructure/jellyfish/src/types.rs
• integration_tests/tests/cucumber.rs
• integration_tests/tests/features/Reorgs.feature
• integration_tests/tests/steps/wallet_steps.rs
• lints.toml

💤 Files with no reviewable changes (3)

• applications/minotari_console_wallet/src/config.rs
• applications/minotari_app_grpc/src/conversions/mod.rs
• applications/minotari_app_grpc/proto/validator_node.proto

🧰 Additional context used

🧠 Learnings (1)

base_layer/common_types/src/types/fixed_hash.rs (1)

Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.

⏰ Context from checks skipped due to timeout of 90000ms (7)

• GitHub Check: test (mainnet, stagenet)
• GitHub Check: test (nextnet, nextnet)
• GitHub Check: test (testnet, esmeralda)
• GitHub Check: ci
• GitHub Check: cargo check with stable
• GitHub Check: Cucumber tests / Base Layer
• GitHub Check: Cucumber tests / FFI

🔇 Additional comments (201)

base_layer/core/src/consensus/consensus_manager.rs (1)

23-23: LGTM! Import consolidation improves code organization.

Moving the TryFrom import to the module level alongside Arc follows Rust best practices and eliminates the need for local imports within functions.

base_layer/wallet/build.rs (1)

1-1: LGTM! Routine copyright year update.

applications/minotari_app_grpc/Cargo.toml (1)

17-17: LGTM! Proper workspace dependency addition.

The tari_sidechain dependency is correctly added using workspace configuration, which ensures version consistency across the project.

applications/minotari_app_grpc/build.rs (1)

12-12: ```shell
#!/usr/bin/env bash
set -e

Display the top of base_node.proto to inspect definitions

echo "base_node.proto (1-200):"
sed -n '1,200p' applications/minotari_app_grpc/proto/base_node.proto

Search for SideChainFeature in base_node.proto

echo -e "\nSideChainFeature definition context:"
rg -n "message SideChainFeature" -n applications/minotari_app_grpc/proto/base_node.proto -A5 -B5

</details>
<details>
<summary>applications/minotari_node/src/commands/command/check_for_updates.rs (1)</summary>

`43-46`: **LGTM! Improved version reporting using compile-time environment variable.**

Using `env!("CARGO_PKG_VERSION")` ensures the displayed version always matches the actual package version from `Cargo.toml`, which is more reliable than importing a separate version constant.

</details>
<details>
<summary>base_layer/wallet/Cargo.toml (1)</summary>

`28-28`: **LGTM: Appropriate dependency addition for sidechain features.**

The addition of `tari_sidechain` dependency aligns with the PR's sidechain functionality objectives and follows the correct workspace dependency pattern.

</details>
<details>
<summary>applications/minotari_console_wallet/src/ui/mod.rs (1)</summary>

`90-90`: **Good improvement: Using compile-time version information.**

Switching from runtime constants to `env!("CARGO_PKG_VERSION")` ensures version accuracy and reduces dependencies. This change is consistent with similar updates across the codebase.

</details>
<details>
<summary>applications/minotari_console_wallet/src/ui/components/menu.rs (1)</summary>

`42-42`: **LGTM: Consistent version handling improvement.**

The change to `env!("CARGO_PKG_VERSION")` maintains consistency with other UI components and provides accurate compile-time version information.

</details>
<details>
<summary>applications/minotari_app_utilities/src/identity_management.rs (1)</summary>

`73-73`: **Good logging improvement: Enhanced visibility for identity failures.**

Upgrading the log level from `debug` to `warn` for node identity loading failures is appropriate, as this is a significant issue that operators should be alerted to, especially in the context of validator nodes and sidechain functionality.

</details>
<details>
<summary>base_layer/mmr/src/balanced_binary_merkle_tree.rs (1)</summary>

`49-49`: **LGTM: Clean addition of Clone trait.**

Adding the `Clone` trait to `BalancedBinaryMerkleTree<D>` is appropriate and enables tree instance cloning for the sidechain migration work. The struct's fields (`Vec<Hash>` and `PhantomData<D>`) are already `Clone`, making this a safe and necessary addition.

</details>
<details>
<summary>base_layer/core/src/chain_storage/template_registation.rs (1)</summary>

`36-40`: **LGTM: Clean getter method implementation.**

The `registration_data()` method provides appropriate read-only access to the `CodeTemplateRegistration` data. The implementation is straightforward and follows Rust conventions.

</details>
<details>
<summary>base_layer/core/src/transactions/transaction_components/mod.rs (1)</summary>

`66-66`: **LGTM: Module visibility change supports sidechain integration.**

Making the `side_chain` module public exposes sidechain transaction components for external use, which is necessary for the comprehensive sidechain feature integration described in the PR. This is an appropriate API expansion.

</details>
<details>
<summary>applications/minotari_ledger_wallet/common/src/common_types.rs (3)</summary>

`112-112`: **LGTM: Well-integrated enum variant addition.**

The `CodeTemplateAuthor` variant follows the existing pattern with appropriate hex value `0x0a` and supports the new template registration functionality.

---

`132-132`: **LGTM: Correct byte-to-enum mapping.**

The `from_byte` method correctly handles the new `CodeTemplateAuthor` variant, maintaining consistency with existing mappings.

---

`303-303`: **LGTM: Comprehensive test coverage.**

The test additions for `CodeTemplateAuthor` follow the existing test pattern and provide complete coverage for both `as_byte` and `from_byte` conversions.



Also applies to: 348-351

</details>
<details>
<summary>base_layer/core/src/base_node/comms_interface/mod.rs (1)</summary>

`27-27`: **LGTM: Appropriate API expansion for validator node features.**

Adding `ValidatorNodeChange` to the public re-exports correctly exposes this type for the validator node lifecycle management features. The change follows the existing pattern and supports the comprehensive sidechain enhancements.

</details>
<details>
<summary>base_layer/core/src/proto/transaction.rs (1)</summary>

`345-345`: **LGTM: Simplified protobuf conversion logic.**

The removal of nested field access streamlines the sidechain feature conversion process and improves code readability.

</details>
<details>
<summary>base_layer/core/src/test_helpers/mod.rs (1)</summary>

`264-271`: **LGTM: Useful utility function for hashing concatenated data.**

The `make_hash2` function provides a clean way to hash two combined inputs and follows the same pattern as the existing `make_hash` function.

</details>
<details>
<summary>applications/minotari_node/src/commands/cli.rs (1)</summary>

`107-107`: **LGTM: Improved version handling using Cargo environment variable.**

Switching from `consts::APP_VERSION` to `env!("CARGO_PKG_VERSION")` eliminates version duplication and ensures consistency with the package version defined in Cargo.toml.



Also applies to: 172-172

</details>
<details>
<summary>base_layer/core/Cargo.toml (1)</summary>

`22-22`: **LGTM: Dependency updates support new sidechain functionality.**

The addition of "tari_comms/rpc" to feature dependencies and the new "tari_sidechain" workspace dependency appropriately support the sidechain features introduced in this PR.



Also applies to: 24-24, 50-50

</details>
<details>
<summary>Cargo.toml (1)</summary>

`17-17`: **LGTM! Well-structured workspace integration.**

The workspace configuration correctly integrates the new `sidechain` and `jellyfish` crates with proper member declarations and dependency entries. The paths follow the established patterns and the changes are consistent across the workspace structure.



Also applies to: 30-30, 51-51, 73-73

</details>
<details>
<summary>base_layer/core/src/base_node/rpc/service.rs (1)</summary>

`301-301`: **LGTM! Efficient reference usage.**

Passing `&signature` instead of moving `signature` improves efficiency by avoiding unnecessary moves, especially since the signature is already cloned earlier for the `fetch_kernel` call.

</details>
<details>
<summary>base_layer/core/src/chain_storage/lmdb_db/helpers.rs (1)</summary>

`39-39`: **LGTM! Enhanced type flexibility.**

Adding `?Sized` to the trait bound is an excellent improvement that enables serialization of unsized types (e.g., `str`, `[T]`, trait objects) while maintaining full backward compatibility. This enhances the function's flexibility for complex sidechain data structures.

</details>
<details>
<summary>applications/minotari_app_grpc/src/conversions/consensus_constants.rs (1)</summary>

`149-150`: **LGTM! Correct consensus constants mapping.**

The addition of `vn_registration_max_vns_initial_epoch` and `vn_registration_max_vns_per_epoch` fields correctly reflects the updated consensus constants structure. The field names are descriptive and the method calls follow the established pattern.

</details>
<details>
<summary>base_layer/core/src/covenants/test.rs (1)</summary>

`33-33`: **LGTM! Updated to new sidechain feature structure.**

The changes correctly adapt the test helper function to use the new `SideChainFeature` struct format with `SideChainFeatureData` and optional `sidechain_id`. This properly reflects the evolution from simple enum variants to a more structured approach for sidechain features.



Also applies to: 81-84

</details>
<details>
<summary>base_layer/core/src/validation/transaction/transaction_chain_validator.rs (1)</summary>

`60-62`: **LGTM: Enhanced validation context.**

The change to pass the full block header instead of just the height provides richer context for aggregate body validation, which is beneficial for the enhanced sidechain and validator node validation logic.

</details>
<details>
<summary>base_layer/core/src/covenants/filters/output_hash_eq.rs (2)</summary>

`35-35`: **LGTM: Cleaner hash comparison.**

The direct comparison `output.hash() == hash` is more readable and cleaner than the previous implementation.

---

`57-57`: **LGTM: Consistent test simplification.**

The test update aligns well with the simplified hash comparison logic.

</details>
<details>
<summary>applications/minotari_node/src/grpc_method.rs (1)</summary>

`65-65`: **LGTM! Comprehensive addition of new GRPC method.**

The addition of `GetValidatorNodeChanges` is implemented correctly with all necessary updates:
- Enum variant added consistently
- Array size updated from 37 to 38
- String mapping follows naming convention
- Test coverage updated

All integration points have been properly maintained.



Also applies to: 74-74, 108-108, 117-117, 165-165, 263-263

</details>
<details>
<summary>base_layer/core/src/chain_storage/tests/temp_db.rs (1)</summary>

`40-40`: ```shell
#!/bin/bash
# Search for the definition of DBS in validator_node_store.rs
echo "=== Searching for DBS definition in validator_node_store.rs ==="
rg -n "DBS" -A 3 base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs

# If not found above, search globally for DBS constant or static
echo "=== Global search for const/static DBS definitions ==="
rg -n "const DBS" -A 3 .
rg -n "static DBS" -A 3 .
rg -n "DBS\s*=" -A 3 .

base_layer/core/src/chain_storage/lmdb_db/lmdb.rs (1)

64-64: Good flexibility improvement for handling unsized types.

Adding ?Sized to the trait bound allows lmdb_insert to work with dynamically sized types while maintaining backward compatibility with sized types. This supports the enhanced sidechain data structures mentioned in the PR summary.

base_layer/core/src/transactions/transaction_components/side_chain/confidential_output.rs (2)

26-26: Good addition of sidechain ID message accessor.

The new sidechain_id_message method provides standardized access to the claim public key bytes, which aligns with the sidechain ID cryptographic proof functionality. The ByteArray import is correctly added to support this.

Also applies to: 33-37

---

28-28: Verify impact of removing Hash trait.

Removing Hash from the derived traits is a breaking change if ConfidentialOutputData was used as a hash key anywhere in the codebase.

Please verify that this struct is not used as a hash key:

#!/bin/bash
# Description: Check if ConfidentialOutputData is used in hash-based collections
# Expected: No usage as HashMap key or in HashSet

rg -A 3 -B 3 "HashMap.*ConfidentialOutputData|HashSet.*ConfidentialOutputData" --type rust

base_layer/sidechain/src/lib.rs (1)

1-16: Well-structured new sidechain library module.

The library structure follows Rust best practices with appropriate public re-exports. Keeping serde and validations modules private while exposing the main functionality makes good sense for the API design.

Please verify that all referenced submodules exist:

#!/bin/bash
# Description: Verify all declared submodules exist as files
# Expected: All 7 modules should exist in the sidechain directory

fd -t f "(command|commit_proof|error|eviction_proof|serde|shard_group|validations)\.rs$" base_layer/sidechain/src/

base_layer/sidechain/Cargo.toml (1)

1-21: LGTM! Well-structured crate manifest.

The new tari_sidechain crate is properly integrated into the workspace with appropriate dependencies for sidechain functionality. The dependency selection looks reasonable, with workspace crates for core functionality and external crates for serialization, logging, and error handling.

base_layer/core/src/validation/block_body/block_body_full_validator.rs (1)

77-79: Enhanced validation context - LGTM!

Passing the full BlockHeader instead of just the height provides richer context for the aggregate body validator, enabling more comprehensive validation logic including sidechain-aware features.

applications/minotari_app_grpc/proto/types.proto (1)

146-147: LGTM! Proper protobuf field additions.

The new validator node registration limit fields are well-named and use appropriate types (uint32) with sequential tag numbers. These fields align with the enhanced epoch-based validator node management functionality.

base_layer/common_types/src/types/fixed_hash.rs (4)

55-55: Good serialization optimization.

The #[serde(transparent)] annotation improves serialization efficiency by avoiding wrapper structure overhead.

---

59-61: Excellent const constructor addition.

The const new function enables compile-time construction of FixedHash instances, improving performance for static hash values.

---

71-73: Useful const accessor method.

The into_array const function provides efficient conversion to the underlying array type.

---

171-174: Let’s inspect the hex serializer to confirm that its to_hex matches {:02x} output:

#!/bin/bash
# Show the Hex trait definition
rg -n "pub trait Hex" -A5 -B5 base_layer/common_types/src/serializers/hex.rs

# Show the slice/array impls of Hex, especially to_hex
rg -n "impl Hex for \[u8" -A5 -B5 base_layer/common_types/src/serializers/hex.rs

# Inspect the to_hex method body
rg -n "fn to_hex" -A5 -B5 base_layer/common_types/src/serializers/hex.rs

applications/minotari_app_grpc/src/conversions/output_features.rs (2)

41-41: Excellent idiomatic improvement.

Using map().transpose() is more idiomatic and cleaner than the previous approach for handling Optional conversions.

---

74-74: Better ownership semantics.

Using as_ref().map() preserves ownership semantics and avoids unnecessary clones when converting optional sidechain features.

base_layer/core/src/transactions/transaction_components/transaction_output.rs (1)

369-371: LGTM! Well-designed convenience method.

The is_burned_to_sidechain method correctly combines the existing is_burned() check with the presence of a sidechain feature. The logic is clear and the method name accurately describes its purpose.

base_layer/core/src/validation/block_body/test.rs (3)

608-614: LGTM! Correct adaptation to slice-based API.

The change from array literal to slice reference correctly adapts to the updated with_permitted_range_proof_types method signature. The logic and test coverage remain unchanged.

---

642-650: LGTM! Consistent with API changes.

Correctly updated to use slice reference syntax, maintaining the same test logic while adapting to the new method signature.

---

676-678: LGTM! Proper slice reference usage.

The syntax change correctly adapts the test to use slice references for the permitted range proof types configuration.

base_layer/core/src/chain_storage/smt_hasher.rs (3)

24-24: LGTM! Required import for new hasher implementation.

The FixedOutput import is necessary for the new ValidatorNodeJmtHasher implementation.

---

31-31: LGTM! Import for validator node hasher.

Correctly imports the new ValidatorNodeMerkleHasherBlake256 type used in the ValidatorNodeJmtHasher implementation.

---

55-73: LGTM! Well-implemented hasher following established pattern.

The ValidatorNodeJmtHasher correctly implements the SimpleHasher trait and follows the same pattern as the existing SmtHasher. The use of finalize_fixed().into() instead of as_fixed_bytes().expect(...) suggests the underlying hasher type (ValidatorNodeMerkleHasherBlake256) has a different API than OutputSmtHasherBlake256, which is appropriate for different domain-separated hashers.

base_layer/sidechain/src/error.rs (1)

4-26: LGTM! Well-designed error enum following Rust best practices.

The SidechainProofValidationError enum is well-structured with:

• Clear, descriptive error messages using thiserror::Error
• Appropriate error variants covering different validation failure scenarios
• Useful utility methods for error classification and construction
• Proper integration with JmtProofVerifyError via From trait

The string-based details provide good flexibility for error context while maintaining type safety.

base_layer/mmr/src/sparse_merkle_tree/tree.rs (3)

241-242: LGTM! Excellent API improvement for type flexibility.

The generalization of key parameters to accept any type convertible to NodeKey is a well-designed enhancement that maintains backward compatibility while improving usability. The implementation correctly uses .into() for safe conversion.

---

261-267: LGTM! Consistent implementation of the generic key parameter.

The insert method properly implements the same generic approach, with appropriate error handling for existing keys. The internal call to upsert with the converted key is correct.

---

480-480: LGTM! Test correctly demonstrates the new API usage.

The test update properly shows how to explicitly convert a byte array to NodeKey using NodeKey::from, validating the new generic interface works as expected.

base_layer/core/src/lib.rs (2)

66-66: LGTM! Proper import update for the new hashing domain.

The change from ValidatorNodeBmtHashDomain to ValidatorNodeMerkleHashDomain correctly aligns with the architectural transition from balanced binary Merkle trees to Jellyfish Merkle Trees for validator node handling.

---

91-91: LGTM! Type alias correctly updated for the new hashing approach.

The ValidatorNodeMerkleHasherBlake256 type alias properly uses DomainSeparatedHasher with the new domain, maintaining the same interface while supporting the JMT-based validator node Merkle root calculation.

base_layer/sidechain/tests/fixtures/eviction_proof1.json (1)

1-511: LGTM! Comprehensive test fixture for eviction proof validation.

This JSON fixture provides excellent test coverage for the new sidechain eviction proof functionality. The structure includes all necessary cryptographic elements:

• Versioned proof structure (V1) for future extensibility
• Complete commit proof with header metadata and quorum certificates
• Realistic inclusion proof with leaf nodes and sibling chains
• Proper hex-encoded cryptographic data for signatures and keys

This fixture will be valuable for testing the validation logic across the sidechain implementation.

base_layer/sidechain/src/serde.rs (2)

14-21: LGTM! Excellent implementation of flexible byte serialization.

The serialize function properly handles both human-readable (hex) and binary formats, providing an efficient and flexible encoding mechanism for byte-like types. The use of AsRef<[u8]> makes it work with various byte container types.

---

24-39: LGTM! Robust deserialization with proper error handling.

The deserialize function correctly:

• Handles both hex string and binary input formats
• Uses Cow types for efficient memory usage
• Provides clear error messages for conversion failures
• Uses appropriate generic bounds for flexible type conversion

This will be very useful for the sidechain cryptographic data structures.

base_layer/core/src/transactions/transaction_components/transaction_input.rs (1)

151-163: LGTM: Clean API refactoring with improved encapsulation.

The method signature change from multiple individual parameters to a single TransactionOutput parameter is a good refactoring that improves API usability and reduces the chance of parameter ordering errors. The rangeproof hash extraction logic correctly handles the optional proof field.

base_layer/common_types/src/epoch.rs (3)

42-44: LGTM: Added serialization support for cross-system compatibility.

The addition of BorshSerialize and BorshDeserialize traits enables efficient binary serialization for the VnEpoch type, which is essential for the sidechain functionality.

---

48-50: LGTM: Useful const constructor for zero epoch.

The zero() const function provides a clean way to create an epoch with value zero, which is likely a common pattern in epoch-based logic.

---

69-87: LGTM: Improved trait implementations with better error handling.

The From<u64> implementation provides convenient conversion, the FromStr now properly uses ParseIntError for better error propagation, and the Display implementation follows standard formatting conventions.

base_layer/core/src/blocks/genesis_block.rs (3)

39-45: LGTM: Good consolidation of empty validator node Merkle root representation.

The introduction of VALIDATOR_MR_EMPTY_PLACEHOLDER_HASH constant unifies the representation of empty validator node Merkle trees across all genesis blocks, improving maintainability and consistency.

---

102-448: LGTM: Consistent usage of the placeholder hash across genesis blocks.

The replacement of hardcoded validator node Merkle root hashes with the centralized constant is a good practice that ensures consistency and easier maintenance.

---

631-731: Verify that commented test code is intentionally disabled.

The validator node collection and verification logic has been commented out. Ensure this is intentional and consider adding a comment explaining why this code is disabled or if it should be removed entirely.

#!/bin/bash
# Check if there are alternative validator node tests or if this functionality is tested elsewhere
rg -A 10 -B 5 "validator.*node.*test|test.*validator.*node" --type rust

base_layer/sidechain/tests/fixtures/commit_proof.json (1)

1-221: LGTM: Well-structured test fixture for sidechain commit proof validation.

The JSON fixture provides comprehensive test data for commit proof validation, including proper structure for headers, quorum certificates, signatures, and chain links. The cryptographic data appears properly formatted with correct hash and signature lengths.

base_layer/core/src/transactions/transaction_components/side_chain/template_registration.rs (3)

33-43: LGTM: Appropriate type change for hash field.

Changing binary_sha from MaxSizeBytes<32> to FixedHash is appropriate since this field represents a hash value. The removal of the Hash trait from the derives is likely correct since the struct contains a signature field which shouldn't be hashed.

---

45-72: LGTM: Well-designed accessor methods and signature message creation.

The new accessor methods provide clean API access to internal fields. The create_signature_message method uses proper domain separation and chains all relevant fields for signature verification. The sidechain_id_message method conveniently uses the compressed public nonce from the author signature.

---

76-76: LGTM: Adding Copy trait to TemplateType is appropriate.

The TemplateType enum is small and contains only simple data types, making it suitable for the Copy trait which will improve performance when the type is used frequently.

base_layer/core/src/chain_storage/active_validator_node.rs (1)

46-52: Consider the ordering behavior for None sidechain_public_key values

The current ordering implementation will place entries with sidechain_public_key = None before entries with Some values. This might lead to unexpected sorting behavior where validators without sidechain keys appear first, regardless of their shard_key values. Consider if this is the intended behavior for your use case.

applications/minotari_console_wallet/src/cli.rs (1)

468-473: Type mismatch in hex parsing for signature and deployment key fields

The parse_hex function returns Vec<u8> (a single byte array), but it's being used to parse validator_node_signature and sidechain_deployment_key which are declared as Vec<Vec<u8>> (vectors of byte arrays). This type mismatch will cause compilation errors.

Either:

1. Change the field types to Vec<u8> if expecting a single hex string
2. Implement a parser that can handle multiple hex strings (e.g., comma-separated) and return Vec<Vec<u8>>
3. Use #[clap(value_parser)] with a custom parser function

Example fix for option 2:

+fn parse_hex_multiple(s: &str) -> Result<Vec<Vec<u8>>, CliParseError> {
+    s.split(',')
+        .map(|hex| Vec::<u8>::from_hex(hex.trim()))
+        .collect::<Result<Vec<_>, _>>()
+        .map_err(|e| CliParseError::HexError(format!("{}", e)))
+}

 pub struct RegisterValidatorNodeArgs {
     pub amount: MicroMinotari,
     pub validator_node_public_key: UniPublicKey,
     pub validator_node_public_nonce: UniPublicKey,
-    #[clap(long, parse(try_from_str = parse_hex), required = true)]
+    #[clap(long, parse(try_from_str = parse_hex_multiple), required = true)]
     pub validator_node_signature: Vec<Vec<u8>>,

Likely an incorrect or invalid review comment.

base_layer/common_types/src/key_branches.rs (2)

43-43: LGTM! New key branch properly integrated

The CodeTemplateAuthor branch is correctly added with all necessary conversions and mappings in place.

Also applies to: 55-55, 72-72, 88-88, 109-109

---

196-200: Nice test simplification

The direct assertions on dereferenced values improve readability.

applications/minotari_node/src/commands/command/list_validator_nodes.rs (2)

47-72: Well-implemented display enhancements

The changes properly handle the new ValidatorNodeRegistrationInfo structure and display the sidechain ID appropriately. Good handling of the optional sidechain_id with a default "" display.

---

88-93: Good error handling for block header fetch

Proper error handling with a clear error message when the block at the specified height is not found.

base_layer/core/src/transactions/transaction_components/output_type.rs (3)

59-64: New output types properly integrated

The three new sidechain-related output types are correctly added with sequential byte values.

---

103-105: Useful helper method

The is_template_registration() method provides a clean way to check for template registration outputs.

---

124-149: Excellent test coverage for enum completeness

The test cleverly uses bit manipulation to ensure all enum variants are covered in the all() method and checks for duplicate values. This will catch any future inconsistencies when new variants are added.

base_layer/sidechain/src/command.rs (3)

11-13: LGTM: Clean trait definition.

The ToCommand trait is well-designed with a single, focused responsibility for converting types into Command instances.

---

15-26: LGTM: Comprehensive command enum with appropriate traits.

The Command enum covers a wide range of sidechain operations and derives all necessary traits for serialization, hashing, and comparison. The variant names are descriptive and follow Rust conventions.

---

28-39: LGTM: Well-implemented accessor methods.

The implementation provides useful utility methods:

• evict_node() safely extracts the EvictNodeAtom when applicable
• hash() uses a specialized command hasher for consistency across the sidechain system

The pattern matching in evict_node() is safe and idiomatic.

base_layer/sidechain/src/eviction_proof.rs (2)

17-54: LGTM: Well-structured eviction proof wrapper.

The EvictionProof struct provides a clean interface around CommandCommitProof<EvictNodeAtom> with:

• Appropriate accessor methods for commonly needed fields
• Delegation of validation to the underlying proof
• A sidechain_id_message() method that returns the node's public key bytes

The design follows good encapsulation principles.

---

56-75: LGTM: Simple and focused data structure.

The EvictNodeAtom struct is well-designed:

• Contains only the essential data (public key of node to evict)
• Provides clear accessor method node_to_evict()
• Correctly implements ToCommand trait to create Command::EvictNode variant
• Derives all necessary traits for the sidechain system

The implementation is clean and follows Rust best practices.

applications/minotari_node/src/grpc/readiness_grpc_server.rs (3)

182-187: LGTM: Updated method signature aligns with protobuf changes.

The get_version method signature update from returning StringValue to BaseNodeGetVersionResponse correctly reflects the updated protobuf service definition mentioned in the AI summary.

---

375-380: LGTM: Consistent implementation of new RPC method.

The new get_validator_node_changes method follows the established pattern of other readiness server methods by returning a "not available" status, which is appropriate for a readiness/health check server.

---

123-373: LGTM: Style improvement by removing explicit returns.

Removing the explicit return statements before Err(self.get_not_available_status()) improves code readability and follows Rust conventions where the last expression is automatically returned.

applications/minotari_console_wallet/src/ui/components/burn_tab.rs (3)

38-38: LGTM: Proper field addition and initialization.

The sidechain_key_field is correctly added to the struct and properly initialized as an empty string in the constructor.

Also applies to: 58-58

---

84-93: LGTM: UI layout updates accommodate new input field.

The layout constraints, widget rendering, and cursor positioning have been properly updated to include the new sidechain key input field. The layout now has 7 vertical chunks instead of 5, and the cursor positioning logic correctly accounts for the new field positions.

Also applies to: 151-157, 162-162, 186-186, 218-220

---

318-322: LGTM: Transaction logic correctly handles optional sidechain key.

The confirmation dialog logic properly extracts the sidechain key as an optional parameter and passes it to the send_burn_transaction method, following the same pattern as other optional fields.

Also applies to: 336-336

base_layer/core/src/base_node/comms_interface/comms_request.rs (3)

29-32: LGTM: Import addition supports new functionality.

The addition of VnEpoch import is necessary for the new FetchValidatorNodeChanges variant and follows the existing import organization.

---

77-88: LGTM: Well-designed validator node request variants.

The new and updated validator node request variants provide comprehensive functionality:

• FetchValidatorNodesKeys now supports optional validator network filtering
• FetchValidatorNodeChanges enables epoch-based queries with optional sidechain scoping
• GetValidatorNode provides precise node lookup by sidechain and public key

The parameter types and optional fields are appropriate for the use cases.

---

128-169: LGTM: Display implementation correctly handles new variants.

The Display implementation has been properly updated to format the new validator node request variants. The formatting:

• Shows optional fields appropriately (e.g., "None" for missing validator_network)
• Provides useful debugging information for each variant
• Maintains consistency with the existing formatting style

base_layer/core/src/proto/types_impls.rs (3)

57-69: LGTM! Good improvement to error handling.

The generic signature conversion implementation is correct and the error mapping now provides more informative messages.

---

71-85: Clean implementation of the From trait split.

Good refactoring to split the From implementations for references and owned values. The delegation pattern in the owned implementation avoids code duplication.

---

58-84: Clean refactoring to support domain-separated signatures

The conversion implementations are properly refactored to support the new domain-separated Schnorr signatures. The split between reference and owned conversions follows Rust best practices.

base_layer/core/src/base_node/comms_interface/local_interface.rs (6)

300-316: Good enhancement for sidechain-aware validator node queries.

The method now supports filtering by validator network and returns structured registration info instead of tuples, improving type safety and functionality.

---

318-332: Well-structured new method for epoch-based validator node changes.

The implementation correctly handles fetching validator node changes by epoch with optional sidechain filtering.

---

333-349: Good refactoring from shard key to validator node query.

The renamed method with sidechain support and structured return type better reflects the actual functionality and provides more comprehensive validator node information.

---

300-316: LGTM! Improved API with structured return type.

The changes to accept an optional validator_network parameter and return structured ValidatorNodeRegistrationInfo objects improve the API's expressiveness and type safety compared to returning tuples.

---

318-331: Good addition for validator node lifecycle tracking.

The new method properly supports fetching validator node changes by epoch with optional sidechain filtering, which aligns with the validator node lifecycle management features.

---

333-349: Appropriate method rename and signature update.

The rename from get_shard_key to get_validator_node better reflects the method's purpose, and the addition of the optional sidechain_id parameter maintains consistency with other validator node methods.

base_layer/core/src/validation/error.rs (4)

128-171: Comprehensive error coverage for sidechain validation.

The new error variants provide clear, descriptive messages and cover all the validation scenarios for sidechain features including knowledge proofs, eviction proofs, and validator node lifecycle management.

---

224-241: Appropriate ban duration for sidechain validation errors.

All new sidechain validation errors correctly map to long ban durations, which is appropriate given these errors indicate potential protocol violations or malicious behavior.

---

128-170: Well-structured error variants for sidechain validation.

The new error variants comprehensively cover sidechain validation scenarios with descriptive messages that include relevant context (public keys, epochs, etc.) for debugging.

---

225-240: Complete and consistent ban reason mapping.

All new error variants are properly mapped to BanPeriod::Long, which is appropriate for validation errors that may indicate malicious behavior.

applications/minotari_app_grpc/src/conversions/signature.rs (3)

31-58: Consistent implementation with proto conversions.

The gRPC signature conversions correctly mirror the proto type conversions with improved error handling and clean separation of reference/owned implementations.

---

31-42: Good generalization with domain separation.

The generic implementation with domain separation trait H improves cryptographic security by preventing signature reuse across different contexts. The improved error handling with to_string() provides more specific error messages.

---

43-58: Clean implementation split for better ergonomics.

Splitting the From implementations for references and owned values improves API usability while maintaining clean, direct byte extraction.

base_layer/core/src/chain_storage/tests/blockchain_database.rs (6)

603-609: Good simplification using placeholder constant.

Using VALIDATOR_MR_EMPTY_PLACEHOLDER_HASH for the genesis block is clearer than calculating an empty BMT.

---

611-651: Test correctly updated for new validator node API.

The test properly uses the new signature method with epoch parameter and calculates the Merkle root from the structured validator node info.

---

652-701: Excellent test coverage for sidechain validator nodes.

The new test comprehensively verifies the sidechain-aware validator node registration flow, including key pair creation, signing with sidechain context, and merkle root calculation.

---

603-609: Appropriate use of placeholder constant.

Using VALIDATOR_MR_EMPTY_PLACEHOLDER_HASH for the genesis block is cleaner than computing an empty merkle root.

---

611-650: Well-updated test for epoch-based validator registration.

The test properly exercises the new epoch-based validator node registration with correct signature creation and merkle root verification.

---

652-701: Excellent test coverage for sidechain validator nodes.

The new test comprehensively covers validator node registration with sidechain keys, including proper signature generation with sidechain public key and merkle root verification.

base_layer/core/src/validation/helpers.rs (4)

273-305: LGTM!

The validator node registration validation logic correctly handles the epoch bounds check and prevents duplicate registrations.

---

273-305: Well-structured validator registration validation.

The function properly validates epoch constraints and prevents duplicate registrations with clear error messages.

---

307-340: Proper exit validation with active state check.

The function correctly ensures that only active validators can exit and validates epoch constraints with informative error messages.

---

342-399: Comprehensive eviction proof validation.

The function thoroughly validates eviction proofs including epoch constraints, validator state, and quorum requirements. The quorum calculation committee_size - (committee_size - 1) / 3 correctly implements a 2/3+ threshold.

base_layer/core/src/chain_storage/async_db.rs (1)

272-280: Good API improvements for validator node queries

The method renaming is more descriptive, and the new ValidatorNodeRegistrationInfo return type provides richer validator node data. The additional epoch-based query methods with optional sidechain filtering align well with the PR's validator node lifecycle management features.

applications/minotari_app_grpc/proto/base_node.proto (3)

49-49: Good improvement to GetVersion RPC

The structured response with separate version and network fields provides better type safety and API extensibility compared to returning a plain string.

Also applies to: 255-258

---

98-98: Well-designed validator node changes API

The use of oneof for polymorphic change types is idiomatic protobuf. The ValidatorNodeChangeAdd includes all necessary registration details while ValidatorNodeChangeRemove keeps it minimal with just the public key.

Also applies to: 510-535

---

537-540: Breaking change: Field renamed from height to epoch

The field rename from height to epoch is semantically correct for shard key requests. Ensure all clients are updated to use the new field name.

Run the following script to check for any remaining references to the old field name:

#!/bin/bash
# Description: Check for any remaining references to height field in GetShardKeyRequest usage

# Search for GetShardKeyRequest usage with height field
rg -A 5 'GetShardKeyRequest.*height'

# Search for protobuf client code that might use the old field
ast-grep --pattern 'GetShardKeyRequest { height: $_ }'

base_layer/core/src/transactions/transaction_components/side_chain/validator_node_registration.rs (2)

37-70: Good design for extended validator node registration

The addition of claim_public_key and max_epoch fields provides important functionality:

• claim_public_key allows validator rewards to be claimed by a different key
• max_epoch enables time-bounded registrations

The signed constructor nicely encapsulates the signing logic.

---

112-114: ```shell
#!/bin/bash

Retrieve struct definition for ValidatorNodeRegistration to inspect its fields

rg -A 10 'struct ValidatorNodeRegistration' base_layer/core/src/transactions/transaction_components/side_chain/validator_node_registration.rs

</details>
<details>
<summary>base_layer/core/src/chain_storage/lmdb_db/composite_key.rs (2)</summary>

`34-66`: **Excellent memory optimization with SmallBytes enum!**

The stack/heap optimization for small byte arrays is a smart performance improvement. The implementation correctly provides both immutable and mutable access methods.

---

`106-113`: **Correct implementation of safe big-endian u64 extraction.**

The method properly validates bounds before reading and correctly handles big-endian byte order conversion.

</details>
<details>
<summary>base_layer/core/src/validation/aggregate_body/aggregate_body_internal_validator.rs (5)</summary>

`149-167`: **Correct signature validation for template registration.**

The function properly validates the author signature using the Schnorr signature scheme with appropriate error handling.

---

`424-434`: **Well-structured validation orchestration.**

The function efficiently orchestrates sidechain validation with early return optimization and logical ordering of checks.

---

`435-441`: **Clean validation of sidechain ID proof.**

The function appropriately delegates validation to the sidechain feature while providing clear error messaging.

---

`443-470`: **Comprehensive validator node registration validation.**

The function properly validates all required constraints: minimum deposit, lock height, and signature validity with clear, informative error messages.

---

`472-482`: **Correct validator node exit validation.**

The function properly validates the exit signature, ensuring exits are authorized by the correct sidechain.

</details>
<details>
<summary>applications/minotari_console_wallet/src/ui/components/register_template_tab.rs (3)</summary>

`157-169`: **Well-integrated UI for sidechain deployment key field.**

The new field follows the existing UI patterns with proper layout constraints and conditional styling based on input mode.



Also applies to: 260-263, 346-356

---

`504-517`: **Proper validation of optional sidechain deployment key.**

The validation correctly handles both empty and non-empty cases, with clear error messaging for invalid secret keys.

---

`392-395`: **Correct cursor positioning for sidechain key field.**

The cursor positioning follows the established pattern and will properly position the cursor when editing the field.

</details>
<details>
<summary>applications/minotari_console_wallet/src/automation/commands.rs (5)</summary>

`64-64`: **LGTM: VnEpoch import added for epoch functionality.**

The import supports the new epoch-based validator node lifecycle management mentioned in the PR summary.

---

`194-212`: **LGTM: Sidechain deployment key parameter correctly added to burn_tari function.**

The function signature properly adds the optional `sidechain_deployment_key` parameter and passes it through to the underlying service call, maintaining backward compatibility.

---

`362-388`: **LGTM: New sidechain parameters correctly added to register_validator_node function.**

The function signature properly extends to include `validator_node_claim_public_key`, `sidechain_deployment_key`, and `epoch` parameters, which are correctly passed through to the underlying service call.

---

`603-607`: **LGTM: BurnTari transaction type correctly handles new parameter.**

The `None` value is appropriately passed for the optional sidechain deployment key parameter in the make_it_rain function.

---

`831-831`: **LGTM: BurnMinotari command correctly handles new parameter.**

The `None` value is appropriately passed for the optional sidechain deployment key parameter.

</details>
<details>
<summary>base_layer/core/src/transactions/transaction_components/side_chain/sidechain_feature.rs (2)</summary>

`38-57`: **LGTM! Well-structured validation logic.**

The refactoring from enum to struct with optional `SideChainId` is clean, and the validation method properly handles all variant types with appropriate message construction.

---

`149-160`: **Robust validation implementation.**

Good error handling with the `let Ok(...) else { return false; }` pattern for both conversions before signature verification.

</details>
<details>
<summary>applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (3)</summary>

`918-926`: **Good handling of optional sidechain deployment key.**

The implementation properly validates the key format and handles the empty case correctly.

---

`1583-1632`: **Excellent refactoring to use higher-level transaction service API.**

The refactored implementation is cleaner and includes comprehensive validation for all parameters with clear error messages. Good use of the `try_into()` pattern with specific error context.

---

`1758-1805`: **Well-implemented eviction proof submission endpoint.**

The implementation follows established patterns with proper validation, error handling, and consensus constant fetching.

</details>
<details>
<summary>base_layer/core/src/transactions/transaction_components/side_chain/validator_node_signature.rs (3)</summary>

`43-76`: **Good separation of registration and exit signing logic.**

The distinct signing methods properly differentiate between registration (with claim key) and exit signatures. The use of domain-separated hashing with `validator_registration_hasher()` is a security best practice.

---

`94-106`: **Verify domain separator usage for exit signatures.**

Both registration and exit methods use `validator_registration_hasher()`. While this provides consistent domain separation, consider if a separate hasher (e.g., `validator_exit_hasher()`) would be more semantically correct for exit signatures.

Please confirm if using the same hasher for both registration and exit signatures is intentional, or if separate domain separators should be used for additional security isolation.

---

`108-138`: **Well-structured validation methods.**

The validation methods properly reconstruct the signature messages and handle conversion errors gracefully. The use of `verify_raw_uniform` is appropriate for the 64-byte challenge format.

</details>
<details>
<summary>applications/minotari_node/src/grpc/base_node_grpc_server.rs (5)</summary>

`39-39`: **LGTM!**

The new imports for `VnEpoch` and `ValidatorNodeRegistrationInfo` are appropriate for the validator node epoch-based functionality.



Also applies to: 62-62

---

`175-178`: **LGTM!**

The addition of `GetMempoolStats` and `GetValidatorNodeChanges` to the second layer methods is appropriate.

---

`2830-2830`: **LGTM!**

The change from field access to method call for `registration_data()` is appropriate.

---

`2891-2893`: **Good fix for off-by-one error!**

The correction to subtract 1 from the count ensures the correct number of blocks is returned. This prevents returning one extra block beyond what was requested.

---

`2921-2921`: **Good addition to include burned outputs!**

Adding the `is_burned_to_sidechain()` check ensures that all sidechain-related outputs are included in the response, not just those with sidechain output types.

</details>
<details>
<summary>base_layer/core/src/chain_storage/blockchain_backend.rs (4)</summary>

`183-186`: **Breaking change: Method renamed and return type improved.**

The rename from `fetch_active_validator_nodes` to `fetch_all_active_validator_nodes` clarifies the method's purpose. The return type change from `Vec<(CompressedPublicKey, [u8; 32])>` to `Vec<ValidatorNodeRegistrationInfo>` improves type safety and provides richer validator node information.

---

`188-194`: **Good addition of sidechain-filtered validator node query.**

The overloaded `fetch_active_validator_nodes` method with optional `sidechain_pk` filtering provides flexibility for querying validator nodes scoped to specific sidechains while maintaining backward compatibility through the `Option` type.

---

`196-206`: **Well-designed epoch-based validator lifecycle queries.**

These methods enable querying validators by their lifecycle state (activating/exiting) in specific epochs, which is essential for managing validator node transitions. The optional sidechain filtering maintains consistency with other methods.

---

`222-234`: **Comprehensive shard group support added.**

The shard group related methods enable checking validator activity and counting validators per shard group, which is crucial for sidechain sharding functionality.

</details>
<details>
<summary>applications/minotari_console_wallet/src/ui/state/tasks.rs (2)</summary>

`181-189`: **Proper sidechain key integration for burn transactions.**

The optional `sidechain_deployment_key` parameter is correctly added and propagated to the `burn_tari` method, enabling sidechain-specific burn outputs while maintaining backward compatibility.




Also applies to: 206-213

---

`328-328`: **Efficient sidechain key handling for template registration.**

Good use of a reference parameter `Option<&PrivateKey>` to avoid unnecessary cloning at call sites. The `.cloned()` call when passing to `register_code_template` is appropriate since the method expects an owned value.




Also applies to: 410-410

</details>
<details>
<summary>base_layer/core/src/chain_storage/lmdb_db/cursors.rs (3)</summary>

`99-111`: **Excellent cursor generalization with cached value pattern.**

The addition of the key type parameter `K` and the `seek_value` cache is a well-designed enhancement. The cache ensures that seek operations preserve the current key-value pair for subsequent retrieval, preventing data loss during cursor navigation.

---

`131-141`: **Smart performance optimization with key-only methods.**

The `next_key()` and `prev_key()` methods are excellent additions that avoid value deserialization costs when only keys are needed. This can significantly improve performance for key-based operations.




Also applies to: 151-160

---

`226-237`: **Improved key type flexibility and error reporting.**

The `CompositeKey` implementation now properly handles variable-length keys with improved error messages. The new `Vec<u8>` implementation adds flexibility for arbitrary key types.




Also applies to: 239-244

</details>
<details>
<summary>base_layer/core/src/test_helpers/blockchain.rs (1)</summary>

`419-516`: **Test helper correctly updated to match trait changes.**

All validator node related methods have been properly updated to match the `BlockchainBackend` trait changes, including:
- Renamed method with updated return type
- New sidechain-filtered queries
- Epoch-based lifecycle queries
- Shard group operations

Each method correctly delegates to the inner database implementation.

</details>
<details>
<summary>base_layer/core/src/transactions/transaction_components/output_features.rs (3)</summary>

`134-149`: **Well-implemented sidechain identity proof for burn outputs.**

The method now properly signs the confidential output data with the optional sidechain deployment key, creating a cryptographic proof of sidechain identity. The structure correctly wraps the data in a `SideChainFeature`.

---

`187-219`: **Comprehensive validator node lifecycle output support.**

The new constructors for validator node exit and eviction properly implement the sidechain signing pattern:
- `for_validator_node_exit`: Enables voluntary validator exits with epoch bounds
- `for_validator_node_eviction`: Supports eviction proofs for misbehaving validators

Both methods correctly sign with the sidechain deployment key and set appropriate output types.

---

`221-241`: **Useful accessor methods for sidechain features.**

The new accessor methods `sidechain_id()` and `confidential_output_data()` provide clean API access to embedded sidechain data, improving usability for consumers of the output features.

</details>
<details>
<summary>base_layer/core/src/consensus/consensus_constants.rs (4)</summary>

`111-111`: **Good improvement: Changed to static slice for flexibility.**

The change from a fixed-size array to a static slice for `permitted_range_proof_types` provides better flexibility for adding or removing range proof types without changing the type signature.



Also applies to: 337-337, 903-903

---

`365-365`: **Good safety improvement: Using saturating multiplication.**

Using `saturating_mul` prevents potential overflow issues when calculating block heights from epochs.

---

`434-436`: **Consider different VN limits for production networks.**

All networks (localnet, igor, esmeralda, stagenet, nextnet, mainnet) use identical validator node registration limits:
- Initial epoch: 50 VNs
- Per epoch: 10 VNs  
- Exits per epoch: 5 VNs

While this consistency might be intentional for testing, production networks (especially mainnet) may require different limits based on their specific requirements and expected validator set sizes.



Also applies to: 501-503, 559-561, 618-620, 670-672, 749-751

---

`803-805`: **LGTM! New sidechain output types properly integrated.**

The new output types (`SidechainCheckpoint`, `SidechainProof`, `ValidatorNodeExit`) are correctly added to both the current and all range proof types configurations with `BulletProofPlus` support.



Also applies to: 816-818

</details>
<details>
<summary>applications/minotari_app_grpc/proto/sidechain_types.proto (2)</summary>

`29-37`: **Well-structured feature enumeration with sidechain ID support.**

Good use of `oneof` for the feature variants and adding `sidechain_id` as a separate field allows associating any feature with a specific sidechain.

---

`100-105`: **Good versioning strategy for forward compatibility.**

Using `oneof version` in `CommitProof` allows for future protocol upgrades without breaking backward compatibility.

</details>
<details>
<summary>applications/minotari_app_grpc/proto/wallet.proto (2)</summary>

`1139-1143`: **Well-structured validator node lifecycle RPCs.**

The three new RPCs (`RegisterValidatorNode`, `SubmitValidatorEvictionProof`, `SubmitValidatorNodeExit`) provide comprehensive support for validator node management with appropriate request/response structures.

---

`1191-1191`: **Consistent sidechain deployment key integration.**

The `sidechain_deployment_key` field is consistently added across all relevant requests, enabling proper sidechain association for burns, templates, validator registrations, evictions, and exits.



Also applies to: 1431-1431, 1514-1514, 1527-1527, 1541-1541

</details>
<details>
<summary>base_layer/sidechain/src/commit_proof.rs (3)</summary>

`99-106`: **Correct implementation of Merkle inclusion proof validation.**

The validation properly:
1. Hashes the command to get the tree hash
2. Uses identity mapping for the leaf key
3. Verifies inclusion against the command Merkle root

---

`319-332`: **Robust signature verification with proper error handling.**

The verification method properly handles potential errors from key and signature conversions before verifying, preventing panics from invalid data.

---

`29-77`: **Well-designed versioned data structure.**

The `CommandCommitProof` enum with versioning provides excellent forward compatibility while maintaining a clean API through delegation methods.

</details>
<details>
<summary>base_layer/core/src/proto/sidechain_feature.proto (1)</summary>

`10-153`: **Consistent with gRPC layer protobuf definitions.**

This file maintains identical message structures with `applications/minotari_app_grpc/proto/sidechain_types.proto`, which is important for consistent serialization between the core and gRPC layers.

</details>
<details>
<summary>base_layer/core/src/chain_storage/blockchain_database.rs (3)</summary>

`1018-1025`: **Good API improvement for validator node queries.**

The rename from `get_shard_key` to `get_validator_node` and returning the full `ValidatorNodeRegistrationInfo` instead of just a shard key provides more comprehensive information to callers. The optional `sidechain_pk` parameter adds useful filtering capability.

---

`1354-1387`: **Well-structured validator node query methods.**

These new methods provide comprehensive APIs for querying validator nodes by activation status, epoch transitions, and sidechain filtering. They follow the established pattern of delegating to the backend implementation.

---

`1816-1816`: **Good simplification of output data handling.**

Passing the entire output object to `add_output_data` is cleaner and more maintainable than unpacking individual fields.

</details>
<details>
<summary>base_layer/core/src/proto/sidechain_feature.rs (2)</summary>

`233-235`: **Good performance optimization by using references.**

The change from `From<T>` to `From<&T>` for various types avoids unnecessary clones.



Also applies to: 318-325, 342-343

---

`452-453`: **Good network validation.**

The validation ensures the network byte both fits in a u8 and represents a valid Network variant.

</details>
<details>
<summary>base_layer/core/src/validation/aggregate_body/aggregate_body_chain_validator.rs (2)</summary>

`71-84`: **Good API improvement for validation context.**

Changing from `height: u64` to `header: &BlockHeader` provides access to epoch information and other header fields needed for sidechain validation.

---

`311-372`: **Well-structured spend rules implementation.**

The function clearly enforces spend rules for different output types with appropriate error messages. The pattern matching on output types makes the logic easy to follow and maintain.

</details>
<details>
<summary>applications/minotari_app_grpc/src/conversions/sidechain_feature.rs (2)</summary>

`68-75`: **Consistent performance improvements using references.**

Good consistency with the proto conversions by using references to avoid unnecessary clones.



Also applies to: 184-201, 273-286, 300-306, 355-362

---

`674-679`: **Good shard key validation.**

The validation ensures the shard key is exactly 32 bytes with a clear error message showing the actual length.

</details>
<details>
<summary>base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (8)</summary>

`134-135`: **LGTM! Clear database naming for validator node lifecycle management.**

The new database names `validator_nodes_activation_queue` and `validator_nodes_exit` clearly indicate their purpose in managing validator node state transitions.

---

`190-191`: **Appropriate database flags for validator node queues.**

The use of `DUPSORT | DUPFIXED` for the activation queue enables efficient storage of multiple validator nodes per epoch, while the standard flags for the exit queue align with its different storage pattern.

---

`514-514`: **Database count correctly updated.**

The array size and new database entries are properly synchronized.



Also applies to: 548-552

---

`1510-1583`: **Well-structured sidechain UTXO handling.**

The method comprehensively handles all sidechain feature types with proper error handling and informative logging for debugging validator node lifecycle transitions.

---

`3047-3092`: **Comprehensive validator node retrieval with proper validation.**

The method correctly fetches validator node data and validates it against the UTXO, ensuring data consistency. The error handling provides clear diagnostics for debugging.

---

`2002-2023`: **Useful helper method for UTXO lookups.**

This method provides a clean abstraction for fetching UTXOs by commitment, reducing code duplication across validator node query methods.

---

`1061-1061`: ```shell
#!/bin/bash
# Locate the definition of get_consensus_constants and inspect its implementation
rg -n "get_consensus_constants" -A10 -B10 --color=never

---

1915-1918: Verify endianness consistency for height serialization.

The use of to_le_bytes() for height serialization should be consistent with other height serializations in the codebase to avoid ordering issues.

#!/bin/bash
# Check for other height serialization patterns in the codebase
rg "height.*to_(le|be)_bytes" --type rust

# Check CompositeKey usage patterns
rg "CompositeKey.*try_from_parts.*height" -A 3 -B 3 --type rust

base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs (4)

119-148: Well-implemented exit logic with proper error handling

The exit method correctly validates that the node exists before processing the exit, properly moves the node from the main store to the exit queue, and handles errors appropriately.

---

149-212: Robust implementation of exit reversal

The undo_exit method correctly handles the complex logic of finding an exited node across epochs and restoring it. The decision not to re-insert into the activation queue is well-documented and appropriate since the node was already activated previously.

---

774-824: Well-structured helper functions

The key creation helper functions follow a consistent pattern and properly handle optional sidechain public keys. The sid_as_slice function elegantly handles the None case by returning a zero-filled array.

---

825-1209: Comprehensive test coverage

The test suite is well-organized and provides thorough coverage of the ValidatorNodeStore functionality, including edge cases, error conditions, and complex scenarios like exit and re-registration.

base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (1)

432-508: Well-implemented validator node handlers

The new request handlers for validator node operations are well-structured with proper error handling and informative logging.

base_layer/core/src/transactions/transaction_components/side_chain/validator_node_exit.rs (1)

41-72: Well-implemented validator node exit functionality

The implementation is clean with proper signature validation, good method organization, and appropriate test coverage for the basic scenarios.

[coderabbitai]

🛠️ Refactor suggestion

Improve test coverage and clarity.

The test has limited validation coverage:

1. The hardcoded value 4 lacks context - consider adding a comment explaining what this parameter represents
2. The closure &|_| Ok(true) always returns success, which may not test realistic validation scenarios
3. The test only verifies that validation doesn't error, but doesn't validate the actual proof logic

Consider enhancing the test:

 #[test]
 fn it_validates_a_correct_proof() {
     let proof = load_fixture::<SidechainBlockCommitProof>("commit_proof.json");
-    proof.validate_committed(4, &|_| Ok(true)).unwrap();
+    // Test with expected block height from fixture
+    const EXPECTED_BLOCK_HEIGHT: u64 = 4;
+    
+    // Test with proper validation logic
+    let result = proof.validate_committed(EXPECTED_BLOCK_HEIGHT, &|shard_group| {
+        // Add realistic validation logic here based on shard_group
+        Ok(true)
+    });
+    
+    assert!(result.is_ok(), "Proof validation should succeed for valid fixture");
 }

Consider adding negative test cases to verify the validation logic properly rejects invalid proofs.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	#[test]
	fn it_validates_a_correct_proof() {
	let proof = load_fixture::<SidechainBlockCommitProof>("commit_proof.json");
	proof.validate_committed(4, &|_| Ok(true)).unwrap();
	}
	#[test]
	fn it_validates_a_correct_proof() {
	let proof = load_fixture::<SidechainBlockCommitProof>("commit_proof.json");
	// Test with expected block height from fixture
	const EXPECTED_BLOCK_HEIGHT: u64 = 4;
	// Test with proper validation logic
	let result = proof.validate_committed(EXPECTED_BLOCK_HEIGHT, &|shard_group| {
	// Add realistic validation logic here based on shard_group
	Ok(true)
	});
	assert!(result.is_ok(), "Proof validation should succeed for valid fixture");
	}

🤖 Prompt for AI Agents

In base_layer/sidechain/tests/commit_proof.rs around lines 9 to 13, improve the
test by adding a comment explaining the significance of the hardcoded value 4 to
provide context. Replace the closure that always returns Ok(true) with more
realistic validation logic or multiple closures to simulate both success and
failure scenarios. Add assertions to verify the correctness of the proof
validation beyond just checking for no errors. Finally, include negative test
cases that supply invalid proofs to ensure the validation logic correctly
rejects them.

[coderabbitai]

🛠️ Refactor suggestion

Add documentation for the new ValidatorNodeRegistrationInfo struct.

The struct lacks documentation explaining its purpose, field meanings, and usage context. This is especially important for a public API that will be used across the codebase.

+/// Information about a validator node registration including sidechain affiliation and epoch details.
+/// 
+/// This struct encapsulates all relevant data for tracking validator node lifecycle,
+/// including activation epochs, sidechain associations, and minimum value commitments.
+#[derive(Debug, Clone)]
 pub struct ValidatorNodeRegistrationInfo {
+    /// The validator node's compressed public key
     pub public_key: CompressedPublicKey,
+    /// Optional sidechain ID this validator is associated with
     pub sidechain_id: Option<CompressedPublicKey>,
+    /// The shard key identifying the validator's responsibility area
     pub shard_key: [u8; 32],
+    /// The epoch when this validator node becomes active
     pub activation_epoch: VnEpoch,
+    /// The original validator node registration transaction component
     pub original_registration: ValidatorNodeRegistration,
+    /// The minimum value promise made by this validator
     pub minimum_value_promise: MicroMinotari,
 }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	#[derive(Debug, Clone)]
	pub struct ValidatorNodeRegistrationInfo {
	pub public_key: CompressedPublicKey,
	pub sidechain_id: Option<CompressedPublicKey>,
	pub shard_key: [u8; 32],
	pub activation_epoch: VnEpoch,
	pub original_registration: ValidatorNodeRegistration,
	pub minimum_value_promise: MicroMinotari,
	}
	/// Information about a validator node registration including sidechain affiliation and epoch details.
	///
	/// This struct encapsulates all relevant data for tracking validator node lifecycle,
	/// including activation epochs, sidechain associations, and minimum value commitments.
	#[derive(Debug, Clone)]
	pub struct ValidatorNodeRegistrationInfo {
	/// The validator node's compressed public key
	pub public_key: CompressedPublicKey,
	/// Optional sidechain ID this validator is associated with
	pub sidechain_id: Option<CompressedPublicKey>,
	/// The shard key identifying the validator's responsibility area
	pub shard_key: [u8; 32],
	/// The epoch when this validator node becomes active
	pub activation_epoch: VnEpoch,
	/// The original validator node registration transaction component
	pub original_registration: ValidatorNodeRegistration,
	/// The minimum value promise made by this validator
	pub minimum_value_promise: MicroMinotari,
	}

🤖 Prompt for AI Agents

In base_layer/core/src/chain_storage/mod.rs around lines 98 to 106, the
ValidatorNodeRegistrationInfo struct lacks documentation. Add a doc comment
above the struct explaining its purpose, describing each field's meaning, and
providing context on how and when this struct should be used to improve code
clarity and maintainability.

[coderabbitai]

⚠️ Potential issue

Partial equality implementation may cause confusion

The equality implementation only considers sidechain_public_key and shard_key, ignoring other important fields like public_key, commitment, activation_epoch, etc. This means two entries with different validator public keys or activation epochs would be considered equal if they share the same sidechain and shard keys, which could lead to unexpected behavior in collections or comparisons.

Consider either:

1. Implementing full equality checking all fields
2. Documenting why this partial equality is necessary
3. Using a custom comparison trait instead of Eq/PartialEq for this specific use case

🤖 Prompt for AI Agents

In base_layer/core/src/chain_storage/active_validator_node.rs around lines 60 to
64, the PartialEq implementation only compares sidechain_public_key and
shard_key, ignoring other fields like public_key, commitment, and
activation_epoch, which can cause incorrect equality results. To fix this,
update the eq method to compare all relevant fields for full equality or
alternatively document clearly why only these fields are compared. If full
equality is not desired, consider creating a custom comparison trait instead of
implementing PartialEq to avoid confusion.

[coderabbitai]

⚠️ Potential issue

Missing keyboard input handling for sidechain key field.

The new sidechain key input widget is rendered but there's no corresponding keyboard input handling added to the on_key_send_input method or key binding in the on_key method. Users won't be able to edit this field.

Add keyboard input handling for the sidechain key field:

1. Add a new variant to BurnInputMode enum:

#[derive(PartialEq, Debug)]
pub enum BurnInputMode {
    None,
    BurntProofPath,
    ClaimPublicKey,
+   SidechainKey,
    Amount,
    PaymentId,
    Fee,
}

2. Add input handling in on_key_send_input method around line 413:

                BurnInputMode::ClaimPublicKey => match c {
                    '\n' => self.burn_input_mode = BurnInputMode::Amount,
                    c => {
                        self.claim_public_key_field.push(c);
                        return KeyHandled::Handled;
                    },
                },
+               BurnInputMode::SidechainKey => match c {
+                   '\n' => self.burn_input_mode = BurnInputMode::Amount,
+                   c => {
+                       self.sidechain_key_field.push(c);
+                       return KeyHandled::Handled;
+                   },
+               },

3. Add key binding in on_key method around line 634:

            'c' => self.burn_input_mode = BurnInputMode::ClaimPublicKey,
+           's' => self.burn_input_mode = BurnInputMode::SidechainKey,  // Note: need different key since 's' is used for send

4. Add backspace handling in on_backspace method around line 687:

            BurnInputMode::ClaimPublicKey => {
                let _ = self.claim_public_key_field.pop();
            },
+           BurnInputMode::SidechainKey => {
+               let _ = self.sidechain_key_field.pop();
+           },

5. Update cursor positioning in draw_burn_form method around line 196:

+           BurnInputMode::SidechainKey => f.set_cursor(
+               vert_chunks[3].x + self.sidechain_key_field.width() as u16 + 1,
+               vert_chunks[3].y + 1,
+           ),

6. Update the input widget styling to highlight when active:

        let sidechain_key_input = Paragraph::new(self.sidechain_key_field.as_ref())
            .style(match self.burn_input_mode {
-               BurnInputMode::ClaimPublicKey => Style::default().fg(Color::Magenta),
+               BurnInputMode::SidechainKey => Style::default().fg(Color::Magenta),
                _ => Style::default(),
            })

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	let sidechain_key_input = Paragraph::new(self.sidechain_key_field.as_ref())
	.style(match self.burn_input_mode {
	BurnInputMode::ClaimPublicKey => Style::default().fg(Color::Magenta),
	_ => Style::default(),
	})
	.block(Block::default().borders(Borders::ALL).title("Sidechain Key:"));
	f.render_widget(sidechain_key_input, vert_chunks[3]);
	let sidechain_key_input = Paragraph::new(self.sidechain_key_field.as_ref())
	.style(match self.burn_input_mode {
	- BurnInputMode::ClaimPublicKey => Style::default().fg(Color::Magenta),
	+ BurnInputMode::SidechainKey => Style::default().fg(Color::Magenta),
	_ => Style::default(),
	})
	.block(Block::default().borders(Borders::ALL).title("Sidechain Key:"));
	f.render_widget(sidechain_key_input, vert_chunks[3]);

🤖 Prompt for AI Agents

In applications/minotari_console_wallet/src/ui/components/burn_tab.rs from lines
151 to 157, the sidechain key input field is rendered but lacks keyboard input
handling, preventing user edits. To fix this, add a new variant to the
BurnInputMode enum for the sidechain key field, then implement input handling
for this mode in the on_key_send_input method near line 413. Also, add a key
binding for switching to this input mode in the on_key method around line 634,
handle backspace for this field in the on_backspace method near line 687, update
cursor positioning in draw_burn_form around line 196 to reflect the active
field, and modify the input widget styling to visually highlight the sidechain
key field when it is active.

[coderabbitai]

⚠️ Potential issue

Incorrect prefix key structure for validator node store cursor seek

The create_vn_store_prefix_key creates a key with structure <sid, epoch>, but the validator node store uses keys with structure <sid, pk, epoch>. This prefix won't correctly position the cursor for epoch-based queries. The current implementation will start from the first validator for the sidechain and iterate through all validators, checking each one's activation epoch.

Consider restructuring the key or using a different approach for epoch-based queries. The current implementation is inefficient and may not work as intended for epoch-based filtering.

🤖 Prompt for AI Agents

In base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs around
lines 580 to 583, the prefix key used for cursor seeking is incorrectly
structured as <sid, epoch> while the store keys are structured as <sid, pk,
epoch>. To fix this, adjust the prefix to include the public key or redesign the
query logic to correctly filter by epoch without iterating over all validators.
This may involve creating a new prefix key function that matches the store's key
structure or implementing a more efficient epoch-based filtering mechanism.

[coderabbitai]

⚠️ Potential issue

Incorrect break condition assumes ordering by activation epoch

The break condition if vn.activation_epoch > end_epoch assumes validators are ordered by activation epoch in the store, but the key structure <sid, pk, epoch> means they're ordered by public key. This could cause the method to incorrectly skip active validators.

Remove the break condition and continue checking all validators for the sidechain:

-if vn.activation_epoch > end_epoch {
-    break;
-}
+if vn.activation_epoch <= end_epoch {
+    count += 1;
+}
-
-count += 1;

Committable suggestion skipped: line range outside the PR's diff.

🤖 Prompt for AI Agents

In base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs around
lines 388 to 390, the break condition that stops iteration when
vn.activation_epoch is greater than end_epoch incorrectly assumes the validators
are ordered by activation epoch, but they are actually ordered by public key. To
fix this, remove the break statement entirely so that the loop continues
checking all validators for the sidechain without prematurely stopping.

[coderabbitai]

⚠️ Potential issue

Fix error message to match the actual comparison

The error message references last_parent but the actual comparison is between justified and header_block_id.

Apply this diff to fix the error message:

         return Err(SidechainProofValidationError::InvalidProof {
             details: format!(
-                "Last parent block ID does not match the block ID in the header. Expected {}, but got {}",
-                header_block_id, last_parent,
+                "Justified block ID does not match the block ID in the header. Expected {}, but got {}",
+                header_block_id, justified,
             ),
         });

🤖 Prompt for AI Agents

In base_layer/sidechain/src/validations.rs around lines 125 to 131, the error
message incorrectly references 'last_parent' instead of the variables actually
compared, 'justified' and 'header_block_id'. Update the error message to reflect
the comparison by replacing 'last_parent' with 'justified' so the message
correctly states the expected and actual values being compared.

[coderabbitai]

⚠️ Potential issue

Use correct variable in error message

The error message should display the actual number of QCs found, not the total number of proof elements.

Apply this diff to fix the error message:

     if num_qcs < expected_len {
         return Err(SidechainProofValidationError::InvalidProof {
             details: format!(
                 "Expected at least {} QC proof elements, but got {}",
-                expected_len,
-                proof_elems.len()
+                expected_len, num_qcs
             ),
         });
     }

🤖 Prompt for AI Agents

In base_layer/sidechain/src/validations.rs around lines 154 to 160, the error
message incorrectly uses proof_elems.len() to report the number of QC proof
elements found. Replace proof_elems.len() with num_qcs in the format string to
correctly display the actual number of QC proof elements found in the error
message.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (4)

base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs (4)

279-281: Performance issue: O(n) search through exit queue

As noted in the TODO comment, this search is O(n) where n is the number of validators in the exit queue for the sidechain at or after end_epoch. For sidechains with many validators, this could be a performance bottleneck.

---

396-398: Incorrect break condition assumes ordering by activation epoch

The break condition if vn.activation_epoch > end_epoch assumes validators are ordered by activation epoch in the store, but the key structure <sid, pk> means they're ordered by public key. This could cause the method to incorrectly skip active validators.

---

588-591: Incorrect prefix key structure for validator node store cursor seek

The create_vn_store_prefix_key creates a key with structure <sid, epoch>, but the validator node store uses keys with structure <sid, pk>. This prefix won't correctly position the cursor for epoch-based queries.

---

826-833: Fix prefix key structure to match validator store keys.

The create_vn_store_prefix_key creates a <sid, epoch> prefix, but the validator store uses <sid, pk> keys. For epoch-based filtering, you should create a <sid> prefix and filter by epoch after retrieving the records.

-fn create_vn_store_prefix_key(sidechain_pk: Option<&CompressedPublicKey>, epoch: VnEpoch) -> [u8; PK_SIZE + U64_SIZE] {
+fn create_vn_store_prefix_key(sidechain_pk: Option<&CompressedPublicKey>) -> [u8; PK_SIZE] {
-    let mut buf = [0u8; PK_SIZE + U64_SIZE];
+    let mut buf = [0u8; PK_SIZE];
     if let Some(pk) = sidechain_pk {
         buf[..PK_SIZE].copy_from_slice(pk.as_bytes());
     }
-    buf[PK_SIZE..].copy_from_slice(&epoch.to_be_bytes());
     buf
 }

🧹 Nitpick comments (1)

base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs (1)

50-51: Misleading comment about key structure.

The comment indicates the key structure is <sid, pk, epoch> but the actual implementation in create_vn_key only uses <sid, pk>. This mismatch could cause confusion for future maintainers.

-/// <sid, pk, epoch>
+/// <sid, pk>
 type ValidatorNodeStoreKey = CompositeKey<{ PK_SIZE + PK_SIZE + U64_SIZE }>;

Wait, the size calculation suggests 3 parts but usage suggests 2. Please clarify the intended key structure.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 2f2a1a5 and fccc817.

📒 Files selected for processing (2)

• base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs (4 hunks)
• base_layer/core/src/consensus/consensus_constants.rs (14 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• base_layer/core/src/consensus/consensus_constants.rs

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1655-1658
Timestamp: 2025-05-29T09:40:09.356Z
Learning: In the Tari codebase, node_id hex strings in the database are guaranteed to be valid because they can only be added via `update_peer_sql(peer: Peer)` which converts from valid NodeId objects, ensuring data integrity at the insertion layer.

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs (3)

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1655-1658
Timestamp: 2025-05-29T09:40:09.356Z
Learning: In the Tari codebase, node_id hex strings in the database are guaranteed to be valid because they can only be added via `update_peer_sql(peer: Peer)` which converts from valid NodeId objects, ensuring data integrity at the insertion layer.

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.

Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: common_sqlite/src/error.rs:88-92
Timestamp: 2025-05-23T07:49:57.349Z
Learning: In the StorageError enum in common_sqlite/src/error.rs, the HexError variant should keep the manual From<HexError> implementation rather than using #[from] attribute, as it stores a String representation of the error rather than the HexError type itself.

🔇 Additional comments (3)

base_layer/core/src/chain_storage/lmdb_db/validator_node_store.rs (3)

67-79: LGTM! Well-structured multi-database approach.

The constructor properly takes references to all three databases needed for the epoch-based validator node lifecycle management.

---

149-211: Complex but sound undo_exit implementation.

The method correctly handles the epoch-based search for exit records and includes proper safeguards against infinite loops. The logic to remove from exit queue and re-insert into the main store is appropriate.

---

839-1222: Excellent test coverage for epoch-based validator node management.

The tests comprehensively cover the main functionality including insert/delete operations, epoch-based activation/exit logic, sidechain filtering, and error conditions. The test structure follows good practices with clear setup and assertions.

[coderabbitai]

Actionable comments posted: 1

🔭 Outside diff range comments (2)

base_layer/core/src/base_node/comms_interface/comms_request.rs (1)

68-80: Fix duplicate enum variant.

CheckOutputSpentStatus(HashOutput) is defined twice (lines 69 and 80), which will cause a compilation error.

Remove the duplicate definition:

 FetchOutputByPayRef(FixedHash),
 CheckOutputSpentStatus(HashOutput),
 FetchTemplateRegistrations {
     start_height: u64,
     end_height: u64,
 },
 FetchUnspentUtxosInBlock {
     block_hash: BlockHash,
 },
 FetchMinedInfoByPayRef(FixedHash),
 FetchMinedInfoByOutputHash(HashOutput),
 FetchOutputMinedInfo(HashOutput),
-CheckOutputSpentStatus(HashOutput),

base_layer/wallet/src/transaction_service/handle.rs (1)

109-116: Action Required: Include sidechain_deployment_key in all BurnTari calls

The new optional sidechain_deployment_key field must be added to every TransactionServiceRequest::BurnTari instantiation. Currently calls in service.rs and handle.rs are missing this parameter and will fail to compile.

Locations needing an update:

• base_layer/wallet/src/transaction_service/service.rs
• base_layer/wallet/src/transaction_service/handle.rs

Example patch for each site:

 TransactionServiceRequest::BurnTari {
     amount,
     selection_criteria,
     fee_per_gram,
     payment_id,
     claim_public_key,
+    sidechain_deployment_key: None, // TODO: supply key when required
 },

♻️ Duplicate comments (4)

applications/minotari_node/src/grpc/base_node_grpc_server.rs (3)

2336-2345: Confirm this breaking API change is documented.

This change from returning a simple string to a structured BaseNodeGetVersionResponse is a breaking change that was flagged in previous reviews. Ensure this is properly documented for API consumers.

---

2721-2740: Inconsistent error handling pattern needs fixing.

The error responses in this method don't use the obscure_error_if_true wrapper that's consistently used throughout the rest of the file. This was flagged in previous reviews and still needs to be addressed.

Apply this fix for consistency:

-            .ok_or_else(|| Status::not_found("Validator node not found"))?;
+            .ok_or_else(|| obscure_error_if_true(report_error_flag, Status::not_found("Validator node not found")))?;

         if validator_node.activation_epoch.as_u64() > epoch {
-            return Err(Status::not_found(format!(
+            return Err(obscure_error_if_true(report_error_flag, Status::not_found(format!(
                 "Validator node found but not active for epoch {epoch}"
-            )));
+            ))));

---

3203-3233: Inconsistent error handling in new method needs fixing.

The internal error at line 3226 should use the obscure_error_if_true wrapper for consistency with the rest of the file. This was flagged in previous reviews.

Apply this fix:

+        let report_error_flag = self.report_error_flag();
         let changes = handler
             .get_validator_node_changes(sidechain_id, VnEpoch(request.epoch))
             .await
             .map_err(|error| {
                 warn!(target: LOG_TARGET, "Base node service error: {}", error);
-                Status::internal("Internal error!")
+                obscure_error_if_true(report_error_flag, Status::internal(error.to_string()))
             })?

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

3022-3023: Replace hard-coded validator limit with configurable value.

The hard-coded limit of 1,000,000 validators should be made configurable through consensus constants or chain configuration to support different network scales.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fccc817 and 4973835.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (30)

• Cargo.toml (4 hunks)
• applications/minotari_app_grpc/proto/base_node.proto (4 hunks)
• applications/minotari_app_grpc/proto/wallet.proto (7 hunks)
• applications/minotari_console_wallet/src/automation/commands.rs (8 hunks)
• applications/minotari_console_wallet/src/cli.rs (2 hunks)
• applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs (7 hunks)
• applications/minotari_node/src/grpc/base_node_grpc_server.rs (11 hunks)
• applications/minotari_node/src/grpc/readiness_grpc_server.rs (1 hunks)
• applications/minotari_node/src/grpc_method.rs (4 hunks)
• base_layer/core/src/base_node/comms_interface/comms_request.rs (5 hunks)
• base_layer/core/src/base_node/comms_interface/comms_response.rs (4 hunks)
• base_layer/core/src/base_node/comms_interface/inbound_handlers.rs (4 hunks)
• base_layer/core/src/base_node/comms_interface/local_interface.rs (3 hunks)
• base_layer/core/src/chain_storage/async_db.rs (2 hunks)
• base_layer/core/src/chain_storage/blockchain_backend.rs (2 hunks)
• base_layer/core/src/chain_storage/blockchain_database.rs (11 hunks)
• base_layer/core/src/chain_storage/lmdb_db/lmdb.rs (1 hunks)
• base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (21 hunks)
• base_layer/core/src/chain_storage/mod.rs (1 hunks)
• base_layer/core/src/consensus/consensus_constants.rs (14 hunks)
• base_layer/core/src/consensus/consensus_manager.rs (1 hunks)
• base_layer/core/src/test_helpers/blockchain.rs (3 hunks)
• base_layer/wallet/Cargo.toml (1 hunks)
• base_layer/wallet/src/transaction_service/error.rs (1 hunks)
• base_layer/wallet/src/transaction_service/handle.rs (14 hunks)
• base_layer/wallet/src/transaction_service/service.rs (14 hunks)
• common/config/presets/c_base_node_b_mining_allow_methods.toml (1 hunks)
• common/config/presets/c_base_node_b_non_mining_allow_methods.toml (1 hunks)
• integration_tests/tests/features/Reorgs.feature (1 hunks)
• integration_tests/tests/steps/wallet_steps.rs (1 hunks)

✅ Files skipped from review due to trivial changes (3)

• integration_tests/tests/features/Reorgs.feature
• common/config/presets/c_base_node_b_non_mining_allow_methods.toml
• common/config/presets/c_base_node_b_mining_allow_methods.toml

🚧 Files skipped from review as they are similar to previous changes (18)

• Cargo.toml
• base_layer/wallet/Cargo.toml
• base_layer/core/src/consensus/consensus_manager.rs
• base_layer/core/src/chain_storage/mod.rs
• base_layer/core/src/chain_storage/lmdb_db/lmdb.rs
• applications/minotari_node/src/grpc_method.rs
• applications/minotari_node/src/grpc/readiness_grpc_server.rs
• applications/minotari_console_wallet/src/cli.rs
• base_layer/core/src/base_node/comms_interface/inbound_handlers.rs
• applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
• base_layer/core/src/base_node/comms_interface/local_interface.rs
• base_layer/core/src/base_node/comms_interface/comms_response.rs
• applications/minotari_app_grpc/proto/base_node.proto
• applications/minotari_console_wallet/src/automation/commands.rs
• applications/minotari_app_grpc/proto/wallet.proto
• base_layer/core/src/chain_storage/blockchain_backend.rs
• base_layer/core/src/chain_storage/async_db.rs
• base_layer/core/src/chain_storage/blockchain_database.rs

🧰 Additional context used

🧠 Learnings (8)

📓 Common learnings

Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1655-1658
Timestamp: 2025-05-29T09:40:09.356Z
Learning: In the Tari codebase, node_id hex strings in the database are guaranteed to be valid because they can only be added via `update_peer_sql(peer: Peer)` which converts from valid NodeId objects, ensuring data integrity at the insertion layer.

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

integration_tests/tests/steps/wallet_steps.rs (1)

undefined

<retrieved_learning>
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
</retrieved_learning>

base_layer/core/src/base_node/comms_interface/comms_request.rs (8)

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.

Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.

Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.

Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.

applications/minotari_node/src/grpc/base_node_grpc_server.rs (4)

undefined

<retrieved_learning>
Learnt from: MCozhusheck
PR: #7262
File: applications/minotari_node/src/grpc/base_node_grpc_server.rs:533-536
Timestamp: 2025-06-30T06:54:18.170Z
Learning: In the Tari codebase, readiness status reporting is split between two gRPC servers: base_node_grpc_server's GetNetworkState method always reports READY when it can respond (indicating network-level readiness), while readiness_grpc_server handles granular readiness states like database initialization and migration progress via ReadinessStatusHandler.
</retrieved_learning>

<retrieved_learning>
Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
</retrieved_learning>

<retrieved_learning>
Learnt from: SolfataraEmit
PR: #6994
File: docs/src/API_GRPC_Explanation.md:236-244
Timestamp: 2025-04-28T11:41:15.722Z
Learning: The minotari_console_wallet reports a fourth field timelocked_balance in the terminal output for the getBalance method, which should be reflected in the documentation for accuracy. The wallet.proto file confirms that timelocked_balance is defined as the fourth field in the GetBalanceResponse message.
</retrieved_learning>

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.
</retrieved_learning>

base_layer/wallet/src/transaction_service/handle.rs (4)

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

base_layer/core/src/test_helpers/blockchain.rs (2)

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the `hard_delete_all_stale_peers` method in `comms/core/src/peer_manager/storage/database.rs`, the SQL query intentionally uses exact equality (`peers.features = ?`) rather than bitwise operations (`peers.features & ? != 0`) when matching `COMMUNICATION_NODE` features. This is the intended behavior to match only peers with exactly the `COMMUNICATION_NODE` feature, excluding those with additional feature flags.

Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.

base_layer/wallet/src/transaction_service/service.rs (6)

undefined

<retrieved_learning>
Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
</retrieved_learning>

<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>

<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>

<retrieved_learning>
Learnt from: SWvheerden
PR: #6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.
</retrieved_learning>

<retrieved_learning>
Learnt from: hansieodendaal
PR: #6974
File: base_layer/wallet/src/output_manager_service/service.rs:2026-2031
Timestamp: 2025-04-23T05:56:30.985Z
Learning: When setting the fee parameter in output_to_self method in the Tari wallet, use an accurate fee estimate instead of fee_per_gram. The input_selection.as_final_fee() method provides a good initial estimate, and the final fee can be obtained later from stp.get_fee_amount().
</retrieved_learning>

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (5)

undefined

<retrieved_learning>
Learnt from: hansieodendaal
PR: #6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.
</retrieved_learning>

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7123
File: comms/core/src/peer_manager/storage/database.rs:1517-1541
Timestamp: 2025-05-29T09:42:20.881Z
Learning: In the hard_delete_all_stale_peers method in comms/core/src/peer_manager/storage/database.rs, the SQL query intentionally uses exact equality (peers.features = ?) rather than bitwise operations (peers.features & ? != 0) when matching COMMUNICATION_NODE features. This is the intended behavior to match only peers with exactly the COMMUNICATION_NODE feature, excluding those with additional feature flags.
</retrieved_learning>

<retrieved_learning>
Learnt from: hansieodendaal
PR: #7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: input: Option<InputMinedInfo> and output: Option<OutputMinedInfo>. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.
</retrieved_learning>

<retrieved_learning>
Learnt from: ksrichard
PR: #7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the serde_valid crate rather than at the HTTP handler level. For example, SyncUtxosByBlockRequest validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.
</retrieved_learning>

<retrieved_learning>
Learnt from: martinserts
PR: #7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.
</retrieved_learning>

⏰ Context from checks skipped due to timeout of 90000ms (6)

• GitHub Check: test (testnet, esmeralda)
• GitHub Check: cargo check with stable
• GitHub Check: test (nextnet, nextnet)
• GitHub Check: test (mainnet, stagenet)
• GitHub Check: Cucumber tests / Base Layer
• GitHub Check: Cucumber tests / FFI

🔇 Additional comments (44)

base_layer/core/src/consensus/consensus_constants.rs (8)

111-111: Good type flexibility improvement.

Changing from a fixed-size array to a static slice allows for variable number of entries in permitted_range_proof_types, which facilitates the addition of new sidechain output types without requiring array size modifications.

---

124-130: Well-designed validator node registration limits.

The new fields provide granular control over validator node lifecycle management:

• vn_registration_max_vns_initial_epoch: Controls initial epoch activation limits
• vn_registration_max_vns_per_epoch: Limits ongoing epoch activations
• vn_registration_max_exits_per_epoch: Limits exits per epoch

This design helps maintain network stability during validator node transitions.

---

365-366: Excellent overflow protection.

Using saturating_mul prevents potential integer overflow when calculating block heights from epochs, which is a defensive programming best practice for consensus-critical calculations.

---

368-378: Consistent getter method pattern.

The new getter methods follow the established naming convention and provide appropriate access to the validator node registration limit fields.

---

439-441: Appropriate network-specific configuration values.

The validator node registration limits are configured appropriately:

• Test networks (localnet, igor): Higher limits (50, 10, 5) for testing flexibility
• Production networks (esmeralda, stagenet, nextnet, mainnet): Conservative limits (0, 0, 0) for initial safety

This graduated approach allows testing while maintaining production stability.

Also applies to: 506-508, 564-566, 623-625, 675-677, 754-756

---

796-812: Comprehensive sidechain output type integration.

The addition of new sidechain output types (SidechainCheckpoint, SidechainProof, ValidatorNodeExit) in both current_permitted_range_proof_types and all_range_proof_types functions properly integrates sidechain functionality into the consensus layer. The type signature changes to static slices are consistent with the earlier field modification.

Also applies to: 814-826

---

908-908: Consistent builder method signature.

The builder method signature correctly reflects the type change from fixed array to static slice, maintaining API consistency.

---

1097-1121: Improved test maintainability.

Replacing manual enum variant cycling with calls to OutputType::all() and RangeProofType::all() makes the test more maintainable and ensures comprehensive coverage when new variants are added.

integration_tests/tests/steps/wallet_steps.rs (1)

3199-3199: LGTM: Proper initialization of new sidechain field for integration tests.

The addition of sidechain_deployment_key: vec![] correctly initializes the new field in CreateBurnTransactionRequest. Using an empty vector is appropriate for integration test scenarios where no actual sidechain deployment key is required, ensuring test compatibility with the updated gRPC API structure.

base_layer/wallet/src/transaction_service/error.rs (1)

209-214: LGTM! Well-designed error variants for sidechain functionality.

The new error variants follow established patterns and provide appropriate context for the expanded sidechain and validator node operations introduced in this PR.

base_layer/core/src/base_node/comms_interface/comms_request.rs (1)

81-92: LGTM! Well-designed enum variants for sidechain functionality.

The new and updated validator node variants properly support epoch-based querying and sidechain filtering, aligning with the PR's sidechain integration objectives.

base_layer/core/src/test_helpers/blockchain.rs (2)

424-521: LGTM! Comprehensive validator node method updates align with sidechain architecture.

The extensive updates properly support epoch-based validator node lifecycle management and sidechain filtering. All methods follow the established delegation pattern to the underlying LMDB database and maintain proper error handling.

---

39-43: LGTM! Appropriate imports for sidechain functionality.

The new imports for VnEpoch, ShardGroup, and ValidatorNodeRegistrationInfo properly support the expanded validator node and sidechain features.

Also applies to: 73-73

applications/minotari_node/src/grpc/base_node_grpc_server.rs (7)

45-45: LGTM!

The new imports for VnEpoch and ValidatorNodeRegistrationInfo are correctly added and used in the validator node functionality implemented later in the file.

Also applies to: 69-69

---

181-181: LGTM!

The new gRPC methods GetMempoolStats and GetValidatorNodeChanges are correctly added to the second layer methods list for proper authorization.

Also applies to: 184-184

---

2754-2782: LGTM!

The sidechain ID filtering functionality is correctly implemented with proper validation and the response structure appropriately includes the sidechain ID information.

---

2859-2859: LGTM!

The method call change from registration() to registration_data() aligns with the updated template registration API.

---

2898-2898: LGTM!

Simplifying error handling by removing conditional obscuring in these specific cases improves code consistency and readability.

Also applies to: 2907-2907, 2912-2912

---

2920-2922: LGTM!

The end height calculation fix correctly accounts for the inclusive range by subtracting 1 from the count before addition.

---

2950-2950: LGTM!

The addition of u.is_burned_to_sidechain() to the filter condition correctly captures outputs that are burned to sidechain in addition to sidechain-type outputs.

base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs (1)

1285-1285: No direct assignment change at line 1285 – keep using add_output_data

The add_output_data method on TransactionInput (in core/src/transactions/transaction_components/transaction_input.rs) remains defined and is used throughout the codebase (e.g., in blockchain_database.rs, aggregate_body_chain_validator.rs, etc.). There is no SpentOutput::create_from_output implementation to replace it, and the existing call

input.add_output_data(utxo_mined_info.output);

is correct and should remain.

Likely an incorrect or invalid review comment.

base_layer/wallet/src/transaction_service/service.rs (10)

39-39: LGTM!

The new imports appropriately support the added validator node lifecycle and sidechain functionality.

Also applies to: 68-77, 94-94, 106-106

---

2478-2482: Good validation logic for burn transaction parameters.

The check correctly ensures that a sidechain deployment key cannot be provided without a corresponding claim public key, preventing invalid transaction states.

---

2730-2741: Excellent signature validation for validator node registration.

The implementation properly validates the validator node signature against all critical parameters (sidechain deployment key, claim public key, and max epoch), ensuring only authorized registrations are processed.

---

2782-2789: Proper signature validation for validator node exit.

The exit signature validation correctly ensures that only authorized validator nodes can submit exit requests, maintaining security consistency with the registration process.

---

2859-2896: Excellent security improvement in template registration.

The refactoring moves signature generation inside the service, using proper key derivation and nonce-based signatures. This is a significant security enhancement over accepting external signatures.

---

2913-2923: Good validation of template registration output.

The code properly verifies that the transaction contains a template registration UTXO and provides clear error messaging if it's missing.

---

918-921: Good payment ID generation for template registration.

Using the template name in the payment ID provides excellent traceability for template registration transactions.

---

2483-2487: Correct handling of burn output features.

The implementation properly creates confidential burn outputs with sidechain deployment keys when provided, while maintaining backward compatibility for regular burns.

---

2742-2747: Proper output features for validator node registration.

The code correctly creates validator node registration output features with all required cryptographic and metadata components.

---

2824-2825: LGTM!

The eviction proof output features are correctly created with the necessary proof data and sidechain deployment key.

base_layer/wallet/src/transaction_service/handle.rs (13)

33-33: LGTM!

The addition of VnEpoch import is appropriate for validator node epoch management.

---

55-59: LGTM!

The new imports for MaxSizeString and EvictionProof are correctly added to support sidechain functionality.

---

157-166: New validator node exit functionality looks complete

The new SubmitValidatorNodeExit request variant follows the same pattern as registration and includes all necessary fields for validator node exit.

---

191-197: LGTM! New eviction proof submission request

The SubmitValidatorEvictionProof request variant is well-structured with appropriate fields for submitting eviction proofs.

---

386-402: Display implementations correctly updated

The Display implementations for RegisterValidatorNode and SubmitValidatorNodeExit properly include the new max_epoch field in the output.

---

479-494: Well-formatted display for eviction proof

The Display implementation for SubmitValidatorEvictionProof helpfully shows the node being evicted using proof.node_to_evict().

---

546-552: Response types properly extended for new functionality

The new response variants correctly support the code registration and eviction proof functionality. Note that CodeRegistrationTransactionSent now returns a template_address which is an enhancement over the previous implementation.

---

778-808: Updated validator node registration method looks correct

The method signature has been expanded with all the new required parameters, and they are properly passed through to the service request. The response handling remains consistent.

---

810-838: New validator node exit method implemented correctly

The submit_validator_node_exit method follows the established pattern and includes all necessary parameters for submitting a validator node exit request.

---

840-871: Code template registration properly updated

The method now accepts an optional sidechain_deployment_key parameter and returns both tx_id and template_address. The response handling correctly extracts both values from the new response type.

---

873-895: Eviction proof submission method well implemented

The new submit_validator_eviction_proof method correctly handles the eviction proof submission with appropriate parameters and follows the established error handling pattern.

---

978-1002: Burn Tari method updated with sidechain support

The burn_tari method signature now includes the optional sidechain_deployment_key parameter, maintaining backward compatibility through the Option type.

---

146-156: All RegisterValidatorNode invocations include the new fields

All usages in the codebase have been updated to match the revised variant definition:

• base_layer/wallet/src/transaction_service/handle.rs
• base_layer/wallet/src/transaction_service/service.rs

No further action required.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

base_layer/core/src/base_node/comms_interface/comms_request.rs (1)

144-149: Fix incorrect display text for GetValidatorNode variant.

The display format incorrectly shows "GetShardKey" instead of "GetValidatorNode".

Apply this fix:

-write!(f, "GetShardKey ({:?}), public key ({:?})", sidechain_id, public_key)
+write!(f, "GetValidatorNode ({:?}), public key ({:?})", sidechain_id, public_key)

🧹 Nitpick comments (2)

lints.toml (2)

56-57: Re-enable clippy::cast_sign_loss as at least a warning

Silently permitting signed-to-unsigned casts that drop the sign bit can mask real bugs (e.g. i64 → u64 underflow). If the deny-level noise was too high, consider moving the lint to the new warn section instead of commenting it out altogether:

-#    'clippy::cast_sign_loss'
+# `cast_sign_loss` is useful but noisy — track as a warning rather than disabling
+warn = [
+    "clippy::cast_sign_loss",
+    # existing entries...
+]

---

60-61: Redundant comment line

Line 60 repeats the rationale already implied by keeping clippy::dbg_macro in the deny list. It’s harmless but clutters the config; feel free to drop it.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4973835 and 8924735.

📒 Files selected for processing (4)

• applications/minotari_node/src/grpc_method.rs (6 hunks)
• base_layer/core/src/base_node/comms_interface/comms_request.rs (4 hunks)
• base_layer/wallet/src/transaction_service/handle.rs (14 hunks)
• lints.toml (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• applications/minotari_node/src/grpc_method.rs

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1655-1658
Timestamp: 2025-05-29T09:40:09.356Z
Learning: In the Tari codebase, node_id hex strings in the database are guaranteed to be valid because they can only be added via `update_peer_sql(peer: Peer)` which converts from valid NodeId objects, ensuring data integrity at the insertion layer.

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

base_layer/core/src/base_node/comms_interface/comms_request.rs (6)

Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: base_layer/core/src/base_node/comms_interface/comms_request.rs:146-148
Timestamp: 2025-06-14T04:56:43.247Z
Learning: In the Tari codebase, `HashOutput` is a type alias for `FixedHash`, and `FixedHash` already implements `Display` trait with hex formatting by calling `self.0.to_hex()` internally. This means when formatting `HashOutput` values, there's no need to explicitly call `.to_hex()` as the Display implementation handles hex conversion automatically.

Learnt from: hansieodendaal
PR: tari-project/tari#7216
File: applications/minotari_app_grpc/src/conversions/aggregate_body.rs:31-46
Timestamp: 2025-06-14T04:46:31.966Z
Learning: When a type implements the Copy trait (like FixedHash/BlockHash), it can be used multiple times in iterator closures without being consumed or moved. The compiler automatically copies Copy types, so no explicit .clone() is needed and will trigger clippy::clone_on_copy warnings.

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.

base_layer/wallet/src/transaction_service/handle.rs (4)

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

⏰ Context from checks skipped due to timeout of 90000ms (7)

• GitHub Check: test (mainnet, stagenet)
• GitHub Check: test (nextnet, nextnet)
• GitHub Check: test (testnet, esmeralda)
• GitHub Check: ci
• GitHub Check: cargo check with stable
• GitHub Check: Cucumber tests / FFI
• GitHub Check: Cucumber tests / Base Layer

🔇 Additional comments (13)

lints.toml (2)

73-74: clippy::result_large_err allowed without justification

Allowing this lint means oversized error enums won’t be flagged, which can bloat binaries and heap allocations. If there is a project-wide decision behind this, add a short # reason: comment to preserve intent; otherwise consider downgrading to warn instead of allow.

---

76-80: 👍 New warn list is well-placed

Introducing an intermediate warning level for large_enum_variant (and optionally uninlined_format_args) strikes a good balance between awareness and failing the build. No issues spotted here.

base_layer/wallet/src/transaction_service/handle.rs (7)

33-33: LGTM! Proper imports for sidechain functionality.

The new imports for VnEpoch, MaxSizeString, and EvictionProof are correctly added to support the expanded sidechain validator node functionality.

Also applies to: 45-45, 50-50, 53-53

---

106-106: LGTM! Consistent parameter additions for sidechain support.

The addition of sidechain_deployment_key to BurnTari and the comprehensive parameter additions to RegisterValidatorNode (including validator_node_claim_public_key, sidechain_deployment_key, and max_epoch) properly support the sidechain validator node registration functionality.

Also applies to: 141-147

---

148-157: LGTM! Well-structured validator node exit request.

The new SubmitValidatorNodeExit request variant includes appropriate parameters that mirror the registration process for consistency in the validator node lifecycle management.

---

163-167: LGTM! Enhanced code template registration.

The additions of binary_sha and sidechain_deployment_key to RegisterCodeTemplate properly extend the template registration functionality for sidechain deployments.

---

168-174: LGTM! Comprehensive eviction proof submission.

The new SubmitValidatorEvictionProof request variant includes all necessary parameters for validator eviction functionality with proper sidechain deployment key support.

---

774-799: LGTM! Consistent handle method implementations.

All the new and updated handle methods follow the established patterns:

• Proper parameter passing to request variants
• Consistent error handling with UnexpectedApiResponse
• Appropriate response type matching

The implementation maintains consistency across all validator node lifecycle operations.

Also applies to: 801-829, 837-862, 864-886

---

537-543: LGTM! Well-structured response variants.

The new response variants CodeRegistrationTransactionSent and ValidatorEvictionProofSent provide appropriate return types for the enhanced functionality, with CodeRegistrationTransactionSent properly returning both tx_id and template_address.

base_layer/core/src/base_node/comms_interface/comms_request.rs (4)

29-31: LGTM! Proper import for epoch functionality.

The addition of VnEpoch import is necessary for the new epoch-based validator node change tracking functionality.

---

79-90: LGTM! Well-designed sidechain-aware request variants.

The updates to the request variants are well-structured:

• FetchValidatorNodesKeys properly extended with optional validator_network parameter
• FetchValidatorNodeChanges includes both epoch and optional sidechain ID for comprehensive querying
• GetValidatorNode supports sidechain-scoped validator node retrieval

These changes enable fine-grained validator node management across different sidechains.

---

130-143: LGTM! Clear display implementation for FetchValidatorNodesKeys.

The display implementation properly handles the optional validator_network parameter with clear formatting that shows "None" when the parameter is not provided.

---

171-177: LGTM! Clear display implementation for FetchValidatorNodeChanges.

The display implementation clearly shows both the sidechain ID and epoch parameters, providing good debugging information for validator node change queries.

[coderabbitai]

Actionable comments posted: 1

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8924735 and b22cb68.

📒 Files selected for processing (4)

• base_layer/core/src/base_node/comms_interface/comms_response.rs (4 hunks)
• base_layer/core/src/base_node/comms_interface/local_interface.rs (3 hunks)
• base_layer/core/src/chain_storage/blockchain_backend.rs (2 hunks)
• base_layer/wallet/src/transaction_service/handle.rs (13 hunks)

🚧 Files skipped from review as they are similar to previous changes (2)

• base_layer/core/src/base_node/comms_interface/local_interface.rs
• base_layer/core/src/base_node/comms_interface/comms_response.rs

🧰 Additional context used

🧠 Learnings (3)

📓 Common learnings

Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1655-1658
Timestamp: 2025-05-29T09:40:09.356Z
Learning: In the Tari codebase, node_id hex strings in the database are guaranteed to be valid because they can only be added via `update_peer_sql(peer: Peer)` which converts from valid NodeId objects, ensuring data integrity at the insertion layer.

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

base_layer/core/src/chain_storage/blockchain_backend.rs (1)

Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.

base_layer/wallet/src/transaction_service/handle.rs (4)

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

Learnt from: martinserts
PR: tari-project/tari#7122
File: base_layer/wallet/src/transaction_service/offline_signing/one_sided_signer.rs:0-0
Timestamp: 2025-06-16T14:49:31.831Z
Learning: In Tari's one-sided transaction offline signing implementation, the script_keys vector in the script offset calculation should only include input.output.script_key_id, not output script keys, change output script keys, or recipient script keys. This is by design for the one-sided transaction protocol.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

Learnt from: SWvheerden
PR: tari-project/tari#6951
File: base_layer/core/src/base_node/tari_pulse_service/mod.rs:327-352
Timestamp: 2025-04-16T07:06:53.981Z
Learning: The discovery_peer and dial_peer methods in the Tari codebase have built-in timeout mechanisms, so adding explicit timeouts with tokio::time::timeout is unnecessary.

⏰ Context from checks skipped due to timeout of 90000ms (5)

• GitHub Check: test (nextnet, nextnet)
• GitHub Check: test (mainnet, stagenet)
• GitHub Check: test (testnet, esmeralda)
• GitHub Check: ci
• GitHub Check: cargo check with stable

🔇 Additional comments (18)

base_layer/wallet/src/transaction_service/handle.rs (11)

33-33: LGTM! New imports properly support sidechain functionality.

The new imports for VnEpoch, MaxSizeString, and EvictionProof are correctly added to support the enhanced sidechain and validator node features.

Also applies to: 55-55, 58-58

---

115-115: LGTM! Sidechain deployment key integration is consistent.

The addition of sidechain_deployment_key parameter to the BurnTari request maintains consistency with other sidechain-aware operations.

---

150-166: Enhanced validator node registration with comprehensive parameters.

The updated RegisterValidatorNode request includes essential new fields like validator_node_claim_public_key, sidechain_deployment_key, and max_epoch. The parameter ordering and types appear appropriate for the sidechain validator node lifecycle.

---

157-166: Well-structured validator node exit functionality.

The new SubmitValidatorNodeExit request variant properly mirrors the registration structure with appropriate parameters for the exit process.

---

172-176: Code template registration enhanced for sidechain deployment.

The addition of binary_sha and sidechain_deployment_key parameters extends the template registration to support sidechain deployment scenarios appropriately.

---

177-183: Validator eviction proof submission properly structured.

The new SubmitValidatorEvictionProof request includes all necessary parameters for eviction proof handling with appropriate types.

---

546-552: Response variants correctly added for new operations.

The new response variants CodeRegistrationTransactionSent and ValidatorEvictionProofSent properly support the enhanced functionality with appropriate return data.

---

873-895: LGTM! New eviction proof method properly implemented.

The submit_validator_eviction_proof method follows the established pattern for transaction service methods with appropriate error handling and response matching.

---

985-985: LGTM! Burn method signature consistently updated.

The addition of sidechain_deployment_key parameter maintains consistency with other sidechain-aware operations.

---

783-808: No remaining breaking-change risks for validator node registration

I’ve confirmed that every invocation of register_validator_node has been updated to match the new signature:

• base_layer/wallet/src/transaction_service/service.rs
• applications/minotari_console_wallet/src/grpc/wallet_grpc_server.rs
• applications/minotari_console_wallet/src/automation/commands.rs

All call sites now supply the full set of parameters, so no further changes are required.

---

850-871: All register_code_template callers correctly handle the new tuple return

Both the core service and UI state layers now destructure the (TxId, FixedHash) tuple, so no additional changes are required:

• In base_layer/wallet/src/transaction_service/service.rs, the response is unpacked with
let (tx_id, template_address) = self.register_code_template(...).await?;
• In applications/minotari_console_wallet/src/ui/state/tasks.rs, the result is matched and unpacked with
let (sent_tx_id, _template_addr) = transaction_service_handle.register_code_template(...).await;

All other callers route through the UI’s register_code_template wrapper, which abstracts away the handle return type.

base_layer/core/src/chain_storage/blockchain_backend.rs (7)

6-6: LGTM! New imports properly support enhanced validator node functionality.

The imports for VnEpoch and ShardGroup are correctly added to support the enhanced validator node querying capabilities.

Also applies to: 9-9

---

11-16: LGTM! Import structure updated for new data types.

The updated imports include ValidatorNodeRegistrationInfo and other necessary types for the enhanced validator node functionality.

---

197-201: Well-designed overloaded method with sidechain filtering.

The new fetch_active_validator_nodes method provides optional sidechain filtering while maintaining the same name pattern. The optional parameter design allows for backward compatibility in usage patterns.

---

203-213: LGTM! Epoch-based validator querying methods properly designed.

The new methods fetch_validators_activating_in_epoch and fetch_validators_exiting_in_epoch provide essential functionality for epoch-based validator lifecycle management with consistent parameter patterns.

---

215-227: Well-structured validator existence and activity checks.

The new methods validator_node_exists and validator_node_is_active provide essential validation capabilities with appropriate sidechain and epoch filtering.

---

229-241: Shard group functionality properly integrated.

The methods validator_node_is_active_for_shard_group and validator_nodes_count_for_shard_group provide shard-specific validator management capabilities essential for sidechain operations.

---

243-247: get_validator_node implementations updated correctly

All occurrences of get_validator_node have been updated to the new signature—accepting an Option<CompressedPublicKey>/Option<&CompressedPublicKey> and returning Result<Option<ValidatorNodeRegistrationInfo>, …>. Every implementation (including blockchain_database.rs, lmdb_db.rs, the async backend, test helpers, and all call sites in the base-node comms and gRPC handlers) now passes both the sidechain parameter and the public key. No further changes are required.

[coderabbitai]

💡 Verification agent

🧩 Analysis chain

Breaking change: Method renamed and return type changed.

The method fetch_active_validator_nodes has been renamed to fetch_all_active_validator_nodes and its return type changed from Vec<(CompressedPublicKey, [u8; 32])> to Vec<ValidatorNodeRegistrationInfo>. This is a breaking change that requires updating all implementations.

---

🏁 Script executed:

#!/bin/bash
# Description: Find all implementations of the old method name to ensure they're updated

# Search for implementations that need to be updated
rg -A 5 "fetch_active_validator_nodes" --type rust

Length of output: 4737

---

Update all callers and implementations of fetch_active_validator_nodes → fetch_all_active_validator_nodes

The signature and return type have changed, so every use site must be updated to the new name and parameters:

• base_layer/core/src/base_node/comms_interface/inbound_handlers.rs

• Change
  .fetch_active_validator_nodes(height, validator_network)
  to
  .fetch_all_active_validator_nodes(height)
• Apply any necessary post‐filtering on validator_network if it’s still required in this layer.

• base_layer/core/src/test_helpers/blockchain.rs

• Rename
  fn fetch_active_validator_nodes(...) -> Result<Vec<ValidatorNodeRegistrationInfo>, …>
  to
  fn fetch_all_active_validator_nodes(height: u64) -> …
• Update its internal call and any callers accordingly.

• base_layer/core/src/chain_storage/blockchain_backend.rs

• Rename the trait method
  fn fetch_active_validator_nodes(&self, …) -> Result<…, …>;
  to
  fn fetch_all_active_validator_nodes(&self, height: u64) -> Result<…, …>;

• base_layer/core/src/chain_storage/async_db.rs

• Update the macro invocation
  make_async_fn!(fetch_active_validator_nodes(…) -> …, "fetch_active_validator_nodes");
  to
  make_async_fn!(fetch_all_active_validator_nodes(height: u64) -> …, "fetch_all_active_validator_nodes");

• base_layer/core/src/chain_storage/blockchain_database.rs

• Rename
  pub fn fetch_active_validator_nodes(&self, …) -> …
  to
  pub fn fetch_all_active_validator_nodes(&self, height: u64) -> …

• base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs

• Rename
  fn fetch_active_validator_nodes(&self, …) -> …
  to
  fn fetch_all_active_validator_nodes(&self, height: u64) -> …

Without these updates, callers will break due to the missing method and signature mismatch.

🤖 Prompt for AI Agents

In base_layer/core/src/chain_storage/blockchain_backend.rs around lines 190 to
193, rename the trait method fetch_active_validator_nodes to
fetch_all_active_validator_nodes and update its signature to accept only height:
u64 and return Result<Vec<ValidatorNodeRegistrationInfo>, ChainStorageError>.
Then, update all callers and implementations accordingly: in
base_layer/core/src/base_node/comms_interface/inbound_handlers.rs, replace calls
to fetch_active_validator_nodes(height, validator_network) with
fetch_all_active_validator_nodes(height) and apply any necessary filtering on
validator_network after the call; in
base_layer/core/src/test_helpers/blockchain.rs, rename the function and update
its internals and callers; in base_layer/core/src/chain_storage/async_db.rs,
update the macro invocation to match the new method name and signature; in
base_layer/core/src/chain_storage/blockchain_database.rs and
base_layer/core/src/chain_storage/lmdb_db/lmdb_db.rs, rename the method and
update its signature to match the trait. This ensures all references align with
the new method name and signature to prevent compilation errors.

[hansieodendaal]

System-level tests are going well so far, no (additional) issues found.

[coderabbitai]

Actionable comments posted: 0

♻️ Duplicate comments (1)

base_layer/core/src/chain_storage/blockchain_backend.rs (1)

201-204: Breaking change already identified in previous review.

The method rename from fetch_active_validator_nodes to fetch_all_active_validator_nodes and signature change was already flagged in the previous review as requiring updates to all implementations and callers.

🧹 Nitpick comments (2)

base_layer/core/src/chain_storage/blockchain_backend.rs (2)

206-212: Verify the reintroduction of fetch_active_validator_nodes method name.

While the old method was renamed to fetch_all_active_validator_nodes, a new method with the same name fetch_active_validator_nodes has been introduced with a different signature. This could be confusing for developers and might lead to incorrect usage.

Consider renaming this method to something more descriptive like fetch_active_validator_nodes_for_sidechain to clearly differentiate it from the original method.

---

226-252: Review validator node existence and activity check methods.

The new methods provide comprehensive validator node status checking capabilities. However, there's a potential inconsistency in parameter naming:

• validator_node_exists uses current_epoch: VnEpoch
• validator_node_is_active uses end_epoch: VnEpoch
• validator_node_is_active_for_shard_group uses epoch: VnEpoch
• validator_nodes_count_for_shard_group uses end_epoch: VnEpoch

Consider standardizing the epoch parameter names to improve consistency and clarity about their semantic meaning.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 12047dd and e33bbb9.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (3)

• base_layer/core/src/chain_storage/blockchain_backend.rs (2 hunks)
• base_layer/core/src/chain_storage/blockchain_database.rs (11 hunks)
• base_layer/core/src/consensus/consensus_constants.rs (15 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

• base_layer/core/src/chain_storage/blockchain_database.rs

🧰 Additional context used

🧠 Learnings (2)

📓 Common learnings

Learnt from: hansieodendaal
PR: tari-project/tari#6963
File: comms/core/src/peer_manager/manager.rs:60-68
Timestamp: 2025-05-26T02:40:23.812Z
Learning: PeerDatabaseSql in the Tari codebase has been specifically refactored to handle concurrent access and mitigate blocking I/O concerns on async executor threads. The implementation has been tested under high load at both system level and through unit tests like test_concurrent_add_or_update_and_get_closest_peers which validates concurrent read/write operations.

Learnt from: hansieodendaal
PR: tari-project/tari#7123
File: comms/core/src/peer_manager/storage/database.rs:1655-1658
Timestamp: 2025-05-29T09:40:09.356Z
Learning: In the Tari codebase, node_id hex strings in the database are guaranteed to be valid because they can only be added via `update_peer_sql(peer: Peer)` which converts from valid NodeId objects, ensuring data integrity at the insertion layer.

Learnt from: ksrichard
PR: tari-project/tari#7129
File: applications/minotari_node/src/http/handler/sync_utxos_by_block.rs:21-29
Timestamp: 2025-05-29T20:59:32.020Z
Learning: In the Tari codebase, request validation for RPC services is handled at the service layer using the `serde_valid` crate rather than at the HTTP handler level. For example, `SyncUtxosByBlockRequest` validates that limit is between 1-5 and page is minimum 0. HTTP handlers should delegate validation to the service layer rather than implementing their own validation.

base_layer/core/src/chain_storage/blockchain_backend.rs (1)

Learnt from: hansieodendaal
PR: tari-project/tari#7266
File: base_layer/core/src/chain_storage/blockchain_database.rs:457-461
Timestamp: 2025-06-26T13:18:55.898Z
Learning: The MinedInfo struct in the Tari blockchain database contains two optional fields: `input: Option<InputMinedInfo>` and `output: Option<OutputMinedInfo>`. This design allows methods returning MinedInfo to always return a valid struct rather than wrapping it in Option, with "not found" scenarios represented through the internal optional fields.

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)

• GitHub Check: ci
• GitHub Check: test (testnet, esmeralda)
• GitHub Check: test (mainnet, stagenet)
• GitHub Check: test (nextnet, nextnet)
• GitHub Check: file licenses
• GitHub Check: cargo check with stable

🔇 Additional comments (13)

base_layer/core/src/chain_storage/blockchain_backend.rs (3)

6-6: LGTM: Import additions support new validator node functionality.

The new imports for VnEpoch, ShardGroup, and ValidatorNodeRegistrationInfo are properly added to support the enhanced validator node management capabilities.

Also applies to: 9-9, 16-16

---

214-224: LGTM: Epoch-based validator lifecycle methods.

The new methods fetch_validators_activating_in_epoch and fetch_validators_exiting_in_epoch provide clear functionality for managing validator node lifecycle based on epochs. The signatures are consistent with the optional sidechain public key pattern.

---

254-258: LGTM: Updated validator node getter method.

The get_validator_node method now properly supports optional sidechain public key filtering and returns the rich ValidatorNodeRegistrationInfo struct instead of a raw byte array, which is more type-safe and informative.

base_layer/core/src/consensus/consensus_constants.rs (10)

111-111: LGTM: Improved flexibility for range proof type mappings.

The change from a fixed-size array to a static slice allows for variable-length mappings, which is necessary to support the new sidechain-related output types.

---

124-130: LGTM: Essential validator node governance controls added.

The new fields provide configurable limits for validator node registration and exits, which are crucial for network stability and governance. The u32 type provides sufficient range for reasonable validator counts.

---

365-366: LGTM: Critical safety improvement with saturating arithmetic.

Using saturating_mul prevents integer overflow in epoch-to-height calculations, which could otherwise lead to incorrect block heights and consensus failures. This is an essential safety improvement.

---

368-378: LGTM: Standard getter methods for new validator node fields.

The getter methods follow the established naming convention and provide appropriate read-only access to the new validator node registration limit fields.

---

439-441: LGTM: Appropriate network-specific validator node configurations.

The configuration strategy is sound:

• Test networks (localnet, igor) enable validator node functionality with reasonable limits for testing
• Production networks have validator node features disabled (zeros), allowing for controlled rollout

This graduated approach provides safe testing while preventing premature activation on production networks.

Also applies to: 506-508, 564-566, 644-646, 696-698, 775-777

---

589-589: LGTM: Height-based consensus changes for esmeralda testnet.

Returning multiple consensus constants allows esmeralda to test protocol upgrades at different block heights, which is consistent with the pattern used by other networks like nextnet and mainnet.

---

817-833: LGTM: Proper integration of new sidechain output types.

The addition of SidechainCheckpoint, SidechainProof, and ValidatorNodeExit output types with BulletProofPlus range proofs is consistent with the sidechain feature expansion. The function signature change properly aligns with the struct field modification.

---

835-847: LGTM: Comprehensive range proof type coverage for testing.

The addition of new sidechain output types to the all_range_proof_types function maintains complete test coverage by allowing all range proof types for all output types, which is essential for comprehensive validation testing.

---

929-929: LGTM: Builder method signature consistent with struct changes.

The parameter type change from array to static slice maintains consistency with the struct field modification and other related function signatures.

---

1118-1142: LGTM: Improved test coverage using generated enum variants.

Replacing manual enumeration with OutputType::all() and RangeProofType::all() ensures automatic test coverage for new variants, significantly improving test maintainability and reducing the risk of incomplete coverage when new types are added.