Restore Taiko FCU timestamp validation allowing equal timestamps#10810
Restore Taiko FCU timestamp validation allowing equal timestamps#10810jmadibekov merged 8 commits intomasterfrom
Conversation
…amps Commit 195829e (Bal devnet 2) renamed IsPayloadAttributesTimestampValid, removed the virtual keyword, and deleted TaikoForkchoiceUpdatedHandler's override that allowed equal timestamps. This broke Taiko Pacaya where multiple L2 blocks derived from a single L1 block share the same timestamp.
|
@copilot implement the following suggestion to this PR::
|
|
@smartprogrammer93 I've opened a new pull request, #10811, to work on those changes. Once the pull request is ready, I'll request review from you. |
Job-level timeout 45m → 20m, add step-level 15m timeout on the test step so timeouts show as failures instead of cancellations.
| if (newHeadBlock.Timestamp >= payloadAttributes.Timestamp) | ||
| if (payloadAttributes.Timestamp < GetMinRequiredTimestamp(newHeadBlock)) | ||
| { | ||
| string error = $"Payload timestamp {payloadAttributes.Timestamp} must be greater than block timestamp {newHeadBlock.Timestamp}."; |
There was a problem hiding this comment.
This message should be adjusted to use GetMinRequiredTimestamp too - save it in variable.
There was a problem hiding this comment.
Also not sure which would be better here < or <= (which would require amending GetMinRequiredTimestamp accordingly), WDYT?
There was a problem hiding this comment.
Regarding the first comment, done! Regarding the second, I'd rather leave it as-is (that is <) because I find that more readable.
There was a problem hiding this comment.
Actually, I went ahead with the approach @dipkakwani suggested below because I found that even more elegant :)
There was a problem hiding this comment.
But the error message for taiko is now wrong?
|
|
||
| // Taiko Pacaya allows equal timestamps because multiple L2 blocks can be derived | ||
| // from a single L1 block, all sharing the same L1 anchor timestamp. | ||
| protected override ulong GetMinRequiredTimestamp(Block newHeadBlock) => newHeadBlock.Timestamp; |
There was a problem hiding this comment.
This change will allow equal timestamps for all the forks, not just Pacaya right?
We need to have conditions to return based on the current fork, similar to taiko geth here: https://github.com/taikoxyz/taiko-geth/blob/1fda81a9372dd79056427487caef38cee2be75b5/consensus/taiko/consensus.go#L158-L168
There was a problem hiding this comment.
Actually, we do already have similar per-fork rules during block validation: https://github.com/NethermindEth/nethermind/blob/master/src/Nethermind/Nethermind.Taiko/TaikoHeaderValidator.cs#L204-L226.
And when it comes to fcu, nmc is actually correctly mirroring here taiko-geth because taiko-geth also allows equal timestamps for all forks unconditionally: https://github.com/taikoxyz/taiko-geth/blob/taiko/miner/worker.go#L174-L187.
So I don't think there is a need for this.
| return null; | ||
| } | ||
|
|
||
| protected virtual ulong GetMinRequiredTimestamp(Block newHeadBlock) => newHeadBlock.Timestamp + 1; |
There was a problem hiding this comment.
This function looks a bit random, the intention of adding 1 to the timestamp is not clear.
Do you think it wil be better to have the complete logic for checking timestamp as virtual, instead of only the minimum required timestamp? Something like IsPayloadTimestampValid in the if statement of line 296, and each module can override that function?
There was a problem hiding this comment.
Just noticed the linked PR, not sure why it was removed. The existing IsPayloadTimestampValid https://github.com/NethermindEth/nethermind/pull/10325/changes#diff-74bac6db7a134940f36bb275f9bd355e45f5428bf5f30cd4bd2b7fccd2b1b186
There was a problem hiding this comment.
Also noticed Lukaz comment, I think we can still make IsPayloadTimestampValid one-liner in the base class instead of using GetMinRequiredTimestamp.
There was a problem hiding this comment.
I like that, done!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #10810 +/- ##
===========================================
- Coverage 50.22% 0 -50.23%
===========================================
Files 3207 0 -3207
Lines 165267 0 -165267
Branches 23135 0 -23135
===========================================
- Hits 83010 0 -83010
+ Misses 76622 0 -76622
+ Partials 5635 0 -5635 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Changes
IsPayloadTimestampValidvirtual method toForkchoiceUpdatedHandler(base:>for strict increase)TaikoForkchoiceUpdatedHandleras a one-liner using>=(allowing equal timestamps)timeout-minutesfrom 45 to 20 inci-surge.ymlandci-taiko.yml, add step-level 15m timeout on the test step so timeouts surface as failures instead of cancellationsPR #10325 renamed
IsPayloadAttributesTimestampValid→ArePayloadAttributesTimestampAndSlotNumberValid, removedvirtual, and deleted the Taiko override. In Taiko, multiple L2 blocks can be derived from a single L1 block sharing the same L1 anchor timestamp, so equal timestamps must be allowed.Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Added 3 test cases in
TaikoEngineApiTestsverifying equal timestamps are accepted, greater timestamps are accepted, and lesser timestamps are rejected.Integration tests passed on this PR:
Documentation
Requires documentation update
Requires explanation in Release Notes