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

Udp support #728

Closed
wants to merge 62 commits into from
Closed

Udp support #728

wants to merge 62 commits into from

Conversation

shargon
Copy link
Member

@shargon shargon commented May 10, 2019

Also, i included the possibility of listen only on certain interfaces, not in all

@shargon shargon requested a review from erikzhang May 10, 2019 08:12
neo/NeoSystem.cs Outdated Show resolved Hide resolved
neo/Network/P2P/Peer.cs Outdated Show resolved Hide resolved
@erikzhang
Copy link
Member

We need to modify VersionPayload too.

Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

@shargon,it looks interesting and more organized.
What are the next changes? For example, at RemoteNode?

neo/NeoSystem.cs Outdated Show resolved Hide resolved
neo/NodeStartConfig.cs Outdated Show resolved Hide resolved
@shargon
Copy link
Member Author

shargon commented May 10, 2019

Ok! maybe we should add some Attributes inside VersionProtocol in order to close #548 ?

@shargon
Copy link
Member Author

shargon commented May 10, 2019

check fe42fcd for the refactor of VersionPayload

@erikzhang erikzhang added this to the NEO 3.0 milestone May 10, 2019
@erikzhang
Copy link
Member

When does a node actively send UDP messages? I didn't find it in the code.

@shargon
Copy link
Member Author

shargon commented May 17, 2019

Only it response for udp message, it never start sending

@erikzhang
Copy link
Member

Only it response for udp message, it never start sending

But who send it first? If no one send UDP, the functions are not used.

@shargon
Copy link
Member Author

shargon commented May 17, 2019

If you want to send a tx to other node (by other client for example) you will receive and be able to response.

Is like Websocket, listen and response

@ThomasLobker
Copy link

ThomasLobker commented May 17, 2019

I think there is a problem, the max package size of UDP is 65535. But a transaction or block can much larger then it.

Both UDP and TCP have a maximum packet size of (about) 64KiB. And the IP layer handles fragmentation anytime the packet is larger than the MTU, there is not really a difference here between UDP and TCP.

If we split the message when it is sent and combine it when it is accepted, then it is better to use TCP directly.

Yes absolutely 👍

Also keep in mind that any bidirectional traffic between a node and a client behind NAT would require things like NAT traversal with keep-alives. Trust me that this will be a huge pain in many scenario's.

For broadcasting unidirectional packets like transactions UDP can be great though and it will give you much better performance. Just sending your transaction to many nodes at once and stop caring about it. For anything else, don't mind UDP and just keep using TCP 😉

@erikzhang
Copy link
Member

If you want to send transactions to other nodes, you'll need to send inv message first, right? Then you will receive getdata messages from these nodes asynchronously. How do you use UDP to broadcast?

@ThomasLobker
Copy link

If you want to send transactions to other nodes, you'll need to send inv message first, right?

How about the RPC sendrawtransaction method? With RPC over UDP you could send your raw transaction to 10 nodes at the same time for extra redundancy.

@shargon
Copy link
Member Author

shargon commented May 17, 2019

Udp messages could be received in wrong order, so we can't combine it. My first idea with udp is only for a listener, and a faster way of receive some messages, not all of them

@erikzhang
Copy link
Member

I think it's difficult to use UDP MultiCast on Internet. Without MultiCast, what is the use of UDP for us?

@shargon
Copy link
Member Author

shargon commented May 17, 2019

I think it's difficult to use UDP MultiCast on Internet. Without MultiCast, what is the use of UDP for us?

Not to many :P

If you wish, i can remove all the udp functionality, and maintain the other changes.

@erikzhang
Copy link
Member

Conflicts

@codecov-io
Copy link

codecov-io commented May 27, 2019

Codecov Report

Merging #728 into master will decrease coverage by <.01%.
The diff coverage is 3.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #728      +/-   ##
==========================================
- Coverage   38.39%   38.39%   -0.01%     
==========================================
  Files         176      178       +2     
  Lines       12465    12526      +61     
==========================================
+ Hits         4786     4809      +23     
- Misses       7679     7717      +38
Impacted Files Coverage Δ
neo/Network/P2P/Capabilities/NodeCapability.cs 61.53% <ø> (ø) ⬆️
neo/Network/P2P/ChannelsConfig.cs 0% <0%> (ø) ⬆️
neo/Network/P2P/RemoteNode.cs 3.75% <0%> (-0.03%) ⬇️
neo/Network/RPC/RpcServer.cs 0% <0%> (ø) ⬆️
neo/Network/P2P/UdpRequest.cs 0% <0%> (ø)
neo/Network/P2P/TaskSession.cs 0% <0%> (ø) ⬆️
neo/Network/P2P/UdpResponse.cs 0% <0%> (ø)
neo/Network/P2P/ProtocolHandler.cs 12.22% <1.36%> (+5.25%) ⬆️
neo/Network/P2P/Capabilities/ServerCapability.cs 73.33% <100%> (ø) ⬆️
neo/Network/P2P/LocalNode.cs 16.93% <6.25%> (-1.42%) ⬇️
... and 5 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d411153...cfbd114. Read the comment docs.

@ZhangTao1596
Copy link
Contributor

ZhangTao1596 commented May 31, 2019

I think we can treat it as a "connection" if two nodes exchange verison via udp.
So every node should have a struct to manage these connections in order to broadcast.

@shargon
Copy link
Member Author

shargon commented Jun 15, 2019

If we decide that this is not useful, we can close this pr

@shargon shargon closed this Jul 11, 2019
@erikzhang erikzhang removed this from the NEO 3.0 milestone Jul 11, 2019
@shargon shargon deleted the udp-server branch August 28, 2019 07:24
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend VersionPayload with service information
6 participants