-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: handle connection to multiple upstream peers #85
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -94,8 +94,12 @@ pub enum ChainSelection<H: Header> { | |
NoChange, | ||
|
||
/// 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>), | ||
Comment on lines
96
to
+102
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainConsider 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 rust Length 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 Recommendations:
Cheers, and let’s give this a proper tidy-up! |
||
} | ||
|
||
/// Builder pattern for `ChainSelector`. | ||
|
@@ -169,16 +173,22 @@ where | |
let (best_peer, best_tip) = self.find_best_chain().unwrap(); | ||
|
||
let result = if best_tip.parent().unwrap() == self.tip.hash() { | ||
info!(target: "amaru::consensus::chain_selection::new_tip", hash = ?header.hash().to_string(), slot = ?header.slot()); | ||
ChainSelection::NewTip(header.clone()) | ||
} else if best_tip.block_height() > self.tip.block_height() { | ||
let fragment = self.peers_chains.get(&best_peer).unwrap(); | ||
ChainSelection::SwitchToFork(fragment.anchor.point(), fragment.headers.clone()) | ||
info!(target: "amaru::consensus::chain_selection::switch_to_fork", peer = ?best_peer, | ||
hash = ?best_tip.hash().to_string(), slot = ?best_tip.slot()); | ||
ChainSelection::SwitchToFork( | ||
best_peer, | ||
fragment.anchor.point(), | ||
fragment.headers.clone(), | ||
) | ||
} else { | ||
ChainSelection::NoChange | ||
}; | ||
|
||
if result != ChainSelection::NoChange { | ||
info!(target: "amaru::consensus::chain_selection::new_tip", hash = ?header.hash().to_string(), slot = ?header.slot()); | ||
self.tip = header; | ||
} | ||
|
||
|
@@ -210,7 +220,11 @@ where | |
let fragment = self.peers_chains.get(&best_peer).unwrap(); | ||
// TODO: do not always switch to anchor if there's a better intersection | ||
// with current chain | ||
ChainSelection::SwitchToFork(fragment.anchor.point(), fragment.headers.clone()) | ||
ChainSelection::SwitchToFork( | ||
best_peer, | ||
fragment.anchor.point(), | ||
fragment.headers.clone(), | ||
) | ||
}; | ||
|
||
self.tip = best_tip.clone(); | ||
|
@@ -328,7 +342,7 @@ mod tests { | |
.map(|header| chain_selector.roll_forward(&bob, *header)) | ||
.last(); | ||
|
||
assert_eq!(SwitchToFork(Point::Origin, chain2), result.unwrap()); | ||
assert_eq!(SwitchToFork(bob, Point::Origin, chain2), result.unwrap()); | ||
} | ||
|
||
#[test] | ||
|
@@ -432,6 +446,6 @@ mod tests { | |
let rollback_point = &chain1[3]; | ||
let result = chain_selector.rollback(&alice, rollback_point.hash()); | ||
|
||
assert_eq!(SwitchToFork(Point::Origin, chain2), result); | ||
assert_eq!(SwitchToFork(bob, Point::Origin, chain2), result); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -21,7 +21,7 @@ use pallas_primitives::conway::Epoch; | |||||||||||||||||||||
use std::collections::HashMap; | ||||||||||||||||||||||
use tracing::{instrument, warn}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
use super::header::ConwayHeader; | ||||||||||||||||||||||
use super::header::{ConwayHeader, Header}; | ||||||||||||||||||||||
|
||||||||||||||||||||||
#[instrument(skip_all)] | ||||||||||||||||||||||
pub fn assert_header<'a>( | ||||||||||||||||||||||
|
@@ -38,8 +38,13 @@ pub fn assert_header<'a>( | |||||||||||||||||||||
FixedDecimal::from(5u64) / FixedDecimal::from(100u64); | ||||||||||||||||||||||
let c = (FixedDecimal::from(1u64) - active_slots_coeff).ln(); | ||||||||||||||||||||||
let block_validator = BlockValidator::new(header, cbor, ledger, epoch_nonce, &c); | ||||||||||||||||||||||
block_validator.validate().or_panic()?; | ||||||||||||||||||||||
block_validator | ||||||||||||||||||||||
.validate() | ||||||||||||||||||||||
.map_err(|e| { | ||||||||||||||||||||||
warn!("fail to validate header {}: {:?}", header.hash(), e); | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
A "best-practice" I've noted from using the 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
That's a good rule of thumb regarding how we slice the code, indeed. |
||||||||||||||||||||||
}) | ||||||||||||||||||||||
.or(Ok(())) // FIXME: Remove this once we have a proper error handling | ||||||||||||||||||||||
Comment on lines
+41
to
+46
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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:
📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||
} else { | ||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||
} | ||||||||||||||||||||||
|
||||||||||||||||||||||
Ok(()) | ||||||||||||||||||||||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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:
📝 Committable suggestion