Skip to content
This repository was archived by the owner on Jan 24, 2024. It is now read-only.

Fix L2 Output Timestamps#416

Merged
trianglesphere merged 4 commits intomainfrom
jg/fix_l2_output_timestamps_v2
May 13, 2022
Merged

Fix L2 Output Timestamps#416
trianglesphere merged 4 commits intomainfrom
jg/fix_l2_output_timestamps_v2

Conversation

@trianglesphere
Copy link
Contributor

Description
There was an off by one error in the L2 Output Oracle contract in the timestamp to
block number conversion. In addition, the L2 Output Submitter (op proposer) did
not recognize that there was a mismatch between the timestamp/block number in
the header and the timestamp/block number from the contract.

This bug causes problems when trying to do withdrawals.

Metadata

  • Fixes ENG-2128

@trianglesphere trianglesphere force-pushed the jg/fix_l2_output_timestamps_v2 branch from 835dc90 to e1be3ce Compare May 5, 2022 23:07
Comment on lines +209 to +213
l2Header, err := d.cfg.L2Client.HeaderByNumber(ctx, nextCheckpointBlock)
if err != nil {
return nil, fmt.Errorf("error resolving checkpoint block: %v", err)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not change the outputRootAtBlock RPC in the rollup-node to serve the L2 block header. That way it's always consistent. The l2 output root submitter shouldn't even need to directly talk to a L2 geth node I think

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That wouldn't solve the original issue nor a race condition on the proof. Original is that the timestamp -> block number conversion was wrong. The race condition is that we get by block number but don't check the proof against the header - the only way to do this is by checking the state root.

Copy link
Contributor

Choose a reason for hiding this comment

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

FWIW, the server of the output root already does get a header and verifies the proof against the state root:

if err := proof.Verify(head.Root); err != nil {

It would be nice to serve the header in the same RPC response, and then the timestamp / block number can be used without extra calls with a separate RPC client to L2 geth.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good idea and you should make an issue for it.

@codecov-commenter
Copy link

codecov-commenter commented May 9, 2022

Codecov Report

Merging #416 (e56e6ca) into main (55db896) will decrease coverage by 0.86%.
The diff coverage is 25.00%.

@@            Coverage Diff             @@
##             main     #416      +/-   ##
==========================================
- Coverage   53.12%   52.26%   -0.87%     
==========================================
  Files          62       63       +1     
  Lines        6500     6703     +203     
==========================================
+ Hits         3453     3503      +50     
- Misses       2608     2764     +156     
+ Partials      439      436       -3     
Impacted Files Coverage Δ
l2os/bindings/l2oo/l2_output_oracle.go 8.60% <ø> (ø)
l2os/drivers/l2output/driver.go 65.19% <14.28%> (-2.05%) ⬇️
opnode/rollup/driver/step.go 61.37% <100.00%> (ø)
opnode/cmd/main.go 7.89% <0.00%> (-0.56%) ⬇️
opnode/service.go 0.00% <0.00%> (ø)
opnode/cmd/stateviz/main.go 0.00% <0.00%> (ø)
opnode/node/node.go 60.15% <0.00%> (+0.15%) ⬆️
opnode/p2p/gossip.go 67.36% <0.00%> (+1.04%) ⬆️
l2os/txmgr/txmgr.go 93.78% <0.00%> (+2.48%) ⬆️
opnode/rollup/driver/state.go 75.24% <0.00%> (+4.00%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 55db896...e56e6ca. Read the comment docs.

@trianglesphere trianglesphere force-pushed the jg/fix_l2_output_timestamps_v2 branch from 3f24a1e to 746e1d3 Compare May 9, 2022 21:02
@trianglesphere trianglesphere force-pushed the jg/fix_l2_output_timestamps_v2 branch from 746e1d3 to 65980a2 Compare May 12, 2022 17:40
@trianglesphere
Copy link
Contributor Author

This is ready for a re-review. The test is failing because the sequencer is reorging on the L1 Info transaction between when it sequences it and when it verifies it

// unsafe portion of the chain which gets reorged on startup. The sequencer has an out of date view
// when it creates it's first block and uses and old L1 Origin. It then does not submit a batch
// for that block and subsequently reorgs to match what the verifier derives when running the
// reconcillation process.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd really like to know why this is happening

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm assuming that it starts block production prior to having a full up to date view of the L1 chain.

@trianglesphere trianglesphere merged commit fb044a6 into main May 13, 2022
@trianglesphere trianglesphere deleted the jg/fix_l2_output_timestamps_v2 branch May 13, 2022 16:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants