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 ping/pong for updating node height #673

Merged
merged 4 commits into from Apr 2, 2019
Merged

Conversation

@ixje
Copy link
Contributor

ixje commented Apr 1, 2019

Currently there is no easy way to request the current height of the remote node. When connecting we get a snapshot of the height in the VersionPayload. This height is static.

neo-cli nodes broadcast inv messages of type Block which could be used to request the the full block (via a getdata) and extract the height from the Block.Index. There are 2 downsides

  1. it is a potentially big payload
  2. it forces you to maintain the block in some local cache until you are synced up and you can actually process it. If you sync from 0 you potentially collect thousands of blocks that you can't process eating up memory. You can't throw them away and request them again later because the TransportSession apparently does not allow requesting data for the same hash twice on a single session. Note that because there is no timeout that cleans up previously requested hashes it could collect the full 3.x million MainNet hashes in memory if synced from 0 without disconnecting).

Instead of forcing a reconnect of the peer to get an updated height via the VersionPayload, this PR introduces a minimal footprint backwards compatible ping/pong function. Old clients will ignore the "ping" command, new ones reply with "pong" + a height.

The output of the show state command in neo-cli can also use this information to stay up to date.

@erikzhang

This comment has been minimized.

Copy link
Member

erikzhang commented Apr 1, 2019

When you receive an inv message of type Block, you will get a hash. Then there will be two situations:

  1. You have not received this hash before, then you will request this block. When you receive the block, you know the height.

  2. You received the block from other nodes, so you know the hash. Then you can get the height from the block you have received before.

So we don't need ping/pong.

@ixje

This comment has been minimized.

Copy link
Contributor Author

ixje commented Apr 1, 2019

I disagree on the need.

You have not received this hash before, then you will request this block. When you receive the block, you know the height.

Yes and then you need to keep that block and all following blocks in memory until you're synced. That's a waste of memory as described above if you're far behind on syncing.

This #620 (comment) and this #620 (comment) seem to discuss the same issue + solution by other people so apparently it's a common problem + idea how to solve it.

If we remove the 1 request per session then I agree this is not needed, because then I can request the block, read the index and delete it until I'm actually close to being in sync. Then I can request it again and persist.

@erikzhang

This comment has been minimized.

Copy link
Member

erikzhang commented Apr 1, 2019

I think not all the clients need to know the height of connected nodes. Maybe we can send ping to request a pong, which contains the height for response.

@ixje

This comment has been minimized.

Copy link
Contributor Author

ixje commented Apr 1, 2019

I don't really understand the objection against the extra 4 bytes, but I updated the PR to reflect your idea. Now we have a "ping" command with no payload and that responds with a "pong" + PongPayload that contains the current height. I do really hope we can get this included in 2.10.0.1

@erikzhang

This comment has been minimized.

Copy link
Member

erikzhang commented Apr 2, 2019

@ixje I modified it. Now both ping and pong have the LastBlockIndex field.

@ixje

This comment has been minimized.

Copy link
Contributor Author

ixje commented Apr 2, 2019

looks good. thanks

@shargon
shargon approved these changes Apr 2, 2019
@shargon

This comment has been minimized.

Copy link
Member

shargon commented Apr 2, 2019

When will be delivered this message?

@erikzhang

This comment has been minimized.

Copy link
Member

erikzhang commented Apr 2, 2019

The client or plugin can broadcast the ping message by telling it to LocalNode. There is currently no mechanism for automatically sending ping messages, because I am worried that this will put an extra burden on the network.

@erikzhang erikzhang merged commit 6e8c74e into neo-project:master Apr 2, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ixje ixje deleted the ixje:ping_pong branch Apr 2, 2019
OnPingMessageReceived(msg.GetPayload<PingPayload>());
break;
case "pong":
OnPongMessageReceived(msg.GetPayload<PingPayload>());

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Apr 2, 2019

Contributor

Is it correct @ixje @erikzhang msg.GetPayload<PingPayload> for Pong?

This comment has been minimized.

Copy link
@ixje

ixje Apr 2, 2019

Author Contributor

Yes, there is only 1 payload for both.

@erikzhang I don't know what your intention of the added nonce field was, but should that maybe have been included in the response as a way of confirming that the pong is a response to our ping?

e.g
-> ping generate nonce e.g. 123
<- pong with nonce set to the received PingPayload.nonce e.g. 123 in this example

This comment has been minimized.

Copy link
@erikzhang

erikzhang Apr 2, 2019

Member

but should that maybe have been included in the response as a way of confirming that the pong is a response to our ping?

Good idea!

This comment has been minimized.

Copy link
@igormcoelho

igormcoelho Apr 2, 2019

Contributor

Yes, there is only 1 payload for both.

Thanks for the clarification. So, perhaps rename to PingPongPayload, just for clarity? That really looked like a typo me :)

This was referenced Apr 2, 2019
@Tommo-L

This comment has been minimized.

Copy link
Contributor

Tommo-L commented Oct 25, 2019

The client or plugin can broadcast the ping message by telling it to LocalNode. There is currently no mechanism for automatically sending ping messages, because I am worried that this will put an extra burden on the network.

I hope we can add a timer to send ping to remote node for synchronization in neo project. It will not burden the network, just like heartbeat.
And it's a litte strange, we create the LastBlockIndex field in RemoteNode, but we depend on neo-cli task to update it, and never used in neo project. If this is the case, we should move it to the neo-cli

Already contains in PR #899

Thacryba pushed a commit to simplitech/neo that referenced this pull request Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.