Skip to content
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

Try to fast confirm past nodes too #2520

Merged
merged 9 commits into from
Aug 6, 2024
Merged

Try to fast confirm past nodes too #2520

merged 9 commits into from
Aug 6, 2024

Conversation

PlasmaPower
Copy link
Collaborator

@PlasmaPower PlasmaPower commented Jul 24, 2024

Fixes NIT-2699

In #2434 we added support for fast confirming nodes as we create them, but if a node is restarted at the wrong time, it's possible we'll miss a fast confirmation opportunity. Since the rollup contract only supports fast confirming the first unresolved node, this PR attempts to fast confirm the first unresolved node if possible before doing work on future nodes.

This also pulls in OffchainLabs/nitro-contracts#215 because there would be merge conflicts doing it separately.

@cla-bot cla-bot bot added the s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA. label Jul 24, 2024
@PlasmaPower PlasmaPower marked this pull request as draft July 24, 2024 17:19
@PlasmaPower PlasmaPower marked this pull request as ready for review July 24, 2024 17:57
amsanghi
amsanghi previously approved these changes Jul 25, 2024
@@ -683,6 +683,43 @@ func (s *Staker) Act(ctx context.Context) (*types.Transaction, error) {
StakeExists: rawInfo != nil,
}

if s.config.EnableFastConfirmation {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as we discussed, we also need a solution for the WatchTower that missed confirming an assertion, or who'se confirm transaction was sent but was later lost

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to account for this

@@ -326,6 +333,9 @@ func NewStaker(
return nil, err
}
}
inactiveValidatedNodes := btree.NewG(2, func(a, b validatedNode) bool {
return a.number < b.number || (a.number == b.number && a.hash.Cmp(b.hash) < 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to compare block hashes?
Since you're using ReplaceOrInsert - it makes sense to me to only keep the last inserted (number,hash) pair that fits a node number

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of my concern here was when I used Get, it could return a validated node with a different hash than requested. I explicitly check for that too, but I thought it'd be safer if they were seen as different. That said, I think you're right that the most recently validated node will always be the canonical one, so we could change it.

Copy link
Contributor

@tsahee tsahee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@joshuacolvin0 joshuacolvin0 merged commit 6073359 into master Aug 6, 2024
14 checks passed
@joshuacolvin0 joshuacolvin0 deleted the fast-conf-retry branch August 6, 2024 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design-approved s Automatically added by the CLA bot if the creator of a PR is registered as having signed the CLA.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants