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 into from
May 13, 2022
Merged

Fix L2 Output Timestamps #416

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cde4a1c
l2os: Show bug with timestamps
trianglesphere Apr 28, 2022
65980a2
L2OutputOracle: Fix off by one error
trianglesphere May 3, 2022
4607df9
opnode: Fix TestL2OutputSubmitter Test
trianglesphere May 12, 2022
e56e6ca
fix lint
trianglesphere May 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion l2os/bindings/l2oo/l2_output_oracle.go
View file
Open in desktop

Large diffs are not rendered by default.

9 changes: 9 additions & 0 deletions l2os/drivers/l2output/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,15 @@ func (d *Driver) CraftTx(
return nil, fmt.Errorf("error resolving checkpoint block: %v", err)
}

l2Header, err := d.cfg.L2Client.HeaderByNumber(ctx, nextCheckpointBlock)
if err != nil {
return nil, fmt.Errorf("error resolving checkpoint block: %v", err)
}
Comment on lines +210 to +213
Copy link
Contributor

@protolambda protolambda May 9, 2022

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

@trianglesphere trianglesphere May 9, 2022

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

@protolambda protolambda May 9, 2022

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:

optimistic-specs/opnode/node/api.go

Line 73 in 5189b9a

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

@trianglesphere trianglesphere May 9, 2022

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.


if l2Header.Time != timestamp.Uint64() {
return nil, fmt.Errorf("invalid timestamp: next timestamp is %v, timestamp of block is %v", timestamp, l2Header.Time)
}

opts, err := bind.NewKeyedTransactorWithChainID(
d.cfg.PrivKey, d.cfg.ChainID,
)
Expand Down
2 changes: 1 addition & 1 deletion opnode/rollup/driver/step.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,7 @@ func attributesMatchBlock(attrs *l2.PayloadAttributes, parentHash common.Hash, b
}
for i, otx := range attrs.Transactions {
if expect := block.Transactions[i]; !bytes.Equal(otx, expect) {
return fmt.Errorf("transaction %d does not match. expected: %x. got: %x", i, expect, otx)
return fmt.Errorf("transaction %d does not match. expected: %v. got: %v", i, expect, otx)
}
}
return nil
Expand Down
9 changes: 9 additions & 0 deletions opnode/test/system_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,15 @@ func TestL2OutputSubmitter(t *testing.T) {
initialSroTimestamp, err := l2OutputOracle.LatestBlockTimestamp(&bind.CallOpts{})
require.Nil(t, err)

// Wait until the second output submission from L2. The output submitter submits outputs from the
// 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

@tynes tynes May 12, 2022

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

@trianglesphere trianglesphere May 13, 2022

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.

l2Verif := sys.Clients["verifier"]
_, err = waitForBlock(big.NewInt(6), l2Verif, 10*time.Duration(cfg.RollupConfig.BlockTime)*time.Second)
require.Nil(t, err)

// Wait for batch submitter to update L2 output oracle.
timeoutCh := time.After(15 * time.Second)
for {
Expand Down
2 changes: 1 addition & 1 deletion packages/contracts/.gas-snapshot
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ L2OutputOracleTest:testCannot_computePreHistoricalL2BlockNumber() (gas: 11093)
L2OutputOracleTest:testCannot_deleteL2Output_ifNotSequencer() (gas: 18793)
L2OutputOracleTest:testCannot_deleteWrongL2Output() (gas: 77307)
L2OutputOracleTest:test_appendingAnotherOutput() (gas: 68605)
L2OutputOracleTest:test_computeL2BlockNumber() (gas: 15003)
L2OutputOracleTest:test_computeL2BlockNumber() (gas: 14755)
L2OutputOracleTest:test_constructor() (gas: 33752)
L2OutputOracleTest:test_deleteL2Output() (gas: 64320)
L2OutputOracleTest:test_getL2Output() (gas: 74601)
Expand Down
1 change: 0 additions & 1 deletion packages/contracts/contracts/L1/L2OutputOracle.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,7 +198,6 @@ contract L2OutputOracle is Ownable {
unchecked {
return
HISTORICAL_TOTAL_BLOCKS +
1 +
((_l2timestamp - STARTING_BLOCK_TIMESTAMP) / L2_BLOCK_TIME);
}
}
Expand Down
6 changes: 3 additions & 3 deletions packages/contracts/contracts/test/L2OutputOracle.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,17 +70,17 @@ contract L2OutputOracleTest is L2OutputOracle_Initializer {
function test_computeL2BlockNumber() external {
// Test with the timestamp of the very first appended block
uint256 argTimestamp = startingBlockTimestamp;
uint256 expected = historicalTotalBlocks + 1;
uint256 expected = historicalTotalBlocks;
assertEq(oracle.computeL2BlockNumber(argTimestamp), expected);

// Test with an integer multiple of the l2BlockTime
argTimestamp = startingBlockTimestamp + 20;
expected = historicalTotalBlocks + 1 + (20 / l2BlockTime);
expected = historicalTotalBlocks + (20 / l2BlockTime);
assertEq(oracle.computeL2BlockNumber(argTimestamp), expected);

// Test with a remainder
argTimestamp = startingBlockTimestamp + 33;
expected = historicalTotalBlocks + 1 + (33 / l2BlockTime);
expected = historicalTotalBlocks + (33 / l2BlockTime);
assertEq(oracle.computeL2BlockNumber(argTimestamp), expected);
}

Expand Down