Macros to define subprotocol packet ids in chain sync

[elferdo]

This PR elaborates on comments by @tomusdrw in #10315

Synchronization of a node's blockchain with other nodes happens through an exchange of packets over the network. In parity we support a number of different synchronization protocols that are transmitted and received as devp2p subprotocols.

In devp2p, subprotocol and the particular message within it are identified with a single number instead of two, one for protocol and one for message. Care must be taken to ensure that each type of package within each subprotocol gets a globally unique id that does not collide with other subprotocols.

This PR is a first proposal of a derive-macro that can be applied to an enum and attributes for the enum variants. Code would possibly look like the following:

#[derive(SyncPackets)]
enum Packet {
   #[protocol(ETH_PROTOCOL)] StatusMessage = 0x01,
...
   #[protocol(WARP_SYNC_PROTOCOL)] GetSnapshotManifestPacket = 0x11,
}

Benefits of the approach:

• The enum takes care itself that ids are globally unique
• The derive-macro implements conversion method from_u8 so that ids read from the network can be directly converted to the appropriate identifier.
• The provided from_u8 method returns an Option<Packet>
• The derive-macro automatically provides functionality to query id and protocol from an instance of Packet
• Protocol and Id are specified in one single compact line
• Further attributes may be easily added, for example a version number.
• Syntax errors are reported as compile errors.

Drawbacks:

• Each message needs to be individually tagged with its protocol attribute. Whole blocks would be more ergonomic.

[parity-cla-bot]

It looks like @elferdo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

[ngotchac]

Couldn't we have something like:

#[derive(EnumMap)]
enum Packet {
   #[packet = ETH_PACKET]
  StatusMessage = 0x01,
...
  #[packet = WARP_SYNC_PACKET] 
  GetSnapshotManifestPacket = 0x11,
}

and Packet::StatusMessage.packet() would return ETH_PACKET?

[elferdo]

You mean that the attribute packet itself defines a new method? So for instance #[version = v63] would automatically define Packet::StatusMessage.version()?

[ngotchac]

That would be the idea yeah, if possible

[elferdo]

I think the syntax would need to look somewhat different, because attributes for derive-macros must be predefined, but I think we can achieve something along those lines. I'll look into it.

For some reason I prefer closed solutions to open-ended ones, or at least try not to generalize too early, but I can't see any big downside to your idea right now. I would see, for example, that instead of #[eth] and #[par] we would provide a fixed set of attributes (protocol and version) for example: #[protocol WARP_SYNC] or #[version v63]. This would at least decouple the macro from the definition of the protocols.

[ngotchac]

@elferdo Can you update the PR description to reflect your recent changes?

[elferdo]

@ngotchac Oh, sure, sorry, I will update it right now.

[dvdplm]

This looks pretty great! :)

One question though. If I try to use #[derive(SyncPackets)] on two enums I get this error:

24 | #[derive(SyncPackets, Clone, Copy, Debug, PartialEq)]
   |          ----------- previous import of the type `PacketId` here
...
30 | #[derive(SyncPackets, Clone, Copy, Debug, PartialEq)]
   |          ^^^^^^^^^^^
   |          |
   |          `PacketId` reimported here
   |          help: remove unnecessary import

Seems like a bug yeah?

Code:

#[derive(SyncPackets, Clone, Copy, Debug, PartialEq)]
enum Packets {
	#[protocol(MY_PROTOCOL0)] Packet0 = 0x00,
	#[protocol(MY_PROTOCOL1)] Packet1 = 0x01,
}

#[derive(SyncPackets, Clone, Copy, Debug, PartialEq)]
enum Bovine {
	#[protocol(BEEF)] Bull = 0x00,
	#[protocol(BEEF)] Cow = 0x01,
}

[dvdplm]

@elferdo It'd be good to have some module level docs here with an example or two. Pretty please?

[elferdo]

@elferdo It'd be good to have some module level docs here with an example or two. Pretty please?

Sure! I'll address your two issues asap.

[elferdo]

Seems like a bug yeah?

The macro was designed with one specific use case in mind, namely that of putting all the p2p protocol IDs into one place. One of the restrictions these IDs must meet is that they are unique, and by allowing only one enum the language itself will take care of that.

Code:

#[derive(SyncPackets, Clone, Copy, Debug, PartialEq)]
enum Packets {
	#[protocol(MY_PROTOCOL0)] Packet0 = 0x00,
	#[protocol(MY_PROTOCOL1)] Packet1 = 0x01,
}

#[derive(SyncPackets, Clone, Copy, Debug, PartialEq)]
enum Bovine {
	#[protocol(BEEF)] Bull = 0x00,
	#[protocol(BEEF)] Cow = 0x01,
}

I guess you don't want:

#[derive(SyncPackets, Clone, Copy, Debug, PartialEq)]
enum Packets {
	#[protocol(MY_PROTOCOL0)] Packet0 = 0x00,
	#[protocol(MY_PROTOCOL1)] Packet1 = 0x01,
	#[protocol(BEEF)] Bull = 0x02,
	#[protocol(BEEF)] Cow = 0x03,
}

right?

Where is the other enum used?

The solution to the problem is quite straightforward, I'm just wondering what the use case is.

[dvdplm]

Where is the other enum used?

The solution to the problem is quite straightforward, I'm just wondering what the use case is.

There isn't one, just me playing around with the code while reviewing it and noticing a surprising error message: I think the expectation is that a macro should work even when applied several times?

If it becomes terribly complicated to fix, I'm fine with some good docs explaining how to go about applying the macro to several enums in a project.

[dvdplm]

Just to be clear: I think this is a terrific improvement and I'm prepared to merge it even if imperfect, just as long as it's properly doc'd. :)

[elferdo]

Where is the other enum used?
The solution to the problem is quite straightforward, I'm just wondering what the use case is.

There isn't one, just me playing around with the code while reviewing it and noticing a surprising error message: I think the expectation is that a macro should work even when applied several times?

If it becomes terribly complicated to fix, I'm fine with some good docs explaining how to go about applying the macro to several enums in a project.

No, not really, I already have it in place. My worries are that it gets abused and then all of a sudden we end up having, again, uncontrolled IDs. Like it should be obvious how to properly update IDs, or add a new one, and it should be difficult to make it wrong.

Let's make it reusable, just in case. The code as it is should be obviously easy to maintain, and if someone wants it s/he can have it.

[dvdplm]

namely that of putting all the p2p protocol IDs into one place

So this is used in libp2p? Should we perhaps extract it to parity-common then so we can share the code?

[dvdplm]

ping @elferdo

[dvdplm]

On a second look I think this is fine as-is. My code example is clearly a misuse and while it'd be nice to get a good compiler error I also think that with good module-level docs and tests for bad usage this is good to go.

[dvdplm]

Put the happy path first?

[dvdplm]

Should this check that there's a single argument in the list as well?

[elferdo]

This is checked in parse_protocol_arguments, do you think here would be a better place?

[dvdplm]

@elferdo ping

[debris]

Sure! I'll address your two issues asap.

@elferdo please address the issues and reopen the pr :)