vasild
commented at 4:04 pm on May 7, 2021:
contributor
This is a roadmap PR. It can be merged, but it can also be split into separate PRs and to get proper thorough review it is split.
Add wrapper methods to the syscalls accept(), setsockopt(), getsockname(), bind(), listen() in the Sock class (e.g. Sock::Accept()). Those methods can be overriden (mocked) by unit tests (existent example in master) and by fuzz tests (existent example in master).
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#27375 (net: support unix domain sockets for -proxy and -onion by pinheadmz)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
#26312 (Remove Sock::Get() and Sock::Sock() by vasild)
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
practicalswift
commented at 1:04 pm on May 8, 2021:
contributor
Strong Concept ACK: mockable means fuzzable.
jonatack
commented at 10:37 am on May 10, 2021:
member
To denote an invalid socket: use an empty shared_ptr/unique_ptr to Sock (that owns nullptr) instead of a Sock object that contains m_socket equal to INVALID_SOCKET. This way high-level callers never need to lookup the internal file descriptor of Sock.
vasild force-pushed
on May 31, 2021
vasild
commented at 2:53 pm on May 31, 2021:
contributor
272ecad4bf...2833c71862: fix a compiler warning
DrahtBot added the label
Needs rebase
on Jun 1, 2021
vasild force-pushed
on Jun 3, 2021
vasild
commented at 2:37 pm on June 3, 2021:
contributor
2833c71862...f029732b34: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Jun 3, 2021
DrahtBot added the label
Needs rebase
on Jun 16, 2021
vasild force-pushed
on Jun 22, 2021
vasild
commented at 8:12 am on June 22, 2021:
contributor
f029732b34...379e9e7279: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Jun 22, 2021
vasild force-pushed
on Jun 30, 2021
vasild
commented at 1:20 pm on June 30, 2021:
contributor
379e9e7279...cdf537545a: during fuzzing, mock also DNS lookup; this is necessary since we now fuzz high level code which may end up doing DNS lookups, for example: CConnman::OpenNetworkConnection() -> CConnman::ConnectNode() -> Lookup()
jamesob
commented at 1:23 pm on June 30, 2021:
member
Concept ACK, excited to dig into this.
DrahtBot added the label
Needs rebase
on Aug 13, 2021
vasild force-pushed
on Aug 25, 2021
vasild
commented at 11:11 am on August 25, 2021:
contributor
cdf537545a...c98c54e3e3: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Aug 25, 2021
DrahtBot added the label
Needs rebase
on Aug 27, 2021
vasild force-pushed
on Sep 28, 2021
vasild
commented at 7:37 am on September 28, 2021:
contributor
c98c54e3e3...f8d0ae7e5d: rebase due to conflicts and address review suggestions from child PRs: #21879 (comment), #21943 (comment).
DrahtBot removed the label
Needs rebase
on Sep 28, 2021
vasild force-pushed
on Sep 28, 2021
vasild
commented at 1:21 pm on September 28, 2021:
contributor
f8d0ae7e5d...39476dc354: rebase and pick minor changes from child PR #21943 (comment).
DrahtBot added the label
Needs rebase
on Oct 4, 2021
vasild force-pushed
on Oct 8, 2021
vasild
commented at 8:39 am on October 8, 2021:
contributor
39476dc354...02aeba0713: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Oct 8, 2021
vasild force-pushed
on Oct 26, 2021
vasild
commented at 8:26 am on October 26, 2021:
contributor
02aeba0713...94f8543cda: rebase and pick changes from child PR: #21943 (comment)
DrahtBot added the label
Needs rebase
on Nov 8, 2021
vasild
commented at 10:06 am on November 10, 2021:
contributor
94f8543cda...3673db7c0a: rebase due to conflicts
vasild force-pushed
on Nov 10, 2021
DrahtBot removed the label
Needs rebase
on Nov 10, 2021
vasild force-pushed
on Nov 12, 2021
vasild
commented at 10:18 am on November 12, 2021:
contributor
3673db7c0a...ff3d6a5937: pick changes from child PR: #21943 (comment)
DrahtBot added the label
Needs rebase
on Nov 18, 2021
vasild
commented at 4:28 pm on November 18, 2021:
contributor
ff3d6a5937...63280ff095: rebase due to conflicts and pick changes from child PR: #21943 (comment)
vasild force-pushed
on Nov 18, 2021
DrahtBot removed the label
Needs rebase
on Nov 18, 2021
laanwj referenced this in commit
64059b78f5
on Nov 24, 2021
DrahtBot added the label
Needs rebase
on Nov 24, 2021
vasild force-pushed
on Nov 26, 2021
vasild
commented at 11:00 am on November 26, 2021:
contributor
63280ff095...8fa0821747: rebase due to conflicts. #21943 which was part of this PR was merged, thus remove its commits from this PR.
DrahtBot removed the label
Needs rebase
on Nov 26, 2021
maflcko referenced this in commit
f2074eeb2d
on Dec 1, 2021
DrahtBot added the label
Needs rebase
on Dec 1, 2021
sidhujag referenced this in commit
7a5e16a331
on Dec 1, 2021
vasild force-pushed
on Dec 1, 2021
vasild
commented at 3:36 pm on December 1, 2021:
contributor
8fa0821747...9dd36c4a72: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Dec 1, 2021
DrahtBot added the label
Needs rebase
on Dec 6, 2021
vasild force-pushed
on Dec 7, 2021
vasild
commented at 9:52 am on December 7, 2021:
contributor
9dd36c4a72...ecd2afdbb8: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Dec 7, 2021
DrahtBot added the label
Needs rebase
on Dec 14, 2021
vasild
commented at 12:44 pm on December 15, 2021:
contributor
ecd2afdbb8...7e74e1f959: rebase due to conflicts
vasild force-pushed
on Dec 15, 2021
DrahtBot removed the label
Needs rebase
on Dec 15, 2021
josibake
commented at 1:50 pm on December 15, 2021:
member
Concept ACK
I see two open PR’s listed in the description as ready for review; are those the best places to start with reviewing this?
vasild
commented at 2:15 pm on December 15, 2021:
contributor
@josibake, yes. #21879 and #23604 contain the first commits from this PR. And if you are curious what’s coming afterwards - browse the subsequent commits in this PR. Thanks!
laanwj referenced this in commit
8f1c28a609
on Jan 5, 2022
DrahtBot added the label
Needs rebase
on Jan 5, 2022
vasild force-pushed
on Jan 6, 2022
vasild
commented at 10:03 am on January 6, 2022:
contributor
The cast was not added in this PR. I am leaving it as is because it is unrelated to the change and without it, it is easier to see that only getsockname has been changed to sock.GetSockName.
in
src/net.cpp:2192
in
cae35129e6outdated
2235@@ -2358,22 +2236,36 @@ bool CConnman::BindListenPort(const CService& addrBind, bilingual_str& strError,
22362237 // Allow binding if the port is still in TIME_WAIT state after
2238 // the program was closed and restarted.
2239- setsockopt(sock->Get(), SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int));
2240+ if (sock->SetSockOpt(SOL_SOCKET, SO_REUSEADDR, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
2247 // some systems don't have IPV6_V6ONLY but are always v6only; others do have the option
2248 // and enable it by default or not. Try to enable it, if possible.
2249 if (addrBind.IsIPv6()) {
2250 #ifdef IPV6_V6ONLY
2251- setsockopt(sock->Get(), IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int));
2252+ if (sock->SetSockOpt(IPPROTO_IPV6, IPV6_V6ONLY, (sockopt_arg_type)&nOne, sizeof(int)) == SOCKET_ERROR) {
510@@ -511,19 +511,23 @@ std::unique_ptr<Sock> CreateSockTCP(const CService& address_family)
511 int set = 1;
512 // Set the no-sigpipe option on the socket for BSD systems, other UNIXes
513 // should use the MSG_NOSIGNAL flag for every send.
514- setsockopt(hSocket, SOL_SOCKET, SO_NOSIGPIPE, (void*)&set, sizeof(int));
515+ if (sock->SetSockOpt(SOL_SOCKET, SO_NOSIGPIPE, (void*)&set, sizeof(int)) == SOCKET_ERROR) {
DrahtBot removed the label
Needs rebase
on Feb 11, 2022
vasild force-pushed
on Feb 15, 2022
vasild
commented at 4:33 pm on February 15, 2022:
contributor
4a26bcd3fe...bca585381d: rebase and minor improvements after self-review and before chopping off the next piece into its own PR: rename Sock::WaitEvents to Sock::Events and make it possible to define an instance of it without specifying the “occurred” event, which does not make sense anyway.
DrahtBot added the label
Needs rebase
on Apr 14, 2022
vasild force-pushed
on Apr 15, 2022
vasild
commented at 11:21 am on April 15, 2022:
contributor
bca585381d...b73dcbc24e: rebase due to conflicts and pick changes from child PRs
DrahtBot removed the label
Needs rebase
on Apr 15, 2022
laanwj referenced this in commit
6300b9556e
on Apr 19, 2022
DrahtBot added the label
Needs rebase
on Apr 19, 2022
vasild force-pushed
on Apr 19, 2022
vasild
commented at 4:42 pm on April 19, 2022:
contributor
b73dcbc24e...a8f889d5e2: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Apr 19, 2022
sidhujag referenced this in commit
f41c649dc6
on Apr 19, 2022
DrahtBot added the label
Needs rebase
on Apr 22, 2022
vasild
commented at 4:12 pm on April 27, 2022:
contributor
a8f889d5e2...e834d610ff: rebase due to conflicts
vasild force-pushed
on Apr 27, 2022
DrahtBot removed the label
Needs rebase
on Apr 27, 2022
DrahtBot added the label
Needs rebase
on May 16, 2022
vasild force-pushed
on May 19, 2022
vasild
commented at 3:35 pm on May 19, 2022:
contributor
e834d610ff...f3e35c4d6c: rebase due to conflicts and pick changes from child PR #24356 (comment)
DrahtBot removed the label
Needs rebase
on May 19, 2022
DrahtBot added the label
Needs rebase
on May 24, 2022
vasild force-pushed
on May 27, 2022
vasild
commented at 9:05 am on May 27, 2022:
contributor
f3e35c4d6c...0aae576f68: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on May 27, 2022
laanwj referenced this in commit
0ea92cad52
on Jun 16, 2022
DrahtBot added the label
Needs rebase
on Jun 16, 2022
sidhujag referenced this in commit
10043f624c
on Jun 17, 2022
vasild
commented at 4:20 pm on June 17, 2022:
contributor
0aae576f68...6077fac5ef: rebase due to conflicts
vasild force-pushed
on Jun 17, 2022
DrahtBot removed the label
Needs rebase
on Jun 17, 2022
laanwj referenced this in commit
a085a55491
on Jun 22, 2022
DrahtBot added the label
Needs rebase
on Jun 22, 2022
vasild force-pushed
on Jun 22, 2022
vasild
commented at 12:22 pm on June 22, 2022:
contributor
6077fac5ef...82a6d4576e: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Jun 22, 2022
laanwj referenced this in commit
ba29911e21
on Jun 28, 2022
DrahtBot added the label
Needs rebase
on Jun 28, 2022
vasild force-pushed
on Jun 28, 2022
vasild
commented at 12:31 pm on June 28, 2022:
contributor
82a6d4576e...a6424d5023: rebase due to conflicts
laanwj referenced this in commit
55c9e2d790
on Jun 28, 2022
vasild force-pushed
on Jun 28, 2022
vasild
commented at 1:39 pm on June 28, 2022:
contributor
a6424d5023...12f848b5cd: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Jun 28, 2022
sidhujag referenced this in commit
9957e2cd9f
on Jun 28, 2022
sidhujag referenced this in commit
968d887367
on Jun 28, 2022
DrahtBot added the label
Needs rebase
on Jul 20, 2022
vasild
commented at 2:28 pm on July 20, 2022:
contributor
12f848b5cd...f07b89e917: rebase due to conflicts
vasild force-pushed
on Jul 20, 2022
DrahtBot removed the label
Needs rebase
on Jul 20, 2022
DrahtBot added the label
Needs rebase
on Jul 27, 2022
achow101
commented at 6:28 pm on October 12, 2022:
member
Should this be marked as draft since it is a parent PR?
glozow referenced this in commit
7e1007a3c6
on Oct 12, 2022
jonatack
commented at 8:02 pm on October 12, 2022:
member
@vasild this may be able to be closed now that all of the child PRs have been merged.
fanquake
commented at 2:57 am on October 13, 2022:
member
vasild this may be able to be closed now that all of the child PRs have been merged.
Given that the majority of commits in this pull haven’t been merged, I don’t think so.
vasild force-pushed
on Oct 14, 2022
vasild
commented at 8:38 am on October 14, 2022:
contributor
f07b89e917...9a27807453: rebase due to conflicts
Should this be marked as draft since it is a parent PR?
Could be, I am not sure. The reason I did not make it a draft is because it can be merged as it is - there is no danger that something bad will happen if it gets merged (assuming there are reviewers who ACK it). The reason I split it into smaller PRs is solely to ease review. Should I make it draft?
@vasild this may be able to be closed now that all of the child PRs have been merged.
Given that the majority of commits in this pull haven’t been merged, I don’t think so.
Right, there are two pieces left - #26312 which I just opened and the tests themselves (which depend on that PR, thus no separate PR for them yet).
DrahtBot removed the label
Needs rebase
on Oct 14, 2022
sidhujag referenced this in commit
f73aef5a64
on Oct 23, 2022
DrahtBot added the label
Needs rebase
on Nov 25, 2022
vasild force-pushed
on Dec 12, 2022
vasild
commented at 10:44 am on December 12, 2022:
contributor
9a27807453...77ab5e8e98: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Dec 12, 2022
DrahtBot added the label
Needs rebase
on Feb 1, 2023
vasild force-pushed
on Feb 9, 2023
vasild
commented at 2:04 pm on February 9, 2023:
contributor
77ab5e8e98...b497200c7a: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Feb 9, 2023
DrahtBot added the label
Needs rebase
on Feb 17, 2023
achow101 marked this as a draft
on Apr 25, 2023
i2p: avoid using Sock::Get() for checking for a valid socket
Peeking at the underlying socket file descriptor of `Sock` and checkig
if it is `INVALID_SOCKET` is bad encapsulation and stands in the way of
testing/mocking/fuzzing.
Instead use an empty unique_ptr to denote that there is no valid socket.
1a23f0cfd6
net: don't check if the socket is valid in GetBindAddress()
The socket is always valid (the underlying file descriptor is not
`INVALID_SOCKET`) when `GetBindAddress()` is called.
f5c57839ea
net: don't check if the socket is valid in ConnectSocketDirectly()
The socket is always valid (the underlying file descriptor is not
`INVALID_SOCKET`) when `ConnectSocketDirectly()` is called.
7426958ad4
net: remove now unnecessary Sock::Get()
`Sock::Get()` was used only in `sock.{cpp,h}`. Remove it and access
`Sock::m_socket` directly.
Unit tests that used `Get()` to test for equality still verify that the
behavior is correct, indirectly, by testing whether the socket is closed
or not.
b740d39dd4
net: remove Sock default constructor, it's not necessary2bcbe87424
fuzz: add CConnman::OpenNetworkConnection() to the tests
Now that all network calls done by `CConnman::OpenNetworkConnection()`
are done via `Sock` they can be redirected (mocked) to `FuzzedSocket`
for testing.
618b3ec0b2
fuzz: add CConnman::CreateNodeFromAcceptedSocket() to the tests1545b55517
fuzz: add CConnman::InitBinds() to the testsbb98574e4b
fuzz: add CConnman::SocketHandler() to the tests80d9dfd0f0
vasild force-pushed
on Aug 16, 2023
vasild
commented at 1:27 pm on August 16, 2023:
contributor
b497200c7a...80d9dfd0f0: rebase due to conflicts
DrahtBot removed the label
Needs rebase
on Aug 16, 2023
ryanofsky referenced this in commit
d0b928b29d
on Oct 3, 2023
DrahtBot added the label
Needs rebase
on Oct 3, 2023
DrahtBot
commented at 3:03 pm on October 3, 2023:
contributor
🐙 This pull request conflicts with the target branch and needs rebase.
vasild
commented at 11:34 am on October 4, 2023:
contributor
All the functional bits of this are now merged. Thank you all!
This enables extending the test framework to be able to call any method, without worries that it may end up doing OS syscalls (try to create sockets, open connections, etc). A PR that does some of that is in #28584 (= last 4 commits from this PR).
Closing as complete.
vasild closed this
on Oct 4, 2023
vasild deleted the branch
on Oct 4, 2023
Frank-GER referenced this in commit
4ea7ddec96
on Oct 13, 2023
PastaPastaPasta referenced this in commit
ab04e7242a
on Feb 27, 2024
PastaPastaPasta referenced this in commit
3b0323a683
on May 14, 2024
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-10-04 13:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me