netbase: extend CreateSock() to support creating arbitrary sockets #30202

vasild · 2024-05-30T13:40:45Z

Allow the callers of CreateSock() to pass all 3 arguments to the socket(2) syscall. This makes it possible to create sockets of any domain/type/protocol. In addition to extending arguments, some extra safety checks were put in place.

The need for this came up during the discussion in #30043 (comment)

DrahtBot · 2024-05-30T13:40:48Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	tdb3, theStack, achow101
Stale ACK	laanwj

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:

#29876 (build: add -Wundef by fanquake)
#29641 (scripted-diff: Use LogInfo/LogDebug over LogPrintf/LogPrint by maflcko)
#29625 (Several randomness improvements by sipa)

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.

laanwj · 2024-05-30T13:51:27Z

Concept and code review (but untested, will use this to make a testing harness for #30043) ACK 77e34ded543e19ba27cab8d0cfc464af27f04bbf

tdb3

ACK for 77e34ded543e19ba27cab8d0cfc464af27f04bbf

Thanks. Seems like a reasonable change.

Sanity checks performed:

Synced the last 1.5 day's worth of blocks on signet. Binding and sync seemed to work fine.
Ran all unit and functional tests (passed).

Left some minor notes (just thinking out loud, so no need to answer/comment unless you feel it's necessary).

src/net.cpp

src/netbase.cpp

theStack

Concept and code-review ACK 77e34ded543e19ba27cab8d0cfc464af27f04bbf

src/netbase.h

vasild · 2024-06-12T06:49:54Z

77e34ded54...5f549c35d9: fix typo in the comment: s/socke/socket/

theStack

re-ACK 5f549c35d9a75c7019fe8a96963b988df5498eed 🧦

tdb3

re-ACK 5f549c35d9a75c7019fe8a96963b988df5498eed

achow101 · 2024-06-13T16:18:46Z

ACK 5f549c35d9a75c7019fe8a96963b988df5498eed

achow101 · 2024-06-13T16:23:26Z

Silent merge conflict

../../../src/test/fuzz/i2p.cpp:30:16: error: no viable overloaded '='
   30 |     CreateSock = [&fuzzed_data_provider](const sa_family_t&) {
      |     ~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   31 |         return std::make_unique<FuzzedSock>(fuzzed_data_provider);
      |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   32 |     };
      |     ~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:469:7: note: candidate function not viable: no known conversion from '(lambda at ../../../src/test/fuzz/i2p.cpp:30:18)' to 'const function<unique_ptr<Sock> (int, int, int)>' for 1st argument
  469 |       operator=(const function& __x)
      |       ^         ~~~~~~~~~~~~~~~~~~~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:487:7: note: candidate function not viable: no known conversion from '(lambda at ../../../src/test/fuzz/i2p.cpp:30:18)' to 'function<unique_ptr<Sock> (int, int, int)>' for 1st argument
  487 |       operator=(function&& __x) noexcept
      |       ^         ~~~~~~~~~~~~~~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:501:7: note: candidate function not viable: no known conversion from '(lambda at ../../../src/test/fuzz/i2p.cpp:30:18)' to 'nullptr_t' (aka 'std::nullptr_t') for 1st argument
  501 |       operator=(nullptr_t) noexcept
      |       ^         ~~~~~~~~~
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:531:2: note: candidate template ignored: requirement '_Callable<(lambda at ../../../src/test/fuzz/i2p.cpp:30:18), (lambda at ../../../src/test/fuzz/i2p.cpp:30:18), std::__invoke_result<(lambda at ../../../src/test/fuzz/i2p.cpp:30:18) &, int, int, int>>::value' was not satisfied [with _Functor = (lambda at ../../../src/test/fuzz/i2p.cpp:30:18)]
  531 |         operator=(_Functor&& __f)
      |         ^
/usr/bin/../lib64/gcc/x86_64-pc-linux-gnu/14.1.1/../../../../include/c++/14.1.1/bits/std_function.h:541:2: note: candidate template ignored: could not match 'reference_wrapper<_Functor>' against '(lambda at ../../../src/test/fuzz/i2p.cpp:30:18)'
  541 |         operator=(reference_wrapper<_Functor> __f) noexcept
      |         ^
1 error generated.
make[2]: *** [Makefile:17184: test/fuzz/fuzz-i2p.o] Error 1

DrahtBot · 2024-06-14T07:32:04Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

_{Debug: https://github.com/bitcoin/bitcoin/runs/26113679661}

Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol.

vasild · 2024-06-14T12:24:43Z

5f549c35d9...1245d1388b: rebase due to (silent) merge conflict

tdb3

re ACK 1245d13

theStack

re-ACK 1245d13

achow101 · 2024-06-20T17:24:25Z

ACK 1245d13

vasild mentioned this pull request May 30, 2024

net: Replace libnatpmp with built-in PCP+NATPMP implementation #30043

Merged

4 tasks

This was referenced May 31, 2024

scripted-diff: Use LogInfo over LogPrintf #29641

Merged

Several randomness improvements #29625

Merged

tdb3 approved these changes Jun 2, 2024

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

src/netbase.cpp Outdated Show resolved Hide resolved

theStack approved these changes Jun 11, 2024

View reviewed changes

src/netbase.h Outdated Show resolved Hide resolved

vasild force-pushed the extend_CreateSock branch from 77e34de to 5f549c3 Compare June 12, 2024 06:49

theStack approved these changes Jun 12, 2024

View reviewed changes

DrahtBot requested review from laanwj and tdb3 June 12, 2024 07:09

tdb3 approved these changes Jun 13, 2024

View reviewed changes

DrahtBot added the CI failed label Jun 14, 2024

netbase: extend CreateSock() to support creating arbitrary sockets

1245d13

Allow the callers of `CreateSock()` to pass all 3 arguments to the `socket(2)` syscall. This makes it possible to create sockets of any domain/type/protocol.

vasild force-pushed the extend_CreateSock branch from 5f549c3 to 1245d13 Compare June 14, 2024 12:24

DrahtBot requested review from achow101 and theStack June 14, 2024 14:03

tdb3 approved these changes Jun 14, 2024

View reviewed changes

DrahtBot removed the CI failed label Jun 14, 2024

theStack approved these changes Jun 18, 2024

View reviewed changes

DrahtBot mentioned this pull request Jun 19, 2024

build: add -Wundef #29876

Merged

achow101 merged commit a961ad1 into bitcoin:master Jun 20, 2024

vasild deleted the extend_CreateSock branch June 21, 2024 03:28

Sjors mentioned this pull request Jun 21, 2024

Stratum v2 Template Provider (take 3) #29432

Closed

24 tasks

bitcoin locked and limited conversation to collaborators Jun 21, 2025

netbase: extend CreateSock() to support creating arbitrary sockets #30202

netbase: extend CreateSock() to support creating arbitrary sockets #30202

Uh oh!

Conversation

vasild commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

laanwj commented May 30, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vasild commented Jun 12, 2024

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 13, 2024

Uh oh!

achow101 commented Jun 13, 2024

Uh oh!

DrahtBot commented Jun 14, 2024

Uh oh!

vasild commented Jun 14, 2024

Uh oh!

tdb3 left a comment

Choose a reason for hiding this comment

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 20, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vasild commented May 30, 2024 •

edited

Loading

DrahtBot commented May 30, 2024 •

edited

Loading