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

migration: Make migration-block-time a required flag #242

Open
wants to merge 2 commits into
base: celo9
Choose a base branch
from

Conversation

palango
Copy link

@palango palango commented Oct 1, 2024

Resolves https://github.com/celo-org/celo-blockchain-planning/issues/627

By making the flag required, we ensure node operators will always add one. So long as the correct number is passed as an argument (to be shared by the sequencer) the resulting block hash will match the sequencer's

@palango palango requested review from piersy and alecps October 1, 2024 15:52
Comment on lines 160 to 161
migrationBlockTime = uint64(time.Now().Unix())
// If the migration block time is not set, use the time of the last block incremented by one.
// This makes sure the migration is deterministic.
migrationBlockTime = header.Time + config.L2BlockTime
Copy link

Choose a reason for hiding this comment

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

I don't think we want to do this, or at least not just one block time, because the sequencer will try to fill all the time between the migration block time and 'now' with blocks when it is started. So it's better to leave some space for the migration to happen, ideally we can pick the time to coincide with the completion of the migration.

Copy link

Choose a reason for hiding this comment

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

So to clarify, we would increment header.time by a number roughly equal (maybe slightly less) than the time it takes to the migration so that the node doesn't try to add too many blocks to fill the gap?

Copy link

Choose a reason for hiding this comment

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

Yeah so we don't end up spamming blocks just as the network starts, so I would either use the flag or hardcode this a few days before we do the next migration, after we've done a trial run and have an idea of how long it would take.

Copy link

@alecps alecps left a comment

Choose a reason for hiding this comment

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

This looks good to me apart from the concern raised by Piers

@piersy piersy marked this pull request as draft October 2, 2024 11:05
@piersy
Copy link

piersy commented Oct 2, 2024

Lets put this on hold till we know roughly how long a mainnet migration will take.

@alecps
Copy link

alecps commented Oct 3, 2024

@piersy Another solution here would be to make the block time flag required and since only whoever is doing the first migration will need to run the script without passing a timestamp, we could add a way to explicitly override the requirement. E.g. maybe the flag is required but if --migration-block-time=now is passed then time.Now() is used. External operators will never need to do this

@piersy
Copy link

piersy commented Oct 3, 2024

@piersy Another solution here would be to make the block time flag required and since only whoever is doing the first migration will need to run the script without passing a timestamp, we could add a way to explicitly override the requirement. E.g. maybe the flag is required but if --migration-block-time=now is passed then time.Now() is used. External operators will never need to do this

@alecps I think just making it required is ok, the person who runs it first can just pick a timestamp by adding a few minutes to the current time.

@alecps alecps marked this pull request as ready for review October 3, 2024 19:32
@alecps alecps requested a review from piersy October 3, 2024 19:32
@alecps alecps changed the title migration: Use deterministic default for migration block timestamp migration: Make-- migration-block-time a required flag Oct 3, 2024
@alecps alecps changed the title migration: Make-- migration-block-time a required flag migration: Make--migration-block-time a required flag Oct 3, 2024
@alecps alecps changed the title migration: Make--migration-block-time a required flag migration: Make migration-block-time a required flag Oct 3, 2024
Copy link

@piersy piersy left a comment

Choose a reason for hiding this comment

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

Looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants