-
Notifications
You must be signed in to change notification settings - Fork 463
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.
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
feat(network): implement EIP-7801 Sharded Blocks Subprotocol (etha) #7973
base: master
Are you sure you want to change the base?
Conversation
Added base EthaProtocol class implementing the sharded blocks subprotocol as specified in EIP-7801.
Added message codes for the etha protocol (GetShardedBlocks, ShardedBlocks, NewShardedBlock)
Added message class for requesting sharded blocks as per EIP-7801
Added message class for responding with sharded blocks data as specified in EIP-7801
Added message class for announcing new sharded blocks as defined in EIP-7801
Added protocol handler for processing etha protocol messages (EIP-7801)
Added implementation for handling GetShardedBlocks, ShardedBlocks and NewShardedBlock messages - GetShardedBlocks: Responds with requested blocks from block tree - ShardedBlocks: Processes received blocks and adds them to block tree - NewShardedBlock: Handles announcements of new sharded blocks
Added unit tests for the etha protocol message handling: - Test for GetShardedBlocks message processing
Added additional test cases: - Test for ShardedBlocks message processing - Test for NewShardedBlock message with unknown block - Test for NewShardedBlock message with known block
Added: - EthaProtocolFactory for creating protocol instances - Registration of etha protocol in NetworkModule
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about ProtocolHandler, but the messages and serializers could be reused from standard eth
protocol?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First of all, thank you for your contribution! Generally it looks good, but need some nitpicks. I left a few comments
|
||
// Register etha protocol factory | ||
services.AddSingleton<EthaProtocolFactory>(); | ||
services.AddSingleton<IProtocolFactory>(sp => sp.GetRequiredService<EthaProtocolFactory>()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need this class, we don't need EthaProtocolFactory
at all
@@ -209,6 +219,9 @@ await StartDiscovery().ContinueWith(initDiscoveryTask => | |||
ThisNodeInfo.AddInfo("Client id :", ProductInfo.ClientId); | |||
ThisNodeInfo.AddInfo("This node :", $"{_api.Enode.Info}"); | |||
ThisNodeInfo.AddInfo("Node address :", $"{_api.Enode.Address} (do not use as an account)"); | |||
|
|||
// Register etha protocol | |||
_protocolsManager.AddProtocol(_ethaProtocolFactory); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for it
|
||
// Add etha protocol capability | ||
_api.ProtocolsManager!.AddSupportedCapability(new Capability(Protocol.Etha, 1)); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unconditional, so should be added to default capabilities. So remove from here, add in ProtocolsManager.DefaultCapabilities
, adjust tests
{ | ||
public override string Name => "etha"; | ||
public override byte Version => 1; | ||
public override int MessageIdSpaceSize => 3; // Number of messages in protocol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would put it directly to EthaProtocolHandler
to make it similar to implementations of other protocols. EthaProtocol
can be removed.
private readonly INodeStatsManager _nodeStatsManager; | ||
private readonly ILogManager _logManager; | ||
|
||
public EthaProtocolFactory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not needed, we can initialize EthaProtocolHandler
directly where needed. This class can be removed
{ | ||
private readonly IBlockTree _blockTree; | ||
private readonly ILogger _logger; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we can add data which is currently in EthaProtocol
@@ -0,0 +1,9 @@ | |||
public static class Protocol | |||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove it please and only update Nethermind.Network.Contract/P2P/Protocol
INethermindApi api, | ||
IProtocolsManager protocolsManager, | ||
IProtocolValidator protocolValidator, | ||
EthaProtocolFactory ethaProtocolFactory) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need it, this class shouldn't be changed at all
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can simply undo all changes in this class. What to do instead is described in other comments :)
{ | ||
case EthaMessageCode.GetShardedBlocks: | ||
Handle(Deserialize<GetShardedBlocksMessage>(packet.Data)); | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be good to add metrics to all messages. Compare please with implementations of other protocols. But it's just good-to-have, not hard requirement - we can add it later, in another PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
e.g. GetShardedBlocksReceived
and here Metrics.GetShardedBlocksReceived++
|
||
namespace Nethermind.Network.P2P.Subprotocols.Etha.Messages | ||
{ | ||
public class GetShardedBlocksMessageSerializer : IMessageSerializer<GetShardedBlocksMessage> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would be good to separate every serializer class to another file
Implements EIP-7801: Sharded Blocks Subprotocol (etha)
Fixes #7944
Changes
Types of changes
What types of changes does your code introduce?
Testing
Requires testing
If yes, did you write tests?
Notes on testing
Added comprehensive test coverage including:
Documentation
Requires documentation update
Documentation needed for the new etha protocol implementation and its usage.
Requires explanation in Release Notes
Added support for EIP-7801 Sharded Blocks Subprotocol (etha), enabling efficient sharded block synchronization between nodes.
Remarks
This implementation follows the specification defined in EIP-7801 and adds a new subprotocol for handling sharded blocks in Ethereum network communication.