fix: the statemachine#7169
Conversation
WalkthroughThis update integrates a Changes
Error: Could not generate a valid Mermaid diagram after multiple attempts.Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes detected. Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms (7)
🔇 Additional comments (4)
✨ Finishing Touches
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. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
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 (3)
base_layer/core/src/base_node/state_machine_service/initializer.rs (1)
46-46: Move the std import to the top of the imports section.For consistency with Rust conventions, std imports should be grouped together at the top of the file.
Move this import to line 23 with the other standard library imports:
use log::*; +use std::sync::Arc; use tari_comms::connectivity::ConnectivityRequester;base_layer/core/src/base_node/state_machine_service/states/starting_state.rs (1)
56-56: Consider making the bootstrap timeout configurable.The 120-second timeout is hardcoded. Consider adding this as a configuration parameter in
BaseNodeStateMachineConfigto allow flexibility for different network conditions.Add a configuration field to
BaseNodeStateMachineConfig:pub struct BaseNodeStateMachineConfig { // ... existing fields ... + /// Timeout duration for waiting for DHT bootstrap events during startup + #[serde(with = "serializers::seconds")] + pub bootstrap_timeout: Duration, }Then use it here:
-let bootstrap_timeout = sleep(Duration::from_secs(120)); +let bootstrap_timeout = sleep(shared.config.bootstrap_timeout);base_layer/core/src/base_node/state_machine_service/states/listening.rs (1)
211-214: Clarify the magic number in set_peer_metadata call.The second parameter
1in theset_peer_metadatacall appears to be a magic number. Consider using a named constant or adding a comment to explain what this value represents.- .set_peer_metadata(peer_metadata.node_id(), 1, peer_data.to_bytes()) + // The '1' represents the metadata type ID for chain metadata + .set_peer_metadata(peer_metadata.node_id(), 1, peer_data.to_bytes())Or better yet, define a constant:
+const CHAIN_METADATA_TYPE_ID: u8 = 1; // ... later in the code - .set_peer_metadata(peer_metadata.node_id(), 1, peer_data.to_bytes()) + .set_peer_metadata(peer_metadata.node_id(), CHAIN_METADATA_TYPE_ID, peer_data.to_bytes())
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
base_layer/core/src/base_node/state_machine_service/initializer.rs(3 hunks)base_layer/core/src/base_node/state_machine_service/state_machine.rs(5 hunks)base_layer/core/src/base_node/state_machine_service/states/listening.rs(1 hunks)base_layer/core/src/base_node/state_machine_service/states/starting_state.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
- GitHub Check: cargo check with stable
- GitHub Check: Cucumber tests / FFI
- GitHub Check: Cucumber tests / Base Layer
🔇 Additional comments (11)
base_layer/core/src/base_node/state_machine_service/initializer.rs (6)
108-120: LGTM! Peer manager integration looks correct.The peer manager is properly retrieved from the service handles and passed to the state machine constructor, enabling direct peer metadata management within the state machine.
28-28: LGTM: Clean import addition for peer manager integration.The import of
PeerManagerfromtari_commsis correctly added and aligns with the refactoring to integrate peer management into the state machine.
46-47: LGTM: Arc import added for thread-safe peer manager sharing.The addition of
std::sync::Arcimport is appropriate for wrapping thePeerManagerin a thread-safe reference counting pointer, which is consistent with the service architecture.
108-108: LGTM: Proper dependency injection of peer manager.The retrieval of the
Arc<PeerManager>handle from the service context follows the established pattern for dependency injection in this codebase. The use ofexpect_handleis appropriate here as the peer manager is a required dependency for the state machine.
113-113: LGTM: DHT handle retrieval maintained.The DHT handle retrieval is preserved and correctly positioned in the initialization sequence.
119-120: LGTM: State machine constructor updated with peer manager dependency.The addition of the
peer_managerparameter to theBaseNodeStateMachine::newconstructor call is well-integrated. The positioning betweenchain_metadata_service.get_event_stream()anddht.subscribe_dht_events()maintains logical grouping of service dependencies.base_layer/core/src/base_node/state_machine_service/state_machine.rs (2)
94-94: LGTM! Peer manager field properly integrated.The peer manager is correctly added as a field to the state machine struct with appropriate Arc wrapping for shared ownership.
Also applies to: 116-116, 131-131
316-316: Good change to reduce log noise.Changing from
warn!todebug!is appropriate here since updating the bootstrap flag outside the Listening state is a normal operational event, not a warning condition.base_layer/core/src/base_node/state_machine_service/states/starting_state.rs (1)
62-101: Well-implemented async bootstrap event handling.The use of
tokio::select!with timeout provides robust handling of DHT bootstrap events. The logic correctly identifies bootstrap completion through eitherBootstrapMethodDeterminedwith "ExistingPeers" orPrimaryBootstrapCompleteevents, and properly handles lagged events.base_layer/core/src/base_node/state_machine_service/states/listening.rs (2)
185-203: Good implementation of peer ban checking.The code properly checks if a peer is banned before processing their metadata, with appropriate error handling for both banned peers and peer manager errors.
167-342: Excellent simplification of the event loop.Removing DHT event handling from the listening state improves code clarity and separation of concerns. The listening state now focuses solely on chain metadata events, making it more maintainable and easier to understand.
Test Results (Integration tests)0 tests 0 ✅ 0s ⏱️ Results for commit bebb388. ♻️ This comment has been updated with latest results. |
Test Results (CI)0 tests 0 ✅ 0s ⏱️ Results for commit bebb388. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
base_layer/p2p/src/peer_seeds.rs(1 hunks)comms/dht/src/rpc/peer_info.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- comms/dht/src/rpc/peer_info.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test (nextnet, nextnet)
- GitHub Check: test (mainnet, stagenet)
- GitHub Check: test (testnet, esmeralda)
SWvheerden
force-pushed
the
sw_fix_state_machine
branch
from
d9b3b18 to
5a15cd9
Compare
SWvheerden
force-pushed
the
sw_fix_state_machine
branch
from
5a15cd9 to
bebb388
Compare
1 participant
Description
Fixes the statemachine to properly do startup-only peer sync.
fixes: #7148
Summary by CodeRabbit