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

PeriodToAllowPostingBatchWithOnlyBatchPostingReport config #2657

Merged
merged 6 commits into from
Oct 25, 2024

Conversation

diegoximenes
Copy link
Contributor

@diegoximenes diegoximenes commented Sep 10, 2024

Resolves NIT-2720.

PeriodToAllowPostingBatchWithOnlyBatchPostingReport config.

SequencerInbox contract adds a batch posting report to the delayed inbox after a batch is posted to the non delayed inbox.
The sequencer will read the batch posting report from the delayed inbox, and the PeriodToAllowPostingBatchWithOnlyBatchPostingReport config allows the batch poster to post batches that only contain batch posting report messages.

This is going to be useful to avoid allowing validation being permissionless if there have been no assertions created within a 2 week period, behavior which is controlled by Nitro's contracts.

@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 Sep 10, 2024
@diegoximenes diegoximenes force-pushed the allow_only_posting_batch_posting_report branch from b3b8501 to df0da33 Compare September 10, 2024 15:38
@diegoximenes diegoximenes marked this pull request as ready for review September 10, 2024 15:39
amsanghi
amsanghi previously approved these changes Sep 23, 2024
ganeshvanahalli
ganeshvanahalli previously approved these changes Oct 4, 2024
arbnode/batch_poster.go Outdated Show resolved Hide resolved
@diegoximenes diegoximenes force-pushed the allow_only_posting_batch_posting_report branch from df0da33 to 52f6a6c Compare October 10, 2024 14:52
@diegoximenes diegoximenes changed the title AllowPostingBatchWithOnlyBatchPostingReport config PeriodToAllowPostingBatchWithOnlyBatchPostingReport config Oct 10, 2024
Dangerous BatchPosterDangerousConfig `koanf:"dangerous"`
ReorgResistanceMargin time.Duration `koanf:"reorg-resistance-margin" reload:"hot"`
CheckBatchCorrectness bool `koanf:"check-batch-correctness"`
PeriodToAllowPostingBatchWithOnlyBatchPostingReport time.Duration `koanf:"period-to-allow-posting-batch-with-only-batch-posting-report"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to find a simpler name.

A batch posting report is automatically added (by the inbox smart-contract) every time a batch is posted. Orbit deployers don't usually understand too well what is a "batch-posting-report". They will experience it as maximum delay between batches even if they are empty.

I want something close to "max-deay" that shows the difference.

Maybe "max-empty-batch-delay"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to max-empty-batch-delay

tsahee
tsahee previously approved these changes Oct 21, 2024
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

PlasmaPower
PlasmaPower previously approved these changes Oct 24, 2024
Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM

@PlasmaPower
Copy link
Collaborator

Seems to be failing CI now due to merge conflicts of firstMsgTime no longer existing. I think substituting msg.Message.Header.Timestamp instead would work

@diegoximenes diegoximenes dismissed stale reviews from tsahee and PlasmaPower via 7c595e2 October 25, 2024 13:47
Copy link
Collaborator

@PlasmaPower PlasmaPower left a comment

Choose a reason for hiding this comment

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

LGTM

@PlasmaPower PlasmaPower merged commit 00d0069 into master Oct 25, 2024
15 checks passed
@PlasmaPower PlasmaPower deleted the allow_only_posting_batch_posting_report branch October 25, 2024 15:47
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.

5 participants