Discussion thread to EIP-2481 arguing for request ids in the eth protocol

[cburgdorf]

This is the discussion thread for #2481

Simple Summary

The EIP proposes a way to slightly increase the efficiency of the eth networking protocol while at the same time reducing complexity in Ethereum node implementations.

[karalabe]

I'll get the elephant in the room out of the way first :D

I've opened an EIP for eth/65 a week ago #2464, so now we have two "conflicting" proposals for 65. Both can be fixed and built upon the other, so we just need to pick an order.

Now, since we already do have an implementation of our EIP, I'd say we are a bit farther ahead in it (i.e. we'd actually like to release in in the next version of Geth) vs. this one I think does not yet have an impl. And the tx propagation EIP has a hotfix effect on the network (will greatly reduce bandwidth) vs. this one is a client nicety with no immediate impact on the network health (i.e. I think the tx prop is a bit more imprtant).

That said, I obviously have a dog in this race, so please chime in others too :P

[cburgdorf]

Yeah, I wasn't exactly sure how to address this. Should I just write the EIP as targeting eth/6x or eth/66 or could these changes be combined in a single update? I didn't mean to cut the line 😅

Should I just reference #2464 and target eth/66 for this EIP? That's totally cool with me :)

[karalabe]

If it's fine with you, just bump it to eth/66 and add requires: 2464 to the header. In that case however, we'll probably need to figure out the request/response ID for transactions too, which is not that obvious, because transaction responses use the same packet as tx broadcasts. So the request ID might need to be optional on the respose, or maybe explicitly set to 0 for broadcasts and non-zero for replies?

[karalabe]

Re rolling the to together, I'd rather not. It's a lot simpler to roll out multiple small changes and check that it works vs. sitting on a big update and have something break in the end.

[cburgdorf]

It's a lot simpler to roll out multiple small changes and check that it works vs. sitting on a big update and have something break in the end.

Yeah, agreed!

If it's fine with you, just bump it to eth/66 and add requires: 2464 to the header

Cool, will do that!

because transaction responses use the same packet as tx broadcasts

So if I understand correctly, the current plan in #2464 is that the response to GetPooledTransactions (0x09) would be answered with a command of Transactions (0x02).

Would there be any downside to add an extra explicit PooledTransactions response command so that these are clearly separate? Then we wouldn't need optional request ids and we wouldn't share the cmd id for the response for two different type of requests.

[karalabe]

Would there be any downside to add an extra explicit PooledTransactions response command so that these are clearly separate? Then we wouldn't need optional request ids and we wouldn't share the cmd id for the response for two different type of requests.

I think that's a reasonable solution.

[karalabe]

Updated the EIP b33fa99

[cburgdorf]

@karalabe Cool, I've also incorporated the change into #2481

[karalabe]

There's a large conflict between the text of the spec and the packet details.

To elaborate, each command is altered in the following way:

1. Create a list with the request_id being the first element.
2. Make the second element the list that defines the whole command in the current scheme.

This is consistent with the request / response pairs in the les protocol

This is absolutely not consistent with either LES or SNAP, or for the matter of fact the example packets given above.

In both LES and SNAP, the request ID is simply the first field, after which come all the other fields in the same flat scoping. There's no extra wrapping. If all the other fields are a single list of hashes or whatnot, obviously you will have them in a list, but that's not "the whole command in the current scheme".

Blindly dropping the whole command in there would simply add a dependency between protocol versions, because we can't just have a standalone eth/66, because we need the eth/65 messages to be scoped in there. Let's just please not even mention eth/65. List the old message formats if you want, list the new message formats and don't try to make a strange rule how one is derived from the other. The new packets should be meaningful standalone, not in context of the old protocol.

[cburgdorf]

This is absolutely not consistent with either LES or SNAP, or for the matter of fact the example packets given above.

I'm not sure if I'm following. The first example lists:

• GetBlockHeaders (0x03)
  • Current (eth/65): [block: {P, B_32}, maxHeaders: P, skip: P, reverse: P in {0, 1}]
  • Then (eth/66): [request_id: P, [block: {P, B_32}, maxHeaders: P, skip: P, reverse: P in {0, 1}]]

So in eth/65 we have a list that holds the block_hash, max_headers, skip and reverse and for eth/66 we turn it into a list that holds the request_id followed by another list that again holds block_hash, max_headers, skip and reverse.

And isn't that exactly what LES does, too?

GetBlockHeaders (0x02)
[reqID: P, [block: {P, B_32}, maxHeaders: P, skip: P, reverse: P in {0, 1}]]

I can't figure out which part you are disagreeing with?

[github-actions]

There has been no activity on this issue for two months. It will be closed in a week if no further activity occurs. If you would like to move this EIP forward, please respond to any outstanding feedback or add a comment indicating that you have addressed all required feedback and are ready for a review.

[github-actions]

This issue was closed due to inactivity. If you are still pursuing it, feel free to reopen it and respond to any feedback or request a review in a comment.