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

feat: Add slot number to header #7594

Closed
Tracked by #7614
LHerskind opened this issue Jul 24, 2024 · 0 comments · Fixed by #7663
Closed
Tracked by #7614

feat: Add slot number to header #7594

LHerskind opened this issue Jul 24, 2024 · 0 comments · Fixed by #7663

Comments

@LHerskind
Copy link
Contributor

LHerskind commented Jul 24, 2024

Include the slot number in the header such that we can constrain based on it later.

We can then update the timestamp of the block to be constrained as GENESIS_TIME + slot_number * SLOT_SIZE such that we don't have the weirdness in timestamps that we have currently.

Currently we are only constraining as

if (_header.globalVariables.timestamp > block.timestamp) {
  revert Errors.Rollup__TimestampInFuture();
}

if (_header.globalVariables.timestamp < _lastBlockTs) {
  revert Errors.Rollup__TimestampTooOld();
}

Which allow some somewhat weird structures, e.g., if you are the proposer for multiple blocks in a row you could deliberately alter the time between blocks to maximise its impact on for example interest rates or twaps.

This will also allow us to get rid of the troubles with #1614 since time of L1 and L2 are directly coupled and the L2 warp can be deleted.

As part of this we will also want to assert block consistency, as shown in the dingo doc

@github-project-automation github-project-automation bot moved this to Todo in A3 Jul 24, 2024
@github-project-automation github-project-automation bot moved this from Todo to Done in A3 Aug 2, 2024
LHerskind added a commit that referenced this issue Aug 9, 2024
Fixes 7656. 

Following an earlier PR (#7594), we added slots into the mix. This PR
tackles actually using those slots for something. When adding these
constraints, several fixes were needed on the sequencing clients and its
components.

So why are we introducing slots? There is really two things that we are
going to use the slots for:
- i) We are going to use it to deal with time within the L2
- Previously a sequencer had flexibility on how the defined the
timestamp, now it is instead defined as `GENESIS + slot * slot_duration`
depending solely on the slot.
- ii) We will be using it as the unit of time in which a specific
proposer have rights to proposer a block and where the block will be
accepted on chain.
- This means that it is a timeliness requirement for block inclusion on
L1
- Since the proposer can only use the "current" slot as the value for
his block, he no flexibility on the timestamp of the L2 block.

The first term is mainly of interest to applications, but the second
term is really important for how our sequencer selection works. If you
do not propose within your block, you are tough out of luck, the block
will not be accepted on L1.

This means that it is also very important for us to figure out a proper
slot duration, since a short slot duration can lead to plenty of empty
slots, while too long lead to loss in throughput.

Introducing such a strict timeliness requirement does impact our current
tests quite a lot, since they are all created without that in mind
(similar was the sequencer client).

Since we in our E2E are the only producer of L1 blocks as well, we have
full control over those. However, using automine on Anvil as the
underlying chain makes the behaviour of sequencing with slots quite odd,
as things that would normally be within the same block (multiple tx)
will now progress time at every tx. This meant that the tests broke
horribly, as we with one tx was publishing the data for our block, and
then when we wanted to publish the block afterwards the time was
progressed, so we might no longer be the proposer.

This was handled by adding a combination call that is doing
`publishAndProcess` of a block, making sure that we don't progress time
unnecessarily.

Secondly, when we are using on-demand mining (automine), we won't have
that we get to the "next" slot after a block have been included, so to
not mess to much with timing, we have specified the `SLOT_DURATION` to
the same as the underlying chain slot duration. Since we are using
anvil, this is by default 1s. However, we also change the block interval
to 12s to behave closer to ethereum.

Having just a single l1 slot for every l2 slot means that the proposer
is rapidly changing as l1 blocks are coming in. Something that WILL NOT
work out the box for a real chain, but is accepted as a first step here
in our setup to get started.

Since we do not want to break everything just ahead of dev net, we have
introduced a flag for turning off the most strict checks and rotation of
the sequencers. By setting `IS_DEV_NET = true` in the `constants.nr` the
rollup will accept blocks from anyone in the validator set (if not
empty), without checking if it is specifically their turn. You can more
specifically look at this in the `Rollup` contract.

Since it is now required to provide attestations from the L2 network if
not `IS_DEV_NET = true` the sequencer client will `collectAttestations`
before publishing the block. Currently it is a war crime of a function
that is just collecting its own signature and providing that as the
attestations. This mean that the current setup ONLY works for validator
set size 1. This is to be addressed with the extensions that @Maddiaa0
is making with #7681.

When body and headers arrive around the same time (in same l1 block) the
archive seemed to run into issues as it would sometime only have loaded
the header and not the body, and then fail as it tries to combine the
two into an l2 block.
If @alexghr, @spypsy, @spalladino or @PhilWindle is able to take a look
to the changes made to it (mainly in
e321449)
that would be great.

Notable changes in this PR:
- `publishAndProcess` 
- Publishing the block body AND processing the pending chain is now done
in the same TX to not cause issues with timeliness
- Block time is set to 12 seconds 
- When deploying the L1 contracts, we are setting the block time of the
underlying Anvil chain to 12 seconds. (Still in automine).
- Introduces `IS_DEV_NET` flag in `constants.nr` 
- When `IS_DEV_NET = true`
- Skips some of the stricter timeliness requirements since they are not
needed for centralised sequencers.
- `IS_DEV_NET = false`
  - Run with all checks on
- Will need signatures from the committee (ordered correctly) to propose
a block
- Made changes to the archiver as it sometimes broke when block body and
header was in the same l1 block.

Future work:
- Collect attestations properly
- E2E should survive running with interval mining and `SLOT_DURATION = n
* ETHEREUM_SLOT_DURATION` for `n>1`.

---

We will have the test pass with `IS_DEV_NET=false` and then we will do
another run where we set it to `true` such that we do not break things
for people unnecessarily.

---

Issues spawned by this pr:
- #7849
- #7821
- #7850 
- #7837
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant