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

Network specification update #1404

Merged
merged 10 commits into from Sep 17, 2019

Conversation

@AgeManning
Copy link
Contributor

commented Sep 8, 2019

Provides updates to the networking specification.

Specifically:

  • Fixes the REQ_RESP_MAX_SIZE
  • Renames the previously similar BeaconBlocks and RecentBeaconBlocks to BeaconBlocksByRange and BeaconBlocksByRoot respectively
  • Removes the 1:1 mapping in BeaconBlocksByRoot, a responder may now return less blocks than requested
  • The responder to a BeaconBlocksByRange request should also limit their response by the REQ_RESP_MAX_SIZE or SSZ_MAX_LIST_SIZE
  • Adds the notion of a response_chunk. Responses that consist of a single SSZ-list (BeaconBlocksByRange, BeaconBlocksByRoot) are now sent back over the stream in individual response_chunk.
  • Adds clarification around the SSZ-encoding of the request/response types
  • Adds clarification to the RPC requests

This extends on the improvements in #1390

specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
AgeManning and others added 5 commits Sep 8, 2019
Copy link
Contributor

left a comment

Cleaned up the language a bit and clarified a some things.
One minor question

specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
@zah

This comment has been minimized.

Copy link

commented Sep 9, 2019

If the hello message is now valid at any time, shall we rename it to status? A lot of people will associate the term "hello" with something that is appropriate only at the start of the session.

@prestonvanloon

This comment has been minimized.

Copy link
Contributor

commented Sep 10, 2019

Instead of

(
  blocks: []BeaconBlock
)

we could define it as

(
  []BeaconBlock
)

What do you think? Having a field name might imply that this is a container. We missed the note originally when implementing the networking spec and sent block response containers.

@AgeManning

This comment has been minimized.

Copy link
Contributor Author

commented Sep 10, 2019

I have updated this PR to accommodate suggestions.
Specifically, we have changed the REQ_RESP_MAX_SIZE to apply to the encoded payload of the request/responses and the RESP_TIMEOUT now applies per response chunk.

Copy link
Contributor

left a comment

Nice work on the chunking strategy! How about leveraging this to mandate/recommend early/streaming validation, with the ability to Reset a stream, or decrement peer reputation on failure? This would create a more secure network.

specs/networking/p2p-interface.md Show resolved Hide resolved
specs/networking/p2p-interface.md Outdated Show resolved Hide resolved
result ::= “0” | “1” | “2” | [“128” ... ”255”]
```

The encoding-dependent header may carry metadata or assertions such as the encoded payload length, for integrity and attack proofing purposes. Because req/resp streams are single-use and stream closures implicitly delimit the boundaries, it is not strictly necessary to length-prefix payloads; however, certain encodings like SSZ do, for added security.

`encoded-payload` has a maximum byte size of `REQ_RESP_MAX_SIZE`.
A `response` is formed by one or more `response_chunk`s. The exact request determines whether a response consists of a single `response_chunk` or possibly many. Responses that consist of a single SSZ-list (such as `BlocksByRange` and `BlocksByRoot`) send each list item as a `response_chunk`. All other response types (non-Lists) send a single `response_chunk`. The encoded-payload of a `response_chunk` has a maximum uncompressed byte size of `REQ_RESP_MAX_SIZE`.

This comment has been minimized.

Copy link
@raulk

raulk Sep 11, 2019

Contributor

This can incur in excessive fragmentation/overhead/underutilisation if list items are small, as you're effectively defining a 1:1 mapping between chunk and list element.

I'd recommend to make the boundaries of chunks full list elements, allowing a chunk to contain multiple complete list elements, and disallowing partial elements or bleeding over between chunks.

It may be worth formally introducing the term chunkable, so you can label message fields that satisfy this property in the schemas below.

This comment has been minimized.

Copy link
@zah

zah Sep 11, 2019

Please note that adding chunks does not increase the size of the overall payload, because we are replacing the leading 4-byte offsets that previously appeared in the SSZ lists with response codes and varint lengths which will often amount to just 2 bytes per chunk (in the small items case).

This comment has been minimized.

Copy link
@AgeManning

AgeManning Sep 12, 2019

Author Contributor

I'd recommend to make the boundaries of chunks full list elements, allowing a chunk to contain multiple complete list elements, and disallowing partial elements or bleeding over between chunks.

This is the case. "Responses that consist of a single SSZ-list (such as BlocksByRange and BlocksByRoot) send each list item as a response_chunk"
Perhaps this is not clear enough?

A chunk represents a single encoded item

This comment has been minimized.

Copy link
@raulk

raulk Sep 12, 2019

Contributor

A chunk represents a single encoded item

Yeah, in practice this can incur in excessive chunking. If you have 3 list items available, and they all fit within one chunk, you can coalesce them into the same chunk. This is especially more efficient if you can fit multiple chunks within a single MTU. I'm not so worried about the byte overhead (although... death by a thousand cuts is a thing), but rather about the fragmentation, and making a protocol with a chunking strategy that's future proof.

This comment has been minimized.

Copy link
@AgeManning

AgeManning Sep 12, 2019

Author Contributor

Not entirely sure I follow.
Different requests have different discrete items. Eg a status message has a single ssz-encoded response (which is a container). It's chunk would be the size of that ssz-container. (It likely won't be a multiple of an MTU).

Responses that have lists, we now split into single items and send them as a chunk wrapped in a "error response". If we grouped multiple items under a single error response, we would require the encoding to be an ssz-list (like we had earlier) and the receiver would need to know which bytes are lists and which are single elements.

An SSZ list is encoded like | offset| offset| offset ..... | item | item | item |. Previously we sent this whole thing as a single error response across one stream, it now more like:
| error_response | item | error_response | item | ...
So we have replaced the offsets with error_responses and allow the receiver to take individual elements rather than wait for the entire stream to end.

Let me know if I've misunderstood your point

specs/networking/p2p-interface.md Outdated Show resolved Hide resolved

The requester MUST wait a maximum of `TTFB_TIMEOUT` for the first response byte to arrive (time to first byte—or TTFB—timeout). On that happening, the requester will allow further `RESP_TIMEOUT` to receive the full response.
The requester MUST wait a maximum of `TTFB_TIMEOUT` for the first response byte to arrive (time to first byte—or TTFB—timeout). On that happening, the requester allows a further `RESP_TIMEOUT` for each subsequent `response_chunk` received. For responses consisting of potentially many `response_chunk`s (an SSZ-list) the requester SHOULD read from the stream until either; a) An error result is received in one of the chunks, b) The responder closes the stream, c) More than `REQ_RESP_MAX_SIZE` bytes have been read for a single `response_chunk` payload or d) More than the maximum number of requested chunks are read. For requests consisting of a single `response_chunk` and a length-prefix, the requester should read the exact number of bytes defined by the length-prefix before closing the stream.

This comment has been minimized.

Copy link
@raulk

raulk Sep 11, 2019

Contributor

Beware of a starvation DoS attack where attackers could deliberately trickle their chunks slowly so as to keep that socket/file descriptor busy as long as possible.

This comment has been minimized.

Copy link
@AgeManning

AgeManning Sep 12, 2019

Author Contributor

Each chunk is bounded by RESP_TIMEOUT. So the slowest an attacker can be is 1 chunk per RESP_TIMEOUT. If the chunk is malicious or useless, the application will likely drop the peer

This comment has been minimized.

Copy link
@raulk

raulk Sep 12, 2019

Contributor

Yes, I'm worried about the case where an attacker has good data but they explicitly starve you off it by trickling it slowly. RESP_TIMEOUT is 10s; imagine you have 1000 chunks, of 1kb each (1mb total). This policy makes it possible to craft an attack where the peer sends you one valid chunk every 9 seconds, overall taking 150 minutes (2.5 hours) to dispatch 1mb of data.

This can be counteracted by a global RPC timeout.

This comment has been minimized.

Copy link
@AgeManning

AgeManning Sep 12, 2019

Author Contributor

Good point. Will discuss :)

This comment has been minimized.

Copy link
@arnetheduck

arnetheduck Sep 12, 2019

Contributor

I'm not sure this is something the spec necessarily needs to discuss - it seems it could be handled with a peer quality algorithm instead - if the trickling peer is the only one you have, it's also the one you want.

Copy link
Contributor

left a comment

LGTM. I would like a recommendation for the scenario when a dialing peer does not send a Status immediately.

@arnetheduck

This comment has been minimized.

Copy link
Contributor

commented Sep 15, 2019

LGTM. I would like a recommendation for the scenario when a dialing peer does not send a Status immediately.

timeout/disconnect - no point spending resources if they're not following the protocol

@djrtwo
djrtwo approved these changes Sep 16, 2019
Copy link
Contributor

left a comment

I'd like to get this merged soon and release to v0.8.4

@hwwhww hwwhww added the networking label Sep 17, 2019
@djrtwo
djrtwo approved these changes Sep 17, 2019
@djrtwo djrtwo merged commit 759d8f4 into ethereum:v08x Sep 17, 2019
6 checks passed
6 checks passed
ci/circleci: checkout_specs Your tests passed on CircleCI!
Details
ci/circleci: deposit_contract Your tests passed on CircleCI!
Details
ci/circleci: install_deposit_contract_test Your tests passed on CircleCI!
Details
ci/circleci: install_pyspec_test Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.