refactor(state_sync)!: sync state transitions by (shard,state_version) - #1543
Conversation
|
Warning Rate limit exceeded@sdbondi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 5 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughRefactors state-sync and storage to a versioned, proof-backed, batched model: adds SubstateUpdateBatch/StateVersionTransitions/StateTreePayload/TreeRootSummary, updates RocksDB schemas, proto/RPC messages, consensus and validator flows, and migrates many APIs and tests to shard+state_version semantics. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant VN as Validator Node
participant RPC as Validator RPC
participant Store as State Store
participant Tree as ShardedStateTree
Note over VN,RPC: Versioned shard-centric state sync
VN->>RPC: SyncStateRequest { shard, start_state_version, until_epoch }
RPC->>Store: state_transitions_get_starting_at(shard, start_state_version, include_values)
Store-->>RPC: StateVersionTransitions { epoch, shard, state_version, updates[] }
RPC->>VN: SyncStateResponse { state_version, updates[], has_more, epoch }
loop chunked pages for same state_version
RPC-->>VN: SyncStateResponse (has_more=true/false)
end
VN->>Store: SubstateRecord::commit_batch(SubstateUpdateBatch{ epoch, updates })
VN->>Tree: calculate_state_root(shard_group)
Tree-->>VN: aggregated_root
VN-->>VN: validate root vs checkpoint
sequenceDiagram
autonumber
participant Cons as Consensus Worker
participant Block as Block model
participant Store as State Store
participant Tree as ShardedStateTree
Note over Cons,Block: Commit block with per-shard versions
Cons->>Tree: calculate_state_root(shard_group)
Tree-->>Cons: aggregated_root
alt no state changes / dummy
Cons->>Block: commit_block_without_state_changes(tx, qc_id)
else state changes present
Cons->>Block: commit_block(tx, qc_id, version_updates{Shard->Version})
Block->>Store: get_substate_updates(tx) -> Vec<SubstateUpdateProof>
Block->>Store: SubstateRecord::commit_batch(SubstateUpdateBatch)
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
Test Results (CI)418 tests +6 394 ✅ +6 1h 17m 56s ⏱️ + 1m 46s For more details on these failures, see this check. Results for commit fe3bf6f. ± Comparison against base commit e943ac1. This pull request removes 1 and adds 7 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
db2aa59 to
6edb4d9
Compare
sdbondi
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
crates/consensus/src/hotstuff/on_propose.rs (1)
486-493: ExtendBlockHeaderto carry per-shard state versionsCurrently in
on_propose.rswe calllet (state_root, _) = calculate_state_merkle_root(…)?; … let mut header = BlockHeader::create_unsigned(…, state_root, …)?;but drop the second return value (the per-shard version summary). The
BlockHeaderstruct and its constructors (create,create_unsigned,genesis) have no field or parameter to accept that summary, so shard-state versions never make it into the header.Required refactors:
- crates/storage/src/consensus_models/block_header.rs
– Add a new field (e.g.shard_tree_summary: IndexMap<Shard, TreeRootSummary>orShardStateVersions) toBlockHeader.
– Updatecreate,create_unsigned, andgenesissignatures to accept this summary.
– Ensurecalculate_id()includes the new field in its hash.- crates/consensus/src/hotstuff/on_propose.rs
– Destructurecalculate_state_merkle_rootinto(state_root, shard_summary)instead of(_, _).
– Passshard_summaryinto the updatedcreate_unsignedcall.- crates/consensus/src/hotstuff/common.rs (if you want reuse)
– Convert the returnedIndexMap<Shard, PendingShardStateTreeDiff>into a serializable summary type.Without these changes, per-shard state versions are never committed in the block header.
utilities/db_inspector/src/webserver/handlers/state_transitions.rs (1)
47-53: Prefix filter bug: using a half-open byte range will over-include results
cf.range_iterator(ordering, key_prefix.as_slice()..)scans all keys ≥ prefix, not only those with the prefix. Use the dedicated prefix iterator to bound the scan correctly.- let iter = if let Some(prefix_hex) = req.query.as_ref() { - let key_prefix = decode_hex_prefix(prefix_hex)?; - cf.range_iterator(ordering, key_prefix.as_slice()..) - } else { - let empty = Vec::<u8>::new(); - cf.range_iterator(ordering, empty.as_slice()..) - }; + let iter = if let Some(prefix_hex) = req.query.as_ref() { + let key_prefix = decode_hex_prefix(prefix_hex)?; + cf.prefix_range_iterator_raw_key(ordering, key_prefix) + } else { + // Full scan + cf.iterator(ordering, OPERATION) + };crates/p2p/proto/consensus.proto (1)
83-96: Do not renumber existing fields in BlockHeader; this is wire-incompatible. Add shard_state_versions at a new tag instead.
Moving epoch_hash from 13→12 and extra_data from 14→13 breaks backward compatibility. To add shard state versions, use an unused tag (9 is free) and leave existing tags as-is. Optionally reserve 12 to avoid future confusion.Apply this diff:
message BlockHeader { bytes parent_id = 1; int32 network = 2; uint64 height = 3; uint64 epoch = 4; uint32 shard_group = 5; bytes proposed_by = 6; bytes state_merkle_root = 7; uint64 total_leader_fee = 8; + // New: shard-state versions committed in this header. Order MUST be canonical: + // [global, shards in ascending order within shard_group]. + ShardStateVersions shard_state_versions = 9; tari.ootle.common.Signature signature = 10; uint64 timestamp = 11; - bytes epoch_hash = 12; - ExtraData extra_data = 13; + // Keep original field numbers to preserve wire compatibility + bytes epoch_hash = 13; + ExtraData extra_data = 14; + // (Optional) Reserve tag 12 to prevent accidental reuse + reserved 12; }Please also update conversions/bridges to populate this field and document the ordering for consensus verification.
crates/p2p/src/conversions/consensus.rs (1)
468-485: BlockHeader shard_state_versions support is incomplete
The proto, Rust model, and conversion logic currently omit the new shard_state_versions field, so versions will never be sent or received.Critical changes needed:
- crates/p2p/proto/consensus.proto:
· Inmessage BlockHeader { … }, add aShardStateVersions shard_state_versions = <next_tag>;- crates/storage/src/consensus_models/block_header.rs:
· Add ashard_state_versions: ShardStateVersionsfield to theBlockHeaderstruct.
· Updatepub fn create(…)andcreate_unsigned(…)signatures to accept aShardStateVersionsparameter.- crates/p2p/src/conversions/consensus.rs:
· Inimpl From<&BlockHeader> for proto::consensus::BlockHeader, set
shard_state_versions: Some(value.shard_state_versions().into()),
· In theTryFrom<proto::consensus::BlockHeader>branch (insidetry_convert_proto_block_header), pass
value.shard_state_versions.map(TryInto::try_into).transpose()?
into theBlockHeader::create(…)call in its correct position.These updates are required before merging to ensure shard state versions are carried end-to-end.
crates/storage/src/consensus_models/substate.rs (1)
419-421: Compile error: incorrect pattern inis_destroy
SubstateUpdateProof::Destroyis a tuple variant. The patternSelf::Destroy { .. }is invalid and won’t compile. UseSelf::Destroy(_).Apply this diff:
- pub fn is_destroy(&self) -> bool { - matches!(self, Self::Destroy { .. }) - } + pub fn is_destroy(&self) -> bool { + matches!(self, Self::Destroy(_)) + }
🧹 Nitpick comments (75)
crates/state_store_rocksdb/src/dbs/transaction.rs (1)
56-60: Cache bypass for multi_get: consider making it conditional or configurableDisabling block cache globally for multi_get can hurt workloads that benefit from cache warming/reuse, especially for smaller batches and hot keys. If you have evidence it helps your specific access patterns, consider:
- Accepting
ReadOptionsfrom the call site for fine-grained control, or- Making
fill_cache(false)conditional on batch size.Given the current signature, a lightweight alternative is to add a separate method (e.g.,
multi_get_cf_no_cache) and use it where profiling shows gains.crates/consensus_tests/src/support/helpers.rs (1)
35-43: Random address generation ignores the end bound; use a uniform sampler over the inclusive rangeCurrent implementation fixes the upper 16 bytes to the range start and randomizes only the lower half. This biases the distribution and ignores
range.end(). Suggest sampling uniformly in[start, end]by working in U256 space and mapping a 256-bit random value into the span.Example refactor:
-// TODO: this biases the start of the shard group -fn random_substate_address_range(range: RangeInclusive<SubstateAddress>) -> SubstateAddress { - let start = range.start(); - let mut bytes = [0u8; 16]; - OsRng.fill_bytes(&mut bytes); - let mut start = start.into_array(); - start[16..32].copy_from_slice(&bytes); - SubstateAddress::from_bytes(&start).unwrap() -} +// Sample uniformly within the inclusive address range +fn random_substate_address_range(range: RangeInclusive<SubstateAddress>) -> SubstateAddress { + let start = range.start().to_u256(); + let end = range.end().to_u256(); + debug_assert!(end >= start, "invalid SubstateAddress range"); + let span = end - start + 1u8.into(); // inclusive + // Rejection-free modulo mapping + let uniform = loop { + let mut r = [0u8; 32]; + OsRng.fill_bytes(&mut r); + // Interpret as big-endian U256 + let x = tari_u256::U256::from_be_bytes(r); + let m = x % span; + break start + m; + }; + let bytes = uniform.to_be_bytes(); + SubstateAddress::from_bytes(&bytes).expect("valid SubstateAddress") +}This uses the full range and removes bias while keeping membership guarantees.
crates/storage/src/consensus_models/substate_update_batch.rs (1)
11-14: Add Debug/Clone derives to ease debugging and testingThese types are passed around in tests and storage layers; deriving Debug (and Clone where appropriate) will help logging, assertions, and simple composition without impacting behavior.
-pub struct SubstateUpdateBatch { +#[derive(Debug, Clone)] +pub struct SubstateUpdateBatch { pub epoch: Epoch, pub updates: IndexMap<Shard, SubstateTransitionData>, } @@ -pub struct SubstateTransitionData { +#[derive(Debug, Clone)] +pub struct SubstateTransitionData { pub state_version: Version, pub transitions: Vec<SubstateTransition>, } -pub enum SubstateTransition { +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SubstateTransition {Also applies to: 42-55
crates/state_store_tests/src/helpers.rs (2)
128-141: Consider passing epoch into build_substate_record to avoid hardcoded Epoch::zero()
created.at_epochis always set to zero here. If callers rely on epoch fidelity (asgen_substatesdoes), consider adding anepoch: Epochparameter or documenting that this helper is only for epoch-agnostic cases.Example signature change (if you choose to generalize):
-pub fn build_substate_record(substate_id: &SubstateId, version: u32, state_version: Version) -> SubstateRecord { +pub fn build_substate_record( + substate_id: &SubstateId, + version: u32, + state_version: Version, + epoch: Epoch, +) -> SubstateRecord { @@ - created: SubstateCreated { - at_epoch: Epoch::zero(), + created: SubstateCreated { + at_epoch: epoch, in_shard: VersionedSubstateIdRef::new(substate_id, version).to_shard(TEST_NUM_PRESHARDS), at_state_version: state_version, },
165-193: Batch construction looks correct; minor note on cloningThe Up transition uses
substate.clone().into_substate_value_or_hash(). In tests this is fine; if this becomes hot-path, consider a zero-copy conversion or moving the value out. As a helper in test code, LGTM as-is.crates/p2p/proto/rpc.proto (2)
210-213: Document TreeRootSummary semanticsThe meaning of
state_versionis non-obvious (checkpoint version? last applied update?). Add a brief comment to lock down semantics.Apply this diff to document intent:
message TreeRootSummary { bytes root_hash = 1; - uint64 state_version = 2; + // The highest committed state version included in this root + uint64 state_version = 2; }
215-219: Prefer using Epoch type and document inclusive/exclusive semantics
until_epochswitches to a raw uint64 while other messages usetari.ootle.common.Epoch. Use the same type for consistency and document whether it’s inclusive.Apply this diff:
message SyncStateRequest { - uint64 start_state_version = 1; - uint32 shard = 2; - uint64 until_epoch = 3; + // Start streaming from this state version (inclusive) + uint64 start_state_version = 1; + uint32 shard = 2; + // Stream updates up to and including this epoch; use Epoch(0)/unset to mean "unbounded" + tari.ootle.common.Epoch until_epoch = 3; }crates/state_tree/src/lib.rs (1)
21-23: LGTM: clear alias for state-tree payloadThe alias is succinct and centralizes the payload type for the state tree.
Optionally re-export
SubstateAddressfor ergonomics in downstream crates so they don’t have to import it separately:// At crate root, alongside other pub uses pub use tari_ootle_common_types::SubstateAddress;crates/state_store_tests/src/blocks.rs (1)
110-111: Prefer deriving shard_group from zero_block to avoid mismatchesYou already consolidated construction via a local
shard_group. For even tighter coupling (and to mirror the pattern in missing_transactions.rs), derive it fromzero_block.shard_group()so changes to the zero block’s preshards cannot drift from the test’s shard group.Apply these diffs:
@@ - let shard_group = ShardGroup::all_shards(NumPreshards::P64); + let shard_group = zero_block.shard_group(); @@ - shard_group, + shard_group, @@ - shard_group, + shard_group,And similarly in block_query_operations:
@@ - let shard_group = ShardGroup::all_shards(NumPreshards::P64); + let shard_group = zero_block.shard_group(); @@ - shard_group, + shard_group, @@ - shard_group, + shard_group, @@ - shard_group, + shard_group,Also applies to: 118-118, 140-140, 241-242, 249-249, 274-274, 306-306
crates/common_types/src/num_preshards.rs (1)
30-31: Document 1-indexed semantics for MAX_SHARDTiny clarity win: make it explicit that shards are 1..=MAX (consistent with all_shards_iter and ShardGroup usage).
Apply this diff:
- pub const MAX_SHARD: Shard = Shard::from_u32(Self::MAX.as_u32()); + /// Highest valid shard identifier for the current maximum presharding (1-indexed). + pub const MAX_SHARD: Shard = Shard::from_u32(Self::MAX.as_u32());applications/tari_validator_node/src/p2p/rpc/block_sync_task.rs (1)
332-333: Nit: error messages don’t match the entity (receipts/transactions vs substates)Adjust the messages to reflect the correct count type for clarity.
Apply this diff:
- self.send(Err(RpcStatus::general("number of substates exceeds u32"))) + self.send(Err(RpcStatus::general("number of transaction receipts exceeds u32"))) .await?;- self.send(Err(RpcStatus::general("number of substates exceeds u32"))) + self.send(Err(RpcStatus::general("number of transactions exceeds u32"))) .await?;Also applies to: 355-356
crates/state_store_rocksdb/src/column_families/state_tree_shard_versions.rs (1)
44-50: Add a brief doc comment to ByShard markerMinor clarity improvement for discoverability in IDEs and docs.
Apply this diff:
-pub struct ByShard; +/// Query marker for shard-scoped lookups on `StateTreeShardVersionCf`. +pub struct ByShard;crates/common_types/src/substate_address.rs (1)
589-594: Duplicate test assertion – remove repetitionThe plus_one(address_at(3, 64)) case appears twice with identical assertions. Please remove the duplicate to keep tests lean.
Apply this diff to remove the duplicate block:
- let group = plus_one(address_at(3, 64)).to_shard_group(NumPreshards::P64, 32); - assert_eq!(group.as_range_inclusive(), Shard::from(3)..=Shard::from(4)); - - let group = plus_one(address_at(3, 64)).to_shard_group(NumPreshards::P64, 32); - assert_eq!(group.as_range_inclusive(), Shard::from(3)..=Shard::from(4)); + let group = plus_one(address_at(3, 64)).to_shard_group(NumPreshards::P64, 32); + assert_eq!(group.as_range_inclusive(), Shard::from(3)..=Shard::from(4));crates/consensus/src/hotstuff/on_receive_local_proposal.rs (2)
484-486: Nit: comment grammar“we should to kick into sync” → “we should kick into sync.”
Apply this diff to fix the comment:
- // If this shard group remains the same, we can just continue. However, if shard groups change and - // we now manage a shard we have not synced, we should to kick into sync. + // If this shard group remains the same, we can just continue. However, if shard groups change and + // we now manage a shard we have not synced, we should kick into sync.
483-495: HotStuffError::NeedsSync correctly triggers a sync in the running state machineVerified: in crates/consensus/src/hotstuff/state_machine/running.rs (lines 43–48), errors matching HotStuffError::NeedsSync are caught and mapped to
Ok(ConsensusStateEvent::NeedSync), which drives the sync logic as intended.Optional improvement:
- Consider emitting a dedicated metric or event alongside the log at this location to monitor shard-group transitions and sync triggers in production.
utilities/db_inspector/src/webserver/handlers/bookkeeping.rs (1)
98-103: Duplicate entry for last_sent_new_viewlast_sent_new_view is added twice in this handler. Remove the duplicate to avoid confusing UI output.
Apply this diff to remove the duplicate:
- add_item( - &tx, - "last_sent_new_view", - column_families::bookkeeping::LastSentNewViewCf, - &mut table, - )?;crates/state_store_tests/src/block_diffs.rs (1)
41-41: Be explicit about state_version type to avoid accidental inference driftPassing a bare
1relies on type inference forstate_version. To guard against future signature or alias changes, consider making the intent explicit (e.g., Version::from(1) or a named constant) if available in scope.crates/common_types/src/committee.rs (1)
318-321: Use VersionedSubstateIdRef for shard resolution — consider a regression testBehavior should be unchanged (version-agnostic shard), but this path is critical. Recommend adding a small unit test that asserts
includes_substate_idreturns identical results for multiple versions of the sameSubstateId, guarding against future refactor regressions into_shard.I can add a focused test under this module to validate version-independence if you’d like.
crates/state_store_tests/src/state_tree.rs (1)
81-89: gen_nodes now returns Node — consider naming clarityImplementation is sound. Minor nit: consider renaming the
versionparameter tostate_versionto disambiguate from “substate version” used elsewhere.crates/common_types/src/shard_group.rs (2)
112-115: Add a doc comment and test for shard_iter_with_globalThe iterator makes sense (global shard first, then group shards). Two suggestions:
- Add a doc comment clarifying order and the guarantee that global (0) won’t duplicate any in-group shard.
- Add a unit test to assert it yields global once followed by all shards in
self.shard_iter()order.Example test sketch (adjust names as needed):
let sg = ShardGroup::new(1, 3); let shards = sg.shard_iter_with_global().collect::<Vec<_>>(); assert_eq!(shards, vec![Shard::global(), Shard::from(1), Shard::from(2), Shard::from(3)]);I can add the test under the existing test module if you prefer.
150-152: Const fn returning RangeInclusive — confirm MSRV compatibilityDeclaring this as
pub const fnis nice, but creatingRangeInclusivein a const context depends on compiler support. If your MSRV doesn’t support this fully, consider droppingconstto avoid build issues.Optional fallback:
-pub const fn as_range_inclusive(&self) -> RangeInclusive<Shard> { +pub fn as_range_inclusive(&self) -> RangeInclusive<Shard> { self.start..=self.end_inclusive }Please confirm this compiles under the repository’s MSRV and toolchain.
crates/state_store_rocksdb/src/range.rs (2)
6-14: Derive Debug/Clone and clarify exclusivity in docsFor a public enum used in APIs, deriving Debug/Clone improves ergonomics in logs/tests, and explicit rustdoc avoids ambiguity around bound inclusivity.
Apply:
-/// A subset of RangeBounds that are possible to query in RocksDB. -pub enum QueryRange<B> { - // start..end - Exclusive { start: B, end: B }, - // start.. - From { start: B }, - // ..end - To { end: B }, -} +/// A subset of RangeBounds that are possible to query in RocksDB. +/// Semantics: +/// - `Exclusive { start, end }` is `start..end` where `start` is inclusive and `end` is exclusive. +/// - `From { start }` is `start..` (inclusive start). +/// - `To { end }` is `..end` (exclusive end). +#[derive(Debug, Clone)] +pub enum QueryRange<B> { + /// start (inclusive) .. end (exclusive) + Exclusive { start: B, end: B }, + /// start (inclusive) .. + From { start: B }, + /// .. end (exclusive) + To { end: B }, +}
16-35: Optional: add conversions for more RangeBounds variants if neededCurrent
Fromimpls coverRange,RangeFrom, andRangeTo. If call sites requireRangeInclusiveorRangeFull(..), consider adding those for parity withRangeBounds. Otherwise, this is sufficient.crates/common_types/src/versioned_substate_id.rs (1)
436-441: Add a doc comment for the ref variant’s to_shardMinor ergonomics: mirror the owned variant’s docs for consistency and discoverability.
- pub fn to_shard(&self, num_preshards: NumPreshards) -> Shard { + /// Calculates and returns the shard for this versioned substate reference. + /// If the underlying substate is global, returns `Shard::global()`. + pub fn to_shard(&self, num_preshards: NumPreshards) -> Shard {crates/state_store_tests/src/misc.rs (1)
169-173: IndexMap usesShardas key
TheEpochCheckpoint::newconstructor expects anIndexMap<Shard, TreeRootSummary>, so usingshard_group.start()(aShard) is correct. To self-document and avoid inference surprises, add an explicit type annotation:• In
crates/state_store_tests/src/misc.rsaround line 169:- let mut shard_summary = IndexMap::new(); + let mut shard_summary: IndexMap<Shard, TreeRootSummary> = IndexMap::new(); shard_summary.insert( shard_group.start(), TreeRootSummary { root_hash: TreeHash::zero(), state_version: 0, }, );crates/common_types/src/shard.rs (1)
49-56: Clarify None semantics and add edge-case tests for relative_to_shard_group_startThe logic correctly returns None when the shard is not in the group and also for the global shard (via contains() or checked_sub underflow). Consider:
- Documenting explicitly that global always returns None.
- Adding unit tests covering:
- shard == group.start() (returns Some(0))
- shard == group.end_inclusive (last index)
- shard outside the group (None)
- global shard (None)
crates/storage/src/consensus_models/substate_change.rs (1)
102-111: into_transition mapping is correct; consider From implementation for ergonomicsThe Up/Down mappings look correct and align with VersionedSubstateId usage for Down. For ergonomics, consider implementing
impl From<SubstateChange> for SubstateTransitionso callers can use.into()directly.crates/consensus/src/hotstuff/on_propose.rs (1)
453-455: Shard derivation for burnt UTXOs: OK; consider naming and magic-number nitUsing VersionedSubstateIdRef to derive the shard is consistent with the broader refactor. Minor:
- Rename
idtoversioned_idfor clarity.- Replace the bare
0with a named constant for initial version to reduce magic numbers.crates/consensus_tests/src/consensus.rs (1)
1403-1416: Template verification across all validators: solid; minor efficiency nitThe verification loop is correct and robust. Minor nit:
hash_template_code(&wasm)is recomputed for each validator; compute once outside the loop and compare to the cached value.- for (addr, vn) in test.validators() { + let expected_hash = hash_template_code(&wasm); + for (addr, vn) in test.validators() { let substate_addr = VersionedSubstateId::new(template_id, 0).to_substate_address(); let template_substate = vn .state_store .with_read_tx(|tx| SubstateRecord::get(tx, &substate_addr)) .unwrap_or_else(|e| panic!("Failed to get template substate from {addr}: {e}")); let binary_hash = template_substate .substate_value .unwrap() .into_template() .expect("Expected template substate") .binary_hash; - assert_eq!(binary_hash, hash_template_code(&wasm), "Template binary does not match"); + assert_eq!(binary_hash, expected_hash, "Template binary does not match"); }crates/storage/src/consensus_models/block_diff.rs (1)
40-49: into_filtered shard_group parameterThe
ShardGroupstruct (twoShardfields) isn’t currently markedCopyand is moved intointo_filtered(self, shard_group: ShardGroup), but at the sole call site you already construct it vialocal_committee_info.shard_group(), so there’s no extra clone or move of an existing instance. Since it’s small (likely 16 bytes), passing by value here is fine.Nit (optional):
- If you’d like to avoid a move when passing an existing
ShardGroup, either
- derive
Copy, CloneonShardGroup(its fields are alreadyCopy), or- change the signature to
&ShardGroup.crates/consensus_tests/src/state_tree.rs (1)
63-82: Transition scan loop is sound; consider early guard to avoid runaway loops in case of DB anomaliesThe loop properly advances next_state_version and breaks on epoch boundary. For test robustness, you could add a sanity cap to iterations to avoid a hang if the DB ever returns repeating versions.
- while let Some(transitions) = tx + let mut scan_iters = 0usize; + while scan_iters < 10_000 && let Some(transitions) = tx .state_transitions_get_after(shard, next_state_version) .optional() .unwrap() { + scan_iters += 1;crates/state_store_rocksdb/src/column_families/state_tree.rs (1)
67-74: Query renamed to ByShardStateVersionQuery with composite key — comment nit and codec orderingThe type Key/KeyCodec changes look correct. The inline comment mentions NodeKeyCodec but the code uses NumberCodec; minor mismatch.
Apply this doc fix:
- // Depends on NodeKeyCodec first serializing the Shard, then the Version. + // Depends on ShardCodec serializing the Shard first, then NumberCodec<Version> for the Version.crates/consensus/src/hotstuff/error.rs (1)
263-275: Nit: fix user-facing error string typos (“an foreign”/“mistmatched”)Minor grammar in error messages that bubble up in logs and APIs.
- #[error( - "Foreign node submitted an foreign proposal {block_id} that did not contain any transaction evidence for this \ - node" - )] + #[error( + "Foreign node submitted a foreign proposal {block_id} that did not contain any transaction evidence for this \ + node" + )] NoTransactionsInCommittee { block_id: BlockId }, - #[error("Foreign node submitted an foreign proposal {block_id} that did not contain a sidechain ID")] + #[error("Foreign node submitted a foreign proposal {block_id} that did not contain a sidechain ID")] MissingSidechainId { block_id: BlockId }, - #[error("Foreign node submitted an foreign proposal {block_id} with an invalid sidechain ID: {reason}")] + #[error("Foreign node submitted a foreign proposal {block_id} with an invalid sidechain ID: {reason}")] InvalidSidechainId { block_id: BlockId, reason: String }, - #[error( - "Foreign node submitted an foreign proposal {block_id} with a mistmatched sidechain ID: expected \ + #[error( + "Foreign node submitted a foreign proposal {block_id} with a mismatched sidechain ID: expected \ {expected_sidechain_id} but got {sidechain_id}" )]crates/state_store_rocksdb/src/cf_api.rs (3)
166-171: Unify key encoding and error messaging in multi_getUse the existing
encode_keyhelper for consistency (it already provides a detailed panic message and centralizes codec use).- let keys = keys.map(|k| { - // We don't support key encoding failing here for mem allocation reasons. Generally, key encoding is - // infallible so we should evaluate whether to change the codec to be infallible. If key encoding on the - // database level ever fails a crash is reasonable. - let key = self.key_codec.encode(k.borrow()).expect("Failed to encode key"); - (self.handle, key) - }); + let keys = keys.map(|k| { + let key = self.encode_key(k.borrow()); + (self.handle, key) + });
224-242: Nit: incorrect operation label in error mappingThis function is
range_iterator, but errors are labeled"range_iterator_with_codecs", which can mislead diagnostics.- res.map_err(|e| RocksDbStorageError::RocksDbError { - operation: "range_iterator_with_codecs", + res.map_err(|e| RocksDbStorageError::RocksDbError { + operation: "range_iterator", source: e, })
456-457: Doc clarity: specify half-open semanticsThe term “exclusive” is ambiguous. Clarify that ranges are half-open [start, end) and that From/To map to [start, ∞)/(-∞, end).
- /// Returns a decoded key value iterator over the range of keys (exclusive). + /// Returns a decoded key-value iterator over a half-open range. For Exclusive, the end is exclusive: [start, end). + /// From is [start, ∞) and To is (-∞, end).utilities/db_inspector/src/webserver/handlers/state_transitions.rs (2)
55-72: Pagination/total mismatch after per-transition expansionYou paginate by KVs (skip/take on the CF iterator) but emit multiple rows per KV. This can inflate page sizes unpredictably and makes
total_entries = cf.count(OPERATION)misleading (it counts KVs, not rows). Consider paginating by transitions or at least reflect total as “KV entries” in the UI.I can propose a lightweight approach to either:
- compute a capped transition count for the current page to honor the requested limit, or
- adjust the UI to clarify totals and page sizes are per KV, not per transition.
63-71: Nit: build row id without intermediate string allocationsMinor readability/alloc nit.
- "id": hex::encode(&encoded_key) + &format!("-{}", i), + "id": format!("{}-{}", hex::encode(&encoded_key), i),crates/storage/src/consensus_models/block_header.rs (1)
193-206: Reuse the local shard_group to avoid driftYou compute
shard_grouplocally, but still inlineShardGroup::all_shards(num_preshards)when constructing the QC. This duplication risks mismatches if the computation ever changes. Reuse the local variable.Apply:
- justify_id: ProposalCertificate::genesis(Epoch::zero(), ShardGroup::all_shards(num_preshards)) + justify_id: ProposalCertificate::genesis(Epoch::zero(), shard_group) .calculate_id(),crates/consensus/src/hotstuff/block_change_set.rs (2)
145-157: Reasonable memory cap handling for block diff vectorClearing and shrinking capacity behind a threshold is a sensible compromise. Consider logging the post-shrink capacity to confirm effectiveness under load, but not required.
448-451: Use accessor consistently for block_idElsewhere you use
self.block.block_id(). For consistency and to avoid potential visibility issues (in case field visibility changes), use the accessor here too.- BlockDiff::insert(tx, &self.block.block_id, &self.local_substate_changes)?; + BlockDiff::insert(tx, self.block.block_id(), &self.local_substate_changes)?;crates/state_store_tests/src/state_transitions.rs (2)
29-37: Minor type-style nit: avoidascast for VersionYou use
1 as Versionhere but rely on type inference for2and3later. Prefer consistent style. IfVersionis a numeric alias, inference is fine; otherwise, wrap/construct via an explicit constructor.- shards.insert( - 1 as Version, + shards.insert( + 1, ( substates.len(), substates.iter().map(|s| s.shard()).collect::<HashSet<_>>(), ), );
29-71: Name shadowing: shards map vs shards setYou use
shardsfor both the outerHashMapand the innerHashSet. Consider renaming the inner binding (e.g.,shard_set) for readability.applications/tari_indexer/src/event_scanner.rs (1)
312-346: Graceful handling of pruned UP substatesWarning on missing values is useful. Two small suggestions:
- Include shard/version context in the warning to aid diagnosis.
- Consider optionally skipping DB insert when value is absent, if your consumers don’t benefit from empty rows.
For richer logs (if feasible in this scope), include shard/version:
- warn!( - target: LOG_TARGET, - "⚠️ Received UP substate {} without value. This indicates that the substate has been pruned. Some event data is not available.", create.substate.as_versioned_substate_id_ref(), - ); + warn!( + target: LOG_TARGET, + "⚠️ Received UP substate {} without value (v{}). Likely pruned; some event data unavailable.", + create.substate.substate_id, create.substate.version + );crates/consensus/src/hotstuff/substate_store/sharded_state_tree.rs (2)
124-131: calculate_state_root: Good addition; minor pre-allocation nit.
To avoid re-allocations, pre-allocate the map since group size is known.Apply this diff:
- let mut shard_state_roots = HashMap::new(); + let mut shard_state_roots = HashMap::with_capacity(shard_group.len() + 1);
183-200: Returning an unordered HashMap may lead to non-determinism; prefer a stable order for shard versions.
Given these versions are destined for the block header, ordering must be canonical. Either:
- Return an ordered map (IndexMap or BTreeMap), or
- Guarantee that the caller sorts by shard before serializing.
I recommend returning IndexMap for deterministic ordering by insertion (e.g., ascending shard).
Apply this diff:
- ) -> Result<HashMap<Shard, Version>, StateTreeError> { + ) -> Result<IndexMap<Shard, Version>, StateTreeError> { @@ - let mut state_versions = HashMap::with_capacity(diffs.len()); + let mut state_versions = IndexMap::with_capacity(diffs.len()); @@ - Ok(state_versions) + Ok(state_versions)And please confirm at call sites that the versions are serialized in a canonical shard order (e.g., global first, then ascending shards within shard_group). If not, I can help patch those.
crates/state_store_tests/src/substates.rs (1)
40-48: State-versioned test records: sanity check.
Both substate1 (v0) and substate1b (v1) are created at the same at_state_version (1). If this is intentional to model multiple substate versions in the same shard-state version, all good. Otherwise, consider setting substate1b's at_state_version to 2 for clearer causality.If desired, I can push a small patch to make the versions strictly monotonic.
crates/common_types/src/shard_state_versions.rs (3)
18-21: Typo in documentation ("forth" -> "fourth")Fix the minor typo in the doc comment.
Apply this diff:
-/// version for shard 1, third is shard 2, and forth is shard 3. +/// version for shard 1, third is shard 2, and fourth is shard 3.
36-38: Misleading expect message in genesis()The message says "Empty vec should always be valid" but the vector is non-empty by construction (len = shard_group.len() + 1).
Apply this diff:
- .expect("Empty vec should always be valid"), + .expect("Non-empty version vec should always be valid"),
93-103: apply_bitmap() should not panic on length mismatchPanicking on user-provided bitmap length makes this API footgunny. Prefer returning a Result and documenting the contract. Also, taking &[bool] avoids an unnecessary allocation and allows callers to reuse buffers.
Suggested signature and early return:
- pub fn apply_bitmap(mut self, bitmap: Vec<bool>) -> Self { - if self.len() != bitmap.len() { - panic!("Length mismatch: expected {} but got {}", self.len(), bitmap.len()); - } + pub fn apply_bitmap(mut self, bitmap: &[bool]) -> Result<Self, BoundedVecOutOfBounds> { + if self.len() != bitmap.len() { + return Err(BoundedVecOutOfBounds::LowerBoundError { expected: self.len(), received: bitmap.len() }); + } let inner_mut: &mut [u64] = self.inner.as_mut(); - for (i, _) in bitmap.into_iter().enumerate().filter(|(_, v)| *v) { + for (i, _) in bitmap.iter().enumerate().filter(|(_, v)| **v) { inner_mut[i] += 1; } - self + Ok(self) }Note: Adjust call sites and tests accordingly.
crates/p2p/src/conversions/consensus.rs (1)
969-986: Substate.created metadata is now required (breaking on older peers)Ok for a breaking change, but make sure all producers (internal/external) set created in proto::consensus::Substate. Otherwise, decoding will fail at runtime.
Consider a transitional period by accepting both legacy fields and new metadata if backward compatibility is needed. Do you want help adding a compatibility decoder?
crates/storage/src/consensus_models/epoch_checkpoint.rs (3)
75-81: get_shard_state_version returns 0 for missing entriesReturning a default Version (0) might be ambiguous vs. "explicitly version 0". Consider returning Option to distinguish absent entries from version zero.
If you want to keep the current API, at least document the "0 means missing" convention in the method doc.
141-149: Length validation checks capacity but not membershipYou guard against "too many" entries but allow out-of-group shard keys. This is acceptable since compute_state_merkle_root only uses group/global, but you may want to warn or reject unknown shards for stricter validation.
I can add a membership check that errors on keys not equal to global or within header shard_group.
176-183: Display label is stale ("count(shard_roots)")Update the label to reflect shard_tree_summary.
Apply this diff:
- "EpochCheckpoint: block_id={}, epoch={}, count(shard_roots)={}", + "EpochCheckpoint: block_id={}, epoch={}, count(shard_tree_summary)={}",crates/consensus/src/hotstuff/substate_store/pending_store.rs (2)
312-313: Stale panic messages after renaming diff -> changesThe expect() messages still say "diff", which is confusing after the rename to changes.
Apply this diff:
- .map(|&pos| self.changes.get(pos).expect("diff and head are not in sync")) + .map(|&pos| self.changes.get(pos).expect("changes and head are not in sync"))- .map(|&pos| self.changes.get(pos).expect("diff and head are not in sync")) + .map(|&pos| self.changes.get(pos).expect("changes and head are not in sync"))- .map(|&pos| self.changes.get_mut(pos).expect("diff and head are not in sync")) + .map(|&pos| self.changes.get_mut(pos).expect("changes and head are not in sync"))- .map(|&pos| self.changes.get(pos).expect("pending map and diff are out of sync")) + .map(|&pos| self.changes.get(pos).expect("pending map and changes are out of sync"))Also applies to: 356-357, 362-363, 700-701
856-859: API shape: return a slice instead of &Vec for changes()Returning &[SubstateChange] is more idiomatic and flexible than &Vec<...>.
Apply this diff and adjust call sites:
- pub fn changes(&self) -> &Vec<SubstateChange> { - &self.changes - } + pub fn changes(&self) -> &[SubstateChange] { + &self.changes + }crates/rpc_state_sync/src/state_sync.rs (1)
123-126: Duplicate not-found branch in checkpoint fetch match.There are two identical Err(RpcError::RequestFailed(err)) if err.is_not_found() arms. The second is dead code and should be removed.
match client .get_checkpoint(GetCheckpointRequest { epoch: prev_epoch.as_u64(), }) .await { Ok(GetCheckpointResponse { checkpoint: Some(checkpoint), }) => match EpochCheckpoint::try_from(checkpoint) { Ok(checkpoint) => { info!(target: LOG_TARGET, "🛜 Checkpoint: {checkpoint}"); self.validate_checkpoint(&checkpoint, prev_committee, prev_epoch)?; self.state_store.with_write_tx(|tx| checkpoint.save(tx))?; self.valid_checkpoints.insert(for_shard_group, checkpoint.clone()); Ok(Some(checkpoint)) }, Err(err) => Err(RpcStateSyncError::InvalidResponse(err)), }, - Err(RpcError::RequestFailed(err)) if err.is_not_found() => Ok(None), Ok(GetCheckpointResponse { checkpoint: None }) => Ok(None), Err(RpcError::RequestFailed(err)) if err.is_not_found() => Ok(None), Err(err) => Err(err.into()), }crates/p2p/src/conversions/rpc.rs (1)
146-153: Minor message wording: “shard roots” -> “shard_tree_summary”.The DoS guard now checks shard_tree_summary, but the error string still says “shard roots”.
- if value.shard_tree_summary.len() > 100_000 { - return Err(anyhow!("too many shard roots (num={})", value.shard_tree_summary.len())); - } + if value.shard_tree_summary.len() > 100_000 { + return Err(anyhow!( + "too many shard_tree_summary entries (num={})", + value.shard_tree_summary.len() + )); + }crates/state_store_rocksdb/src/column_families/state_transition.rs (1)
32-37: Data model LGTM; consider whether state_version duplication is intentionalThe V1 record includes state_version while the CF key is (Shard, Version). If the in-value state_version is only for redundancy/self-describing records, fine. If not strictly required, dropping it would reduce storage and avoid potential inconsistencies.
If you decide to remove duplication:
pub struct StateTransitionModelDataV1 { - pub epoch: Epoch, - pub transitions: Vec<StateTransitionRecordData>, - pub state_version: Version, + pub epoch: Epoch, + pub transitions: Vec<StateTransitionRecordData>, }And adjust writer/reader code that accesses this field accordingly.
crates/storage/src/consensus_models/block.rs (5)
83-87: Unused BlockError variantInvalidShardStateVersions isn’t used in this file. Either wire it into commit path validation or remove it to avoid dead code.
- #[error("Invalid shard state versions: {details}")] - InvalidShardStateVersions { details: String }, + // #[error("Invalid shard state versions: {details}")] + // InvalidShardStateVersions { details: String },Or have commit_block use this variant when shard versions are missing/mismatched.
588-595: Guard commit_block_without_state_changes against accidental diffsThis helper unconditionally calls commit_block with an empty version map. If a diff exists (unexpectedly), commit_block will error due to missing shard versions. Add a defensive check so this path is strictly “no state changes”.
pub fn commit_block_without_state_changes<TTx>(&self, tx: &mut TTx, commit_qc_id: &QcId) -> Result<(), StorageError> where TTx: StateStoreWriteTransaction + Deref, TTx::Target: StateStoreReadTransaction, { - self.commit_block(tx, commit_qc_id, &HashMap::new()) + // Early guard: ensure there are no changes to commit + if self.get_diff(&**tx).optional()?.map_or(false, |d| d.len() > 0) { + return Err(StorageError::QueryError { + reason: format!( + "commit_block_without_state_changes called but block {} has substate changes", + self + ), + }); + } + self.commit_block(tx, commit_qc_id, &HashMap::new()) }Also consider renaming to commit_header_only to better convey behavior.
617-626: Optional: The None-path likely never triggers with RocksDB reader returning empty diffsReader.block_diffs_get returns a BlockDiff even when there are zero changes. The optional()? branch implying NotFound may never occur, making this early return unreachable. Consider simplifying to a length check instead.
- let Some(block_diff) = self.get_diff(&**tx).optional()? else { - info!(target: LOG_TARGET, "🌳 COMMIT block {} with no substate change(s)", self); - // No diff to commit - return Ok(()); - }; + let block_diff = self.get_diff(&**tx)?; + if block_diff.len() == 0 { + info!(target: LOG_TARGET, "🌳 COMMIT block {} with no substate change(s)", self); + return Ok(()); + }
637-681: Validate shard-version coverage up-front and improve error messageCurrently, version lookups happen per change and produce a DataInconsistency error with “NEVER HAPPEN” in the message. Pre-validate once, and use a neutral error message.
- let updates = changes.into_iter() + // Pre-validate that a state version is provided for every shard present in this block’s changes + { + use std::collections::HashSet; + let shards_in_block: HashSet<Shard> = changes.iter().map(|c| c.shard()).collect(); + let missing: Vec<_> = shards_in_block + .iter() + .filter(|s| !version_updates.contains_key(*s)) + .copied() + .collect(); + if !missing.is_empty() { + return Err(StorageError::DataInconsistency { + details: format!( + "Missing shard state version(s) for shard(s) {} in block {}", + missing.iter().map(|s| s.to_string()).collect::<Vec<_>>().join(", "), + self.id() + ), + }); + } + } + + let updates = changes.into_iter() .filter(|change| { if self.shard_group().contains_or_global(&change.shard()) { true } else { // This should have been filtered out already, but just in case warn!( target: LOG_TARGET, "❓️ Skipping substate change {} for shard {} in block {} because it is not in the shard group {}", change.as_change_string(), change.shard(), self.id(), self.shard_group() ); false } }) // Group by shard .try_fold(IndexMap::new(), |mut acc, change| { - let Some(state_version) = - version_updates - .get(&change.shard()) - .copied() else { - // A panic may be more appropriate here, this should never happen - return Err(StorageError::DataInconsistency { - details: format!( - "NEVER HAPPEN: Shard state version for shard {} not found in block {}", - change.shard(), - self.id() - ), - }); - }; + let state_version = *version_updates + .get(&change.shard()) + .expect("prevalidated: state version exists for shard");Remove “NEVER HAPPEN” wording from end-user messages.
789-842: Block sync update proofs are coherent; slight readability tweak possibleThe Up/Down mapping to SubstateUpdateProof looks correct. Minor readability nit: consider renaming local variable substate in the zip loop to substate_rec to avoid confusion with the concept of “substate” elsewhere.
- for substate in substates { + for substate_rec in substates { - if substate.is_destroyed() { + if substate_rec.is_destroyed() { ... - updates.push(SubstateUpdateProof::Destroy(SubstateDestroyedProof { - substate_id: substate.substate_id.clone(), - version: substate.version, + updates.push(SubstateUpdateProof::Destroy(SubstateDestroyedProof { + substate_id: substate_rec.substate_id.clone(), + version: substate_rec.version, })); } else { updates.push(SubstateUpdateProof::Create(SubstateCreatedProof { - substate: substate.into(), + substate: substate_rec.into(), })); }; }crates/state_store_rocksdb/src/reader.rs (4)
1592-1601: “OPERATION” label mismatchThe operation string is “state_transitions_get_n_after” but the method is state_transitions_get_after. Align the label for consistency in logs/metrics.
- const OPERATION: &str = "state_transitions_get_n_after"; + const OPERATION: &str = "state_transitions_get_after";
1620-1643: Avoid shadowing and improve readability in transitions loopThe loop variable data shadows the outer data binding. Rename the inner binding to rec for clarity.
- for (data, substate) in data.transitions.iter().zip(substates) { - let update = match data.transition { + for (rec, substate) in data.transitions.iter().zip(substates) { + let update = match rec.transition { StateTransitionType::Up => { ... - SubstateUpdateProof::Create(SubstateCreatedProof { + SubstateUpdateProof::Create(SubstateCreatedProof { substate: SubstateData { substate_id: substate.substate_id, version: substate.version, value, }, }) }, StateTransitionType::Down => ... }; updates.push(update); }
1660-1671: Convenience helper for version-scoped node reads looks goodThe query and mapping to (NodeKey, Node) is clear. Consider documenting ordering guarantees (if any).
1680-1716: Avoid panics in shard-group version reads; return errors insteadThere are a few expect calls that will panic on unexpected DB state. Prefer returning StorageError to avoid crashing a node on malformed data.
- let index = - ShardStateVersions::shard_to_index(shard_group, sg_shard).expect("sg_shard must be in shard group"); + let Some(index) = ShardStateVersions::shard_to_index(shard_group, sg_shard) else { + return Err(StorageError::DataInconsistency { + details: format!("Shard {sg_shard} not in shard group {shard_group}"), + }); + }; ... - let index = ShardStateVersions::shard_to_index(shard_group, shard) - .expect("BUG: we checked the end of the shard group, so shard must be in shard group"); + let Some(index) = ShardStateVersions::shard_to_index(shard_group, shard) else { + return Err(StorageError::DataInconsistency { + details: format!("Shard {shard} not in shard group {shard_group}"), + }); + }; ... - let shard_tree_versions = - ShardStateVersions::from_vec(shard_tree_versions).expect("BUG: more shard tree versions than shards"); + let shard_tree_versions = ShardStateVersions::from_vec(shard_tree_versions).map_err(|_| { + StorageError::DataInconsistency { + details: "More shard tree versions than shards".to_string(), + } + })?;crates/state_store_rocksdb/src/writer.rs (1)
1197-1264: Batch commit implementation is coherent; minor cleanup and head index consistency
- Good: per-shard batching with a single StateTransitionModelDataV1 write keyed by (Shard, Version).
- Nit: re-fetching HeadIndex CF in the Down branch despite having head_cf already.
- db.cf(substate::HeadIndex)?.put( + head_cf.put( &substate.substate_id, &SubstateHeadData { version: substate.version(), is_up: false, }, OPERATION, - )?; + )?;Also consider skipping StateTransitionCf writes if transitions is empty (defensive), although upstream grouping currently prevents that.
applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs (4)
52-66: End-epoch early stop drops the fetched batch without notifying clientWhen
end_epochis set and the fetchedtransitions.epoch > end_epoch, the stream returnsOk(())without sending a terminal response. If clients interprethas_more == falseas “end of stream,” they won’t receive that signal here and may hang, depending on their expectations.
- If the protocol requires a terminal frame, consider sending a final response (with
has_more: false) indicating the reason (end-epoch reached) before returning.- Alternatively, document that the server may close the stream without a terminal frame once end-of-range is reached.
64-66: Version progression logic is correct but relies on inclusive “get_after” semanticsIncrementing to
transitions.state_version + 1is correct ifget_for_shardreturns the first record withversion >= current_state_version. From the RocksDB reader, this is true. Consider a brief comment to lock in this assumption, since changing the storage query to strictly “after” would otherwise skip versions.Apply this diff to document the inclusive semantics:
- current_state_version = transitions.state_version + 1; + // Note: get_for_shard returns the first record with version >= current_state_version. + // Advance to the next version to avoid re-fetching the same record. + current_state_version = transitions.state_version + 1;
109-131: Chunking looks good; clarifyhas_moresemantics and remove stale commented code
has_moreis set per chunk within a singlestate_version. If clients treathas_more == falseas “no more items in the overall stream,” this will be misleading as further versions may still be streamed. Confirm consumer expectations. If necessary, rename tohas_more_in_versionin proto, or add an overall termination flag in a separate field/message.- The commented block (Lines 110-118) should be removed.
Apply this diff to clean up comments:
- // let updates = transitions.updates.into_iter().map(Into::into).collect(); - // self.send(Ok(SyncStateResponse { - // state_version: transitions.state_version, - // updates, - // has_more: false, - // epoch: Some(transitions.epoch.into()), - // })) - // .await?;
76-76: Minor log tweak for readabilityAdd a space after the emoji for consistency with other logs.
Apply this diff:
- info!(target: LOG_TARGET, "🌍sync complete ({}). {} update(s) sent.", current_state_version, counter); + info!(target: LOG_TARGET, "🌍 sync complete ({}). {} update(s) sent.", current_state_version, counter);crates/storage/src/state_store/mod.rs (1)
593-595: Typo: “Diagonstics” -> “Diagnostics”Small spelling correction in the comment.
Apply this diff:
- // -------------------------------- Diagnotics -------------------------------- // + // -------------------------------- Diagnostics -------------------------------- //crates/storage/src/consensus_models/substate.rs (1)
464-468: Consider adding parityFrom<SubstateDestroyedProof>conversionYou added
From<SubstateCreatedProof> for SubstateUpdateProofbut not the symmetricFrom<SubstateDestroyedProof>. Adding it improves ergonomics.Apply this diff:
impl From<SubstateCreatedProof> for SubstateUpdateProof { fn from(value: SubstateCreatedProof) -> Self { Self::Create(value) } } + +impl From<SubstateDestroyedProof> for SubstateUpdateProof { + fn from(value: SubstateDestroyedProof) -> Self { + Self::Destroy(value) + } +}
| Self::HighestSeenBlock => 10, | ||
| Self::LastSentNewView => 11, | ||
| } |
There was a problem hiding this comment.
Potential on-disk data corruption: key re-numbering without a migration
Reassigning byte keys changes where existing values are stored (orphaning old values and reading wrong ones) for existing databases. Previously, PreviousEpochStateRoot=10, HighestSeenBlock=11, LastSentNewView=12 (per PR context). This change shifts HighestSeenBlock to 10 and LastSentNewView to 11, which will misread prior data unless you provide a migration that re-keys persisted entries or you keep ordinal stability.
Two options:
- Keep ordinal stability (preferred) by leaving gaps and preserving the previous numbers.
- Add an explicit DB migration step that reads old keys and writes them to the new ordinals, bumping
DatabaseMigrationVersion.
If keeping ordinal stability, apply:
- Self::HighestSeenBlock => 10,
- Self::LastSentNewView => 11,
+ // 10 reserved for removed PreviousEpochStateRoot to preserve ordinal stability
+ Self::HighestSeenBlock => 11,
+ Self::LastSentNewView => 12,If opting for a migration, ensure it:
- Iterates all column family entries for keys 11 and 12 (old mapping),
- Moves them to the new ordinals,
- Clears old entries,
- Bumps and checks
DatabaseMigrationVersion.
🤖 Prompt for AI Agents
In crates/state_store_rocksdb/src/column_families/bookkeeping.rs around lines
67-69, the enum ordinal values were changed and that will re-number on-disk keys
and corrupt existing DBs; restore ordinal stability by preserving the previous
numeric assignments (set HighestSeenBlock back to 11 and LastSentNewView back to
12, leaving any gaps used previously) so existing data continues to map to the
same column family keys; if you cannot preserve ordinals, instead implement a DB
migration routine that (1) reads entries from the old ordinals (11 and 12), (2)
writes them to the new ordinals, (3) deletes the old keys, and (4)
increments/checks DatabaseMigrationVersion before and after migration.
6edb4d9 to
3b6bde5
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/p2p/src/conversions/consensus.rs (1)
964-986: Remove legacycreated_*fields across the codebaseThere are still call sites and definitions referencing the old
created_*fields instead of the newcreated: SubstateCreatedMetadataanddestroyed: SubstateDestroyedMetadata. Please update or remove these to align with the updated proto and conversion logic:• utilities/db_inspector/src/webserver/handlers/tables.rs
– Column::new("created_by_shard", …) and Column::new("created_at_epoch", …)
• crates/p2p/src/conversions/rpc.rs:44
– commented‐outcreated_justifyfield
• crates/p2p/proto/rpc.proto:150–151
– legacycreated_justifyin message SubstateCreatedProof
• bindings/src/types/SubstateRecord.ts:13–16
– propertiescreated_justify,created_block,created_by_shard,created_at_epoch
• applications/tari_validator_node/openrpc.json:2388, 2390
–created_justify/destroyed_justifyexamplesThese must be removed or migrated to use the new metadata messages.
♻️ Duplicate comments (9)
crates/state_store_rocksdb/src/column_families/bookkeeping.rs (1)
67-68: Do not renumber on-disk keys: preserve ordinals or add a migrationChanging HighestSeenBlock to 10 and LastSentNewView to 11 will misread existing databases (old 10 was PreviousEpochStateRoot), potentially causing decode errors or silent corruption. Preserve ordinal stability or ship a migration that re-keys persisted entries.
Preferred fix: keep old ordinals and leave 10 reserved.
Apply:
Self::HighTc => 9, - Self::HighestSeenBlock => 10, - Self::LastSentNewView => 11, + // 10 reserved for removed PreviousEpochStateRoot to preserve ordinal stability + Self::HighestSeenBlock => 11, + Self::LastSentNewView => 12,If you must renumber, implement a migration that:
- Reads old keys at 11 (HighestSeenBlock) and 12 (LastSentNewView),
- Writes them to the new ordinals (10 and 11),
- Deletes the old keys,
- Bumps and validates DatabaseMigrationVersion.
I can draft the migration scaffold and tests if you want.
crates/p2p/proto/consensus.proto (1)
105-107: ShardStateVersions is defined but not committed anywhere. Add it to BlockHeader.PR objective is to commit shard state versions in the block header. Defining the message without a field on BlockHeader does not achieve this. Add a field (tag 9 is available) and document the canonical order so all nodes can deterministically reconstruct and compare.
Apply this diff to BlockHeader (tag 9) and add ordering docs:
message ShardStateVersions { - repeated uint64 versions = 1; + // Canonical order: index 0 is the global state/version, followed by versions + // for each shard in ascending Shard ID order (0..=MAX_SHARD). + repeated uint64 versions = 1; } message BlockHeader { bytes parent_id = 1; int32 network = 2; uint64 height = 3; uint64 epoch = 4; uint32 shard_group = 5; bytes proposed_by = 6; bytes state_merkle_root = 7; uint64 total_leader_fee = 8; + // State version commitments for the global tree and all shards (see canonical order above) + ShardStateVersions shard_state_versions = 9; tari.ootle.common.Signature signature = 10; uint64 timestamp = 11; bytes epoch_hash = 13; ExtraData extra_data = 14; }Run this script to find and update conversions/validators to handle the new field:
#!/bin/bash set -euo pipefail # Locate proto-to-Rust conversions that must include shard_state_versions rg -n -C2 -e '\b(BlockHeader|from\(\s*proto::consensus::BlockHeader|\bInto<.*BlockHeader\b)' --type=rust # Find any existing use of ShardStateVersions (should now include references in conversions and consensus) rg -n -C2 'ShardStateVersions|shard_state_versions'crates/common_types/src/shard_state_versions.rs (1)
59-74: Incorrect capacity check in shard_to_index (duplicate of prior review)Comparing shard_group.end() (absolute id) to MAX_SHARDS (capacity) is wrong; it rejects valid groups with large absolute shard ids. Check the group’s length (+1 for global) instead.
Apply:
pub fn shard_to_index(shard_group: ShardGroup, shard: Shard) -> Option<usize> { if shard.is_global() { return Some(0); } if !shard_group.contains_or_global(&shard) { return None; } - shard_group.checked_len()?; - if shard_group.end().as_u32() as usize > MAX_SHARDS { - return None; - } + // Ensure capacity is not exceeded (including the global shard at index 0) + shard_group.checked_len()?; + let group_len = shard_group.len(); + if group_len + 1 > MAX_SHARDS { + return None; + } let index = shard.as_u32().checked_sub(shard_group.start().as_u32())? as usize; // + 1 to account for the global shard at index 0 Some(index + 1) }Recommend adding a test to prevent regression:
#[test] fn shard_to_index_handles_large_absolute_ids() { // Large absolute shard ids but small group size let sg = ShardGroup::new(10_000u32, 10_015u32); // Versions length must be sg.len() + 1 (global) let versions = ShardStateVersions::from_vec(vec![0; sg.len() + 1]).unwrap(); // Global is index 0 assert_eq!(ShardStateVersions::shard_to_index(sg, Shard::global()), Some(0)); // First non-global shard maps to index 1 assert_eq!( ShardStateVersions::shard_to_index(sg, Shard::from(10_000)), Some(1) ); // Last non-global shard maps to index sg.len() assert_eq!( ShardStateVersions::shard_to_index(sg, Shard::from(10_015)), Some(sg.len()) ); }crates/consensus/src/hotstuff/substate_store/pending_store.rs (1)
860-863: repeat_n is not in std; this won’t compile on stableUse a stable alternative to initialize the bitmap.
Apply this diff:
- let shard_group = self.parent_block.shard_group(); - let mut bitmap = iter::repeat_n(false, shard_group.len() + 1).collect::<Vec<_>>(); + let shard_group = self.parent_block.shard_group(); + let mut bitmap = vec![false; shard_group.len() + 1];crates/storage/src/consensus_models/state_transition.rs (1)
18-43: into_chunks has a correctness bug (infinite loop when len == size, reversed order, possible empty chunk).As previously noted, split_off(size) leaves self.updates at length size and returns an empty Vec when len == size, causing an infinite loop. It also pushes tail-first, reversing batch order.
Fix by draining from the front until empty; assert size > 0.
Apply this diff:
impl StateVersionTransitions { - pub fn into_chunks(mut self, size: usize) -> Vec<Self> { - let num_chunks = self.updates.len().div_ceil(size); - let mut chunks = Vec::with_capacity(num_chunks); - loop { - if self.updates.len() < size { - chunks.push(Self { - epoch: self.epoch, - shard: self.shard, - state_version: self.state_version, - updates: self.updates, - }); - break; - } - - let chunk = self.updates.split_off(size); - - chunks.push(Self { - epoch: self.epoch, - shard: self.shard, - state_version: self.state_version, - updates: chunk, - }); - } - chunks - } + pub fn into_chunks(mut self, size: usize) -> Vec<Self> { + assert!(size > 0, "chunk size must be greater than 0"); + let mut chunks = Vec::with_capacity(self.updates.len().div_ceil(size)); + let mut updates = std::mem::take(&mut self.updates); + while !updates.is_empty() { + let take = updates.len().min(size); + let chunk_updates: Vec<_> = updates.drain(..take).collect(); + chunks.push(Self { + epoch: self.epoch, + shard: self.shard, + state_version: self.state_version, + updates: chunk_updates, + }); + } + chunks + }crates/p2p/proto/rpc.proto (3)
221-226: Field name should be lower_snake_case (“epoch”, not “Epoch”).Proto field style requires lower_snake_case.
Apply this diff:
message SyncStateResponse { uint64 state_version = 1; repeated SubstateUpdate updates = 2; bool has_more = 3; - tari.ootle.common.Epoch Epoch = 4; + tari.ootle.common.Epoch epoch = 4; }
102-104: Avoid sentinel “0 means not destroyed”; use presence semantics.Proto3 supports optional. Using 0 conflates “not destroyed” with “destroyed at version 0”.
Apply this diff (if optional is available in your toolchain):
message GetSubstateResponse { bytes address = 1; uint32 version = 2; // Encoded Substate bytes substate = 3; SubstateStatus status = 4; - uint64 created_at_state_version = 5; - // Optional (i.e. 0 if not destroyed) - uint64 destroyed_at_state_version = 6; + // State version at which this substate was created + uint64 created_at_state_version = 5; + // Only present if the substate was destroyed + optional uint64 destroyed_at_state_version = 6; }If optional is not usable, prefer google.protobuf.UInt64Value and import wrappers.proto.
207-208: Wire-incompatible type change on tag 2; use a new field number and reserve 2.Switching the value type on an existing map field breaks compatibility with deployed peers and stored messages.
Apply this diff to preserve backward compatibility:
message EpochCheckpoint { bytes proof = 1; - map<uint32, TreeRootSummary> shard_tree_summary = 2; + // Do not reuse tag 2: previously shard_roots (map<uint32, bytes>) + reserved 2; + // New field for the enriched type + map<uint32, TreeRootSummary> shard_tree_summary = 3; }Coordinate a protocol/version bump and dual-read (old/new) during rollout if backward compatibility must be maintained.
crates/storage/src/consensus_models/substate.rs (1)
45-47: Bindings/JSON-RPC mismatch: SubstateRecord.created and SubstateDestroyed shapes changedRust now exposes created: SubstateCreated and destroyed: Option (with at_epoch/in_shard/at_state_version), removing the old flattened created_* and legacy destroyed fields. TypeScript bindings and JSON-RPC schemas still reference the old fields.
Please update:
- bindings/src/types/SubstateRecord.ts to have created: SubstateCreated; destroyed: SubstateDestroyed | null; and remove created_justify/created_block/created_by_shard/created_at_epoch.
- bindings/src/types/SubstateDestroyed.ts to match { at_epoch: Epoch; at_state_version: number } (or equivalent).
- Any OpenRPC/JSON schemas and consumers referencing the removed fields.
To locate stale references:
#!/bin/bash rg -n -C2 --glob bindings --glob applications '\bcreated_(justify|block|by_shard|at_epoch)\b|SubstateDestroyed\b.*(justify|by_block|by_shard)'Also applies to: 296-321
🧹 Nitpick comments (32)
crates/state_store_rocksdb/src/column_families/bookkeeping.rs (1)
65-66: Naming mismatch: BookKeepingKey::HighQc vs HighPc value typeEnum variant is HighQc, but the CF value type is HighPc (Line 189). This is easy to misread and causes maintenance confusion even if functionally correct.
Two low-risk options:
- Add a clarifying comment near as_byte() for HighQc noting it stores HighPc and the ordinal must not change.
- Or rename the variant to HighPc without changing its ordinal (8) and update references accordingly. If you pick the rename, be careful to keep the ordinal stable.
Example comment:
- Self::HighQc => 8, + // Historical name: key 8 stores HighPc; do not change the ordinal + Self::HighQc => 8,Also applies to: 184-191
crates/storage/src/consensus_models/state_tree_diff.rs (1)
28-30: load duplicates new; delegate to avoid duplication (or remove if unused).Minor cleanup to keep a single initialization path.
Apply this diff:
- pub fn load(version: Version, diff: StateHashTreeDiff<StateTreePayload>) -> Self { - Self { version, diff } - } + pub fn load(version: Version, diff: StateHashTreeDiff<StateTreePayload>) -> Self { + Self::new(version, diff) + }If load is purely semantic sugar for database reads and is unused, consider removing it altogether.
utilities/db_inspector/src/webserver/handlers/state_transitions.rs (3)
60-64: Avoid repeated hex encoding and string concatenation in the inner loopMinor allocation/CPU wins: compute the hex once per key and use format! instead of String + format!.
Apply this diff (included in the pagination refactor above, but safe to apply independently):
- for (i, transition) in data.transitions.into_iter().enumerate() { + let key_hex = hex::encode(&encoded_key); + for (i, transition) in data.transitions.into_iter().enumerate() { let substate = substate_cf.get(&transition.substate_address, OPERATION).optional()?; table.add_row(json!({ - "id": hex::encode(&encoded_key) + &format!("-{}", i), + "id": format!("{}-{}", key_hex, i), "epoch": data.epoch, "shard": shard, "state_version": state_version,
61-61: Reduce N+1 reads: cache substate lookups within the pageEach transition triggers a substate_cf.get. For pages with many transitions, this becomes expensive. A small in-memory cache keyed by substate_address will avoid duplicate reads when transitions reference the same substate.
Apply this diff within the loop and add the following supporting code outside the selected range:
- for (i, transition) in data.transitions.into_iter().enumerate() { + for (i, transition) in data.transitions.into_iter().enumerate() { - let substate = substate_cf.get(&transition.substate_address, OPERATION).optional()?; + let substate = if let Some(v) = substate_cache.get(&transition.substate_address) { + v.clone() + } else { + let v = substate_cf.get(&transition.substate_address, OPERATION).optional()?; + substate_cache.insert(transition.substate_address.clone(), v.clone()); + v + };Support changes required outside the selected range:
// At the top of the file: use std::collections::HashMap; // Just before iterating transitions for the current page (e.g., after computing pagination vars): let mut substate_cache: HashMap<_, Option<_>> = HashMap::new();If RocksDB wrapper supports multi-get, that would be even better, but the cache is a low-effort improvement.
73-75: Verify total_entries semantics: count now reflects keys, not rows (transitions)After expanding one key into multiple rows, cf.count(OPERATION) no longer matches the number of rows emitted. If TableResponse.total_entries is used for pagination, this will mislead clients.
Options:
- Update total_entries to reflect total transitions (requires summing transition counts or maintaining an aggregate counter during writes).
- If computing the true total is too costly, consider leaving total_entries unset/None for this endpoint, or clearly documenting that the total reflects number of keys, not rows.
- Alternatively, expose both totals (keys vs. transitions) if the UI needs both.
Confirm how the frontend uses total_entries and adjust accordingly. I can help draft a small aggregation helper if you maintain per-key transition counts during writes.
crates/consensus_tests/src/state_tree.rs (1)
22-22: Avoid hard-coded /tmp paths; prefer per-test tempdirs.Using a fixed path like “/tmp/test{}” risks collisions and leftover state between runs. Use a tempdir (e.g., tempfile) or the test harness’s per-test directory helper.
Example:
- .with_rocks_path("/tmp/test{}") + .with_temp_rocks_path()If with_temp_rocks_path doesn’t exist, I can help wire it using tempfile.
applications/tari_validator_node/src/p2p/rpc/service_impl.rs (1)
380-405: sync_state now keyed by (shard, start_state_version) with range checks — LGTM.
- Shard range validation against NumPreshards::MAX_SHARD.
- start_state_version > 0 guard prevents ambiguous “from genesis” semantics.
- Optional end_epoch and NonZeroUsize conversion for batch size are correct.
Consider including the peer address in the info log for easier tracing of sync sessions.
applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs (2)
48-67: Run loop semantics look correct; minor wording/log nits.
- Good: advances by one state_version per batch and stops at end_epoch.
- Nits:
- “Fetched … up to v{}” reads like an inclusive range; since each batch corresponds to a single version, “for v{}” is clearer.
- Comment still mentions “id”. Rename to “state_version”.
Apply this diff to clarify logs/comments:
- info!(target: LOG_TARGET, "🌍 Fetched {} state transition(s) up to v{}", transitions.updates.len(), transitions.state_version); + info!(target: LOG_TARGET, "🌍 Fetched {} update(s) for v{}", transitions.updates.len(), transitions.state_version); ... - // "State transition not found with id={current_state_version}" + // "State transitions not found for state_version={current_state_version}" - info!(target: LOG_TARGET, "🌍sync complete ({}). {} update(s) sent.", current_state_version, counter); + info!(target: LOG_TARGET, "🌍 sync complete at v{}. {} update(s) sent.", current_state_version, counter);
110-127: Chunk materialization may be unnecessary; stream as you split.Calling into_chunks(...).len() materializes the full Vec of chunks to compute has_more. For large batches, consider deriving has_more from the iterator (peekable) to avoid allocating all chunks up front.
Example:
- let chunks = transitions.into_chunks(self.batch_size.get()); - let num_chunks = chunks.len(); - for (i, chunk) in chunks.into_iter().enumerate() { + let mut chunks = transitions.into_chunks(self.batch_size.get()).into_iter().peekable(); + while let Some(chunk) = chunks.next() { + let has_more = chunks.peek().is_some(); let updates = chunk.updates.into_iter().map(Into::into).collect(); self.send(Ok(SyncStateResponse { state_version: chunk.state_version, updates, - has_more: i < num_chunks - 1, + has_more, epoch: Some(chunk.epoch.into()), })) .await?; }crates/state_tree/src/tree.rs (1)
70-80: Internal naming nit: next_version vs version.calculate_substate_changes takes next_version, but the inner version variable is renamed. For consistency/readability, consider aligning parameter names across the file.
- current_version: Option<Version>, - next_version: Version, + current_version: Option<Version>, + version: Version,And update call sites within this file accordingly.
crates/common_types/src/lib.rs (1)
70-71: Public API surface expanded: confirm intent and scope of re-exportsRe-exporting everything from shard_state_versions makes all items public at the crate root. If the intent is to only surface the stable types (e.g., ShardStateVersions and its primary error), consider narrowing the export to avoid accidental API growth.
Two suggestions:
- Restrict re-exports to specific items to reduce churn in dependents:
-mod shard_state_versions; -pub use shard_state_versions::*; +mod shard_state_versions; +pub use shard_state_versions::{ShardStateVersions, BoundedVecOutOfBounds};
- Since the PR title is marked breaking (“!”) but the PR description says “Breaking Changes: None”, please reconcile this in the PR notes/CHANGELOG if this public API change is intentional. Do you want me to draft a short CHANGELOG entry?
crates/consensus_tests/src/dummy_blocks.rs (1)
28-28: ShardGroup change: verify range/semantics and keep test invariant explicitYou switched to a shared local shard_group = ShardGroup::new(1, 127) and thread it through. Looks good for consistency, but please double-check that starting at 1 (instead of 0) matches the intended shard space slice for these tests and any JSON fixtures that assume a specific group.
To make the intent explicit and catch regressions, add an assertion right after creating genesis:
assert_eq!(genesis.shard_group(), shard_group);If helpful, I can update the test to include this assertion.
Also applies to: 33-33, 51-51, 66-66
crates/common_types/src/committee.rs (1)
318-321: Use of VersionedSubstateIdRef avoids unnecessary allocation; clarify the “0” sentinelGood change: computing the shard via a borrowed VersionedSubstateIdRef avoids constructing a SubstateAddress.
Small clarity nit: the literal 0 is a sentinel for “version doesn’t affect shard”. Consider a named constant or a variable name change to avoid confusion with SubstateAddress.
Apply rename for readability:
- let addr = VersionedSubstateIdRef::new(substate_id, 0); - let shard = addr.to_shard(self.num_shards); + let vsid = VersionedSubstateIdRef::new(substate_id, 0); + let shard = vsid.to_shard(self.num_shards);If there’s a domain type alias (e.g., StateVersion::GENESIS or similar), using it instead of 0 would be even clearer.
crates/consensus/src/hotstuff/worker.rs (1)
1058-1059: Compute genesis state root only if neededYou compute the shard state root even when the genesis block already exists. This adds unnecessary IO/CPU on startup.
Consider moving the calculate_state_root call after the genesis existence check, computing only when inserting the genesis block. If a pre-check requires a header instance to call exists(), add a separate existence predicate that avoids constructing a full block.
crates/consensus/src/hotstuff/common.rs (1)
263-308: Checkpoint now tracks (root_hash, state_version) per shard; watch duplicate global computationThe overall change to shard_tree_summary is correct. One thing: if shard_group includes the global shard (start == global), the loop will recompute and reinsert the global entry. IndexMap preserves insertion order so this won’t reorder, but it does extra work.
- Skip the global shard in the loop:
- for shard in shard_group.shard_iter() { + for shard in shard_group.shard_iter() { + if shard.is_global() { + continue; + } let Some(version) = tx.state_tree_versions_get_latest(shard)? else { // At v0 there have been no state changes shard_tree_summary.insert(shard, TreeRootSummary { root_hash: SPARSE_MERKLE_PLACEHOLDER_HASH, state_version: 0, }); continue; }; // ... }Alternatively, after fixing ShardGroup::shard_iter_with_global to avoid duplicates, you could simplify by iterating shard_group.shard_iter_with_global() and dropping the special “global first” block (caps stays correct at len()+1).
crates/storage/src/consensus_models/block_header.rs (1)
195-206: zero_block minor refactor reads betterLocal shard_group variable improves readability. The TODO about whether zero block is needed is worth tracking; consider opening an issue to evaluate removal.
I can open an issue to track the “do we need a zero block anymore?” question with pros/cons and migration notes if you’d like.
applications/tari_indexer/src/event_scanner.rs (1)
312-346: Consider wideningversionfield and using compact JSON encoding
- Verified that
NewSubstate.version(and the correspondingversioncolumn in thesubstatestable) is currently defined as an i32/SQLite INTEGER. If there’s any chance a substate’s version could exceed i32::MAX, you’ll need to update both the Rust model and your migrations to use i64/BIGINT.- By default
encode_substateusesserde_json::to_string_pretty, which adds extra whitespace. To reduce storage and I/O overhead, switch to the compact form:- fn encode_substate(substate: &SubstateValue) -> Result<String, anyhow::Error> { - let pretty_json = serde_json::to_string_pretty(&substate)?; - Ok(pretty_json) - } + fn encode_substate(substate: &SubstateValue) -> Result<String, anyhow::Error> { + let json = serde_json::to_string(&substate)?; + Ok(json) + }These changes are optional refactors—no blocker if you’re confident versions will stay within 32-bit bounds.
crates/common_types/src/shard_state_versions.rs (2)
34-39: Avoid panicking in genesis on out-of-bounds shard groupsgenesis constructs a bounded vec and .expect(...)s; this will panic for oversize groups. Either assert with a clear message or return a Result to propagate the bounded-vec error.
Minimal improvement without changing the signature:
- Self { - inner: BoundedVersionVec::try_from(vec![0; shard_group.len() + 1]) - .expect("Empty vec should always be valid"), - } + let len = shard_group.len() + 1; + assert!( + len <= MAX_SHARDS, + "ShardStateVersions::genesis: shard group size ({}) exceeds MAX_SHARDS ({})", + len, + MAX_SHARDS + ); + Self { + inner: BoundedVersionVec::try_from(vec![0; len]) + .expect("INVARIANT: len checked against bounds"), + }If you can change the API, prefer
pub fn try_genesis(...) -> Result<Self, BoundedVecOutOfBounds>.
93-104: apply_bitmap should not panic on length mismatchPanicking in a common types crate is brittle. Consider a non-panicking variant (e.g., try_apply_bitmap) returning Result<Self, Error>.
Example shape:
pub fn try_apply_bitmap(mut self, bitmap: &[bool]) -> Result<Self, &'static str> { if self.len() != bitmap.len() { return Err("Length mismatch"); } for (i, inc) in bitmap.iter().copied().enumerate() { if inc { self.inner.as_mut()[i] += 1; } } Ok(self) }crates/storage/src/consensus_models/epoch_checkpoint.rs (1)
179-183: Nit: log label still says count(shard_roots)Minor naming mismatch in Display; consider updating the label to reflect shard_tree_summary.
Apply this diff:
- "EpochCheckpoint: block_id={}, epoch={}, count(shard_roots)={}", + "EpochCheckpoint: block_id={}, epoch={}, count(shard_tree_summary)={}",crates/consensus/src/hotstuff/substate_store/pending_store.rs (2)
4-4: Remove unused import of iterAfter replacing repeat_n, iter is unused and may trigger warnings or CI failures if unused imports are denied.
Apply this diff:
-use std::{borrow::Cow, collections::HashMap, fmt::Display, iter}; +use std::{borrow::Cow, collections::HashMap, fmt::Display};
307-317: Nit: error messages still reference “diff”Update panic messages to reference “changes” to align with the renamed field and avoid confusion.
Apply this diff:
- .map(|&pos| self.changes.get(pos).expect("diff and head are not in sync")) + .map(|&pos| self.changes.get(pos).expect("changes and head are not in sync"))- .map(|&pos| self.changes.get(pos).expect("diff and head are not in sync")) + .map(|&pos| self.changes.get(pos).expect("changes and head are not in sync"))- .map(|&pos| self.changes.get(pos).expect("pending map and diff are out of sync")) + .map(|&pos| self.changes.get(pos).expect("pending map and changes are out of sync"))Also applies to: 353-363, 697-705
crates/p2p/src/conversions/rpc.rs (1)
151-153: Nit: error text still refers to “shard roots”.This path now handles shard_tree_summary. Align the error message for clarity.
Apply this diff:
- if value.shard_tree_summary.len() > 100_000 { - return Err(anyhow!("too many shard roots (num={})", value.shard_tree_summary.len())); + if value.shard_tree_summary.len() > 100_000 { + return Err(anyhow!( + "too many shard tree summary entries (num={})", + value.shard_tree_summary.len() + ));crates/storage/src/consensus_models/state_transition.rs (1)
49-56: Clarify inclusive semantics of get_for_shard().The RocksDB impl returns the first transition at-or-after the given state_version. The name “get_after” suggests exclusive. Consider documenting or renaming for clarity.
Apply a doc comment:
impl StateTransition { - pub fn get_for_shard<TTx: StateStoreReadTransaction>( + /// Returns the first set of transitions at or after `state_version` for `shard`. + /// If there are no transitions at exactly `state_version`, the next available version is returned. + pub fn get_for_shard<TTx: StateStoreReadTransaction>(crates/storage/src/state_store/mod.rs (1)
278-285: Document include_values semantics on state_transitions_get_after.Reader currently loads full records even when include_values is false (see TODO). Clarify current behavior and intent to avoid surprises.
Proposed doc:
- fn state_transitions_get_after( + /// Returns transitions at or after `state_version` for `shard`. + /// NOTE: For now, values may still be read from storage even if `include_values` is false (implementation detail). + fn state_transitions_get_after(crates/state_store_rocksdb/src/reader.rs (4)
193-193: Nit: fix grammar in doc comment.“in the pending chain; otherwise an empty list is returned.”
Apply this diff:
- /// in the pending chain if not an empty list is returned. + /// in the pending chain; otherwise an empty list is returned.
1598-1598: Nit: OPERATION label out of date.The function is state_transitions_get_after; update the trace/metrics label.
Apply this diff:
- const OPERATION: &str = "state_transitions_get_n_after"; + const OPERATION: &str = "state_transitions_get_after";
1619-1622: Perf: include_values = false still deserializes full substates.The TODO is valid; this adds unnecessary IO/CPU under hash-only syncs. Consider a lighter path that fetches only IDs, versions, and state_hash (e.g., a lean index/CF or selective columns).
Happy to sketch a read path that joins HeadIndex with a slim hash CF to avoid decoding full values in the common case.
1689-1695: Avoid potential overflow when computing the end shard in sg_range.While MAX_SHARDS likely prevents u32::MAX, prefer checked/saturating add to be defensive.
Apply this diff:
- let sg_range = shard_group.start()..Shard::from(shard_group.end().as_u32() + 1); + let end_plus_one = shard_group + .end() + .as_u32() + .checked_add(1) + .expect("shard_group.end() + 1 must not overflow"); + let sg_range = shard_group.start()..Shard::from(end_plus_one);crates/state_store_rocksdb/src/writer.rs (2)
1239-1246: Minor: reuse head_cf instead of reacquiring CF handleThe Down branch reacquires HeadIndex via db.cf(...) despite head_cf being available, adding unnecessary handle lookups.
Apply:
- db.cf(substate::HeadIndex)?.put( - &substate.substate_id, - &SubstateHeadData { - version: substate.version(), - is_up: false, - }, - OPERATION, - )?; + head_cf.put( + &substate.substate_id, + &SubstateHeadData { + version: substate.version(), + is_up: false, + }, + OPERATION, + )?;
1205-1219: Optional: assert shard consistency for created recordsWe rely on the provided shard when recording SubstateCreated.in_shard. Add a debug assertion that the computed substate address (or shard) derived from (id, version) matches shard to catch upstream mismatches early.
If SubstateAddress exposes shard(), consider:
- debug_assert_eq!(address.shard(), shard);
If not, compute from id/version using the same function the rest of the system uses and assert equality.
crates/storage/src/consensus_models/substate.rs (1)
109-114: Nit: fix grammar in comment"cant" -> "can't". Also consider clarifying: “do not trust this value from untrusted sources; validate locally by deriving the shard from (SubstateId, Version) and current NumPreshards.”
- /// WARN: you cant trust this if this is deserialized from an untrusted source, this should be validated by locally - /// calculating which shard this substate falls. + /// WARN: don't trust this if deserialized from an untrusted source. Validate locally by deriving + /// the shard from (SubstateId, Version) and the current NumPreshards.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
utilities/db_inspector/src/webserver/handlers/state_transitions.rs (1)
47-53: Bound prefix iteration in state_transitions.rs
Usingcf.range_iterator(ordering, key_prefix.as_slice()..)with aRangeFromonly sets a lower bound—your loop will continue past the desired prefix and scan the entire CF. To enforce a true prefix‐only scan (and avoid expensive full-table iteration):
- Replace the raw
range_iteratorcall with the prefix-aware API:let iter = if let Some(prefix_hex) = req.query.as_ref() { let key_prefix = decode_hex_prefix(prefix_hex)?; - cf.range_iterator(ordering, key_prefix.as_slice()..) + cf.prefix_range_iterator_raw_key(ordering, key_prefix) } else { let empty = Vec::<u8>::new(); - cf.range_iterator(ordering, empty.as_slice()..) + cf.range_iterator(ordering, empty.as_slice()..) };- If you must stick with
range_iterator, add an earlybreakoncecf.encode_key(&key).starts_with(&key_prefix)fails.This change will ensure you only scan keys matching your prefix rather than the entire column family.
crates/storage/src/consensus_models/substate.rs (1)
216-223: Bug: removing from HashSet uses wrong key type; causes a mismatch.substate_ids is HashSet<VersionedSubstateIdRef<'a>> but remove is called with &SubstateId. This won’t match and will leave entries in “missing”.
Fix by removing with a VersionedSubstateIdRef constructed from the found record:
- for f in &found { - substate_ids.remove(f.substate_id()); - } + for f in &found { + let key = VersionedSubstateIdRef::new(f.substate_id(), f.version()); + substate_ids.remove(&key); + }
♻️ Duplicate comments (10)
crates/p2p/proto/rpc.proto (2)
102-104: Avoid sentinel 0; use presence for destroyed_at_state_version + reserve removed field nameUsing 0 to mean “not destroyed” is ambiguous on the wire and in generated bindings. Prefer proto3
optional(or wrappers) so absence indicates “not destroyed”. Also reserve any previously-removed field names to prevent accidental reuse.Apply this diff:
message GetSubstateResponse { bytes address = 1; uint32 version = 2; // Encoded Substate bytes substate = 3; SubstateStatus status = 4; - uint64 created_at_state_version = 5; - // Optional (i.e. 0 if not destroyed) - uint64 destroyed_at_state_version = 6; + // State version at which this substate was created + uint64 created_at_state_version = 5; + // Optional; only present if the substate was destroyed + optional uint64 destroyed_at_state_version = 6; + // Prevent reuse of previously removed field names + reserved "quorum_certificates"; }If your toolchain doesn’t support
optionalin proto3, replace the field withgoogle.protobuf.UInt64Value destroyed_at_state_version = 6;and addimport "google/protobuf/wrappers.proto";at the top.
205-208: Changing the type on tag 2 is wire-incompatible; move to a new field number and reserve 2Switching
map<uint32, bytes>tomap<uint32, TreeRootSummary>on the same tag breaks compatibility with existing peers and stored messages. Use a new tag and reserve the old one.If a coordinated network upgrade is already enforced (protocol version gating), reply here and we can downgrade this to documentation.
Apply this diff:
message EpochCheckpoint { bytes proof = 1; - map<uint32, TreeRootSummary> shard_tree_summary = 2; + // Reserved to avoid wire-incompatible reuse of the old type on tag 2 + reserved 2; + // New field number for the new map value type + map<uint32, TreeRootSummary> shard_tree_summary = 3; }crates/common_types/src/versioned_substate_id.rs (1)
75-83: Doc and implementation now correctly handle global substatesDocs now state the global-substate behavior and the code short-circuits to
Some(Shard::global()). This resolves the previously raised discrepancy.bindings/src/types/SubstateDestroyed.ts (1)
4-4: BigInt shape change acknowledged; confirm shard derivation is sufficient for consumersSwitching to
{ at_epoch, at_state_version: bigint }matches the new model. Sincein_shardis omitted here (present in SubstateCreated), confirm consumers can derive shard fromsubstate_idconsistently for destruction metadata. Also ensure BigInt handling (serialization/TS target) per the SubstateCreated comment.crates/state_store_tests/src/substates.rs (1)
40-48: Fix: duplicate state_version for successive versions of the same substateBoth v0 and v1 of substate1 are created at state_version=1. This can produce two transitions for the same (shard, state_version) and the same logical substate across versions, which is semantically inconsistent and risks non-deterministic ordering at commit time. Make the later version occur at the next state_version.
- let substate1b = build_substate_record(&substate1_id, 1, 1); + let substate1b = build_substate_record(&substate1_id, 1, 2);crates/consensus_tests/src/support/harness.rs (1)
182-187: Good: created.in_shard now matches computed shardReplacing the hard-coded Shard::first() with the shard derived from the versioned ID fixes metadata inconsistencies flagged earlier.
crates/rpc_state_sync/src/state_sync.rs (1)
242-301: Critical: Stop consuming stream after reaching checkpoint versionAfter validating and matching the checkpoint state root inside the write transaction, the code returns Ok(()) from the closure but continues the outer while loop, which risks over-sync if a peer streams additional updates. Set a flag inside the closure and break the outer loop afterward.
- async fn start_state_sync( + async fn start_state_sync( &mut self, client: &mut ValidatorNodeRpcClient, shard: Shard, checkpoint: &EpochCheckpoint, mut maybe_persisted_state_version: Option<Version>, ) -> Result<(Option<Version>, Vec<TemplateChange>), RpcStateSyncError> { let mut template_changes = vec![]; let checkpoint_shard_root = checkpoint.get_shard_root(shard); let checkpoint_state_version = checkpoint.get_shard_state_version(shard); + let mut reached_checkpoint = false; @@ - self.state_store.with_write_tx(|tx| { + self.state_store.with_write_tx(|tx| { info!( target: LOG_TARGET, "🛜 Next state updates batch of size {} from v{}", updates.len(), state_version ); @@ - if state_version == checkpoint_state_version { + if state_version == checkpoint_state_version { if local_state_root != checkpoint_shard_root { @@ - info!( + info!( target: LOG_TARGET, "🛜 ✅ State root for {shard} matches checkpoint: {local_state_root} (v{state_version})", ); maybe_persisted_state_version = Some(state_version); store.set_state_version(state_version)?; - // Done - return Ok(()); + // Mark to break outer loop after tx commits + reached_checkpoint = true; + return Ok(()); } @@ Ok::<_, RpcStateSyncError>(()) })?; + if reached_checkpoint { + break; + }crates/storage/src/consensus_models/state_transition.rs (1)
20-36: Fix confirmed: chunking now preserves order and avoids empty batches.This resolves the previous split_off-based reversal/empty-chunk issue by draining from the front until empty. Good use of NonZeroUsize to prevent size=0.
crates/state_store_rocksdb/src/writer.rs (1)
1260-1266: Unpruned downed-values index maintenance added — good.Recording (epoch, shard, state_version) -> [addresses] ensures prune sees entries to clear values later. This addresses earlier leak concerns.
crates/storage/src/consensus_models/substate.rs (1)
409-421: LGTM: tuple-variant pattern fix applied for is_destroy().matches!(self, Self::Destroy(_)) is correct for tuple variants.
🧹 Nitpick comments (25)
crates/p2p/proto/rpc.proto (3)
210-213: Add brief docs for TreeRootSummary fieldsClarify what hash and version these represent for easier interop.
message TreeRootSummary { - bytes root_hash = 1; - uint64 state_version = 2; + // Hash of the shard's state commitment (e.g., tree root) at the given state_version + bytes root_hash = 1; + // State version corresponding to root_hash + uint64 state_version = 2; }
215-219: Specify request semantics (inclusive/exclusive, bounds, and defaults)Document whether start_state_version is inclusive, how until_epoch is applied, and any sentinel values or maximums. This reduces client/server ambiguity.
message SyncStateRequest { - uint64 start_state_version = 1; - uint32 shard = 2; - uint64 until_epoch = 3; + // Inclusive start state version to sync from for the given shard + uint64 start_state_version = 1; + // Shard identifier whose transitions are requested + uint32 shard = 2; + // Optional upper bound: fetch updates until (and including?) this epoch. + // Clarify whether 0 means "no upper bound". + uint64 until_epoch = 3; }If
until_epochis optional, consideroptional uint64 until_epoch = 3;(or a wrapper) to avoid sentinel values.
221-226: Clarify state_version meaning in the response and epoch semanticsIt’s unclear if state_version is the last applied version in updates, or the “next cursor” the client should request from. Also confirm whether epoch is the epoch of the last update in this page or a watermark.
message SyncStateResponse { - uint64 state_version = 1; - repeated SubstateUpdate updates = 2; - bool has_more = 3; - tari.ootle.common.Epoch epoch = 4; + // Cursor semantics: last state version included in updates (or next to request from). Specify explicitly. + uint64 state_version = 1; + // Batched substate updates for this shard and range + repeated SubstateUpdate updates = 2; + // True if more updates are available after this page + bool has_more = 3; + // Epoch context for these updates; clarify whether this is the epoch watermark of the batch + tari.ootle.common.Epoch epoch = 4; }crates/state_store_rocksdb/src/column_families/substate.rs (2)
26-26: Alias Version to StateVersion for clarity and consistency with PR semanticsThe PR revolves around shard + state_version transitions. Aliasing the imported Version clarifies intent and avoids ambiguity with other “version” concepts.
Apply:
- use tari_state_tree::Version; + use tari_state_tree::Version as StateVersion;And update usages:
- type Key = (Epoch, Shard, Version); - type KeyCodec = (EpochCodec, ShardCodec, NumberCodec<Version>); + type Key = (Epoch, Shard, StateVersion); + type KeyCodec = (EpochCodec, ShardCodec, NumberCodec<StateVersion>);Also applies to: 77-78
77-78: Validate key order vs shard+state_version sync path; consider reordering or a secondary indexYour PR objective emphasizes syncing by (shard, state_version). With the key ordered as (Epoch, Shard, Version), prefix/range scans by (shard, version) won’t be optimal. If the hot-path requires shard+version iteration, please confirm this CF isn’t on that path. Otherwise, consider reordering or introducing a sibling CF keyed by (Shard, StateVersion[, Epoch]).
Option (illustrative reordering in this file, cascading changes expected elsewhere):
- type Key = (Epoch, Shard, StateVersion); - type KeyCodec = (EpochCodec, ShardCodec, NumberCodec<StateVersion>); + type Key = (Shard, StateVersion, Epoch); + type KeyCodec = (ShardCodec, NumberCodec<StateVersion>, EpochCodec);Additionally, to avoid accidental reuse of existing on-disk data (given schema changes), consider bumping the CF name:
- "substates_unpruned_idx" + "substates_unpruned_idx_v2"utilities/db_inspector/src/webserver/handlers/state_transitions.rs (1)
60-84: Avoid needless per-transition work after the page is filled; bulk-skip within entriesCurrent logic continues iterating and performing substate lookups after the page is full to compute an exact total. You can keep exact totals while significantly reducing CPU/IO by:
- Counting transitions per key up-front.
- Bulk-skipping transitions within a key when still below row_skip (no per-transition loop).
- Skipping substate lookups entirely once emitted >= row_limit (but continue counting using len).
Apply this diff to make the hot path cheaper for large datasets and big page offsets:
- for result in iter { - let ((shard, state_version), data) = result?; - let encoded_key = cf.encode_key(&(shard, state_version)); - let key_hex = hex::encode(&encoded_key); - for (i, transition) in data.transitions.into_iter().enumerate() { - if skipped < row_skip { - skipped += 1; - continue; - } - if emitted < row_limit { - let substate = substate_cf.get(&transition.substate_address, OPERATION).optional()?; - table.add_row(json!({ - "id": format!("{}-{}", key_hex, i), - "epoch": data.epoch, - "shard": shard, - "state_version": state_version, - "substate_id": substate.as_ref().map(|s| s.substate_id()), - "version": substate.as_ref().map(|s| s.version()), - "transition": transition.transition, - })); - emitted += 1; - } - - count += 1; - } - } + for result in iter { + let ((shard, state_version), data) = result?; + let trans_len = data.transitions.len(); + count += trans_len; + + // Bulk skip within this key if we still have rows to skip. + let mut start_i = 0usize; + if skipped < row_skip { + let to_skip = (row_skip - skipped).min(trans_len); + skipped += to_skip; + start_i = to_skip; + } + + // If page is already full, avoid any further per-transition work on this key + if emitted >= row_limit { + continue; + } + + let encoded_key = cf.encode_key(&(shard, state_version)); + let key_hex = hex::encode(&encoded_key); + for (i, transition) in data.transitions.into_iter().enumerate().skip(start_i) { + if emitted >= row_limit { + break; + } + let substate = substate_cf.get(&transition.substate_address, OPERATION).optional()?; + table.add_row(json!({ + "id": format!("{}-{}", key_hex, i), + "epoch": data.epoch, + "shard": shard, + "state_version": state_version, + "substate_id": substate.as_ref().map(|s| s.substate_id()), + "version": substate.as_ref().map(|s| s.version()), + "transition": transition.transition, + })); + emitted += 1; + } + }bindings/src/types/ShardStateVersions.ts (2)
3-10: Fix minor JSDoc typo and tighten the example wordingTypo: “forth” -> “fourth”. Also consider making the shard indexing language a touch clearer.
Apply upstream (in the Rust doc that generates this) so it regenerates here:
- * version for shard 1, third is shard 2, and forth is shard 3. + * version for shard 1, the third is shard 2, and the fourth is shard 3.
11-11: uint64 state versions mapped to TSnumber[]risk precision loss beyond 2^53-1Proto and Rust indicate these are 64-bit integers. TS
numbercannot exactly represent all uint64 values. If state_version can grow large, precision errors will silently corrupt client-side logic.Recommendations (adjust in the Rust ts-rs annotations so this file regenerates):
- Prefer bigint for exact integers in TS:
ts(type = "{ inner: bigint[] }")(note: JSON serialization of bigint needs care).- Or use strings for maximal compatibility:
ts(type = "{ inner: string[] }").Illustrative TS diff (generated output), if choosing bigint:
-export type ShardStateVersions = { inner: number[] }; +export type ShardStateVersions = { inner: bigint[] };If choosing string:
-export type ShardStateVersions = { inner: number[] }; +export type ShardStateVersions = { inner: string[] };Would you like me to point to the exact Rust location and attribute needed for ts-rs?
crates/common_types/src/versioned_substate_id.rs (1)
437-442: NewVersionedSubstateIdRef::to_shardmirrors owned variant; add a short doc commentFunctionality looks correct and consistent with the owned type. Consider adding a brief doc comment mirroring the global-substate note for discoverability.
Suggested doc (apply above the method):
+ /// Returns the shard for this versioned substate id. + /// If the substate is global, returns `Shard::global()` regardless of version. + /// Otherwise, computes the shard from the address using the given `NumPreshards`. pub fn to_shard(&self, num_preshards: NumPreshards) -> Shard {applications/tari_validator_node/src/state_bootstrap.rs (2)
47-48: Prefer symbolic zero if available for consistencyYou use
Epoch::zero()below; ifVersionprovides azero()as well, prefer that to keep style consistent. If not, this is fine as-is.Example:
-const INITIAL_STATE_VERSION: Version = 0; +const INITIAL_STATE_VERSION: Version = Version::zero();
176-203: Each create_substate commits separately; consider batching for atomicity and fewer writesRight now, every substate creation creates a fresh
SubstateUpdateBatchand commits it. Batching all initial creations into a single batch per epoch/state_version reduces I/O, ensures atomic bootstrap, and guarantees a consistent state_version across the set.One approach: accumulate transitions and commit once in
bootstrap_state. For minimal impact, introduce a helper that pushes into a provided batch:-fn create_substate<TTx, TId, TVal>( +fn create_substate<TTx, TId, TVal>( tx: &mut TTx, num_preshards: NumPreshards, substate_id: TId, value: TVal, ) -> Result<(), StorageError> where @@ - let substate_id = substate_id.into(); - let shard = VersionedSubstateIdRef::new(&substate_id, 0).to_shard(num_preshards); - let mut batch = SubstateUpdateBatch::new(Epoch::zero()); - batch - .with_transition(shard, INITIAL_STATE_VERSION) - .push(SubstateTransition::Up { - id: substate_id, - version: 0, - substate_or_hash: value.into().into(), - }); - - SubstateRecord::commit_batch(tx, batch)?; + let substate_id = substate_id.into(); + let shard = VersionedSubstateIdRef::new(&substate_id, 0).to_shard(num_preshards); + let mut batch = SubstateUpdateBatch::new(Epoch::zero()); + batch + .with_transition(shard, INITIAL_STATE_VERSION) + .push(SubstateTransition::Up { + id: substate_id, + version: 0, + substate_or_hash: value.into().into(), + }); + SubstateRecord::commit_batch(tx, batch)?;Alternatively, I can provide a patch to:
- create a single
SubstateUpdateBatchinbootstrap_state,- change
create_*helpers to push into that batch, and- commit once at the end.
Want me to draft that full refactor?
applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs (1)
88-97: Fetching values unconditionally may be wastefulYou always set
include_values = true. If some clients only need hashes/metadata, this increases payload size and latency.Consider:
- adding an
include_values: boolfield toStateSyncTask,- wiring it via
new(...), and- passing it through here instead of hard-coding
true.bindings/src/types/BlockHeader.ts (1)
41-41: Type aligns with bindings; specify encoding in docs
RistrettoPublicKeyBytesis astringalias. Consider clarifying expected encoding (hex, base64, etc.) in the upstream Rust docs so the generated TS includes it. This helps clients avoid mismatches.Example (upstream Rust doc):
/// The public key of the proposer as hex-encoded bytes.crates/consensus_tests/src/substate_store.rs (1)
217-226: Clarify sentinelstate_version = 0and avoid naming ambiguity
- I confirmed that
SubstateUpdateBatch::with_transition(&mut self, shard, state_version: Version)
does not enforcestate_version > 0, so using0as a test‐only sentinel is valid.- To improve readability and prevent confusing the batch’s
state_versionwith the substate’s ownversionfield:
• Add a brief comment where you pass0to indicate it’s a test sentinel.
• Optionally rename the localversionparameter tosubstate_version.Example refactor in
crates/consensus_tests/src/substate_store.rs:- fn add_substate(store: &TestStore, seed: u8, version: u32) -> VersionedSubstateId { + fn add_substate(store: &TestStore, seed: u8, substate_version: u32) -> VersionedSubstateId { let id = new_substate_id(seed); let value = new_substate_value(seed); let mut batch = SubstateUpdateBatch::new(Epoch::zero()); - batch.with_transition(Shard::first(), 0).push(SubstateTransition::Up { - id: id.clone(), - version, - substate_or_hash: value.into(), - }); + // Use 0 as a sentinel state_version in tests + batch + .with_transition(Shard::first(), 0) + .push(SubstateTransition::Up { + id: id.clone(), + version: substate_version, + substate_or_hash: value.into(), + }); store .with_write_tx(|tx| SubstateRecord::commit_batch(tx, batch)) .unwrap();crates/storage/src/consensus_models/substate_update_batch.rs (2)
11-14: Expose batch structure is fine; consider deriving Debug/Clone for ergonomicsPublic fields and nested map model by (shard, state_version) look good for the new design. Deriving Debug/Clone improves testability and diagnostics.
-pub struct SubstateUpdateBatch { +#[derive(Debug, Clone)] +pub struct SubstateUpdateBatch { pub epoch: Epoch, pub updates: IndexMap<Shard, IndexMap<Version, Vec<SubstateTransition>>>, }
29-38: Disambiguate “version” field to avoid confusion with state_versionThis “version” is the per-substate version, not the global state_version. Adding a short doc comment avoids future misreads.
-pub enum SubstateTransition { +#[derive(Debug, Clone)] +pub enum SubstateTransition { Up { - id: SubstateId, - version: u32, + id: SubstateId, + /// Per-substate version (NOT the global state_version) + version: u32, substate_or_hash: SubstateValueOrHash, }, Down { id: VersionedSubstateId, }, }crates/consensus_tests/src/support/harness.rs (1)
192-200: Minor: reuse substate.shard() instead of recomputingYou can avoid recomputing the shard each time by using SubstateRecord::shard(), which reads created.in_shard and guarantees consistency with record metadata.
- for substate in &substates { - let shard = substate.to_versioned_substate_id().to_shard(TEST_NUM_PRESHARDS); - if v.shard_group.contains(&shard) { - batch - .with_transition(shard, substate.created().at_state_version) - .push(substate.clone().into_transition()); - } - } + for substate in &substates { + let shard = substate.shard(); + if v.shard_group.contains(&shard) { + batch + .with_transition(shard, substate.created().at_state_version) + .push(substate.clone().into_transition()); + } + }crates/state_store_tests/src/helpers.rs (1)
165-194: Use substate.shard() for consistency and readabilitySlight simplification and avoids repeating shard derivation logic. This prevents future drift between computed shard and record metadata.
- batch - .with_transition( - substate.to_versioned_substate_id().to_shard(TEST_NUM_PRESHARDS), - destroyed.at_state_version, - ) + batch + .with_transition(substate.shard(), destroyed.at_state_version) .push(tari_ootle_storage::consensus_models::SubstateTransition::Down { id: VersionedSubstateId::new(substate.substate_id.clone(), substate.version), }); @@ - batch - .with_transition( - substate.to_versioned_substate_id().to_shard(TEST_NUM_PRESHARDS), - substate.created().at_state_version, - ) + batch + .with_transition(substate.shard(), substate.created().at_state_version) .push(tari_ootle_storage::consensus_models::SubstateTransition::Up { id: substate.substate_id.clone(), version: substate.version, substate_or_hash: substate.clone().into_substate_value_or_hash(), });crates/rpc_state_sync/src/state_sync.rs (2)
122-126: Remove duplicate not_found match armThe RequestFailed(is_not_found) case appears twice. The second is unreachable and will trigger an “unreachable pattern” warning.
- Err(RpcError::RequestFailed(err)) if err.is_not_found() => Ok(None), - Ok(GetCheckpointResponse { checkpoint: None }) => Ok(None), - Err(RpcError::RequestFailed(err)) if err.is_not_found() => Ok(None), + Err(RpcError::RequestFailed(err)) if err.is_not_found() => Ok(None), + Ok(GetCheckpointResponse { checkpoint: None }) => Ok(None),
640-709: Template extraction logic is reasonableCreate path handles Template adds; destroy path logs deprecations (currently not possible) without side-effects. Consider auditing when/if template DOWNs are introduced.
If/when template DOWNs are allowed, ensure we gate TemplateChange::Deprecate generation behind a protocol version and add validation that the “destroy” proof indeed refers to a template.
crates/storage/src/consensus_models/state_transition.rs (2)
20-36: Optional: avoid O(n^2) memmoves from repeated drain(..take).Draining from the front on a Vec repeatedly causes shifting of the tail each iteration. For large batches, consider VecDeque to keep this O(n).
Apply along these lines:
- use std::num::NonZeroUsize; + use std::{collections::VecDeque, num::NonZeroUsize}; impl StateVersionTransitions { pub fn into_chunks(self, size: NonZeroUsize) -> Vec<Self> { - let num_chunks = self.updates.len().div_ceil(size.get()); - let mut chunks = Vec::with_capacity(num_chunks); - let mut updates = self.updates; - while !updates.is_empty() { - let take = updates.len().min(size.get()); - let chunk_updates: Vec<_> = updates.drain(..take).collect(); + let size = size.get(); + let mut chunks = Vec::with_capacity(self.updates.len().div_ceil(size)); + let mut updates: VecDeque<_> = self.updates.into(); + while !updates.is_empty() { + let take = updates.len().min(size); + let mut chunk_updates = Vec::with_capacity(take); + for _ in 0..take { + chunk_updates.push(updates.pop_front().expect("deque not empty")); + } chunks.push(Self { epoch: self.epoch, shard: self.shard, state_version: self.state_version, updates: chunk_updates, }); } chunks } }
41-49: Clarify inclusive semantics ofget_afterTests confirm that
state_transitions_get_afteris actually inclusive—it returns the transition at exactly the givenstate_versionif present. To avoid confusion, please:• Add a doc comment above
StateStoreReadTransaction::state_transitions_get_afterin
crates/storage/src/state_store/mod.rsnoting that it returns transitions at or after the specified version (inclusive).
• Add a matching doc comment aboveStateTransition::get_for_shardin
crates/storage/src/consensus_models/state_transition.rs.Optionally, if you want to improve API discoverability (at the cost of a breaking rename), consider renaming the method to
state_transitions_get_at_or_after(and the wrapper toget_for_shard_at_or_after) in a follow-up.crates/state_store_rocksdb/src/writer.rs (2)
1282-1301: Prune routine is correct but counts index entries, not values.
- Clears values for each downed substate address and removes the index key.
- Log message reports number of index entries pruned (count), not number of substates. If useful, consider summing addresses.len() for more granular metrics.
Apply if desired:
- let mut count = 0usize; + let mut entries = 0usize; + let mut values = 0usize; for result in iter { let (key, addresses) = result?; - // TODO(perf): consider storing the actual values in a separate column family to avoid get/set + // TODO(perf): consider storing the actual values in a separate column family to avoid get/set for substate_addr in addresses { let mut substate = substates_cf.get(&substate_addr, OPERATION)?; substate.clear_substate_value(); substates_cf.put(&substate_addr, &substate, OPERATION)?; + values += 1; } unpruned_index.delete(&key, OPERATION)?; - count += 1; + entries += 1; } info!( target: LOG_TARGET, - "🗑️ Pruned {count} downed substates for epoch {epoch} from unpruned values index" + "🗑️ Pruned {values} downed substate values across {entries} entries for epoch {epoch}" );
1274-1276: Optional: guard against accidental overwrite of an existing (shard, state_version) transition.If duplicate writes for the same key are unexpected, add an exists check or use put_if_absent semantics and log/return an error to prevent silent overwrite.
Would you like a patch to check existence and return a QueryError on duplicates?
crates/storage/src/consensus_models/substate.rs (1)
203-211: Nit: parameter name is misleading in substate_is_up.The parameter is a SubstateAddress, not a Shard.
Consider renaming for clarity:
- pub fn substate_is_up<TTx: StateStoreReadTransaction>( - tx: &TTx, - shard: &SubstateAddress, + pub fn substate_is_up<TTx: StateStoreReadTransaction>( + tx: &TTx, + address: &SubstateAddress, ) -> Result<bool, StorageError> { // TODO: consider optimising - let rec = tx.substates_get(shard)?; + let rec = tx.substates_get(address)?; Ok(rec.is_up()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (25)
applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs(3 hunks)applications/tari_validator_node/src/state_bootstrap.rs(10 hunks)bindings/src/index.ts(1 hunks)bindings/src/types/BlockHeader.ts(2 hunks)bindings/src/types/ShardStateVersions.ts(1 hunks)bindings/src/types/SubstateCreated.ts(1 hunks)bindings/src/types/SubstateDestroyed.ts(1 hunks)bindings/src/types/SubstateRecord.ts(2 hunks)crates/common_types/src/versioned_substate_id.rs(2 hunks)crates/consensus_tests/fixtures/block.json(2 hunks)crates/consensus_tests/fixtures/block_with_dummies.json(2 hunks)crates/consensus_tests/src/substate_store.rs(3 hunks)crates/consensus_tests/src/support/harness.rs(4 hunks)crates/p2p/proto/rpc.proto(2 hunks)crates/rpc_state_sync/src/state_sync.rs(9 hunks)crates/state_store_rocksdb/src/column_families/substate.rs(2 hunks)crates/state_store_rocksdb/src/writer.rs(6 hunks)crates/state_store_tests/src/helpers.rs(9 hunks)crates/state_store_tests/src/substates.rs(3 hunks)crates/storage/src/consensus_models/block.rs(8 hunks)crates/storage/src/consensus_models/state_transition.rs(1 hunks)crates/storage/src/consensus_models/substate.rs(9 hunks)crates/storage/src/consensus_models/substate_update_batch.rs(1 hunks)crates/storage/src/state_store/mod.rs(6 hunks)utilities/db_inspector/src/webserver/handlers/state_transitions.rs(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/consensus_tests/fixtures/block_with_dummies.json
- crates/consensus_tests/fixtures/block.json
- crates/storage/src/consensus_models/block.rs
🧰 Additional context used
🧬 Code Graph Analysis (19)
bindings/src/types/SubstateCreated.ts (2)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/Shard.ts (1)
Shard(3-3)
bindings/src/types/BlockHeader.ts (1)
bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/state_store_rocksdb/src/column_families/substate.rs (3)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/Shard.ts (1)
Shard(3-3)bindings/src/types/SubstateAddress.ts (1)
SubstateAddress(3-3)
bindings/src/types/SubstateDestroyed.ts (1)
bindings/src/types/Epoch.ts (1)
Epoch(3-3)
crates/storage/src/consensus_models/substate_update_batch.rs (7)
crates/storage/src/consensus_models/substate_change.rs (2)
substate(58-63)shard(66-71)bindings/src/types/SubstateId.ts (1)
SubstateId(15-24)crates/storage/src/consensus_models/substate.rs (2)
shard(112-114)new(50-65)bindings/src/types/Shard.ts (1)
Shard(3-3)bindings/src/types/Epoch.ts (1)
Epoch(3-3)bindings/src/types/VersionedSubstateId.ts (1)
VersionedSubstateId(4-4)crates/common_types/src/versioned_substate_id.rs (4)
new(25-30)new(197-199)new(314-319)new(433-435)
crates/consensus_tests/src/substate_store.rs (3)
crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)crates/storage/src/consensus_models/substate.rs (2)
new(50-65)commit_batch(174-180)crates/common_types/src/shard.rs (1)
first(27-29)
bindings/src/types/SubstateRecord.ts (1)
bindings/src/types/SubstateCreated.ts (1)
SubstateCreated(5-5)
utilities/db_inspector/src/webserver/handlers/state_transitions.rs (4)
crates/storage/src/consensus_models/substate.rs (6)
new(50-65)substate_id(79-81)substate_id(390-392)substate_id(423-428)version(105-107)version(430-435)utilities/db_inspector/src/webserver/handlers/types.rs (2)
new(25-30)new(55-64)crates/state_store_rocksdb/src/reader.rs (3)
iter(305-308)iter(696-698)iter(1671-1673)crates/state_store_rocksdb/src/codecs/tuple.rs (4)
encode(33-37)encode(64-69)encode(103-113)encode(132-136)
crates/common_types/src/versioned_substate_id.rs (3)
crates/common_types/src/substate_address.rs (2)
to_shard(145-195)to_shard(421-474)crates/common_types/src/num_preshards.rs (1)
num_shards(36-38)crates/common_types/src/shard.rs (1)
global(33-35)
crates/rpc_state_sync/src/state_sync.rs (4)
crates/storage/src/consensus_models/substate.rs (11)
from(359-361)from(365-367)new(50-65)version(105-107)version(430-435)commit_batch(174-180)value(329-334)value(378-380)substate_id(79-81)substate_id(390-392)substate_id(423-428)crates/state_store_rocksdb/src/reader.rs (1)
new(169-175)crates/p2p/src/conversions/rpc.rs (15)
try_from(29-37)try_from(52-57)try_from(73-79)try_from(96-105)try_from(132-141)try_from(149-162)try_from(182-187)from(41-46)from(61-67)from(83-90)from(109-115)from(121-126)from(166-175)from(191-196)value(155-159)crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)
crates/consensus_tests/src/support/harness.rs (4)
bindings/src/types/SubstateCreated.ts (1)
SubstateCreated(5-5)bindings/src/types/SubstateRecord.ts (1)
SubstateRecord(7-14)crates/storage/src/consensus_models/substate.rs (5)
shard(112-114)new(50-65)value(329-334)value(378-380)commit_batch(174-180)crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)
applications/tari_validator_node/src/state_bootstrap.rs (3)
crates/storage/src/consensus_models/substate.rs (5)
new(50-65)substate_id(79-81)substate_id(390-392)substate_id(423-428)commit_batch(174-180)crates/common_types/src/versioned_substate_id.rs (8)
new(25-30)new(197-199)new(314-319)new(433-435)substate_id(46-48)substate_id(226-228)substate_id(325-327)substate_id(444-446)crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)
applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs (2)
crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)crates/storage/src/consensus_models/state_transition.rs (1)
get_for_shard(42-49)
crates/state_store_tests/src/substates.rs (2)
crates/state_store_tests/src/helpers.rs (4)
assert_eq_debug(254-257)build_substate_record(128-143)create_rocksdb(81-88)create_substate_update_batch(165-194)crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)
crates/storage/src/consensus_models/state_transition.rs (2)
crates/storage/src/consensus_models/substate.rs (1)
shard(112-114)crates/storage/src/consensus_models/substate_change.rs (1)
shard(66-71)
crates/storage/src/state_store/mod.rs (4)
bindings/src/types/ShardStateVersions.ts (1)
ShardStateVersions(11-11)crates/state_store_rocksdb/src/reader.rs (5)
state_transitions_get_after(1592-1655)state_tree_nodes_get(1657-1662)state_tree_nodes_get_all_by_state_version(1664-1675)state_tree_versions_get_latest(1677-1682)state_tree_versions_get_latest_for_shard_group(1684-1720)crates/storage/src/consensus_models/substate.rs (1)
shard(112-114)crates/state_store_rocksdb/src/writer.rs (1)
substates_commit_batch(1197-1280)
crates/state_store_rocksdb/src/writer.rs (6)
bindings/src/types/SubstateCreated.ts (1)
SubstateCreated(5-5)bindings/src/types/SubstateDestroyed.ts (1)
SubstateDestroyed(4-4)bindings/src/types/SubstateRecord.ts (1)
SubstateRecord(7-14)crates/storage/src/state_store/mod.rs (1)
substates_commit_batch(493-493)crates/storage/src/consensus_models/substate.rs (4)
shard(112-114)new(50-65)version(105-107)version(430-435)crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)
crates/state_store_tests/src/helpers.rs (5)
bindings/src/types/SubstateCreated.ts (1)
SubstateCreated(5-5)bindings/src/types/SubstateRecord.ts (1)
SubstateRecord(7-14)crates/common_types/src/versioned_substate_id.rs (12)
substate_id(46-48)substate_id(226-228)substate_id(325-327)substate_id(444-446)version(58-60)version(222-224)version(333-335)version(448-450)new(25-30)new(197-199)new(314-319)new(433-435)crates/state_store_tests/src/substates.rs (1)
substate_id(21-24)crates/storage/src/consensus_models/substate.rs (9)
substate_id(79-81)substate_id(390-392)substate_id(423-428)version(105-107)version(430-435)value(329-334)value(378-380)destroyed(120-122)shard(112-114)
crates/storage/src/consensus_models/substate.rs (5)
bindings/src/types/SubstateCreated.ts (1)
SubstateCreated(5-5)bindings/src/types/SubstateDestroyed.ts (1)
SubstateDestroyed(4-4)crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)crates/storage/src/consensus_models/substate_change.rs (3)
version(54-56)shard(66-71)into_transition(102-111)crates/state_tree/src/tree.rs (1)
id(212-217)
⏰ 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). (5)
- GitHub Check: check stable
- GitHub Check: machete
- GitHub Check: test
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (33)
crates/state_store_rocksdb/src/column_families/substate.rs (2)
77-80: LGTM: Versioned key and batched value are aligned with the new model
- Key moves to (Epoch, Shard, Version) and uses NumberCodec<…> — appropriate for ordered iteration.
- Value becomes Vec with DefaultCodec — matches the new batched semantics.
No blockers from my side on this change.
79-80: Serde support confirmed; review Vec sizes for write amplification
- SubstateAddress is annotated with
#[derive(..., Serialize, Deserialize, ...)].type ValueCodec = DefaultCodec<Vec<SubstateAddress>>is an alias forBincode<Vec<SubstateAddress>>, which enforces the serde bounds.No runtime decode errors should occur. Please validate that the typical length of the
Vec<SubstateAddress>values stays within your expected size limits to avoid excessive RocksDB write amplification.utilities/db_inspector/src/webserver/handlers/state_transitions.rs (2)
32-36: Header update matches the new (shard, state_version) modelReplacing Seq with State Version and removing Substate Address is consistent with the new composite-key and per-transition row expansion.
55-87: Row-level pagination fix is correct and addresses earlier feedbackSwitching to row-level counters (row_skip/emitted) inside the per-transition loop yields deterministic at-most-limit pages and correct semantics when a key expands to multiple rows. Nice work.
applications/tari_validator_node/src/state_bootstrap.rs (2)
190-198: Shard derivation via VersionedSubstateIdRef is correctDeriving the shard using
VersionedSubstateIdRef::to_shardwith version 0 aligns with global/non-global rules and the new shard+state_version model.
57-62: Signature update verified: all relevant call sites updated
- In
applications/tari_validator_node/src/bootstrap.rs:224,bootstrap_state(tx, config.network, consensus_constants.num_preshards)now correctly passes the newnum_preshardsparameter.- The
bootstrap_stateincrates/template_test_tooling/src/template_test.rsis an unrelated method on a test struct and isn’t affected by this change.No remaining calls use the old signature.
applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs (1)
110-124: Chunked sending logic looks solid
has_moreis computed correctly from the chunk index and you preservestate_versionandepochper chunk. Good.bindings/src/types/BlockHeader.ts (1)
11-11: Doc rewording LGTM“This is computed” better reflects the immutable, derived nature of the ID.
bindings/src/types/SubstateCreated.ts (2)
2-3: LGTM on type-only importsType-only imports keep the runtime bundle clean and match the generated patterns.
5-5: Confirm BigInt support and serialization
- All
tsconfig.jsonfiles in the repo target ES2020 or ESNext, so the nativebiginttype is supported at compile time (even wherelibisn’t explicitly listed, the default libs for ES2020+ include BigInt definitions).- We didn’t find any direct
JSON.stringifycalls onbigintvalues in the codebase.- As a reminder, when you expose these types over JSON (e.g. in a public API), convert
at_state_version: bigintto a string or supply a custom replacer/toJSON method to avoid runtime errors.bindings/src/index.ts (1)
99-107: ✅ New type exports verifiedScripts confirm:
bindings/src/types/ShardStateVersions.tsexists and is exported once (line 99 inbindings/src/index.ts)bindings/src/types/SubstateCreated.tsexists and is exported once (line 107 inbindings/src/index.ts)No duplicate exports found.
crates/consensus_tests/src/substate_store.rs (2)
8-8: LGTM: import cleanupImporting only
BlockId, LeafBlock(dropping unused types) keeps tests tidy.
25-32: LGTM: test imports align with batched update pathThe new imports (
SubstateTransition,SubstateUpdateBatch, etc.) match the refactor towards batched transitions.bindings/src/types/SubstateRecord.ts (2)
2-2: LGTM: switch to composed creation metadataImporting
SubstateCreatedsimplifies the record shape and reflects the new model.
12-12: No legacycreated_*usages detected – shape change is safeI’ve scanned all
.ts/.tsxfiles and confirmed there are no remaining references to the oldcreated_*fields (justify,block,by_shard,at_epoch). The newcreated: SubstateCreatedexport is present and consumers/imports update correctly. No downstream code or tests need further migration.crates/storage/src/consensus_models/substate_update_batch.rs (1)
24-26: Method name vs. behavior: returns a transition buffer for in-place pushwith_transition correctly ensures map entries and returns the buffer to mutate. No issues; just noting the "builder" style is intentional and clear from usage.
crates/state_store_tests/src/substates.rs (1)
131-141: Down transition sequencing looks correctDOWN for substate2 at state_version=2 after creation at 1 is consistent with monotonic state_version progression per shard.
crates/state_store_tests/src/helpers.rs (3)
128-143: Builder looks correct and consistent with new modelbuild_substate_record correctly sets created metadata (epoch, shard, state_version) and computes state_hash from value/version.
234-250: gen_substates API/usage is coherentPassing epoch and state_version through to SubstateCreated keeps test scaffolding aligned with (shard, state_version) semantics.
252-257: Nice touch: track_caller on assert helperThis makes panics point to the caller site, simplifying debugging.
crates/rpc_state_sync/src/state_sync.rs (3)
140-147: Guard: early-exit when already at checkpoint rootThe short-circuit when local root equals checkpoint root is correct and avoids unnecessary sync.
206-238: Iterator sizing is fineUsing ExactSizeIterator::len() on the mapped Vec iterator is valid; no action needed.
323-349: Batching commit path is aligned with storage APIBuilding SubstateUpdateBatch per (shard, state_version) and committing via SubstateRecord::commit_batch is consistent with the storage refactor.
crates/storage/src/state_store/mod.rs (5)
193-198: LGTM: targeted accessor for last change of a substate.Narrow read reduces unnecessary I/O for callers needing only the last change.
278-285: Confirm/document inclusive semantics for “get_after”.Matches the RocksDB reader’s inclusive behavior at the requested state_version. Consider clarifying in the trait docstring so backends stay consistent.
288-299: LGTM: state-tree payload/type updates are coherent.Using Node across read paths and adding “get_all_by_state_version” and shard-group latest versions aligns with the new model.
491-494: LGTM: batched substates commit API is a good abstraction boundary.The write API matches the reader/writer separation and enables efficient batch processing.
523-529: LGTM: writer signature updated to StateTreePayload.Consistent with reader changes and the payload-based tree.
crates/state_store_rocksdb/src/writer.rs (3)
140-145: LGTM: new StateTransition model types wired into writer.*Imports match downstream usage in commit paths.
1197-1279: Batched substate commit path looks correct and consistent.
- Up: writes SubstateRecord with created metadata, updates head, records transition.
- Down: marks destroyed with epoch/state_version, updates head, records transition, buffers addresses for pruning.
- Emits one StateTransitionModelDataV1 per (shard, state_version).
This matches the new (shard, state_version) keyed transition model.
1403-1411: LGTM: state-tree batch insert switched to StateTreePayload.Matches the API shift; write path remains straightforward.
crates/storage/src/consensus_models/substate.rs (2)
136-151: LGTM: into_transition maps record state to transition correctly.Uses value-or-hash for Up and constructs VersionedSubstateId for Down — consistent with the new transition model.
296-321: LGTM: SubstateCreated/Destroyed metadata and TS exports.Nested created/destroyed metadata structures are clean and align with bindings; this unblocks cross-language consumers.
01a1d35 to
86c6371
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/template_lib/src/resource/builder/stealth.rs (3)
15-16: Fix incorrect terminology in type docs: “Confidential” → “Stealth”The struct docs say “Confidential” but this is
StealthResourceBuilder. Update for accuracy.Apply this diff:
-/// Implements the builder pattern for Confidential resources. +/// Implements the builder pattern for Stealth resources. pub struct StealthResourceBuilder {
186-201: Doc examples reference the wrong builderExamples show
ResourceBuilder::confidential()in the stealth builder docs. Replace withResourceBuilder::stealth()to avoid confusion.Apply this diff:
-/// ResourceBuilder::confidential() +/// ResourceBuilder::stealth() /// .with_authorization_hook(CallerContext::current_component_address(), "my_hook") /// .build(); @@ -/// ResourceBuilder::confidential() +/// ResourceBuilder::stealth() /// .with_authorization_hook(*alloc.address(), "my_hook") /// .build();
222-231: Correctinitial_supplydocs: they contradict the function signature and behaviorThe docs claim the method returns an address and that stealth resources do not return a bucket, but the function returns
Bucketand the engine tests depend on it. Clarify that the returned bucket is a transient handle that must be used withstealth_transferto produce depositable output(s).Apply this diff:
-/// Sets up how many tokens are going to be minted on resource creation -/// This builds the resource and mints the initial supply of tokens, returning the address of the resource. -/// NOTE that stealth resources do not return the bucket of the initial supply since -/// they are minted as individual UTXO substates and cannot be placed in vault. +/// Sets up how many tokens are going to be minted on resource creation. +/// This builds the resource and mints the initial supply of tokens, returning a transient `Bucket` +/// that references the minted UTXO outputs. +/// NOTE: The returned `Bucket` cannot be deposited directly into a vault; you must first perform a +/// `stealth_transfer` to reveal outputs and obtain a depositable bucket. pub fn initial_supply(self, initial_supply: Amount) -> Bucket {crates/rpc_state_sync/src/state_sync.rs (1)
121-124: Unreachable duplicate match arm for RequestFailed(NotFound)There are two identical arms handling
Err(RpcError::RequestFailed(err)) if err.is_not_found(). The second one is unreachable and will cause a compiler error.Apply this diff:
- Err(RpcError::RequestFailed(err)) if err.is_not_found() => Ok(None), - Ok(GetCheckpointResponse { checkpoint: None }) => Ok(None), - Err(RpcError::RequestFailed(err)) if err.is_not_found() => Ok(None), + Err(RpcError::RequestFailed(err)) if err.is_not_found() => Ok(None), + Ok(GetCheckpointResponse { checkpoint: None }) => Ok(None),
♻️ Duplicate comments (1)
crates/rpc_state_sync/src/state_sync.rs (1)
128-137: Stop consuming the stream after reaching the checkpointWhen the checkpoint state_version is reached and validated, the code returns
Ok(())from inside the write-tx closure but continues the outer stream loop. A malicious peer could send additional (unvalidated) updates beyond the checkpoint, leading to over-sync risk.Apply this diff to set a function-scoped flag and break after the transaction:
@@ async fn start_state_sync( @@ - let mut template_changes = vec![]; + let mut template_changes = vec![]; + // Break the outer stream loop once the checkpoint version is reached and validated + let mut reached_checkpoint = false; @@ - maybe_persisted_state_version = Some(state_version); - store.set_state_version(state_version)?; - // Done - return Ok(()); + maybe_persisted_state_version = Some(state_version); + store.set_state_version(state_version)?; + // Mark for breaking the outer loop after the transaction commits + reached_checkpoint = true; + return Ok(()); @@ - })?; + })?; + if reached_checkpoint { + break; + }Also applies to: 268-292, 299-301
🧹 Nitpick comments (8)
crates/template_lib/src/resource/builder/stealth.rs (2)
29-31: Constructor doc nit: “confidential” → “stealth”The constructor doc still references “confidential resource builder”.
Apply this diff:
-/// Returns a new confidential resource builder +/// Returns a new stealth resource builder pub(super) fn new() -> Self {
138-142: Grammar fix: “whom” → “who”Minor readability nit in docs.
Apply this diff:
-/// Sets up whom (apart from the owner) can update the access rules of the resource. +/// Sets up who (apart from the owner) can update the access rules of the resource. pub fn update_access_rules(mut self, rule: AccessRule) -> Self {crates/rpc_state_sync/src/state_sync.rs (6)
202-204: Preferreserveoverreserve_exactfor amortized growth
reserve_exactmay cause more frequent reallocations across many batches.reserveis generally preferred unless you have a strong reason to match capacity exactly.Apply this diff:
- tree_changes.reserve_exact(msg.updates.len()); - updates.reserve_exact(msg.updates.len()); + tree_changes.reserve(msg.updates.len()); + updates.reserve(msg.updates.len());
205-223: Validate update shards match the shard being syncedBefore buffering, validate that each update’s substate ID belongs to
shard. Otherwise a malicious peer could mix in updates from other shards. IfSpreadPrefixStateTree/ShardScopedTreeStoreWriteralready enforces this, consider returning a clear error when mismatched instead of relying on lower-level errors.Follow-up: If you want, I can draft a small helper that extracts the shard from
SubstateUpdateProofand checks it againstshardhere.
214-223: Minor: avoid calling.len()on the iterator after mappingThis is fine because
Map<IntoIter<Vec<_>>>implementsExactSizeIterator, but if readability is a concern, collect to a small vector before logging/iterating. Not urgent.
268-281: Rollback path is good; consider logging the number of buffered updates discarded on mismatchWhen the checkpoint root mismatches, we bail out (and the transaction should roll back). It may help troubleshooting to include the number of updates buffered for this
state_versionin the error context.
322-347: Batch mapping looks correct; consider duplicate-detection within a versionMapping from
SubstateUpdateProoftoSubstateTransitionis solid. Depending on upstream guarantees, you may want to reject duplicate updates for the same substate ID within the same(shard, state_version)to fail fast.
676-706: Template DOWN handling is logged as Deprecate but comments say “currently not possible”Destroying a template currently “should not happen”, but the code emits a Deprecate change. This is fine as a placeholder, but please confirm that downstream consumers will treat this as a no-op or a safe deprecation. Otherwise, consider ignoring or hard-failing on template destroy updates until deprecation semantics are formalized.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
crates/engine/tests/templates/stealth/src/lib.rs(1 hunks)crates/rpc_state_sync/src/state_sync.rs(8 hunks)crates/template_lib/src/resource/builder/confidential.rs(1 hunks)crates/template_lib/src/resource/builder/stealth.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
crates/engine/tests/templates/stealth/src/lib.rs (1)
crates/engine_types/src/resource.rs (1)
view_key(130-132)
crates/template_lib/src/resource/builder/stealth.rs (3)
crates/template_lib/src/resource/builder/confidential.rs (2)
with_view_key(86-88)with_view_key_opt(93-96)crates/engine_types/src/resource.rs (1)
view_key(130-132)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/template_lib/src/resource/builder/confidential.rs (3)
crates/template_lib/src/resource/builder/stealth.rs (2)
with_view_key(89-91)with_view_key_opt(96-99)crates/engine_types/src/resource.rs (1)
view_key(130-132)bindings/src/types/RistrettoPublicKeyBytes.ts (1)
RistrettoPublicKeyBytes(6-6)
crates/rpc_state_sync/src/state_sync.rs (3)
crates/storage/src/consensus_models/substate.rs (8)
from(359-361)from(365-367)new(50-65)version(105-107)version(430-435)commit_batch(174-180)value(329-334)value(378-380)crates/p2p/src/conversions/rpc.rs (15)
try_from(29-37)try_from(52-57)try_from(73-79)try_from(96-105)try_from(132-141)try_from(149-162)try_from(182-187)from(41-46)from(61-67)from(83-90)from(109-115)from(121-126)from(166-175)from(191-196)value(155-159)crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (3)
crates/engine/tests/templates/stealth/src/lib.rs (1)
23-23: LGTM: cleaner optional view-key handlingSwitching to
with_view_key_opt(view_key)simplifies conditional chaining and aligns with the new builder API. No functional concerns.crates/template_lib/src/resource/builder/confidential.rs (1)
85-89: LGTM: by-valuewith_view_keyand newwith_view_key_optare sound and consistent
with_view_key(self, ...) -> Selfdelegating towith_view_key_optkeeps chaining ergonomics and avoids boilerplate.with_view_key_optcorrectly sets the internalOption<RistrettoPublicKeyBytes>, enabling both set and clear semantics at build time.Also applies to: 90-96
crates/template_lib/src/resource/builder/stealth.rs (1)
89-91: LGTM: unified optional view-key API
- By-value
with_view_key(self, ...)andwith_view_key_opt(mut self, Option<...>)mirror the confidential builder and make conditional application trivial.- Semantics are clear and consistent with the internal
Optionfield.Also applies to: 96-99
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (10)
crates/consensus_tests/src/state_tree.rs (3)
22-22: Make RocksDB path portable and still keep the {}-placeholder behaviorHardcoding /tmp makes this test non-portable on non-Unix platforms. If with_rocks_path preserves a literal {} for later substitution (as intended), prefer using std::env::temp_dir to build the base path while keeping the {} placeholder.
Apply this diff:
- .with_rocks_path("/tmp/test{}") + .with_rocks_path(&format!("{}/tari-consensus-tests-{{}}", std::env::temp_dir().display()))
66-85: Harden the transitions scan: avoid potential infinite loops and improve error messagesTwo improvements:
- Add a monotonicity assertion to guard against a buggy store returning the same/lower state_version, which would loop forever.
- Replace unwrap() with expect() to surface shard/version context on failure.
Apply this diff:
- let mut all_transitions = vec![]; - let mut next_state_version = 1; - while let Some(transitions) = tx - .state_transitions_get_starting_at(shard, next_state_version, false) - .optional() - .unwrap() - { + let mut all_transitions = vec![]; + let mut next_state_version = 1; + while let Some(transitions) = tx + .state_transitions_get_starting_at(shard, next_state_version, false) + .optional() + .expect(&format!( + "Failed to fetch state transitions for shard {shard} starting at state_version {next_state_version}" + )) + { + // Sanity check to prevent potential infinite loops if the store misbehaves + assert!( + transitions.state_version >= next_state_version, + "Non-monotonic state_version returned: got {}, expected >= {} for shard {}", + transitions.state_version, + next_state_version, + shard + ); if transitions.epoch > checkpoint.epoch() { break; } next_state_version = transitions.state_version + 1; all_transitions.push(transitions); }
105-109: Confirm the version argument to put_substate_changes; consider using the last observed state_version instead of a constantThe second argument is currently 1. If this parameter represents the state_version (or a versioned commit index), passing the last observed state_version will better reflect the tree state at the checkpoint.
Apply this diff:
- let values = all_transitions + let values = all_transitions .iter() .flat_map(|t| &t.updates) .map(|transition| transition.to_tree_change()); - let root = tree.put_substate_changes(None, 1, values).unwrap(); + let target_state_version = all_transitions + .last() + .map(|t| t.state_version) + .unwrap_or(1); + let root = tree.put_substate_changes(None, target_state_version, values).unwrap();Optional follow-up: Instead of batching all updates at once, apply each transition in sequence to validate intermediate root evolution as well:
for t in &all_transitions { let changes = t.updates.iter().map(|u| u.to_tree_change()); let _ = tree.put_substate_changes(None, t.state_version, changes).unwrap(); } // Now read the final root and compare to checkpoint let root = tree.root();I can draft this into a separate test if you want to validate per-transition correctness too.
crates/storage/src/state_store/mod.rs (1)
278-285: Document “starting_at” inclusive semantics and NotFound behaviorPlease add a short doc comment clarifying:
- The method returns the first available transition record for (shard, state_version) where state_version’ >= requested (inclusive).
- NotFound is returned only if no transitions exist at or after the requested state_version for the shard.
This reduces ambiguity for integrators updating to the new API.
- fn state_transitions_get_starting_at( + /// Returns the first transition record whose (shard, state_version) is equal to or greater than the requested + /// tuple (inclusive). If no transition exists at or after the requested state_version for the shard, a NotFound + /// error is returned. + fn state_transitions_get_starting_at( &self, shard: Shard, state_version: Version, include_values: bool, ) -> Result<StateVersionTransitions, StorageError>;crates/state_store_tests/src/state_transitions.rs (2)
23-23: Prefer using the EPOCH constant consistentlyYou introduced EPOCH; use it in batch creation to keep test intent obvious.
- let batch = create_substate_update_batch(Epoch::zero(), &substates); + let batch = create_substate_update_batch(EPOCH, &substates);Apply similarly for subsequent batch creations in this file.
42-52: Optional: add a value-inclusion checkConsider one assertion with include_values = true to confirm Value vs Hash behavior on Up transitions (and Hash fallback when pruned).
crates/storage/src/consensus_models/state_transition.rs (1)
20-36: into_chunks preserves order but is O(n^2); switch to split_off to avoid repeated head drainsDraining from the head repeatedly causes repeated memmoves (quadratic behavior). Use split_off to take the tail once per chunk and rotate.
impl StateVersionTransitions { - pub fn into_chunks(self, size: NonZeroUsize) -> Vec<Self> { - let num_chunks = self.updates.len().div_ceil(size.get()); - let mut chunks = Vec::with_capacity(num_chunks); - let mut updates = self.updates; - while !updates.is_empty() { - let take = updates.len().min(size.get()); - let chunk_updates: Vec<_> = updates.drain(..take).collect(); - chunks.push(Self { - epoch: self.epoch, - shard: self.shard, - state_version: self.state_version, - updates: chunk_updates, - }); - } - chunks - } + pub fn into_chunks(mut self, size: NonZeroUsize) -> Vec<Self> { + let size = size.get(); + let mut chunks = Vec::with_capacity(self.updates.len().div_ceil(size)); + let mut updates = std::mem::take(&mut self.updates); + while updates.len() > size { + let tail = updates.split_off(size); + chunks.push(Self { + epoch: self.epoch, + shard: self.shard, + state_version: self.state_version, + updates, + }); + updates = tail; + } + if !updates.is_empty() { + chunks.push(Self { + epoch: self.epoch, + shard: self.shard, + state_version: self.state_version, + updates, + }); + } + chunks + } }crates/state_store_rocksdb/src/reader.rs (3)
1598-1602: Rename OPERATION label for consistencyThe OPERATION name still says "state_transitions_get_n_after". Rename to "state_transitions_get_starting_at" to match the new API and ease metrics/log searches.
- const OPERATION: &str = "state_transitions_get_n_after"; + const OPERATION: &str = "state_transitions_get_starting_at";
1619-1622: Perf note is valid; consider storing state_hash in transition recordsSince include_values=false still requires a Substate CF read to obtain the hash, consider storing the state_hash alongside the substate_address in the transition record to avoid the extra read when values are not requested.
Happy to propose a minimal schema bump strategy if desired.
1684-1720: Latest versions for shard group — LGTM; tiny range nitLogic for global slot, range scan, and gap filling is solid. As a small readability nit, you can use an inclusive end range instead of +1 arithmetic.
- let sg_range = shard_group.start()..Shard::from(shard_group.end().as_u32() + 1); + let sg_range = shard_group.start()..=shard_group.end();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/consensus_tests/src/state_tree.rs(3 hunks)crates/rpc_state_sync/Cargo.toml(0 hunks)crates/state_store_rocksdb/src/reader.rs(6 hunks)crates/state_store_tests/src/state_transitions.rs(2 hunks)crates/storage/src/consensus_models/state_transition.rs(1 hunks)crates/storage/src/state_store/mod.rs(6 hunks)
💤 Files with no reviewable changes (1)
- crates/rpc_state_sync/Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-19T12:04:31.325Z
Learnt from: sdbondi
PR: tari-project/tari-ootle#1543
File: applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs:64-66
Timestamp: 2025-08-19T12:04:31.325Z
Learning: The `state_transitions_get_after` function in the Tari codebase returns transitions equal to and after the given version (inclusive), not strictly after as the name might suggest.
Applied to files:
crates/storage/src/consensus_models/state_transition.rs
🧬 Code Graph Analysis (5)
crates/state_store_tests/src/state_transitions.rs (4)
crates/state_store_tests/src/helpers.rs (3)
create_rocksdb(81-88)create_substate_update_batch(165-194)gen_substates(234-250)crates/storage/src/consensus_models/block.rs (3)
zero_block(207-219)new(157-173)epoch(356-358)crates/storage/src/consensus_models/substate_update_batch.rs (1)
new(17-22)crates/storage/src/consensus_models/substate.rs (2)
new(50-65)shard(112-114)
crates/storage/src/consensus_models/state_transition.rs (2)
crates/storage/src/consensus_models/substate.rs (1)
shard(112-114)crates/storage/src/consensus_models/substate_change.rs (1)
shard(66-71)
crates/consensus_tests/src/state_tree.rs (1)
crates/consensus_tests/src/support/harness.rs (2)
builder(80-82)new(626-658)
crates/state_store_rocksdb/src/reader.rs (4)
crates/storage/src/state_store/mod.rs (5)
state_transitions_get_starting_at(279-284)state_tree_nodes_get(288-288)state_tree_nodes_get_all_by_state_version(289-293)state_tree_versions_get_latest(294-294)state_tree_versions_get_latest_for_shard_group(295-298)crates/common_types/src/shard_group.rs (2)
format(246-246)len(64-66)crates/common_types/src/shard_state_versions.rs (3)
len(81-83)shard_to_index(59-74)from_vec(41-45)crates/common_types/src/shard.rs (1)
global(33-35)
crates/storage/src/state_store/mod.rs (3)
bindings/src/types/ShardStateVersions.ts (1)
ShardStateVersions(11-11)crates/state_store_rocksdb/src/reader.rs (5)
state_transitions_get_starting_at(1592-1655)state_tree_nodes_get(1657-1662)state_tree_nodes_get_all_by_state_version(1664-1675)state_tree_versions_get_latest(1677-1682)state_tree_versions_get_latest_for_shard_group(1684-1720)crates/state_store_rocksdb/src/writer.rs (1)
substates_commit_batch(1197-1280)
⏰ 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). (4)
- GitHub Check: test
- GitHub Check: check stable
- GitHub Check: check nightly
- GitHub Check: clippy
🔇 Additional comments (20)
crates/consensus_tests/src/state_tree.rs (3)
8-14: Imports align with the new payload/tree APIs — LGTMBrings in Optional, StateStore, and state_tree types cleanly and matches the refactor direction.
32-32: LGTM: Extra batch broadens transition coverageQueuing additional transactions increases the chance of per-shard transitions and strengthens the assertions later in the test.
90-101: LGTM: Placeholder root short-circuitUsing SPARSE_MERKLE_PLACEHOLDER_HASH to assert the absence/presence of transitions per shard is appropriate and keeps the test crisp.
crates/storage/src/state_store/mod.rs (4)
34-39: Public surface updated to new versioned model — LGTMThe new imports (ShardStateVersions, StateTreePayload, StateVersionTransitions, SubstateUpdateBatch) align with the PR’s objectives and downstream usage.
Also applies to: 60-66
286-299: State-tree payload/type updates — LGTMReturning Node and adding the by-state-version accessor matches the RocksDB reader/writer changes and the new payload model.
523-529: Batch insert signature updated — no stale Node usagesVerified that there are no remaining
Node<Version>references and all calls tostate_tree_nodes_batch_insertnow passNode<StateTreePayload>.
491-494: All StateStoreWriteTransaction implementations updatedThe only implementor (
RocksDbStateStoreWriteTransaction) now includes both new methods, so no action required:
- crates/state_store_rocksdb/src/writer.rs:
fn substates_commit_batchat line 1197fn substates_prune_downed_valuesat line 1282- No other
StateStoreWriteTransactionimpls found.crates/state_store_tests/src/state_transitions.rs (4)
4-8: Imports and setup align with the new API — LGTM
29-40: Record-then-assert pattern is sound — LGTMTracking per-(state_version, shard) and asserting via the new API validates the intended “starting_at” semantics.
53-63: Batching across versions is exercised — LGTM
64-73: Assertions cover epoch, version, shard, and count — LGTMThese invariants squarely test the new API’s contract.
crates/storage/src/consensus_models/state_transition.rs (2)
13-18: StateVersionTransitions struct fits the new model — LGTM
41-49: Thin wrapper to the read transaction — LGTMName and behavior align with the “starting_at (inclusive)” semantics, matching past expectations.
crates/state_store_rocksdb/src/reader.rs (7)
60-62: ShardGroup/ShardStateVersions imports — LGTM
75-84: Consensus type imports updated to proof-based model — LGTM
96-97: State-tree payload types (Node) — LGTM
139-147: state_tree and shard_versions CF imports — LGTM
1657-1662: state_tree_nodes_get returning Node — LGTM
1664-1675: By-state-version node fetch — LGTMThe prefix scan and mapping to (NodeKey, Node) look correct.
1679-1681: Latest shard version — LGTM
2 participants
Description
refactor(state_sync)!: sync state transitions by (shard,state_version)
Motivation and Context
State sync uses the (shard, state_version) tuple to begin syncing a specific shard from a particular version of state.
This will allow clients to monitor (sync) state transitions similarly.
This refactor also removes the need for shard sequence tracking in the db level.
How Has This Been Tested?
Manually, existing tests updated
What process can a PR reviewer use to test or verify this change?
Sync a node
Breaking Changes
Summary by CodeRabbit
New Features
Refactor
Chores
Tests