Skip to content

Restore Taiko FCU timestamp validation allowing equal timestamps #10810

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.

Sign up for GitHub

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

Merged
jmadibekov merged 8 commits into from
Mar 13, 2026
Merged

Restore Taiko FCU timestamp validation allowing equal timestamps #10810

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
2d3b223
Restore Taiko FCU timestamp validation override allowing equal timest…
jmadibekov Mar 12, 2026
e8fdfe3
Resolve comment
jmadibekov Mar 12, 2026
8bd8bb2
Reduce CI timeout for Surge/Taiko integration tests
jmadibekov Mar 12, 2026
3ff4ff8
Use variable
jmadibekov Mar 13, 2026
be50a25
Merge branch 'master' into jmadibekov/fix-taiko-fcu-timestamp-validation
jmadibekov Mar 13, 2026
40de70b
Replace GetMinRequiredTimestamp with IsPayloadTimestampValid one-liner
jmadibekov Mar 13, 2026
6020b8d
Use generic error message for payload timestamp validation
jmadibekov Mar 13, 2026
8ce5731
Merge branch 'master' into jmadibekov/fix-taiko-fcu-timestamp-validation
LukaszRozmej Mar 13, 2026
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
4 changes: 3 additions & 1 deletion src/Nethermind/Nethermind.Merge.Plugin/Handlers/ForkchoiceUpdatedHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,10 +288,12 @@ protected virtual bool IsOnMainChainBehindHead(Block newHeadBlock, ForkchoiceSta
return null;
}

protected virtual ulong GetMinRequiredTimestamp(Block newHeadBlock) => newHeadBlock.Timestamp + 1;

@dipkakwani dipkakwani Mar 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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?

@dipkakwani dipkakwani Mar 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just noticed the linked PR, not sure why it was removed. The existing IsPayloadTimestampValid https://github.com/NethermindEth/nethermind/pull/10325/changes#diff-74bac6db7a134940f36bb275f9bd355e45f5428bf5f30cd4bd2b7fccd2b1b186

@dipkakwani dipkakwani Mar 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also noticed Lukaz comment, I think we can still make IsPayloadTimestampValid one-liner in the base class instead of using GetMinRequiredTimestamp.

@jmadibekov jmadibekov Mar 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I like that, done!


protected bool ArePayloadAttributesTimestampAndSlotNumberValid(Block newHeadBlock, ForkchoiceStateV1 forkchoiceState, PayloadAttributes payloadAttributes,
[NotNullWhen(false)] out ResultWrapper<ForkchoiceUpdatedV1Result>? errorResult)
{
if (newHeadBlock.Timestamp >= payloadAttributes.Timestamp)
if (payloadAttributes.Timestamp < GetMinRequiredTimestamp(newHeadBlock))
{
string error = $"Payload timestamp {payloadAttributes.Timestamp} must be greater than block timestamp {newHeadBlock.Timestamp}.";

@LukaszRozmej LukaszRozmej Mar 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This message should be adjusted to use GetMinRequiredTimestamp too - save it in variable.

@LukaszRozmej LukaszRozmej Mar 13, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Also not sure which would be better here < or <= (which would require amending GetMinRequiredTimestamp accordingly), WDYT?

@jmadibekov jmadibekov Mar 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Regarding the first comment, done! Regarding the second, I'd rather leave it as-is (that is <) because I find that more readable.

@jmadibekov jmadibekov Mar 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Actually, I went ahead with the approach @dipkakwani suggested below because I found that even more elegant :)

@LukaszRozmej LukaszRozmej Mar 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

But the error message for taiko is now wrong?

errorResult = ForkchoiceUpdatedV1Result.Error(error, MergeErrorCodes.InvalidPayloadAttributes);
Expand Down
54 changes: 54 additions & 0 deletions src/Nethermind/Nethermind.Taiko.Test/TaikoEngineApiTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@

using NUnit.Framework;
using Nethermind.Core;
using Nethermind.Consensus.Producers;
using Nethermind.Merge.Plugin.Handlers;
using System.Threading.Tasks;
using Nethermind.Blockchain;
using Nethermind.Consensus.Processing;
using Nethermind.Consensus;
using Nethermind.Core.Crypto;
using Nethermind.Core.Specs;
using Nethermind.Logging;
using Nethermind.Merge.Plugin.BlockProduction;
Expand Down Expand Up @@ -68,4 +70,56 @@ static void AddBlock(IBlockTree blockTree, Block block)
blockTree.HeadHash.Returns(block.Hash!);
}
}

[TestCase(100ul, 100ul, true, TestName = "Equal timestamps allowed for Pacaya")]
[TestCase(100ul, 101ul, true, TestName = "Greater timestamp allowed")]
[TestCase(100ul, 99ul, false, TestName = "Lesser timestamp rejected")]
public async Task Test_ForkchoiceUpdatedHandler_Allows_Equal_Timestamps(ulong headTimestamp, ulong payloadTimestamp, bool shouldSucceed)
{
IBlockTree blockTree = Substitute.For<IBlockTree>();

Block headBlock = Build.A.Block.WithNumber(1).WithTimestamp(headTimestamp).TestObject;

blockTree.FindBlock(headBlock.Hash!, BlockTreeLookupOptions.DoNotCreateLevelIfMissing).Returns(headBlock);
blockTree.GetInfo(headBlock.Number, headBlock.Hash!).Returns((new BlockInfo(headBlock.Hash!, 0) { WasProcessed = true }, new ChainLevelInfo(true)));
blockTree.Head.Returns(headBlock);
blockTree.HeadHash.Returns(headBlock.Hash!);
blockTree.IsMainChain(headBlock.Header).Returns(true);

TaikoForkchoiceUpdatedHandler handler = new(
blockTree,
Substitute.For<IManualBlockFinalizationManager>(),
Substitute.For<IPoSSwitcher>(),
Substitute.For<IPayloadPreparationService>(),
Substitute.For<IBlockProcessingQueue>(),
Substitute.For<IBlockCacheService>(),
Substitute.For<IInvalidChainTracker>(),
Substitute.For<IMergeSyncController>(),
Substitute.For<IBeaconPivot>(),
Substitute.For<IPeerRefresher>(),
Substitute.For<ISpecProvider>(),
Substitute.For<ISyncPeerPool>(),
new MergeConfig(),
Substitute.For<ILogManager>()
);

PayloadAttributes payloadAttributes = new()
{
Timestamp = payloadTimestamp,
PrevRandao = Keccak.Zero,
SuggestedFeeRecipient = Address.Zero
};

ForkchoiceStateV1 forkchoiceState = new(headBlock.Hash!, headBlock.Hash!, headBlock.Hash!);
ResultWrapper<ForkchoiceUpdatedV1Result> result = await handler.Handle(forkchoiceState, payloadAttributes, 1);

if (shouldSucceed)
{
Assert.That(result.Data.PayloadStatus.Status, Is.EqualTo(PayloadStatus.Valid));
}
else
{
Assert.That(result.Result.Error, Does.Contain("must be greater than"));
}
}
}
4 changes: 4 additions & 0 deletions src/Nethermind/Nethermind.Taiko/Rpc/TaikoForkchoiceUpdatedHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,10 @@ protected override bool IsOnMainChainBehindHead(Block newHeadBlock, ForkchoiceSt
return blockHeader;
}

// 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;

@dipkakwani dipkakwani Mar 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@jmadibekov jmadibekov Mar 13, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

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.


protected override bool TryGetBranch(Block newHeadBlock, out Block[] blocks)
{
// Allow resetting to any block already on the main chain (including genesis)
Expand Down
Loading