Optimism CL P2P #7297

deffrian · 2024-07-30T10:44:36Z

Fixes Closes Resolves #
Implementation of p2p part op Optimism CL client. We have few todos. Discovery is blocked by discv5 implementation. This is why we have pre-set peers. And GetBlockByNumber protocol. This is also kinda blocked by libp2p. I have a working implementation but it's super ugly. So better to merge it later.

Changes

Adds op cl client

Types of changes

What types of changes does your code introduce?

Bugfix (a non-breaking change that fixes an issue)
New feature (a non-breaking change that adds functionality)
Breaking change (a change that causes existing functionality not to work as expected)
Optimization
Refactoring
Documentation update
Build-related changes
Other: Description

Testing

Requires testing

Yes
No

If yes, did you write tests?

Yes
No

Documentation

Requires documentation update

Yes
No

Requires explanation in Release Notes

Yes
No

# Conflicts: # src/Nethermind/Nethermind.Optimism/OptimismPlugin.cs

# Conflicts: # src/Nethermind/Nethermind.Runner/configs/op-sepolia.json

emlautarom1 · 2024-12-18T14:06:45Z

src/Nethermind/Nethermind.Optimism/CL/P2P/PayloadDecoder.cs

+
+public class PayloadDecoder : IPayloadDecoder
+{
+    public static readonly PayloadDecoder Instance = new();


If we're going the singleton route then we should make the constructor private

emlautarom1 · 2024-12-18T14:09:01Z

src/Nethermind/Nethermind.Optimism/CL/P2P/PayloadDecoder.cs

+        }
+
+        int offset = 0;
+        payload.ParentBeaconBlockRoot = new(data[offset..(offset + 32)]);


I think we can leverage the ReadOnlySpan<byte>.TakeAndMove() extension method to avoid the bookkeeping of offset

We don't have constructor(ReadOnlySpan) for some types. Imo we can leave it as is for now, add span constructors later and adjust this function. wdyt?

Sounds good, lets proceed with the current approach for now. Just keep TODO to remind ourselves about it later.

TakeAndMove is our extension method, you can create one for Span<> too. But ReadOnlySpan should probably work with all below types - if it doesn't we should consider adding it.

emlautarom1 · 2024-12-18T14:12:49Z

src/Nethermind/Nethermind.Optimism/CL/P2P/PayloadDecoder.cs

+
+namespace Nethermind.Optimism.CL;
+
+public class PayloadDecoder : IPayloadDecoder


It would be nice to have some tests for both DecodePayload and DecodeTransactions, in particular for the later one since the implementation does not look straightforward

emlautarom1 · 2024-12-18T14:17:05Z

src/Nethermind/Nethermind.Optimism/CL/OptimismCL.cs

+        _logger = logManager.GetClassLogger();
+        _chainSpecEngineParameters = engineParameters;
+
+        _p2p = new OptimismCLP2P(specProvider.ChainId, engineParameters.Nodes, config, _chainSpecEngineParameters.SequencerP2PAddress, timestamper, logManager, engineRpcModule);


We never dispose of _p2p. Either we make OptimismCL an instance of IDisposable and call _p2p.Dispose, or we make Start construct an instance and dispose of it by using it.

emlautarom1 · 2024-12-18T14:24:17Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+
+        _blocksV2TopicId = $"/optimism/{chainId}/2/blocks";
+
+        _serviceProvider = new ServiceCollection()


Do we actually need a ServiceProvider? Could we store these dependencies as private readonly fields?

This is needed for libp2p DI.

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

emlautarom1 · 2024-12-18T14:27:14Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+        }
+    }
+
+    private bool TryValidateAndDecodePayload(byte[] msg, [MaybeNullWhen(false)] out ExecutionPayloadV3 payload)


Should we return also an error message or we don't care about it?

We don't care. Just reject it

emlautarom1 · 2024-12-18T14:27:42Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+            return false;
+        }
+
+        byte[] decompressed = new byte[length];


stackalloc?

length might be pretty big. Not sure if it stack overflow safe

ArrayPoolList then to use pooled memory?

src/Nethermind/Nethermind.Optimism/CL/P2P/P2PBlockValidator.cs

emlautarom1 · 2024-12-18T15:07:26Z

src/Nethermind/Nethermind.Optimism/CL/P2P/IP2PBlockValidator.cs

+public interface IP2PBlockValidator
+{
+    ValidityStatus Validate(ExecutionPayloadV3 payload, P2PTopic topic);
+    ValidityStatus ValidateSignature(byte[] payloadData, byte[] signature);


If payloadData and signature are only read then we could use ReadOnlySpan<byte> for both args

I'm not sure why but SpanSecP256k1.RecoverKeyFromCompact needs Span, not ReadOnlySpan

LukaszRozmej · 2024-12-20T11:56:55Z

src/Nethermind/Nethermind.Optimism/OptimismPlugin.cs

+        StepDependencyException.ThrowIfNull(_api.EthereumEcdsa);
+
+        ICLConfig clConfig = _api.Config<ICLConfig>();
+        if (clConfig.Enabled)
+        {
+            CLChainSpecEngineParameters chainSpecEngineParameters = _api.ChainSpec.EngineChainSpecParametersProvider
+                .GetChainSpecParameters<CLChainSpecEngineParameters>();
+            _cl = new OptimismCL(_api.SpecProvider, chainSpecEngineParameters, clConfig, _api.EthereumJsonSerializer,
+                _api.EthereumEcdsa, _api.Timestamper, _api!.LogManager, opEngine);
+            _cl.Start();
+        }
+


Why not in InitNetworkProtocol?

This is CL startup code. In the future it will launch all CL components

LukaszRozmej · 2024-12-20T12:01:29Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+            return false;
+        }
+
+        byte[] decompressed = new byte[length];


ArrayPoolList then to use pooled memory?

LukaszRozmej · 2024-12-20T12:02:05Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+    private async void OnMessage(byte[] msg)
+    {
+        await _semaphore.WaitAsync();
+        try
+        {
+            if (TryValidateAndDecodePayload(msg, out var payload))
+            {
+                if (_logger.IsTrace) _logger.Trace($"Received payload prom p2p: {payload}");
+
+                if (_headPayloadNumber >= (ulong)payload.BlockNumber)
+                {
+                    // Old payload. skip
+                    return;
+                }
+
+                if (await SendNewPayloadToEL(payload) && await SendForkChoiceUpdatedToEL(payload.BlockHash))
+                {
+                    _headPayloadNumber = (ulong)payload.BlockNumber;
+                }
+            }
+        }
+        finally
+        {
+            _semaphore.Release();
+        }
+    }


Potentially: Maybe channel better than lock?

LukaszRozmej · 2024-12-20T12:02:39Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+        byte[] signature = decompressed[0..65];
+        byte[] payloadData = decompressed[65..];


use spans instead of allocating arrays?

Reply for the comment bellow: Payloads are ssz encoded, so data offsets are encoded and they are relative to the beginning. We potentially can calculate total prefix offset and subtract it from encoded offsets, but I don't think it's a good idea.

LukaszRozmej · 2024-12-20T12:07:10Z

src/Nethermind/Nethermind.Optimism/CL/P2P/PayloadDecoder.cs

+        }
+
+        int offset = 0;
+        payload.ParentBeaconBlockRoot = new(data[offset..(offset + 32)]);


TakeAndMove is our extension method, you can create one for Span<> too. But ReadOnlySpan should probably work with all below types - if it doesn't we should consider adding it.

emlautarom1

LGTM as an initial PR. Some small comments though.

emlautarom1 · 2024-12-27T15:01:19Z

src/Nethermind/Nethermind.Optimism/CL/P2P/PayloadDecoder.cs

+        offset += 256;
+        payload.PrevRandao = new(data[offset..(offset + 32)]);
+        offset += 32;
+        payload.BlockNumber = (long)BitConverter.ToUInt64(data[offset..(offset + 8)]);


Not sure about this one but I got suggested some time ago that we should use BinaryPrimitives rather than BitConverter (performance?)

emlautarom1 · 2024-12-27T15:03:35Z

src/Nethermind/Nethermind.Optimism/CL/P2P/P2PBlockValidator.cs

+        // domain(all zeros) + chain id + payload hash
+        Span<byte> sequencerSignedData = stackalloc byte[32 + 32 + 32];
+
+        // Array.Copy(_chainId, 0, new byte[10], 32, 32);


Commented out code

emlautarom1 · 2024-12-27T15:06:07Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+        _ = _router.RunAsync(_localPeer, new Settings
+        {
+            DefaultSignaturePolicy = Settings.SignaturePolicy.StrictNoSign,
+            GetMessageId = (message => CalculateMessageId(message))


Suggested change

GetMessageId = (message => CalculateMessageId(message))

GetMessageId = CalculateMessageId

LukaszRozmej · 2025-01-02T10:19:38Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+        catch (Exception e)
+        {
+            if (_logger.IsError) _logger.Error("Unhandled exception in Optimism CL P2P:", e);
+        }


unhandled exceptions in async void methods crash the process

LukaszRozmej · 2025-01-02T12:39:55Z

src/Nethermind/Nethermind.Optimism/CL/P2P/OptimismCLP2P.cs

+
+    private async Task MainLoop()
+    {
+        while (true)


maybe await foreach (ExecutionPayloadV3 payload in reader.ReadAllAsync()) instead?

Will we ever close the channel, shouldn't we close it when app close (we have a CancelationToken available for that)?

deffrian added 2 commits September 7, 2024 20:53

P2P

be22107

Blob support

f7266d0

deffrian force-pushed the feature/op-cl branch from b83d57f to f7266d0 Compare September 7, 2024 17:57

deffrian force-pushed the feature/op-cl branch from ca5fd35 to f7266d0 Compare October 28, 2024 09:15

deffrian added 23 commits October 28, 2024 12:27

Add decoders

7e78b03

Merge branch 'master' into feature/op-cl

d58ffef

# Conflicts: # src/Nethermind/Nethermind.Optimism/OptimismPlugin.cs

nuget libp2p

702bad1

Delete submodule

af7dcb2

Span batch decoder

52446cf

Merge branch 'master' into feature/op-cl

3b08e35

# Conflicts: # src/Nethermind/Nethermind.Runner/configs/op-sepolia.json

Fix decoding

50c6309

Merge branch 'master' into feature/op-cl

fe7868f

Fix build

a0da6fc

Fix p2p

a507ee4

More derivation

f48b495

Merge branch 'master' into feature/op-cl

0c35f65

# Conflicts: # src/Nethermind/Nethermind.Runner/configs/op-sepolia.json

Add pubKey verification

9086435

Fix payload decoder & peers

971386d

Change signature validation

e962edf

Add peers

eecb968

sequencerP2PAddress

8baef3e

Merge branch 'master' into feature/op-cl

2f05fe1

Fix formatting

3a94ced

Fix formatting

8230d31

Remove derivation

1b8000e

Cleanup

9b669d4

Merge branch 'master' into feature/op-cl

aaf2f44

deffrian changed the title ~~[WIP]Optimism CL~~ [WIP]Optimism CL P2P Dec 18, 2024

Limit decoded data size

e51e2bc

emlautarom1 self-requested a review December 18, 2024 13:57

emlautarom1 requested changes Dec 18, 2024

View reviewed changes

deffrian added 5 commits December 19, 2024 11:28

Fix suggestions

4a19925

Merge branch 'master' into feature/op-cl

e1f9b9d

Fix formating

dc2d0de

Merge remote-tracking branch 'origin/feature/op-cl' into feature/op-cl

9c30ea4

Add payload decoder tests

9275b17

deffrian changed the title ~~[WIP]Optimism CL P2P~~ Optimism CL P2P Dec 20, 2024

deffrian marked this pull request as ready for review December 20, 2024 10:30

deffrian requested a review from rubo as a code owner December 20, 2024 10:30

LukaszRozmej reviewed Dec 20, 2024

View reviewed changes

deffrian added 2 commits December 26, 2024 11:07

Fix suggestions

bf593a7

Merge branch 'master' into feature/op-cl

543584f

emlautarom1 approved these changes Dec 27, 2024

View reviewed changes

deffrian and others added 2 commits December 30, 2024 10:48

Fix suggestions

85670e9

Add catch on async void to avoid process crash + refactors

68e0aaa

LukaszRozmej reviewed Jan 2, 2025

View reviewed changes

Use channels

da337da

LukaszRozmej reviewed Jan 2, 2025

View reviewed changes

deffrian added 2 commits January 2, 2025 15:57

Close channel

8343884

Minor adjustments

c04d17c

LukaszRozmej approved these changes Jan 2, 2025

View reviewed changes

Merge branch 'master' into feature/op-cl

2b46ff9

deffrian merged commit 6bb6835 into master Jan 2, 2025
79 checks passed

deffrian deleted the feature/op-cl branch January 2, 2025 14:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optimism CL P2P #7297

Optimism CL P2P #7297

deffrian commented Jul 30, 2024 •

edited

Loading

emlautarom1 Dec 18, 2024

emlautarom1 Dec 18, 2024

deffrian Dec 19, 2024

emlautarom1 Dec 19, 2024

LukaszRozmej Dec 20, 2024

emlautarom1 Dec 18, 2024

emlautarom1 Dec 18, 2024

emlautarom1 Dec 18, 2024

deffrian Dec 19, 2024

emlautarom1 Dec 18, 2024

deffrian Dec 19, 2024

emlautarom1 Dec 18, 2024

deffrian Dec 19, 2024

LukaszRozmej Dec 20, 2024

emlautarom1 Dec 18, 2024

deffrian Dec 19, 2024

LukaszRozmej Dec 20, 2024

deffrian Dec 26, 2024

LukaszRozmej Dec 20, 2024

LukaszRozmej Dec 20, 2024

LukaszRozmej Dec 20, 2024

deffrian Dec 26, 2024 •

edited

Loading

LukaszRozmej Dec 20, 2024

emlautarom1 left a comment

emlautarom1 Dec 27, 2024

emlautarom1 Dec 27, 2024

emlautarom1 Dec 27, 2024

LukaszRozmej Jan 2, 2025

LukaszRozmej Jan 2, 2025


		namespace Nethermind.Optimism.CL;

		public class PayloadDecoder : IPayloadDecoder


		_blocksV2TopicId = $"/optimism/{chainId}/2/blocks";

		_serviceProvider = new ServiceCollection()

		byte[] signature = decompressed[0..65];
		byte[] payloadData = decompressed[65..];

	GetMessageId = (message => CalculateMessageId(message))
	GetMessageId = CalculateMessageId

Optimism CL P2P #7297

Optimism CL P2P #7297

Conversation

deffrian commented Jul 30, 2024 • edited Loading

Changes

Types of changes

What types of changes does your code introduce?

Testing

Requires testing

If yes, did you write tests?

Documentation

Requires documentation update

Requires explanation in Release Notes

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

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

Choose a reason for hiding this comment

deffrian Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

emlautarom1 left a 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

deffrian commented Jul 30, 2024 •

edited

Loading

deffrian Dec 26, 2024 •

edited

Loading