feat(protocol): apply multiple improvements for "ontake" fork [do not merge] #17734

dantaik · 2024-07-04T08:51:40Z

BEGIN_COMMIT_OVERRIDE
feat(protocol): allow contract proposers to use calldata for DA
feat(protocol): add preconfirmation support based on #17654 (with some renaming) (#14793)
feat(protocol): add proposeBlock2 in TaikoL1.sol and approve2 in GuardianProver.sol
refactor(protocol): avoid writing livenessBond, proposedAt, and proposedIn to storage
refactor(protocol): enable AddressManager to cache address(0)
END_COMMIT_OVERRIDE

This PR is ready to be reviewed again for merge.

#17679) BEGIN_COMMIT_OVERRIDE feat(protocol): allow contract proposers to use calldata as well feat(protocol): add preconfirmation support based on Brecht's PR (with some renaming) to implement #14793 feat(protocol): add proposeBlock2 in TaikoL1.sol and approve2 in GuardianProver.sol to be used post a fork refactor(protocol): avoid writing livenessBond to storage to remove unnecessary SSTORE and SLOAD. refactor(protocol): enable AddressManager to cache address(0) without reading from storage END_COMMIT_OVERRIDE

openzeppelin-code · 2024-07-04T09:03:04Z

feat(protocol): apply multiple improvements for "ontake" fork [do not merge]

Generated at commit: c5d75198feaf858c56db1bd78a78894a9f843f56

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	2 2 0 8 41 53
Dependencies	Critical High Medium Low Note Total	0 0 0 0 0 0

For more details view the full report in OpenZeppelin Code Inspector

adaki2004 · 2024-07-05T09:54:28Z

packages/protocol/contracts/L1/TaikoL1.sol

@@ -246,7 +270,7 @@ contract TaikoL1 is EssentialContract, ITaikoL1 {
            blockMaxGasLimit: 240_000_000,
            livenessBond: 125e18, // 125 Taiko token
            stateRootSyncInternal: 16,
-            checkEOAForCalldataDA: true
+            ontakeForkHeight: 324_512 * 2


Why not a constant ?

Changed to a fixed value

adaki2004 · 2024-07-05T09:57:57Z

packages/protocol/contracts/L1/libs/LibProposing.sol

+                // one
+                // in the previous L2 block.
+                if (
+                    params.anchorBlockId + 64 < block.number //


I think last info was to make it 4 epoch instead of 2. #17654 (comment)

I increased it for the testnet to make sure there is enough flexibility and depending on an L1 block that can be reorged can be avoided, though there are still other reorg risks. So "the best" value still TBD I guess.

I introduced a config var so testnets can have a different value.

adaki2004 · 2024-07-05T09:59:29Z

packages/protocol/contracts/L1/libs/LibProposing.sol

+                    params.timestamp + 64 * 12 < block.timestamp
+                        || params.timestamp > block.timestamp || params.timestamp < parentBlk.proposedAt


same here, 128 * 12 (4 epoch)

Brechtpd · 2024-07-05T17:59:26Z

packages/protocol/contracts/L1/libs/LibProposing.sol

+                // one
+                // in the previous L2 block.
+                if (
+                    params.anchorBlockId + 64 < block.number //


I increased it for the testnet to make sure there is enough flexibility and depending on an L1 block that can be reorged can be avoided, though there are still other reorg risks. So "the best" value still TBD I guess.

Brechtpd · 2024-07-05T18:00:53Z

packages/protocol/contracts/L1/libs/LibProposing.sol

-                difficulty: 0, // to be initialized below
+            meta_ = TaikoData.BlockMetadata2({
+                anchorBlockHash: blockhash(params.anchorBlockId),
+                difficulty: keccak256(abi.encode("TAIKO_DIFFICULTY", local.b.numBlocks)),


This does make the difficulty completely deterministic for an infinite amount of time. Which is probably okay, but makes it even harder to use it as some random value (which I guess people shouldn't do anyway).

Brechtpd · 2024-07-06T02:26:29Z

packages/protocol/contracts/L1/TaikoData.sol

-        uint64 proposedAt; // timestamp
-        uint64 proposedIn; // L1 block number, required/used by node/client.
+        // Before the fork, this field is the L1 timestamp when this block is proposed.
+        // After the fork, this is the timestamp of the L2 block.
+        // In a later fork, we an rename this field to `timestamp`.
+        uint64 proposedAt;
+        // Before the fork, this field is the L1 block number where this block is proposed.
+        // After the fork, this is the L1 block number input for the anchor transaction.
+        // In a later fork, we an rename this field to `anchorBlockId`.
+        uint64 proposedIn;


Same here, we could still use the values like we did previously. Or is this part of the optimization?

Moved "proposedAt" and "proposedIn" to the metadata to save gas, but since we need to write "timestamp" and "anchorBlockId" to storage, to avoid writing to a new slot, I simply reused the "proposedAt" and "proposedIn" fields without renaming them. So yes it's optimizaiton.

I think no need to write write timestamp and anchorBlockId to storage? I believe those are only used on L2, not on L1, so just having them as part of the meta hash is sufficient as far as I know.

When a new block is proposed, we now read the parent block's timestamp and anchorBlockId to make sure the new block's values are greater than its parent's. Therefore we need to write these two values into storage.

Oh right I also implemented it like that haha. In my mind this was already checked on L2 instead, but all good!

Brechtpd · 2024-07-06T02:27:30Z

packages/protocol/contracts/L1/TaikoData.sol

@@ -87,6 +95,25 @@ library TaikoData {
        address sender; // a.k.a proposer
    }

+    struct BlockMetadata2 {


We could still use BlockMetadata if we want to, or any specific reason for this change? We could just use the values as they were previously (that's what we do on the helder deployment). I guess maybe part of the optimization for the storage writing, but still stored in Transition below so not sure.

I would like to take this hard fork opportunity to further optimize storage reads/writes. With this v2 metadata definition, I removed writing "proposedAt" and "proposedIn" into the Block struct in storage. The previous two fields inside the Block struct "proposedAt", and "proposedIn" are used as the L2 block's "timestamp" and "anchorBlockHeight" instead. Otherwise, we'll introduce two new fields into the Block struct.

Brechtpd · 2024-07-06T02:38:08Z

packages/protocol/contracts/L1/tiers/TierProviderBase.sol

@@ -37,6 +37,17 @@ abstract contract TierProviderBase is ITierProvider {
            });
        }

+        if (_tierId == LibTiers.TIER_SGX2) {


This the SGX verifier with "ontake" fork support?

dantaik requested review from Brechtpd, davidtaikocha, YoGhurt111, adaki2004, smtmfft and swarna1101 July 4, 2024 08:51

dantaik marked this pull request as ready for review July 4, 2024 08:52

dantaik changed the title ~~feat(protocol): apply multiple improvements for *ontake* fork~~ feat(protocol): apply multiple improvements for "ontake" fork Jul 4, 2024

davidtaikocha added the area.protocol label Jul 5, 2024

adaki2004 reviewed Jul 5, 2024

View reviewed changes

Brechtpd reviewed Jul 6, 2024

View reviewed changes

respond to comments

3f93b70

dantaik requested review from Brechtpd and adaki2004 July 6, 2024 04:54

dantaik and others added 4 commits July 6, 2024 13:06

Update LibProposing.sol

96544ce

Update LibProposing.sol

39a784e

Merge branch 'main' into ontake_fork

a9c1d00

Update TaikoL1TestGroupBase.sol

e878336

dantaik changed the title ~~feat(protocol): apply multiple improvements for "ontake" fork~~ feat(protocol): apply multiple improvements for "ontake" fork [do not merge] Jul 7, 2024

more

c5d7519

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(protocol): apply multiple improvements for "ontake" fork [do not merge] #17734

feat(protocol): apply multiple improvements for "ontake" fork [do not merge] #17734

dantaik commented Jul 4, 2024 •

edited

Loading

openzeppelin-code bot commented Jul 4, 2024 •

edited

Loading

adaki2004 Jul 5, 2024

dantaik Jul 6, 2024

dantaik Jul 6, 2024 •

edited

Loading

adaki2004 Jul 5, 2024

Brechtpd Jul 5, 2024

dantaik Jul 6, 2024

adaki2004 Jul 5, 2024

Brechtpd Jul 5, 2024

Brechtpd Jul 5, 2024

dantaik Jul 6, 2024 •

edited

Loading

Brechtpd Jul 6, 2024

dantaik Jul 6, 2024

Brechtpd Jul 6, 2024

dantaik Jul 7, 2024

Brechtpd Jul 8, 2024

Brechtpd Jul 6, 2024

dantaik Jul 6, 2024

Brechtpd Jul 6, 2024

dantaik Jul 6, 2024

		params.timestamp + 64 * 12 < block.timestamp
		\|\| params.timestamp > block.timestamp \|\| params.timestamp < parentBlk.proposedAt

feat(protocol): apply multiple improvements for "ontake" fork [do not merge] #17734

Are you sure you want to change the base?

feat(protocol): apply multiple improvements for "ontake" fork [do not merge] #17734

Conversation

dantaik commented Jul 4, 2024 • edited Loading

openzeppelin-code bot commented Jul 4, 2024 • edited Loading

feat(protocol): apply multiple improvements for "ontake" fork [do not merge]

🚨 Report Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantaik Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantaik Jul 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantaik commented Jul 4, 2024 •

edited

Loading

openzeppelin-code bot commented Jul 4, 2024 •

edited

Loading

dantaik Jul 6, 2024 •

edited

Loading

dantaik Jul 6, 2024 •

edited

Loading