add reorg, sequence delayed messages functionality by tanishqjasoria · Pull Request #131 · NethermindEth/nethermind-arbitrum

tanishqjasoria · 2025-08-11T04:41:41Z

No description provided.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Copilot

Pull Request Overview

This PR adds reorg and sequence delayed message functionality to the Arbitrum module, enabling blockchain reorganization and processing of delayed messages from L1.

Key changes:

Adds reorg functionality to handle blockchain reorganizations with message reprocessing
Implements sequence delayed message processing for L1-originated delayed messages
Introduces message parsing utilities for round-trip transaction encoding/decoding

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/Nethermind.Arbitrum/Modules/IArbitrumRpcModule.cs	Adds interface definitions for Reorg and SequenceDelayedMessage RPC methods
src/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs	Implements reorg and delayed message sequencing with event handling and message reprocessing
src/Nethermind.Arbitrum/Exceptions/ArbitrumBlockProductionException.cs	New exception class for block production errors with error codes
src/Nethermind.Arbitrum/Data/Transactions/NitroL2MessageParser.cs	Adds transaction-to-message parsing for round-trip message construction
src/Nethermind.Arbitrum/Data/DigestMessageParameters.cs	Defines parameter classes for reorg and delayed message operations
src/Nethermind.Arbitrum.Test/Rpc/SequenceDelayedMessagesTests.cs	Test suite for delayed message sequencing functionality
src/Nethermind.Arbitrum.Test/Rpc/DigestMessage/NitroL2MessageParserTests.cs	Tests for round-trip message parsing functionality
src/Nethermind.Arbitrum.Test/Rpc/ArbitrumFullSimulationRunTests.cs	Removes TODO comment from existing test

src/Nethermind.Arbitrum/Exceptions/ArbitrumBlockProductionException.cs

src/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs

…ption.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

AnkushinDaniil

There is some commented code that can possibly be removed.
Good job!

src/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs

AnkushinDaniil · 2025-08-11T14:03:57Z

src/Nethermind.Arbitrum/Data/Transactions/NitroL2MessageParser.cs

+                    (ulong)_decoder.GetLength(tx, RlpBehaviors.SkipTypedWrapping) + 1);
+                stream.Write(sizeBuf);
+                stream.WriteByte((byte)ArbitrumL2MessageKind.SignedTx);
+                _decoder.Encode(stream, tx, RlpBehaviors.SkipTypedWrapping);


It seems that it doesn't filter failed transactions as Nitro does

yes, we dont have hooks implemented yet
was planning to discuss this today

AnkushinDaniil · 2025-08-11T14:14:37Z

src/Nethermind.Arbitrum/Data/Transactions/NitroL2MessageParser.cs

+    private static readonly TxDecoder _decoder;
+    static NitroL2MessageParser()
+    {
+        TxDecoder decoder = TxDecoder.Instance;


Can we use a safer private, isolated decoder?

that needed a change in nethermind and that is why i did it this way.

will change it

codecov · 2025-08-12T06:13:34Z

Codecov Report

❌ Patch coverage is 49.51923% with 105 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.72%. Comparing base (1b15f82) to head (e326f74).

Files with missing lines	Patch %	Lines
...c/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs	42.23%	93 Missing ⚠️
...ethermind.Arbitrum/Data/DigestMessageParameters.cs	28.57%	10 Missing ⚠️
...rum/Exceptions/ArbitrumBlockProductionException.cs	0.00%	2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (49.51%) is below the target coverage (60.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #131      +/-   ##
==========================================
- Coverage   83.93%   83.72%   -0.22%     
==========================================
  Files          89       90       +1     
  Lines        5123     5314     +191     
  Branches      726      762      +36     
==========================================
+ Hits         4300     4449     +149     
- Misses        823      865      +42

Flag	Coverage Δ
unittests	`83.72% <49.51%> (-0.22%)`	⬇️

Files with missing lines	Coverage Δ
...Arbitrum/Data/Transactions/NitroL2MessageParser.cs	`80.20% <100.00%> (+5.88%)`	⬆️
...rum/Exceptions/ArbitrumBlockProductionException.cs	`0.00% <0.00%> (ø)`
...ethermind.Arbitrum/Data/DigestMessageParameters.cs	`72.50% <28.57%> (-23.66%)`	⬇️
...c/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs	`63.88% <42.23%> (-25.70%)`	⬇️

... and 7 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

wurdum

From PR description it's not clear whether it's finished PR and what is ommitted here. Lack of tests for reorg, unused vars and lots of todos hints there are more work to finish, but it's not communicated.

Also, we need to refactor ArbitrumRpcModule to remove blockchain logic from it. It should be responsible only for RPC transport specific concerns. Let's introduce something like ArbitrumBlockchain, ArbitrumFacade, ArbitrumApi, etc. and move block production, reorg, etc. logic there. That could be a separate PR.

src/Nethermind.Arbitrum/Data/Transactions/NitroL2MessageParser.cs

src/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs

wurdum · 2025-08-12T10:50:16Z

src/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs

+        /// <summary>
+        /// Notifies that a resequencing (reorg) operation is about to start.
+        /// </summary>
+        public event EventHandler<ResequenceOperationNotifier>? ResequenceOperationStarting;


What's the purpose of this event? Is it being used?

src/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs

wurdum · 2025-08-12T10:52:31Z

src/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs

+                ResultWrapper<MessageResult> msgResult;
+                try
+                {
+                    Block? block = await ProduceBlockWhileLockedAsync(parameters.Message, headBlockHeader.Number + 1, headBlockHeader);


Can we make ProduceBlockWhileLockedAsync return ResultWrapper to avoid magic exceptions?

src/Nethermind.Arbitrum/Modules/ArbitrumRpcModule.cs

wurdum · 2025-08-12T11:10:37Z

src/Nethermind.Arbitrum.Test/Rpc/DigestMessage/NitroL2MessageParserTests.cs

        }

+        [Test]
+        public static void Parse_RoundTrip_EthLegacy_ParsesCorrectly()


Test name doesn't follow the convention.

wurdum · 2025-08-12T11:11:19Z

src/Nethermind.Arbitrum.Test/Rpc/DigestMessage/NitroL2MessageParserTests.cs

+        }
+
+        [Test]
+        public static void Parse_RoundTrip_DynamicFeeTx_ParsesCorrectly()


Test name doesn't follow the convention.

wurdum · 2025-08-12T11:11:41Z

src/Nethermind.Arbitrum.Test/Rpc/DigestMessage/NitroL2MessageParserTests.cs

+        }
+
+        [Test]
+        public static void Parse_RoundTrip_EthLegacyAndDynamicFeeTx_ParsesCorrectly()


Test name doesn't follow the convention.

tanishqjasoria · 2025-08-26T09:20:46Z

replaced by #172

tanishqjasoria and others added 30 commits July 30, 2025 09:47

use block header and add tests

86bc18e

try somthing

459a8bf

final changes

7f50c55

Merge branch 'main' into tanishq/fix/digest-message

221a69c

remove reorg test

11a108c

remove unused stuuf

9a81581

update name

f2cc003

revert change

054c4b9

address comments and add mutex

1415076

whitespace

76f59a9

Apply suggestion from @Copilot

89c6614

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix logs

71a3833

add parameters

d1f3615

Add reorg functions

990fa03

Refac

436d69e

Add reorg sequencing skelton

d7873e5

Add reverse parser

b8f435b

complete function implementations

e6bd6dd

Refactor

aed052d

more changes

931c86d

add another function

ffa4ab5

fix decoer

12e4c35

fix encoder and tests

d745432

fix

df1439b

Merge branch 'main' into tanishq/feat/reorg-new

e0a1153

Rearrange public/private

0098e13

refactor

cb8b98c

add tests

0e8f511

refac

65adaf4

another refactor

456ccb3

tanishqjasoria added 2 commits August 11, 2025 10:49

format

3887843

refac

27932b9

tanishqjasoria marked this pull request as ready for review August 11, 2025 05:44

Copilot AI review requested due to automatic review settings August 11, 2025 05:44

tanishqjasoria requested review from AnkushinDaniil, damian-orzechowski, hudem1 and wurdum as code owners August 11, 2025 05:44

Copilot AI reviewed Aug 11, 2025

View reviewed changes

tanishqjasoria and others added 2 commits August 11, 2025 11:24

Update src/Nethermind.Arbitrum/Exceptions/ArbitrumBlockProductionExce…

78a1755

…ption.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix issues

7ef0648

AnkushinDaniil approved these changes Aug 11, 2025

View reviewed changes

tanishqjasoria added 2 commits August 12, 2025 11:36

inside namespace

dda695b

fix issue

e326f74

wurdum reviewed Aug 12, 2025

View reviewed changes

tanishqjasoria marked this pull request as draft August 14, 2025 09:20

tanishqjasoria added 3 commits August 15, 2025 13:35

fix issues

bbf8546

Merge remote-tracking branch 'origin/main' into tanishq/feat/reorg-new

e9b54ab

fix issues and refac

702f759

tanishqjasoria closed this Aug 26, 2025

Comments

Conversation

tanishqjasoria commented Aug 11, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

AnkushinDaniil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Aug 12, 2025

Codecov Report

Uh oh!

wurdum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tanishqjasoria commented Aug 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants