-
Notifications
You must be signed in to change notification settings - Fork 6
Conversation
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.
it would be good to clarify a bit some of the commands. But overall LGTM.
|
||
for h in arguments.from_epoch..=arguments.to_epoch { | ||
let bundle = provider.get_bottom_up_bundle(&subnet, h).await?; | ||
println!("{bundle:?}"); |
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.
Would you mind making the print a bit prettier like I did for top-down cross-messages? The debug output is quite hard to read: https://github.com/consensus-shipyard/ipc/blob/65e92a324e4c3818ceeee90f1416d10d8ad9a107/ipc/cli/src/commands/crossmsg/topdown_cross.rs#L44
@@ -74,6 +80,12 @@ pub(crate) struct BottomUpRelayerArgs { | |||
pub subnet: String, | |||
#[arg(long, short, help = "The number of seconds to submit checkpoint")] | |||
pub checkpoint_interval_sec: Option<u64>, | |||
#[arg( |
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.
This can generally be 0
as in Tendermint we don´t expect re-orgs. That being said, this may come handy once we support other types of consensus algorithms in child subnets.
Actually, I think the default here should be zero, no need to wait for any finalization buffer.
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.
Currently it is default to 0 if user does not set it.
@@ -27,6 +28,8 @@ pub struct BottomUpCheckpointManager<T> { | |||
metadata: CheckpointConfig, | |||
parent_handler: T, | |||
child_handler: T, | |||
/// The number of blocks away from the chain head that is considered final | |||
finalization_blocks: ChainEpoch, |
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.
As described above, I don't think we need this parameter (at least for Tendermint). We can leave it here, but let's leave the default as 0
@@ -151,21 +169,41 @@ impl<T: BottomUpCheckpointRelayer + Send + Sync + 'static> BottomUpCheckpointMan | |||
async fn submit_next_epoch(&self, submitter: &Address) -> Result<()> { | |||
let next_submission_height = self.next_submission_height().await?; |
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.
Just a sanity-check: remember that relayers are allowed to submit a checkpoint for height h
even if it has been submitted already as long as no submission have been made yet for h+1
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.
yep, there is a submit last method
Co-authored-by: adlrocha <[email protected]>
Co-authored-by: adlrocha <[email protected]>
Co-authored-by: adlrocha <[email protected]>
Co-authored-by: adlrocha <[email protected]>
Query the quorum reached event to trigger processing of checkpoint submission.