Skip to content
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

Add payload threshold for using uTP, and show work #339

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions portal-wire-protocol.md
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,39 @@ The transmission of data that is too large to fit a single packet is done using

> The Portal wire protocol currently implements uTP over the `TALKREQ/TALKRESP` messages. Future plans are to move to the [sub-protocol data transmission](https://github.com/ethereum/devp2p/issues/229) in order to use a protocol native mechanism for establishing packet streams between clients.

Currently, the standard is to switch to uTP when the payload exceeds 1165 bytes. This may change over time, because it depends on a number of other variables. See an example derivation in rust:
Copy link
Collaborator

@kdeme kdeme Sep 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great for adding this in the specifications, but I believe the value should be 1175 bytes.

That's the value that gets calculated by Fluffy at least, and IIRC this was at some point in the past also looked at as reference by Trin, but perhaps it was not taken over exactly or got altered over time.

Calculation in Fluffy happens here: https://github.com/status-im/nimbus-eth1/blob/master/fluffy/network/wire/portal_protocol.nim#L123 & https://github.com/status-im/nimbus-eth1/blob/master/fluffy/network/wire/portal_protocol.nim#L404-L406

I don't have an exact test for this, but I do remember verifying this byte per byte on a encoded talkresp message.
I will make the consts more clear and can add a test for this also to show.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have moved the calculation for talkreq overhead to our discv5 module and added tests there: status-im/nim-eth@aa1e738

So I'm pretty certain that these are the correct values (at least with our discv5 impl., but that shouldn't differ really).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like this test is for the max size of the Talk Response payload, but the relevant payloads for a Content message are transported in a Talk Request. Does that make a difference?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

@carver carver Sep 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll update the spec to note that we're talking about the size of the payload inside the message, and point out those extra 2 bytes.

I don't see a test in trin for this content size max, so I'll take your values here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, but it looks like the message overhead is different for the different scenarios, like find nodes vs find content, so maybe I'll just ignore the 2 bytes here, and look at the total encoded payload size

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it will be different for each of the wire protocol messages. But I guess the most important one here is the one that differentiates between sending content over the protocol response or over uTP

```rs
/// The maximum size of a Discv5 packet.
const MAX_DISCV5_PACKET_SIZE: usize = 1280;

/// The maximum size of a Discv5 talk request payload.
///
/// Discv5 talk request overhead:
/// * masking IV length: 16
/// * static header (protocol ID || version || flag || nonce || authdata-size) length: 23
/// * authdata length: 32
/// * HMAC length: 16
/// * (max) talk request ID length: 8
/// * (max assumed) talk request protocol length: 8
/// * RLP byte array overhead: 6
const MAX_DISCV5_TALK_REQ_PAYLOAD_SIZE: usize =
MAX_DISCV5_PACKET_SIZE - 16 - 23 - 32 - 16 - 8 - 8 - 6;

// NOTE: The wire constant below relies on the following SSZ constants:
// * `ssz::BYTES_PER_UNION_SELECTOR`: 1
// * `ssz::BYTES_PER_LENGTH_OFFSET`: 4

/// The maximum size of a portal CONTENT payload. At the time of writing, this payload either
/// corresponds to a `connection_id`, `enrs`, or `content` payload.
///
/// Portal wire overhead:
/// * portal message SSZ union selector
/// * CONTENT SSZ union selector
/// * CONTENT SSZ length offset for List `enrs` or `content`
const MAX_PORTAL_CONTENT_PAYLOAD_SIZE: usize = MAX_DISCV5_TALK_REQ_PAYLOAD_SIZE
- (ssz::BYTES_PER_UNION_SELECTOR * 2)
- ssz::BYTES_PER_LENGTH_OFFSET;
```

## Request - Response Messages

Expand Down
Loading