Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC5ef3fe294f9f: net: add RAII socket and use it instead of bare SOCKET, add Sock unit tests

Summary

Introduce a class to manage the lifetime of a socket - when the object that contains the socket goes out of scope, the underlying socket will be closed.

In addition, the new Sock class has a Send(), Recv() and Wait() methods that can be overridden by unit tests to mock the socket operations.

The Wait() method also hides the #ifdef USE_POLL poll() #else select() #endif technique from higher level code.

This is a backport of core#20788 [3/5] and core#21159 (bugfix)
https://github.com/bitcoin/bitcoin/pull/20788/commits/ba9d73268f9585d4b9254adcf54708f88222798b
https://github.com/bitcoin/bitcoin/pull/20788/commits/615ba0eb96cf131364c1ceca9d3dedf006fa1e1c
https://github.com/bitcoin/bitcoin/pull/21159/commits/9cc8e30125df14fe47e21e55ab3bf26f4d416565

Depends on D11004

Test Plan

ninja all check-all

Run the seeder:
./src/seeder/bitcoin-seeder

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

PiRK created this revision.Feb 7 2022, 17:29

Herald added a reviewer: Restricted Project. · View Herald TranscriptFeb 7 2022, 17:29

PiRK requested review of this revision.Feb 7 2022, 17:29

teamcity edited the summary of this revision. (Show Details)Feb 7 2022, 17:29

PiRK edited the summary of this revision. (Show Details)Feb 7 2022, 17:29

PiRK added a parent revision: D11004: net: move CloseSocket() from netbase to util/sock.

PiRK added a child revision: D11006: net: use Sock in InterruptibleRecv() and Socks5().Feb 7 2022, 17:30

teamcity edited the summary of this revision. (Show Details)Feb 7 2022, 17:34

Harbormaster completed remote builds in B18181: Diff 32251.Feb 7 2022, 17:55

Fabien requested changes to this revision.Feb 7 2022, 19:55

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/seeder/bitcoin.cpp
219 ↗	(On Diff #32251)	This is a change in behavior. Now the socket is not closed as it's not getting out of scope. For this to work you need to have it defined locally.
290 ↗	(On Diff #32251)	dito
src/test/sock_tests.cpp
7 ↗	(On Diff #32251)	Nit: should be first on its line, it's what you're testing
83 ↗	(On Diff #32251)	Move the comment on it's own line

This revision now requires changes to proceed.Feb 7 2022, 19:55

Move a comment to its own line.
Move include of util/sock.h to first line in sock_tests.cpp.

Call sock.reset() in places where the socket was previously closed in the seeder. I checked that resetting a unique pointer causes the destructor to be called, which now takes care of closing it.

PiRK edited the summary of this revision. (Show Details)Feb 8 2022, 08:25

Harbormaster completed remote builds in B18187: Diff 32257.Feb 8 2022, 08:33

Build Bitcoin ABC Diffs / Diff Testing (build-debug) failed.

Failed tests logs:

====== Bitcoin ABC functional tests: abc_p2p_getavaaddr.py ======

------- Stdout: -------
2022-02-08T08:31:48.275000Z TestFramework (INFO): Initializing test directory /work/abc-ci-builds/build-debug/test/tmp/test_runner_₿₵_  _20220208_082631/abc_p2p_getavaaddr_191
2022-02-08T08:31:51.619000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:04.366000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.677000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.678000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.679000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.680000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.680000Z TestFramework.p2p (WARNING): Connection lost to 127.0.0.1:22354 due to [Errno 104] Connection reset by peer
2022-02-08T08:32:17.713000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/work/test/functional/test_framework/test_framework.py", line 132, in main
    self.run_test()
  File "/work/test/functional/abc_p2p_getavaaddr.py", line 202, in run_test
    self.address_test(maxaddrtosend=100, num_proof=5, num_avanode=10)
  File "/work/test/functional/abc_p2p_getavaaddr.py", line 182, in address_test
    requester = node.add_p2p_connection(AddrReceiver())
  File "/work/test/functional/test_framework/test_node.py", line 701, in add_p2p_connection
    p2p_conn.wait_for_verack()
  File "/work/test/functional/test_framework/p2p.py", line 604, in wait_for_verack
    self.wait_until(test_function, timeout=timeout)
  File "/work/test/functional/test_framework/p2p.py", line 518, in wait_until
    timeout_factor=self.timeout_factor)
  File "/work/test/functional/test_framework/util.py", line 273, in wait_until_helper
    if predicate():
  File "/work/test/functional/test_framework/p2p.py", line 514, in test_function
    assert self.is_connected
AssertionError
2022-02-08T08:32:17.765000Z TestFramework (INFO): Stopping nodes
------- Stderr: -------
Traceback (most recent call last):
  File "/work/test/functional/test_framework/authproxy.py", line 116, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1069, in _send_output
    self.send(chunk)
  File "/usr/lib/python3.7/http/client.py", line 991, in send
    self.sock.sendall(data)
BrokenPipeError: [Errno 32] Broken pipe

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/work/test/functional/abc_p2p_getavaaddr.py", line 206, in <module>
    AvaAddrTest().main()
  File "/work/test/functional/test_framework/test_framework.py", line 152, in main
    exit_code = self.shutdown()
  File "/work/test/functional/test_framework/test_framework.py", line 280, in shutdown
    self.stop_nodes()
  File "/work/test/functional/test_framework/test_framework.py", line 514, in stop_nodes
    node.stop_node(wait=wait, wait_until_stopped=False)
  File "/work/test/functional/test_framework/test_node.py", line 426, in stop_node
    self.stop(wait=wait)
  File "/work/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/work/test/functional/test_framework/authproxy.py", line 161, in __call__
    'POST', self.__url.path, postdata.encode('utf-8'))
  File "/work/test/functional/test_framework/authproxy.py", line 122, in _request
    self.__conn.request(method, path, postdata, headers)
  File "/usr/lib/python3.7/http/client.py", line 1260, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1306, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1255, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/usr/lib/python3.7/http/client.py", line 1030, in _send_output
    self.send(msg)
  File "/usr/lib/python3.7/http/client.py", line 970, in send
    self.connect()
  File "/usr/lib/python3.7/http/client.py", line 942, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/usr/lib/python3.7/socket.py", line 727, in create_connection
    raise err
  File "/usr/lib/python3.7/socket.py", line 716, in create_connection
    sock.connect(sa)
ConnectionRefusedError: [Errno 111] Connection refused

Each failure log is accessible here:
Bitcoin ABC functional tests: abc_p2p_getavaaddr.py

PiRK planned changes to this revision.Feb 8 2022, 09:05

rebase again after making sure master has D11002

Harbormaster completed remote builds in B18189: Diff 32259.Feb 8 2022, 09:53

Fabien requested changes to this revision.Feb 8 2022, 12:04

Fabien added inline comments.

src/seeder/bitcoin.cpp
291 ↗	(On Diff #32259)	No need for the else clause, and can be simplified: res = sock != nullptr; sock.reset();
src/test/sock_tests.cpp
93 ↗	(On Diff #32259)	I don't think you can create a message with less than 0 chars.

This revision now requires changes to proceed.Feb 8 2022, 12:04

PiRK added inline comments.Feb 8 2022, 12:55

src/test/sock_tests.cpp
93 ↗	(On Diff #32259)	This comes from squashing with https://github.com/bitcoin/bitcoin/pull/21159. It is a "fix" because some compiler and/or boost version does not like comparing 'const long' and 'const unsigned long' I thought I updated the summary to explain, but I probably only updated my local commit message . And I forgot to mention it in the diff message. Sorry about that. I will add it to the summary.

PiRK added inline comments.Feb 8 2022, 13:00

src/seeder/bitcoin.cpp
291 ↗	(On Diff #32259)	OK. I was not sure if if was legal to reset a nullptr. Apparently it is possible.

simplify res. Update summary to mention squashing with core#21159

Harbormaster completed remote builds in B18193: Diff 32263.Feb 8 2022, 13:15

PiRK edited the test plan for this revision. (Show Details)Feb 8 2022, 13:35

Fabien accepted this revision.Feb 8 2022, 13:39

This revision is now accepted and ready to land.Feb 8 2022, 13:39

Closed by commit rABC5ef3fe294f9f: net: add RAII socket and use it instead of bare SOCKET, add Sock unit tests (authored by Vasil Dimov <vd@FreeBSD.org>, committed by PiRK). · Explain WhyFeb 8 2022, 13:43

This revision was automatically updated to reflect the committed changes.

PiRK added a commit: rABC5ef3fe294f9f: net: add RAII socket and use it instead of bare SOCKET, add Sock unit tests.

PiRK mentioned this in D11460: seeder: don't ignore connection errors.May 13 2022, 14:35

PiRK mentioned this in rABCf5925ad5ae5f: seeder: don't ignore connection errors.May 14 2022, 10:10

net: add RAII socket and use it instead of bare SOCKET, add Sock unit tests
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 32269

src/net.cpp

src/netbase.h

src/netbase.cpp

src/seeder/bitcoin.h

src/seeder/bitcoin.cpp

src/test/CMakeLists.txt

src/test/sock_tests.cpp

src/util/sock.h

src/util/sock.cpp

src/util/time.h

src/util/time.cpp

net: add RAII socket and use it instead of bare SOCKET, add Sock unit testsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 32269

src/net.cpp

src/netbase.h

src/netbase.cpp

src/seeder/bitcoin.h

src/seeder/bitcoin.cpp

src/test/CMakeLists.txt

src/test/sock_tests.cpp

src/util/sock.h

src/util/sock.cpp

src/util/time.h

src/util/time.cpp

net: add RAII socket and use it instead of bare SOCKET, add Sock unit tests
ClosedPublic
Actions

Revision Contents
Changeset List