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.

Sign up for GitHub

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

Jump to bottom

Add ping version 2 description. #526

Merged
merged 2 commits into from
Apr 25, 2024
Merged

Add ping version 2 description. #526

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e002148
Add ping version 2 description.
mitchelli Mar 1, 2024
b02e4d9
Added that ping is the only supported protocol
mitchelli Mar 14, 2024
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
27 changes: 23 additions & 4 deletions sync/p2p_messages.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,14 +80,33 @@ Message is RLP encoded, fields:

Message is RLP encoded, fields:

- `Port :: int` - listen port
- `Share :: int` - number of peers to share
### Version 1

- `Port :: int` - listen port.
- `Share :: int` - number of peers to share.
- `GenesisHash :: byte_array`
- `Difficulty :: int` - the total difficulty of the chain.
- `TopHash :: byte_array`
- `SyncAllowed :: bool` - if the sender of this ping message is accepting
synchronization messages.
- `Peers :: [byte_array]` - list of shared peers.


### Version 2

- `Versions :: list`
- `Protocol :: binary` - type of message, currently only "ping" is supported.
- `Vsns :: [int]` - versions supported
- `Port :: int` - listen port.
- `Share :: int` - number of peers to share.
- `GenesisHash :: byte_array`
- `Height :: int` - current height.
Copy link
Member

Choose a reason for hiding this comment

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

Isn't height kind of "per database" (node) field? Why is it shared? If you know the remote TopHash isn't it possible to derive its height as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked Ulf and this is his response, "It's true that the height can be derived, but only by first downloading headers. While working on fast synch strategies, I felt that getting the height up front was a convenience at fairly low cost."

- `Difficulty :: int` - the total difficulty of the chain.
- `TopHash :: byte_array`
- `sync_allowed :: bool` - if the sender of this ping message is accepting
- `SyncAllowed :: bool` - if the sender of this ping message is accepting
Copy link
Member

Choose a reason for hiding this comment

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

Since we have a capability field now, isn't it appropriate to move this there? Sounds like a capability to me.

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 could be, the current example capability is more complicated though, it is for a sync capability "chain_poi", which is:

{height, int}
{root_hash, binary}
{genesis, binary}
{top, binary}
{poi, binary}

A downside would be that it increases the size of the ping.

Copy link
Member

Choose a reason for hiding this comment

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

Okay, maybe a misunderstand "capabilities"

Copy link
Member

Choose a reason for hiding this comment

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

I don't think we are sure that is how capabilities should be used, let's see when we actually find a use-case for them. My idea was that the capabilities should signal what additional messages that the node will understand and react to 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ulf wanted to use it for additional methods of sync. aeternity/aeternity#4189

synchronization messages.
- `Peers :: [byte_array]` - list of shared peers
- `Capabilities :: byte_array` - list of supported capabilites.
- `Peers :: [byte_array]` - list of shared peers.

Peers are serialized/deserialized in `aec_peer_messages`

Expand Down