Drop Windows Socket dependency for `randomenv.cpp`

hebasto commented at 11:13 am on April 2, 2024: member

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

DrahtBot commented at 11:13 am on April 2, 2024: contributor

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 commented at 11:14 am on April 2, 2024: member

Friendly ping @sipa @TheCharlatan @sipsorcery :)

sipa commented at 11:16 am on April 2, 2024: member

utACK 4d8ad853ee914ebfc6442f02dda9c626b4c037ce

in src/randomenv.cpp:366 in 4d8ad853ee outdated

361+    // Not using gethostname from Windows Sockets because
362+    // the use of the Winsock DLL might be not initiated.
363+    constexpr DWORD max_size = MAX_COMPUTERNAME_LENGTH + 1;
364+    char hname[max_size];
365+    DWORD size = max_size;
366+    if (GetComputerName(hname, &size) != 0) {

maflcko commented at 11:41 am on April 2, 2024:

0D:\a\bitcoin\bitcoin\src\randomenv.cpp(366,9): error C2664: 'BOOL GetComputerNameW(LPWSTR,LPDWORD)': cannot convert argument 1 from 'char [16]' to 'LPWSTR' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]
1D:\a\bitcoin\bitcoin\src\randomenv.cpp(366,25): message : Types pointed to are unrelated; conversion requires reinterpret_cast, C-style cast or parenthesized function-style cast [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]
2C:\Program Files (x86)\Windows Kits\10\Include\10.0.22621.0\um\winbase.h(7492,1): message : see declaration of 'GetComputerNameW' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]
3  rawtransaction_util.cpp
4D:\a\bitcoin\bitcoin\src\randomenv.cpp(366,9): message : while trying to match the argument list '(char [16], DWORD *)' [D:\a\bitcoin\bitcoin\build_msvc\libbitcoin_util\libbitcoin_util.vcxproj]
5  request.cpp

hebasto commented at 12:30 pm on April 2, 2024:

Fixed.

in src/randomenv.cpp:367 in 4d8ad853ee outdated

362+    // the use of the Winsock DLL might be not initiated.
363+    constexpr DWORD max_size = MAX_COMPUTERNAME_LENGTH + 1;
364+    char hname[max_size];
365+    DWORD size = max_size;
366+    if (GetComputerName(hname, &size) != 0) {
367+        hasher.Write((const unsigned char*)hname, size);

maflcko commented at 11:42 am on April 2, 2024:

For new code, please use the safe UCharCast helpers, if possible. Alternatively, you can use a byte-span.

hebasto commented at 12:30 pm on April 2, 2024:

Thanks! Done.

in src/randomenv.cpp:363 in 4d8ad853ee outdated

356@@ -357,10 +357,21 @@ void RandAddStaticEnv(CSHA512& hasher)
357     hasher << &hasher << &RandAddStaticEnv << &malloc << &errno << &environ;
358 
359     // Hostname
360+#ifdef WIN32
361+    // Not using gethostname from Windows Sockets because
362+    // the use of the Winsock DLL might be not initiated.

fanquake commented at 12:02 pm on April 2, 2024:

// the use of the Winsock DLL might be not initiated.

Why not? Comments being added to our RNG code should not be this vauge.

hebasto commented at 12:04 pm on April 2, 2024:

When using libbitcoinkernel, it depends on its user, no?

fanquake commented at 12:36 pm on April 2, 2024:

I don’t know, given the kernel and it’s API/runtime requirements don’t exist yet. It’s also less clear, because there is nothing else in this codebase documenting this requirement, or preventing winsock usage (or any other DLL that might require manual initialization) from inadvertantly ending up back in the kernel.

hebasto commented at 12:48 pm on April 2, 2024:

… there is nothing else in this codebase … preventing winsock usage (or any other DLL that might require manual initialization) from inadvertantly ending up back in the kernel.

Well, the upcoming CMake-based build system will likely complain if Winsock dependency is about to be reintroduced.

fanquake commented at 12:54 pm on April 2, 2024:

CMake-based build system will likely complain

Complain about what? Putting aside all the other points here, why are we changing code in our RNG, supposedly based on some thing that CMake may or may not do, that you didn’t mention at all in the PR description?

hebasto commented at 1:01 pm on April 2, 2024:

Perhaps, I didn’t understand your point correctly. Sorry about that.

This PR goal is to remove winsock dependency from libbitcoinkernel. The current build system does not prevent from reintroducing this dependency inadvertantly. But CMake does.

sipa commented at 1:10 pm on April 2, 2024:

@fanquake I don’t understand the antagonism here. The PR description mentions, and has mentioned, the removal of a dependency of the RNG code on the winsock library. If easily doable, like this demonstrates, then that seems like a simple improvement.

fanquake commented at 1:26 pm on April 2, 2024:

@sipa see my comment here: #29786#pullrequestreview-1973587507. It stems somewhat from this being a recurring issue, that rather than fixing “properly”, keeps just getting worked around, it comes somewhat from vauge / useless comments, being added to important parts of the codebase (that themselves point to an underlying issue not actually being documented/fixed), as well as the case that, if the point is to remove a dependency / check for it’s usage, then a test or similar should be added that actually enforces that, rather than just hoping that will remain the case (which would undermine the point of the change). It also comes somewhat from the claims that “CMake will fix/enforce this”, when, in reality, after offline discussion, that also is not actually the case.

hebasto commented at 1:30 pm on April 2, 2024:

it comes somewhat from vauge / useless comments

A code comment has been dropped.

TheCharlatan commented at 9:06 pm on April 2, 2024:

then a test or similar should be added that actually enforces that

Could be something as simple as doing a symbol check for bitcoin-chainstate and ensuring WS2_32 is not in its import table. I guess we should be doing this anyway at some point, but not sure where I would put such a check for non-release, experimental binaries.

fanquake commented at 8:15 am on April 9, 2024:

I guess we should be doing this anyway at some point,

I will try follow up here with something more globally applicable.

fanquake commented at 12:10 pm on April 2, 2024: member

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 commented at 12:28 pm on April 2, 2024: member

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 force-pushed on Apr 2, 2024

Drop Windows Socket dependency for `randomenv.cpp`

This change drops a dependency on the ws2_32 library for our
libbitcoinkernel, which may not be desirable.

03b87a3e64

hebasto force-pushed on Apr 2, 2024

DrahtBot added the label CI failed on Apr 2, 2024

sipsorcery commented at 8:16 pm on April 2, 2024: member

utACK 03b87a3e64305ba651e22a730e35271dea8fea64.

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

DrahtBot requested review from sipa on Apr 2, 2024

TheCharlatan commented at 9:02 pm on April 2, 2024: contributor

Concept ACK

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

DrahtBot removed the label CI failed on Apr 5, 2024

laanwj commented at 2:43 pm on April 8, 2024: member

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 03b87a3e64305ba651e22a730e35271dea8fea64.

DrahtBot requested review from TheCharlatan on Apr 8, 2024

fanquake approved

fanquake commented at 8:17 am on April 9, 2024: member

ACK 03b87a3e64305ba651e22a730e35271dea8fea64

fanquake merged this on Apr 9, 2024

fanquake closed this on Apr 9, 2024

hebasto deleted the branch on Apr 9, 2024

Drop Windows Socket dependency for randomenv.cpp #29786

Code Coverage

Reviews

Drop Windows Socket dependency for `randomenv.cpp` #29786