feat: handle connection to multiple upstream peers #85
Conversation
WalkthroughThe updates modify the handling of peer connections across several modules. The command-line arguments and configuration structures now support multiple peer addresses by changing fields from a single string to a vector of strings. The bootstrapping process and consensus chain functions have been adapted accordingly to handle multiple peer clients and sessions. Instrumentation improvements enhance logging by adding peer context. Additionally, the public API of the peer structure has been updated for easier external access. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Daemon
participant PeerClientFactory
participant Bootstrap
participant PeerSessionManager
CLI->>Daemon: Provides multiple peer addresses
Daemon->>PeerClientFactory: Create a PeerClient for each address
PeerClientFactory-->>Daemon: Return multiple PeerClient instances (wrapped in Arc/Mutex)
Daemon->>Bootstrap: Pass vector of clients
Bootstrap->>PeerSessionManager: Create PeerSessions for each client
PeerSessionManager-->>Bootstrap: Return constructed PeerSessions
sequenceDiagram
participant Consensus
participant StageManager
participant PeerSessionMap
Consensus->>StageManager: Invoke roll_forward/rollback with peer info
StageManager->>PeerSessionMap: Retrieve PeerSession using peer key
PeerSessionMap-->>StageManager: Return matching PeerSession
StageManager->>Consensus: Process block forwarding or fork switching with peer session
Possibly related PRs
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
abailly
force-pushed
the
abailly/multiple-upstream-peers
branch
from
31d0a9d to
ad49d82
Compare
abailly
force-pushed
the
naive_block_selection
branch
from
495ed4b to
7fabb2b
Compare
abailly
force-pushed
the
abailly/multiple-upstream-peers
branch
from
ad49d82 to
2f0e09f
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
crates/amaru/src/bin/amaru/cmd/daemon.rs (1)
68-74: Add graceful error handling for peer connections.While the implementation is solid, it might be a bit of a sticky wicket if one peer connection fails. Consider implementing a more resilient approach that continues with successfully connected peers even if some fail.
let mut clients: Vec<(String, Arc<Mutex<PeerClient>>)> = vec![]; for peer in &config.upstream_peers { - let client = PeerClient::connect(peer.clone(), config.network_magic as u64) - .await - .into_diagnostic()?; - clients.push((peer.clone(), Arc::new(Mutex::new(client)))); + match PeerClient::connect(peer.clone(), config.network_magic as u64).await { + Ok(client) => { + clients.push((peer.clone(), Arc::new(Mutex::new(client)))); + } + Err(e) => { + error!("Failed to connect to peer {}: {}", peer, e); + continue; + } + } } +if clients.is_empty() { + return Err(miette!("Failed to connect to any peers")).into_diagnostic(); +}crates/consensus/src/consensus/chain_selection.rs (2)
345-345: Tests updated correctly, but could use more coverage.The test updates for
SwitchToForkare spot on, but we might want to add a few more test cases to cover different peer scenarios.Consider adding tests for:
- Multiple peers providing the same fork
- Multiple peers providing different forks of the same length
- A peer disconnecting during fork switch
Also applies to: 449-449
81-81: Consider performance optimizations for peer chain tracking.The
HashMapfor peer chains is a good choice, but for systems with many peers, we might want to consider some optimizations.Suggestions:
- Consider using a capacity hint when creating the HashMap if we know the expected number of peers
- We could implement a simple LRU cache for frequently accessed peer chains
- Consider adding metrics for peer chain lengths to help with monitoring and tuning
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/amaru/src/bin/amaru/cmd/daemon.rs(4 hunks)crates/amaru/src/sync/mod.rs(3 hunks)crates/amaru/src/sync/pull.rs(1 hunks)crates/consensus/src/consensus/chain_selection.rs(5 hunks)crates/consensus/src/consensus/mod.rs(7 hunks)crates/ouroboros/src/protocol/peer.rs(1 hunks)
🔇 Additional comments (25)
crates/amaru/src/sync/mod.rs (12)
24-25: Neat usage of funnel_ports
These imports seem fitting for channelling multiple ports into a single flow. No immediate concerns here, mate.
45-45: Smashing approach for multiple upstream peers
Switching from a single string to aVec<String>paves the way for multi-peer connectivity. Marvellous job.
69-72: Great extension to manage multiple clients
Passing the entire collection is sensible. Just be mindful if the number of peers becomes massive, as that could get resource-heavy—for now, though, this is top-notch.
78-84: Building peer sessions
Perfectly logical construction of sessions. One small heads-up: ifpeer_namecollisions are possible, it might overwrite or cause confusion. You may consider verifying uniqueness.
86-89: Multiple pull stages look right
This code elegantly loops over each session. No major quibbles—looks bonzer.
91-91: Chain Selector usage
Brilliant step passing all peer sessions tomake_chain_selector. All seems hunky-dory.
93-93: Consensus stage with all sessions
Nicely done. Providing the entire set of sessions ensures each peer is accounted for.
102-106: Funnelling multiple outputs
This is a tidy approach to gather data from all pulls. Keep an eye on the backlog if you end up with heaps of peers and messages—50 might need revisiting.
112-115: Spawning multiple pulls
A clean approach to launching each stage with the same policy. Just ensure concurrency is well-handled if peer counts balloon.
120-121: Adding extra stages
Neatly poppingheader_validationandledgerinto the pulls vector. Good on you.
123-123: Returning the aggregated stages
Laughably straightforward, but no issues. Nicely ensures a single handle to all threads.
129-129: Pointer to peer sessions
Accepting a reference to multiplePeerSessions is grand. Nothing to fuss over.crates/consensus/src/consensus/mod.rs (7)
44-44: HashMap for peer sessions
Swapping from a single session to a HashMap is brilliant for multi-peer scenarios. Cracking update.
65-74: Building a HashMap from the vector
This approach is quite dandy. Just be aware that any duplicated peer names might override each other. If that’s not desired, it might need extra logic.
93-93: Expanded forward_block signature
Spot on including the peer as a parameter. This method is neat and tidy—no complaints from me.
114-125: Fork switching logic
Introducing the peer parameter here ensures forward_block gets the correct session. Cracking design—makes the switch to forklift easy-peasy.
165-165: Roll forward with consistent usage
Good job usingforward_blockagain. Shows thorough consistency in the codebase.
173-175: SwitchToFork scenario
Top marks for managing the peer-specific fork scenario. Good clear flow without any tangles.
202-204: Rollback SwitchToFork
A symmetrical approach to handle rollbacks with forks. This is tidy as a teapot, no drama spotted.crates/ouroboros/src/protocol/peer.rs (1)
23-23: Public peer name
Exposingnameis a pragmatic step, especially for identification among multiple peers. Just take care if you need to shield internal details in future.crates/amaru/src/bin/amaru/cmd/daemon.rs (2)
30-35: Brilliant work on the CLI argument handling!The documentation and configuration for multiple peer addresses is spot-on. Making it required with append action ensures users must specify at least one peer, while allowing multiple peers to be added. Proper tidy work!
116-116: LGTM! Clean and straightforward.The config update is spot on, mate! Simple and effective.
crates/amaru/src/sync/pull.rs (1)
69-69: Top-notch observability improvement!Adding the peer context to the logs is a cracking idea! It'll be dead useful when debugging issues with multiple peers.
crates/consensus/src/consensus/chain_selection.rs (2)
97-102: Good catch with the FIXME comment!The temporary nature of needing the peer in
SwitchToForkis well documented. However, let's not let this slip through the cracks.Would you like me to create an issue to track this technical debt? The issue would focus on refactoring the fork switching mechanism to work with stored blocks, eliminating the peer dependency.
180-182: Lovely logging enhancement!The addition of peer context to the logs is consistent with the observability improvements throughout the codebase. Good stuff!
KtorZ
force-pushed
the
abailly/multiple-upstream-peers
branch
from
2f0e09f to
ff16cfc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/consensus/src/consensus/mod.rs (1)
93-104:⚠️ Potential issueReplace expect with proper error handling
G'day! We're still using that risky
expectcall for peer lookup. Let's handle it more gracefully, eh?- let peer_session = self - .peer_sessions - .get(peer) - .expect("Unknown peer, bailing out"); + let peer_session = self + .peer_sessions + .get(peer) + .ok_or_else(|| WorkerError::from(miette!("Unknown peer: {}", peer)))?;
🧹 Nitpick comments (2)
crates/amaru/src/sync/mod.rs (2)
70-73: Add error handling for client initializationCrikey! While the function signature change looks good, we might want to handle potential client initialization failures gracefully.
Consider adding error handling:
pub fn bootstrap( config: Config, clients: Vec<(String, Arc<Mutex<PeerClient>>)>, -) -> miette::Result<Vec<Tether>> { +) -> miette::Result<Vec<Tether>> { + if clients.is_empty() { + return Err(miette::miette!("No upstream peers provided")); + }
79-85: Add logging for peer session initializationA bit of logging would be a proper treat here, helping us track which peers we're connecting to.
+ tracing::info!("Initializing {} peer sessions", clients.len()); let peer_sessions: Vec<PeerSession> = clients .iter() .map(|(peer_name, client)| { + tracing::debug!("Creating session for peer: {}", peer_name); PeerSession { peer: Peer::new(peer_name), peer_client: client.clone(), } }) .collect();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
crates/amaru/src/bin/amaru/cmd/daemon.rs(4 hunks)crates/amaru/src/sync/mod.rs(3 hunks)crates/amaru/src/sync/pull.rs(1 hunks)crates/consensus/src/consensus/chain_selection.rs(5 hunks)crates/consensus/src/consensus/mod.rs(7 hunks)crates/ouroboros/src/protocol/peer.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/ouroboros/src/protocol/peer.rs
- crates/consensus/src/consensus/chain_selection.rs
- crates/amaru/src/sync/pull.rs
- crates/amaru/src/bin/amaru/cmd/daemon.rs
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Build on ubuntu-latest with target wasm32-unknown-unknown
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
🔇 Additional comments (3)
crates/amaru/src/sync/mod.rs (1)
44-50: Spot on with the Config struct changes!The field rename from
upstream_peertoupstream_peersis as clear as a bell, mate. Good choice using Vec for multiple peer support.crates/consensus/src/consensus/mod.rs (2)
41-61: Brilliant use of HashMap for peer sessions!Using a HashMap for peer_sessions is a proper smart choice - gives us O(1) lookup time, which is faster than a rat up a drainpipe!
112-128: Clean as a whistle!The switch_to_fork implementation is properly sorted with the new peer parameter. Nice and consistent with the multi-peer changes.
|
@abailly I didn't look much further but I am now getting an error when using the command as I used to (single local I've checked against |
|
I find it surprising, I tried (after a rebase) to run: (without otlp stack) and it "works": It does not crash and shows progress and headers from both peers defined. |
abailly
force-pushed
the
abailly/multiple-upstream-peers
branch
from
ff16cfc to
633d89b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
76-78: Enhanced Multiple Peer GuidanceCrikey, this new line makes it abundantly clear that you can now add multiple peers, which is brilliant for preventing any mix-ups. It might be even more helpful if you tossed in a quick example on how to correctly supply multiple
--peer-addressflags in a command. This little extra bit could save users from scratching their heads, especially on a foggy morning!🧰 Tools
🪛 LanguageTool
[style] ~76-~76: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2542 characters long)
Context: ....0.1:3000 \ --network=preprod ``` > [!TIP] > Replace--peer-addresswith you...(EN_EXCESSIVE_EXCLAMATION)
crates/consensus/src/consensus/mod.rs (1)
63-87: Add duplicate peer handling, mate!The constructor's looking sharp, but it might get a bit dodgy if we've got duplicate peers in that Vec. The
.collect()will quietly keep the last one, which could lead to some head-scratching moments later.Consider this safer approach:
- let peer_sessions = peer_sessions - .into_iter() - .map(|p| (p.peer.clone(), p)) - .collect::<HashMap<_, _>>(); + let mut peer_sessions = HashMap::new(); + for session in peer_sessions { + if peer_sessions.insert(session.peer.clone(), session).is_some() { + tracing::warn!("Duplicate peer found, keeping the latest session"); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)crates/amaru/src/bin/amaru/cmd/daemon.rs(4 hunks)crates/amaru/src/sync/mod.rs(3 hunks)crates/amaru/src/sync/pull.rs(1 hunks)crates/consensus/src/consensus/chain_selection.rs(5 hunks)crates/consensus/src/consensus/mod.rs(7 hunks)crates/ouroboros/src/protocol/peer.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/ouroboros/src/protocol/peer.rs
- crates/amaru/src/bin/amaru/cmd/daemon.rs
- crates/amaru/src/sync/pull.rs
- crates/consensus/src/consensus/chain_selection.rs
- crates/amaru/src/sync/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
🔇 Additional comments (3)
crates/consensus/src/consensus/mod.rs (3)
41-61: Brilliant restructuring of the Stage struct!The change from a single
peer_sessionto aHashMapofpeer_sessionsis spot on for handling multiple peers. It's like upgrading from a one-on-one chat to a proper group chat - much more sociable! 😄
93-110: LGTM - Keeping it temporary as planned!The
expect()usage has already been flagged in a previous review and acknowledged as temporary. Fair enough for a stopgap solution while waiting for the P2P handling component.
112-208: Clean as a whistle!The peer parameter flows through the methods like a well-poured pint - smooth and consistent. Good job maintaining the peer context throughout the chain of operations.
|
@KtorZ Were you able to go further? |
|
Not yet, but about to try! |
abailly
force-pushed
the
abailly/multiple-upstream-peers
branch
from
633d89b to
76b760c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/consensus/src/consensus/mod.rs (1)
96-101:⚠️ Potential issueThat expect() is still lurking around like a spider in the bathtub!
This is still using
expect()which could cause a nasty crash. I know this was flagged before and deferred, but it's worth mentioning again as it's particularly important with multiple peers.- .expect("Unknown peer, bailing out"); + .ok_or_else(|| WorkerError::from( + miette!("Unknown peer: {:?}", peer) + ))?;
🧹 Nitpick comments (4)
crates/amaru/src/sync/mod.rs (2)
66-69: Add error handling for empty clients vectorCrikey! We might want to handle the case where no peers are provided. A proper error message would be better than letting it fail downstream.
pub fn bootstrap( config: Config, clients: Vec<(String, Arc<Mutex<PeerClient>>)>, ) -> miette::Result<Vec<Tether>> { + if clients.is_empty() { + return Err(miette::miette!("No upstream peers provided")); + }
75-86: Consider more descriptive variable namesThe variable names could be a bit more descriptive. For example,
peer_namecould bepeer_addresssince it represents the network address.- .map(|(peer_name, client)| PeerSession { - peer: Peer::new(peer_name), + .map(|(peer_address, client)| PeerSession { + peer: Peer::new(peer_address),crates/consensus/src/consensus/mod.rs (2)
112-128: Add some logging for better observabilityWould be ace to add some logging here to track which peer we're switching forks for. Could be a lifesaver when debugging!
async fn switch_to_fork( &mut self, peer: &Peer, rollback_point: &Point, fork: Vec<ConwayHeader>, ) -> Result<(), WorkerError> { + tracing::info!( + peer = ?peer, + rollback = ?rollback_point, + fork_length = fork.len(), + "Switching to fork" + );
181-208: Add error context for better debuggingWhen things go pear-shaped, it'd be helpful to have more context about which peer caused the rollback to fail.
async fn handle_roll_back(&mut self, peer: &Peer, rollback: &Point) -> Result<(), WorkerError> { let result = self .chain_selector .lock() .await - .rollback(peer, point_hash(rollback)); + .rollback(peer, point_hash(rollback)) + .map_err(|e| miette!("Failed to rollback for peer {:?}: {}", peer, e))?;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(1 hunks)crates/amaru/src/bin/amaru/cmd/daemon.rs(4 hunks)crates/amaru/src/sync/mod.rs(3 hunks)crates/amaru/src/sync/pull.rs(1 hunks)crates/consensus/src/consensus/chain_selection.rs(5 hunks)crates/consensus/src/consensus/mod.rs(7 hunks)crates/ouroboros/src/protocol/peer.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- README.md
- crates/ouroboros/src/protocol/peer.rs
- crates/amaru/src/sync/pull.rs
- crates/consensus/src/consensus/chain_selection.rs
- crates/amaru/src/bin/amaru/cmd/daemon.rs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
🔇 Additional comments (1)
crates/amaru/src/sync/mod.rs (1)
43-43: Spot on with the Vec for upstream peers!The change from a single peer to a vector of peers aligns perfectly with the PR's objective.
Signed-off-by: Arnaud Bailly <[email protected]>
abailly
force-pushed
the
abailly/multiple-upstream-peers
branch
from
76b760c to
2734c7c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/consensus/src/consensus/mod.rs (1)
96-100:⚠️ Potential issueStill using expect() here, mate! Remember our chat about this?
As discussed in the previous review, using
expect()here is about as safe as a game of cricket in a china shop.The previous suggestion still stands:
- .expect("Unknown peer, bailing out"); + .ok_or_else(|| { + WorkerError::from(miette!("Unknown peer: {}", peer)) + })?;
🧹 Nitpick comments (2)
crates/amaru/src/sync/mod.rs (1)
66-69: Let's make this signature a bit more readable, mate!While the changes are solid, that tuple type is about as clear as a London fog.
Consider adding a type alias at the top of the file:
type PeerConnection = (String, Arc<Mutex<PeerClient>>);Then update the signature:
-pub fn bootstrap( - config: Config, - clients: Vec<(String, Arc<Mutex<PeerClient>>)>, -) -> miette::Result<Vec<Tether>> { +pub fn bootstrap( + config: Config, + clients: Vec<PeerConnection>, +) -> miette::Result<Vec<Tether>> {crates/consensus/src/consensus/mod.rs (1)
93-110: Add some proper tracing to this forward_block method.Let's make debugging easier than finding a four-leaf clover!
Add more detailed tracing:
+ #[instrument(skip(self, header), fields(peer = ?peer, point = ?header.point()))] async fn forward_block(&mut self, peer: &Peer, header: &dyn Header) -> Result<(), WorkerError> {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
README.md(2 hunks)crates/amaru/src/bin/amaru/cmd/daemon.rs(4 hunks)crates/amaru/src/sync/mod.rs(3 hunks)crates/amaru/src/sync/pull.rs(1 hunks)crates/consensus/src/consensus/chain_selection.rs(5 hunks)crates/consensus/src/consensus/header_validation.rs(2 hunks)crates/consensus/src/consensus/mod.rs(7 hunks)crates/ouroboros/src/protocol/peer.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/amaru/src/sync/pull.rs
- crates/consensus/src/consensus/chain_selection.rs
- crates/amaru/src/bin/amaru/cmd/daemon.rs
- README.md
- crates/ouroboros/src/protocol/peer.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
🔇 Additional comments (4)
crates/consensus/src/consensus/header_validation.rs (1)
24-24: LGTM! Clean import addition.The addition of the
Headertype import alongsideConwayHeaderis spot on.crates/amaru/src/sync/mod.rs (2)
43-43: Spot on! Good struct field update.Converting
upstream_peertoupstream_peersas a Vec is a proper job for supporting multiple peers.
123-140: Clean as a whistle! Nice chain selector implementation.The changes to
make_chain_selectorand tether handling are well thought out.crates/consensus/src/consensus/mod.rs (1)
173-175: Brilliant chain selection handling!The peer-specific chain selection is implemented as neat as a pin.
| block_validator | ||
| .validate() | ||
| .map_err(|e| { | ||
| warn!("fail to validate header {}: {:?}", header.hash(), e); | ||
| }) | ||
| .or(Ok(())) // FIXME: Remove this once we have a proper error handling |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Crikey! Let's make this error handling proper sturdy.
While moving from panic to warning is a step in the right direction (like trading Marmite for Vegemite), we could make it even better. The current FIXME suggests this is temporary, but we should consider a more robust approach.
Consider this more comprehensive error handling:
- block_validator
- .validate()
- .map_err(|e| {
- warn!("fail to validate header {}: {:?}", header.hash(), e);
- })
- .or(Ok(())) // FIXME: Remove this once we have a proper error handling
+ block_validator.validate().map_err(|e| {
+ warn!("Header validation failed for {}: {:?}", header.hash(), e);
+ WorkerError::from(miette!("Header validation error: {}", e))
+ })This way, we:
- Maintain proper error propagation
- Keep the warning for observability
- Convert the error into a proper WorkerError
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| block_validator | |
| .validate() | |
| .map_err(|e| { | |
| warn!("fail to validate header {}: {:?}", header.hash(), e); | |
| }) | |
| .or(Ok(())) // FIXME: Remove this once we have a proper error handling | |
| block_validator.validate().map_err(|e| { | |
| warn!("Header validation failed for {}: {:?}", header.hash(), e); | |
| WorkerError::from(miette!("Header validation error: {}", e)) | |
| }) |
| let peer_sessions: Vec<PeerSession> = clients | ||
| .iter() | ||
| .map(|(peer_name, client)| PeerSession { | ||
| peer: Peer::new(peer_name), | ||
| peer_client: client.clone(), | ||
| }) | ||
| .collect(); | ||
|
|
||
| let mut pulls = peer_sessions | ||
| .iter() | ||
| .map(|session| pull::Stage::new(session.clone(), vec![tip.clone()])) | ||
| .collect::<Vec<_>>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add some error handling to this peer session setup.
At the moment, if anything goes wrong during peer session creation, it'll be about as graceful as a kangaroo in roller skates.
Consider adding error handling:
- let peer_sessions: Vec<PeerSession> = clients
- .iter()
- .map(|(peer_name, client)| PeerSession {
- peer: Peer::new(peer_name),
- peer_client: client.clone(),
- })
- .collect();
+ let peer_sessions: miette::Result<Vec<PeerSession>> = clients
+ .iter()
+ .map(|(peer_name, client)| {
+ if peer_name.is_empty() {
+ return Err(miette!("Empty peer name not allowed"));
+ }
+ Ok(PeerSession {
+ peer: Peer::new(peer_name),
+ peer_client: client.clone(),
+ })
+ })
+ .collect();
+ let peer_sessions = peer_sessions?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let peer_sessions: Vec<PeerSession> = clients | |
| .iter() | |
| .map(|(peer_name, client)| PeerSession { | |
| peer: Peer::new(peer_name), | |
| peer_client: client.clone(), | |
| }) | |
| .collect(); | |
| let mut pulls = peer_sessions | |
| .iter() | |
| .map(|session| pull::Stage::new(session.clone(), vec![tip.clone()])) | |
| .collect::<Vec<_>>(); | |
| let peer_sessions: miette::Result<Vec<PeerSession>> = clients | |
| .iter() | |
| .map(|(peer_name, client)| { | |
| if peer_name.is_empty() { | |
| return Err(miette!("Empty peer name not allowed")); | |
| } | |
| Ok(PeerSession { | |
| peer: Peer::new(peer_name), | |
| peer_client: client.clone(), | |
| }) | |
| }) | |
| .collect(); | |
| let peer_sessions = peer_sessions?; | |
| let mut pulls = peer_sessions | |
| .iter() | |
| .map(|session| pull::Stage::new(session.clone(), vec![tip.clone()])) | |
| .collect::<Vec<_>>(); |
| block_validator | ||
| .validate() | ||
| .map_err(|e| { | ||
| warn!("fail to validate header {}: {:?}", header.hash(), e); |
There was a problem hiding this comment.
| warn!("fail to validate header {}: {:?}", header.hash(), e); | |
| warn!(header.hash = ?header.hash(), validation.error = ?e, "failed_to_validate"); |
A "best-practice" I've noted from using the tracing library (but which apply to telemetry in general) is to leverage fields as much as possible, and avoid embedding fields/payloads in the log message. So said differently, embrace structured logs/traces ! This is what allows us to have nice JSON logs out-of-the-box 😛
Also by convention for now, I've always used snake_cased names for all traces & spans; really more out of paranoia than out of any legit reason.
There was a problem hiding this comment.
Totally agreed, that's why I voluntarily wrote this temporary kludge as a textual log entry so that we spot it more easily and change it to something consistent with the rest.
In general, I think we should only ever need instrument!, any other kind of logs is probably unneeded, or is begging for a function to instrument :)
There was a problem hiding this comment.
I think we should only ever need instrument!
That's a good rule of thumb regarding how we slice the code, indeed.
As per EDR-007 Signed-off-by: KtorZ <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/consensus/src/consensus/mod.rs (1)
67-76: Consider adding validation for duplicate peers.Crikey! The peer session collection looks good, but we might want to handle the case where the same peer appears multiple times in the input vector.
let peer_sessions = peer_sessions .into_iter() + .inspect(|p| { + if peer_sessions.iter().filter(|x| x.peer == p.peer).count() > 1 { + tracing::warn!("Duplicate peer detected: {:?}", p.peer); + } + }) .map(|p| (p.peer.clone(), p)) .collect::<HashMap<_, _>>();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/consensus/src/consensus/chain_selection.rs(10 hunks)crates/consensus/src/consensus/header_validation.rs(2 hunks)crates/consensus/src/consensus/mod.rs(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/consensus/src/consensus/header_validation.rs
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
🔇 Additional comments (5)
crates/consensus/src/consensus/mod.rs (3)
46-46: LGTM! Good choice using HashMap for peer sessions.The HashMap provides O(1) lookup for peer sessions, which is spot on for managing multiple peers.
98-102: Previous review comment still applies.G'day! I see we're still using
expecthere. As per the previous review discussion, this will be handled later.
114-127: LGTM! Clean peer integration.The peer parameter addition is well integrated with the existing fork switching logic.
crates/consensus/src/consensus/chain_selection.rs (2)
166-167: Brilliant logging improvements!The DEBUG level is spot on for these events, and the added context will be a lifesaver during troubleshooting. Good on ya!
Also applies to: 176-181
343-343: Tests properly updated for multi-peer support.The test cases have been properly updated to handle the new peer parameter in SwitchToFork. Bob's your uncle!
Also applies to: 447-447
| /// The current best chain has switched to given fork starting at | ||
| /// given hash. | ||
| SwitchToFork(Point, Vec<H>), | ||
| /// given hash, from given Peer. | ||
| /// | ||
| /// FIXME: The peer should not be needed here, as the fork should be | ||
| /// comprised of known blocks. It is only needed to download the blocks | ||
| /// we don't currently store. | ||
| SwitchToFork(Peer, Point, Vec<H>), |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider decoupling block storage from peer information.
The FIXME raises a fair dinkum point! We might want to separate the concerns of block storage and peer management. Perhaps introduce a block cache or local storage service?
Let's check if there's any existing block storage implementation:
🏁 Script executed:
#!/bin/bash
# Search for block storage related code
rg -i "block.*store|store.*block" --type rustLength of output: 172
Decoupling Peer Information from Block Storage
After a friendly little spelunking through our codebase, the search turned up a solitary reference to block storage—specifically in the ledger rewards (see the count_blocks function). In contrast, our consensus module’s SwitchToFork(Peer, Point, Vec<H>) variant still couples peer information right into the chain switching process. This confirms the architectural concern raised by the FIXME comment: the Peer parameter is being used as a crutch for downloading blocks, rather than relying on a dedicated block storage or caching mechanism.
Recommendations:
- Refactor the
SwitchToForkvariant: Remove the Peer dependency if possible, and decouple block storage concerns from peer management. - Consider a dedicated block storage mechanism: Explore implementing or reusing a block cache or local storage service, in line with the isolated block storage practices found elsewhere (e.g. within ledger rewards).
Cheers, and let’s give this a proper tidy-up!
This small PR adds the ability for an Amaru node to connect to multiple upstream peers. This is a temporary solution to test and demo the chain selection process, while we wait for @scarmuega's P2P handling component.
Note: This PR sits on top of #75
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Peerstruct now exposes thenamefield publicly, allowing for external access.