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.

Sign up for GitHub

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

Jump to bottom

Drop Windows Socket dependency for randomenv.cpp #29786

Merged
merged 1 commit into from
Apr 9, 2024
Merged

Drop Windows Socket dependency for randomenv.cpp #29786

merged 1 commit into from
Apr 9, 2024

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Apr 2, 2024 •
edited
Loading

This change drops a dependency on the ws2_32 library for our libbitcoinkernel by switching to GetComputerName function.

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 2, 2024 •
edited
Loading

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 sipsorcery, laanwj, fanquake
Concept ACK TheCharlatan
Stale ACK sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@hebasto
Copy link
Member Author

hebasto commented Apr 2, 2024

Friendly ping @sipa @TheCharlatan @sipsorcery :)

@sipa
Copy link
Member

sipa commented Apr 2, 2024

utACK 4d8ad85

maflcko
maflcko reviewed Apr 2, 2024
View reviewed changes
src/randomenv.cpp Outdated Show resolved Hide resolved
maflcko
maflcko reviewed Apr 2, 2024
View reviewed changes
src/randomenv.cpp Outdated Show resolved Hide resolved
fanquake
fanquake reviewed Apr 2, 2024
View reviewed changes
src/randomenv.cpp Outdated Show resolved Hide resolved
fanquake
fanquake reviewed Apr 2, 2024
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

requires the Winsock DLL being initiated in advance, which is not always guaranteed, for example, in the libbitcoinkernel.

Surely this is the problem that should be getting solved here? This seems like a short-term fixup, and doesn't prevent it from occuring again in the future? Is this something that's a problem elsewhere in the codebase?

Seems similar to #28486, which iirc hasn't been followed up with / fixed properly. If we can't rely on things being available/usable on Windows, seems like we need to rearchitect code that we can do that, rather than continually patching around/catching this ad-hoc.

@hebasto
Copy link
Member Author

hebasto commented Apr 2, 2024

requires the Winsock DLL being initiated in advance, which is not always guaranteed, for example, in the libbitcoinkernel.

Surely this is the problem that should be getting solved here? This seems like a short-term fixup, and doesn't prevent it from occuring again in the future? Is this something that's a problem elsewhere in the codebase?

Seems similar to #28486, which iirc hasn't been followed up with / fixed properly. If we can't rely on things being available/usable on Windows, seems like we need to rearchitect code that we can do that, rather than continually patching around/catching this ad-hoc.

This PR main goal is to get rid of the ws2_32 library dependency for the libbitcoinkernel.

However, I agree that Window Socket enabling code could be rearchitected.

@hebasto hebasto force-pushed the 240402-drop-winsock branch from 4d8ad85 to 459559e Compare April 2, 2024 12:29
@hebasto
Drop Windows Socket dependency for randomenv.cpp
03b87a3 
This change drops a dependency on the ws2_32 library for our
libbitcoinkernel, which may not be desirable.
@hebasto hebasto force-pushed the 240402-drop-winsock branch from 459559e to 03b87a3 Compare April 2, 2024 13:29
@sipsorcery
Copy link
Contributor

utACK 03b87a3.

If the hostname hash can be generated without requiring network initialisation that seems like a worthwhile imrpovement.

@DrahtBot DrahtBot requested a review from sipa April 2, 2024 20:16
@TheCharlatan
Copy link
Contributor

Concept ACK

This does indeed remove the dependency on ws2_32 from the kernel lib.

@0xB10C 0xB10C mentioned this pull request Apr 3, 2024
Merged
@DrahtBot DrahtBot removed the CI failed label Apr 5, 2024
@laanwj
Copy link
Member

laanwj commented Apr 8, 2024

Looks correct to me "If the function succeeds, the return value is a nonzero value.".

I'm not sure the netbios name is always the name as the hostname, but it is a good enough equivalent for randomness purposes, and I agree it is preferable not to depend on winsock for the kernel. The API, introduced in Windows 2000 is also old enough.

Code review ACK 03b87a3.

@DrahtBot DrahtBot requested a review from TheCharlatan April 8, 2024 14:43
fanquake
fanquake approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@fanquake fanquake left a comment

Choose a reason for hiding this comment

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

ACK 03b87a3

@fanquake fanquake merged commit 71f96c2 into bitcoin:master Apr 9, 2024
16 checks passed
@hebasto hebasto deleted the 240402-drop-winsock branch April 9, 2024 13:17
Pttn added a commit to RiecoinTeam/Riecoin that referenced this pull request Apr 13, 2024
@Pttn
Merge bitcoin#29786
a50b5fa
@hebasto hebasto mentioned this pull request Apr 16, 2024
Closed
hebasto added a commit to hebasto/bitcoin that referenced this pull request Apr 18, 2024
@hebasto
fixup! cmake: Build bitcoinkernel library
132cd54 
Linking to `ws2_32` is no longer needed after bitcoin#29786
@hebasto hebasto mentioned this pull request Apr 18, 2024
Merged
hebasto added a commit to hebasto/bitcoin that referenced this pull request Apr 23, 2024
@hebasto
Merge #149: cmake: Backport changes from PR29786
32f3698 
132cd54 fixup! cmake: Build `bitcoinkernel` library (Hennadii Stepanov)

Pull request description:

  Linking to `ws2_32` is no longer needed after bitcoin#29786.

  How to test:
  ```
  cmake -B build --toolchain depends/x86_64-w64-mingw32/toolchain.cmake -DBUILD_UTIL_CHAINSTATE=ON
  cmake --build build -t bitcoin-chainstate
  ```

ACKs for top commit:
  m3dwards:
    ACK 132cd54

Tree-SHA512: b8e5242556ca7723bcd636e0a9fcb0a4e66fcbc27882ac3533acf304ac70ddbe54e77f8a803ac4f16774d0c2fb50e0b48941de9d6b92a7f4f267f5fee881cedc
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 25, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#29786: Drop Windows Socket dependency for randomenv.cpp
af3be8d 
03b87a3 Drop Windows Socket dependency for `randomenv.cpp` (Hennadii Stepanov)

Pull request description:

  This change drops a dependency on the ws2_32 library for our libbitcoinkernel by switching to [`GetComputerName`](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcomputernamew) function.

ACKs for top commit:
  sipsorcery:
    utACK 03b87a3.
  laanwj:
    Code review ACK 03b87a3.
  fanquake:
    ACK 03b87a3

Tree-SHA512: a4abd5499176634d5f3fbf4e794a7504c40232fb73bd7f41955fbfb2cc7c44bc7ea4518c5203836e52f552c30414c6c3e1b24f0922641dbf1c8377981c0ffaf0
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Oct 26, 2024
@fanquake @PastaPastaPasta
Merge bitcoin#29786: Drop Windows Socket dependency for randomenv.cpp
4ecb761 
03b87a3 Drop Windows Socket dependency for `randomenv.cpp` (Hennadii Stepanov)

Pull request description:

  This change drops a dependency on the ws2_32 library for our libbitcoinkernel by switching to [`GetComputerName`](https://learn.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-getcomputernamew) function.

ACKs for top commit:
  sipsorcery:
    utACK 03b87a3.
  laanwj:
    Code review ACK 03b87a3.
  fanquake:
    ACK 03b87a3

Tree-SHA512: a4abd5499176634d5f3fbf4e794a7504c40232fb73bd7f41955fbfb2cc7c44bc7ea4518c5203836e52f552c30414c6c3e1b24f0922641dbf1c8377981c0ffaf0
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Oct 27, 2024
@PastaPastaPasta
Merge #6363: backport: trivial 2024 10 25 pr2
f211bb9 
700b8c5 Merge bitcoin#29658: Bugfix: GUI: Help messages already have a trailing newline, so don't add an extra one (merge-script)
a0cd305 Merge bitcoin#29948: test: add missing comparison of node1's mempool in MempoolPackagesTest (Ava Chow)
f1907ea Merge bitcoin#29984: net: Replace ifname check with IFF_LOOPBACK in Discover (merge-script)
daa6eee Merge bitcoin#29960: depends: pass verbose through to cmake based makefiles (merge-script)
61a5832 Merge bitcoin#29907: test: Fix `test/streams_tests.cpp` compilation on SunOS / illumos (merge-script)
23f25a9 Merge bitcoin#29872: test: Add missing Assert(mock_time_in >= 0s) to SetMockTime (merge-script)
a7daee7 Merge bitcoin#29689: lint: scripted-diff verification also requires GNU grep (Ava Chow)
3df1ca1 Merge bitcoin#29953: doc: Bash is needed in gen_id and is not installed on FreeBSD by default (merge-script)
b53b854 Merge bitcoin#29850: net: Decrease nMaxIPs when learning from DNS seeds (Ava Chow)
c4a147c Merge bitcoin#28340: security: restrict abis in bitcoind.service (Ryan Ofsky)
acfdf9e Merge bitcoin#28373: doc: Add example of mixing private and public keys in descriptors (Ava Chow)
51bc8bd Merge bitcoin#29859: build: Fix false positive `CHECK_ATOMIC` test (merge-script)
bb4102c Merge bitcoin#29893: test: fix intermittent failure in p2p_compactblocks_hb.py (glozow)
4ecb761 Merge bitcoin#29786: Drop Windows Socket dependency for `randomenv.cpp` (fanquake)
1a8e805 Merge bitcoin#29498: test: Update --tmpdir doc string to say directory must not exist (fanquake)
81ca71c Merge bitcoin#29781: depends: add new LLVM debug macro (fanquake)
5ce92ca Merge bitcoin#29527: depends: add -g to DEBUG=1 flags (fanquake)

Pull request description:

  ## Issue being fixed or feature implemented
  Trivial backports

  ## What was done?

  ## How Has This Been Tested?
  built locally

  ## Breaking Changes

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] I have performed a self-review of my own code
  - [ ] I have commented my code, particularly in hard-to-understand areas
  - [ ] I have added or updated relevant unit/integration/functional/e2e tests
  - [ ] I have made corresponding changes to the documentation
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

ACKs for top commit:
  UdjinM6:
    utACK 700b8c5 but pls confirm Guix is happy now before merging
  kwvg:
    utACK 700b8c5

Tree-SHA512: 9722979c4f0589cb02bfeaf39373713372a4aa1c8c5a55aa5b4d33388f73ef19231de3963b80ffaad25fbe0db90a133de5080baac691daed41b86a762b867b2d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@maflcko maflcko maflcko left review comments

@fanquake fanquake fanquake approved these changes

@sipa sipa Awaiting requested review from sipa

@TheCharlatan TheCharlatan Awaiting requested review from TheCharlatan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hebasto @DrahtBot @sipa @sipsorcery @TheCharlatan @laanwj @fanquake @maflcko