fix(state_sync)!: sync state version - #1540
Conversation
WalkthroughAdds a per-instance configurable batch size for state sync, introduces a public STATE_SYNC_MAX_BATCH_SIZE, refactors state sync to process transitions grouped by state_version, updates proto to include state_version and removes legacy sync messages, and plumbs state_version through conversions, storage models, reader/writer, and processing. Changes
Sequence Diagram(s)sequenceDiagram
participant VN as Validator Node
participant SST as StateSyncTask
participant RPC as Peer RPC Server
participant RSS as RpcStateSync
participant SS as State Store
VN->>SST: Start sync_state(max_batch_size)
SST->>RPC: Request state transitions (batched)
RPC-->>SST: Return transitions (<= MAX_BATCH_SIZE)
SST->>RSS: Process batch
loop For each state_version group
RSS->>RSS: Validate transitions, build tree changes
RSS->>SS: Apply SubstateTreeChange (batch)
RSS->>SS: Persist new state_version
end
RSS->>SS: Validate state root vs checkpoint
SST-->>VN: Sync complete / error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
crates/state_store_rocksdb/src/column_families/state_transition.rs (1)
26-26: Persisting state_version: good change; document semantics and make the upgrade path explicitThe addition of Version and the new state_version field is aligned with the PR goal. However, because ValueCodec uses DefaultCodec over a serde-serialized struct, adding a non-optional field changes on-disk encoding and will fail to deserialize older records. The PR notes a breaking change; consider making the upgrade path explicit at runtime (e.g., a friendly startup check) or bumping the CF name to prevent silent partial reads from mixed versions.
Also, please document the semantics of state_version to reduce confusion with substate version.
Apply this diff to add doc comments:
#[derive(Debug, Clone, Serialize, Deserialize)] pub struct StateTransitionModelData { pub substate_address: SubstateAddress, pub transition: StateTransitionType, + /// Global state tree (JMT) version at which this transition was applied. + /// Used to group and order transitions during state sync. Not the substate's own version. pub state_version: Version, }Also applies to: 33-38
crates/validator_node_rpc/src/lib.rs (1)
28-29: Expose intent and usage via doc comments; consider making this configurableThe constant is reasonable as a guardrail. Follow-up: consider deriving this from node config/CLI to allow operators to tune based on environment.
Apply this diff to clarify intent:
-pub const STATE_SYNC_MAX_BATCH_SIZE: usize = 100; +/// Hard cap for transitions per state-version batch streamed over RPC. +/// Guardrail for memory usage and message size; consider making this configurable. +pub const STATE_SYNC_MAX_BATCH_SIZE: usize = 100;applications/tari_validator_node/src/p2p/rpc/service_impl.rs (1)
57-57: Wiring the per-instance batch size is correct; consider sourcing from configPassing STATE_SYNC_MAX_BATCH_SIZE through to StateSyncTask::new matches the new API and intent. As a follow-up, consider plumbing this from node configuration to make it tunable without a code change.
Also applies to: 391-397
crates/p2p/proto/rpc.proto (1)
221-225: Add clarification comment for state_version to avoid confusion with substate versionThe new field is well-placed and backward-compatible. A short comment will help distinguish it from SubstateData.version.
Apply this diff:
message StateTransition { StateTransitionId id = 1; SubstateUpdate update = 2; - uint64 state_version = 3; + // JMT state version at which this transition was committed. + // Not to be confused with SubstateData.version (the substate's version). + uint64 state_version = 3; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
applications/tari_validator_node/src/p2p/rpc/service_impl.rs(2 hunks)applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs(3 hunks)crates/p2p/proto/rpc.proto(1 hunks)crates/p2p/src/conversions/rpc.rs(2 hunks)crates/rpc_state_sync/src/state_sync.rs(6 hunks)crates/state_store_rocksdb/src/column_families/state_transition.rs(2 hunks)crates/state_store_rocksdb/src/reader.rs(1 hunks)crates/state_store_rocksdb/src/writer.rs(2 hunks)crates/storage/src/consensus_models/state_transition.rs(1 hunks)crates/validator_node_rpc/src/lib.rs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs (1)
crates/storage/src/consensus_models/state_transition.rs (1)
get_n_after(41-48)
crates/rpc_state_sync/src/state_sync.rs (2)
crates/storage/src/consensus_models/state_transition.rs (3)
shard(105-107)new(73-75)epoch(101-103)crates/p2p/src/conversions/rpc.rs (9)
try_from(30-43)try_from(58-68)try_from(84-90)try_from(107-116)try_from(143-152)try_from(160-175)try_from(193-195)try_from(213-226)value(219-223)
crates/state_store_rocksdb/src/writer.rs (1)
crates/storage/src/consensus_models/state_transition.rs (3)
seq(109-111)new(73-75)shard(105-107)
⏰ 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: machete
- GitHub Check: clippy
- GitHub Check: check nightly
🔇 Additional comments (21)
crates/state_store_rocksdb/src/reader.rs (1)
1649-1654: LGTM: state_version is plumbed through StateTransition constructionCorrectly propagates the persisted state_version into the in-memory StateTransition.
crates/storage/src/consensus_models/state_transition.rs (2)
12-12: LGTM! Import added forVersiontype.The addition of
Versionto the imports fromtari_state_treeis correct and necessary for the newstate_versionfield.
19-19: Breaking change: New required field inStateTransitionstruct.The addition of the
state_versionfield is a breaking change that requires all existing code creatingStateTransitioninstances to be updated. This aligns with the PR objective of synchronizing state versions across validators.applications/tari_validator_node/src/p2p/rpc/state_sync_task.rs (4)
24-24: LGTM! Field added for configurable batch size.The addition of
batch_sizefield allows for dynamic configuration of the batch size, replacing the hardcoded constant approach.
33-33: LGTM! Constructor signature updated correctly.The constructor now accepts
batch_sizeas a parameter and properly stores it in the struct.Also applies to: 40-40
45-45: LGTM! Buffer capacity uses instance field.The buffer initialization now correctly uses the instance's
batch_sizefield instead of a compile-time constant.
97-97: LGTM! Batch retrieval uses instance field.The call to
StateTransition::get_n_afternow correctly usesself.batch_sizefor dynamic batch sizing.crates/p2p/src/conversions/rpc.rs (2)
170-174: LGTM! Proto-to-internal conversion handlesstate_version.The conversion correctly populates the new
state_versionfield from the proto message.
183-183: LGTM! Internal-to-proto conversion handlesstate_version.The conversion correctly writes the
state_versionfield to the proto message.crates/state_store_rocksdb/src/writer.rs (4)
1232-1232: LGTM! State version correctly added to transition data.The
state_versionis properly included in theStateTransitionModelDatafor UP transitions.
1280-1285: LGTM! Consistent state version handling for DOWN transitions.The implementation correctly retrieves and includes the state version for DOWN transitions, maintaining consistency with UP transitions.
1288-1288: LGTM! State version included in DOWN transition data.The
state_versionis properly added to theStateTransitionModelDatafor DOWN transitions.
1219-1224: Default Fallback for New Shard State Versions is ExpectedThe call to
optional()?.unwrap_or_default()will return0for shards that haven’t been seen before, which is the intended “initial version” behavior. No error or warning is necessary here. If you’d like visibility into that case, you can add adebug!log immediately before or after the fallback, but no change is required to handle this as an error.crates/rpc_state_sync/src/state_sync.rs (8)
4-7: LGTM! Required imports added for new functionality.The addition of
BTreeMapfor grouping transitions by state version is appropriate.
61-61: LGTM! Batch size constant imported.The import of
STATE_SYNC_MAX_BATCH_SIZEreplaces the hardcoded local constant.
208-214: Good defensive programming: Batch size validation added.The check against
STATE_SYNC_MAX_BATCH_SIZEprevents potential DoS attacks by limiting the number of transitions that can be processed in a single batch.
219-227: Excellent refactoring: Transitions grouped by state version.The use of
BTreeMapto group transitions bystate_versionensures ordered processing and aligns with the PR objective of synchronizing state versions. The error handling withtry_foldis properly implemented.
230-233: LGTM! Clear loop termination with informative logging.The
pop_first()usage ensures ordered processing, and the informative log message helps with debugging.
338-338: LGTM! State root calculation uses persisted version.The state root calculation correctly uses
maybe_persisted_state_versionto verify against the checkpoint.
354-354: LGTM! Success logging shows final state version.The log message correctly reports the final persisted state version.
328-329: I can’t find aSpreadPrefixStateTreetype in the repository. It looks like the implementation in question isStateTree<S, M>incrates/state_tree/src/tree.rs, which correctly:
- Accepts both
current_version: Option<Version>andnext_version: Version.- Delegates to
calculate_substate_changes, which calls intojmt.batch_put_value_set(..., current_version, next_version).- Inserts new nodes and records stale ones tagged with
next_version.This matches the intended version‐transition logic in JellyfishMerkleTree’s batch API. No special handling appears missing.
Conclusion: The batch_put_substate_changes implementation already handles version transitions correctly; no further changes needed.
2 participants
Description
fix(state_sync)!: sync state version
Motivation and Context
The shard state version is now synchronised across validators. However, this invariant is not enforced by consensus.
A future PR (or the change to the JMT #1472) may enforce this in future.
This is the first part that enables clients to periodically sync state changes using a (shard, state_version) tuple.
How Has This Been Tested?
Manually by deleting a validator node's data and restarting it, observing sync logs and checking the database against the sync node
What process can a PR reviewer use to test or verify this change?
Sync should work as before
Breaking Changes
Summary by CodeRabbit
New Features
Refactor
Chores