net: implement poll #14336

pstratem · 2018-09-26T22:37:06Z

Implement poll() on systems which support it properly.

This eliminates the restriction on maximum socket descriptor number.

DrahtBot · 2018-09-27T02:37:31Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

No conflicts as of last run.

pstratem · 2018-09-29T00:51:55Z

It seems that lots of the functional tests have races cause this pull request keeps triggering random seeming failures.

src/net.cpp

jimpo

Concept ACK

src/net.cpp

jimpo · 2018-09-29T23:37:44Z

src/net.cpp

+        int nErr = WSAGetLastError();
+        LogPrintf("socket select error %s\n", NetworkErrorString(nErr));
+        for (unsigned int i = 0; i <= hSocketMax; i++)
+            FD_SET(i, &fdsetRecv);


I see this is the same as the code currently, but it seems like this should be fdsetError instead of fdsetRecv, even though they are handled exactly the same.

I don't think it should be there at all really. select() failures dont mean the sockets themselves have failed, but i guess it could mean we made a mistake and called select() with a closed socket?

Yeah, then iterating through each one and calling recv would identify which peer to disconnect. Though, if it's fdsetError and not fdsetRecv, it probably shouldn't disconnect when nBytes == 0.

laanwj · 2018-10-02T19:37:32Z

Concept ACK, will review in detail when my brain works again.

theuni · 2018-10-02T20:19:32Z

Concept ACK. Will also review shortly.

jamesob

Some minor comments and a question about conservatively falling back to select for MacOS systems. Code looks good!

src/net.cpp

This separates the socket event collection logic from the logic deciding which events we're interested in at all.

This separates the select() logic from the socket handling logic, setting up for a switch to poll().

pstratem · 2018-11-30T23:04:01Z

With 87 connections the effect of generating the pollfd list shows up on a profile but isn't significant.

The poll() implementation is on the order of 1000 ns slower per call than the select() implementation.

I don't think that's significant.

jamesob

utACK 20c9601

Code looks good, feel free to ignore the small comments. I'm running tests at the moment and will report back with results within the next day - testnet happy-path behavior looks good so far.

src/net.cpp

jamesob · 2018-12-03T15:56:50Z

src/net.cpp

+    }
+}
+#else
+void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set)


(non-blocking)

diff --git a/src/net.cpp b/src/net.cpp index 7b8b6e5ea2..03d165d701 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -1355,6 +1355,7 @@ void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_s } } #else +// Use select() on platforms where poll() is unavailable. void CConnman::SocketEvents(std::set<SOCKET> &recv_set, std::set<SOCKET> &send_set, std::set<SOCKET> &error_set) { std::set<SOCKET> recv_select_set, send_select_set, error_select_set;

src/netbase.cpp

jamesob · 2018-12-03T16:12:13Z

src/netbase.cpp

                struct timeval tval = MillisToTimeval(std::min(endTime - curTime, maxWait));
                fd_set fdset;
                FD_ZERO(&fdset);
                FD_SET(hSocket, &fdset);
                int nRet = select(hSocket + 1, &fdset, nullptr, nullptr, &tval);
+#endif
                if (nRet == SOCKET_ERROR) {


(non-blocking) This might be academic, but since this clause is handling both select() and poll() might be safer to say if (nRet < 0)? Though right now there's no difference in the return value for both APIs (and I doubt that'll ever change...).

This eliminates the restriction on maximum socket descriptor number.

jamesob

Tested ACK 4927bf2

Tested on Linux 4.15.0-39-generic and macOS 10.12.1. Tested IBD on testnet as well as tweaking -maxconnections and observing connections with lsof -a -itcp -p $(pidof bitcoind).

Only code change since my last utACK is a small cleanup in netbase.cpp.

Thanks for the work here @pstratem.

laanwj · 2018-12-13T18:41:59Z

utACK 4927bf2

4927bf2 Increase maxconnections limit when using poll. (Patrick Strateman) 11cc491 Implement poll() on systems which support it properly. (Patrick Strateman) 28211a4 Move SocketEvents logic to private method. (Patrick Strateman) 7e403c0 Move GenerateSelectSet logic to private method. (Patrick Strateman) 1e6afd0 Introduce and use constant SELECT_TIMEOUT_MILLISECONDS. (Patrick Strateman) Pull request description: Implement poll() on systems which support it properly. This eliminates the restriction on maximum socket descriptor number. Tree-SHA512: b945cd9294afdafcce96d547f67679d5cdd684cf257904a239cd1248de3b5e093b8d6d28d8d1b7cc923dc0b2b5723faef9bc9bf118a9ce1bdcf357c2323f5573

BlockMechanic · 2019-10-12T09:47:20Z

Crashes on android 7 8 and 9 armv7a. May need to implement a work around

Summary: Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]] bitcoin/bitcoin@1e6afd0 Test Plan: ninja ninja check Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6459

Summary: This separates the socket event collection logic from the logic deciding which events we're interested in at all. Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]] bitcoin/bitcoin@7e403c0 Depends on D6459 Test Plan: ninja ninja check-all src/bitcoind -debug=net Verify normal node connection behavior Reviewers: O1 Bitcoin ABC, #bitcoin_abc, Fabien, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, Fabien, jasonbcox Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D6460

Summary: This separates the select() logic from the socket handling logic, setting up for a switch to poll(). Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]] bitcoin/bitcoin@28211a4 Depends on D6460 Test Plan: ninja ninja check-all src/bitcoind -debug=net Verify normal node behavior. Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta, jasonbcox Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta, jasonbcox Differential Revision: https://reviews.bitcoinabc.org/D6461

Summary: This eliminates the restriction on maximum socket descriptor number. Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]] bitcoin/bitcoin@11cc491 Depends on D6461 Test Plan: ninja ninja check-all src/bitoind Verify normal node behavior. Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta Differential Revision: https://reviews.bitcoinabc.org/D6470

Summary: Partial backport of Core [[bitcoin/bitcoin#14336 | PR14336]] bitcoin/bitcoin@4927bf2 Depends on D6470 Test Plan: ninja ninja check-all src/bitcoind --debug=net Verify normal node behavior. Reviewers: O1 Bitcoin ABC, #bitcoin_abc, majcosta Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, majcosta Subscribers: majcosta Differential Revision: https://reviews.bitcoinabc.org/D6471

They are outdated since bitcoin#14336.

ee7891a doc: Remove outdated comments (Hennadii Stepanov) Pull request description: The first removed comment was introduced in #5288, the second one in #13503. Both are outdated since #14336. ACKs for top commit: duncandean: crACK ee7891a Tree-SHA512: a2d6071919e81c916bfc2178109bbc464417321bcc567ed0644448c5faea8e58cb08a7657afa1b6ffe1fb63e114a2a47b31c893e471839ba9d49a3986e68b2a7

ee7891a doc: Remove outdated comments (Hennadii Stepanov) Pull request description: The first removed comment was introduced in bitcoin#5288, the second one in bitcoin#13503. Both are outdated since bitcoin#14336. ACKs for top commit: duncandean: crACK ee7891a Tree-SHA512: a2d6071919e81c916bfc2178109bbc464417321bcc567ed0644448c5faea8e58cb08a7657afa1b6ffe1fb63e114a2a47b31c893e471839ba9d49a3986e68b2a7

…t correctly 8ff3743 Revert "doc: Remove outdated comments" (Hennadii Stepanov) Pull request description: Unfortunately, in #23094 the assumption that #14336 makes comments outdated is wrong. As pointed in #23094 (comment), the #14336 just moved the relevant code a few lines down. This PR reverts commit ee7891a, and moves the comments into the right place. I apologize about that. ACKs for top commit: MarcoFalke: cr ACK 8ff3743 laanwj: ACK 8ff3743 Tree-SHA512: 84aca627bb5b49c06fc172778f9b9407482c5a873ccbc3dc40167e6a8ad0bc60475d6a469c843b7b42712e35cf3fc2d3518923e791d5e0c59628e042acc72747

… comment correctly 8ff3743 Revert "doc: Remove outdated comments" (Hennadii Stepanov) Pull request description: Unfortunately, in bitcoin#23094 the assumption that bitcoin#14336 makes comments outdated is wrong. As pointed in bitcoin#23094 (comment), the bitcoin#14336 just moved the relevant code a few lines down. This PR reverts commit ee7891a, and moves the comments into the right place. I apologize about that. ACKs for top commit: MarcoFalke: cr ACK 8ff3743 laanwj: ACK 8ff3743 Tree-SHA512: 84aca627bb5b49c06fc172778f9b9407482c5a873ccbc3dc40167e6a8ad0bc60475d6a469c843b7b42712e35cf3fc2d3518923e791d5e0c59628e042acc72747

They are outdated since bitcoin#14336.

fanquake added the P2P label Sep 26, 2018

pstratem force-pushed the 2018-09-24-socket-handler-poll branch 3 times, most recently from f5aa8cf to 6bcefaa Compare September 27, 2018 02:07

pstratem force-pushed the 2018-09-24-socket-handler-poll branch from 6bcefaa to a97afdf Compare September 27, 2018 03:18

This was referenced Sep 27, 2018

net: Refactor message parsing (CNetMessage), adds flexibility #14046

Closed

Add p2p layer encryption with ECDH/ChaCha20Poly1305 #14032

Closed

pstratem force-pushed the 2018-09-24-socket-handler-poll branch 5 times, most recently from 80e59ca to 7fd976b Compare September 29, 2018 00:16

practicalswift reviewed Sep 29, 2018

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

practicalswift reviewed Sep 29, 2018

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

practicalswift reviewed Sep 29, 2018

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

practicalswift reviewed Sep 29, 2018

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

jimpo reviewed Sep 29, 2018

View reviewed changes

pstratem force-pushed the 2018-09-24-socket-handler-poll branch 3 times, most recently from 5cf67c9 to e4db5b3 Compare October 1, 2018 19:44

pstratem force-pushed the 2018-09-24-socket-handler-poll branch from cd28216 to 499bdef Compare October 2, 2018 19:43

jnewbery mentioned this pull request Oct 8, 2018

[tests] Remove rpc_zmq.py #14419

Merged

pstratem force-pushed the 2018-09-24-socket-handler-poll branch 2 times, most recently from 7acadee to 84fae8d Compare October 16, 2018 15:34

jamesob reviewed Oct 16, 2018

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

jamesob reviewed Oct 16, 2018

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

pstratem added 2 commits November 30, 2018 18:02

Move GenerateSelectSet logic to private method.

7e403c0

This separates the socket event collection logic from the logic deciding which events we're interested in at all.

Move SocketEvents logic to private method.

28211a4

This separates the select() logic from the socket handling logic, setting up for a switch to poll().

pstratem force-pushed the 2018-09-24-socket-handler-poll branch from cc6edb4 to 20c9601 Compare November 30, 2018 23:03

jamesob approved these changes Dec 3, 2018

View reviewed changes

pstratem added 2 commits December 3, 2018 14:25

Implement poll() on systems which support it properly.

11cc491

This eliminates the restriction on maximum socket descriptor number.

Increase maxconnections limit when using poll.

4927bf2

pstratem force-pushed the 2018-09-24-socket-handler-poll branch from 20c9601 to 4927bf2 Compare December 3, 2018 19:26

jamesob approved these changes Dec 4, 2018

View reviewed changes

laanwj merged commit 4927bf2 into bitcoin:master Jan 2, 2019

codablock mentioned this pull request Apr 7, 2020

Backport poll() code from Bitcoin dashpay/dash#3396

Merged

codablock added a commit to codablock/dash that referenced this pull request Apr 8, 2020

--- Begin backporting bitcoin#14336

d901bae

codablock added a commit to codablock/dash that referenced this pull request Apr 8, 2020

--- Begin backporting bitcoin#14336

8481d6c

hebasto added a commit to hebasto/bitcoin that referenced this pull request Sep 25, 2021

doc: Remove outdated comments

ee7891a

They are outdated since bitcoin#14336.

hebasto mentioned this pull request Sep 25, 2021

doc: Remove outdated comments #23094

Merged

hebasto mentioned this pull request Sep 29, 2021

doc: Revert "Remove outdated comments" and place comment correctly #23130

Merged

rebroad pushed a commit to rebroad/bitcoin that referenced this pull request Oct 13, 2021

doc: Remove outdated comments

147cd30

They are outdated since bitcoin#14336.

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

net: implement poll #14336

net: implement poll #14336

Uh oh!

Conversation

pstratem commented Sep 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

pstratem commented Sep 29, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimpo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jimpo Sep 29, 2018

Choose a reason for hiding this comment

Uh oh!

pstratem Oct 1, 2018

Choose a reason for hiding this comment

Uh oh!

jimpo Oct 2, 2018

Choose a reason for hiding this comment

Uh oh!

laanwj commented Oct 2, 2018

Uh oh!

theuni commented Oct 2, 2018

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pstratem commented Nov 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jamesob Dec 3, 2018

Choose a reason for hiding this comment

Uh oh!

jamesob left a comment

Choose a reason for hiding this comment

Uh oh!

laanwj commented Dec 13, 2018

Uh oh!

BlockMechanic commented Oct 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pstratem commented Sep 26, 2018 •

edited

Loading

DrahtBot commented Sep 27, 2018 •

edited

Loading

pstratem commented Nov 30, 2018 •

edited

Loading

BlockMechanic commented Oct 12, 2019 •

edited

Loading