laanwj
commented at 6:57 pm on October 1, 2024:
member
Instead of a gethostname hack, which is not guaranteed to return all addresses, use the official way of calling GetAdaptersAddresses to get local network addresses on Windows.
Do the same checks as the UNIX path: interface is up, interface is not loopback.
Suggested by Ava Chow.
Addiional changes:
Cleanup: move out FromSockAddr in netif.cpp from MacOS and use it everywhere appropriate. This avoids code duplication.
laanwj added the label
Windows
on Oct 1, 2024
laanwj added the label
P2P
on Oct 1, 2024
DrahtBot
commented at 6:57 pm on October 1, 2024:
contributor
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Added an upper limit here to avoid hanging forever or consuming absurd amounts of resources in case of a windows bug. i would imagine 4MB of adapter addresses ought to be enough for anyone.
laanwj
commented at 1:04 pm on January 13, 2025:
member
Can anyone review here please?
Mind that the first commit “Add optional length checking to CService::SetSockAddr” is also part of #31022.
Edit: needs rebasing for #31022 (review) assuming that goes in first
Edit.2: done, took over the same commit, network address length determination logic was moved to FromSockAddr for the unknown-length case
laanwj force-pushed
on Jan 14, 2025
jonatack
commented at 9:48 pm on January 15, 2025:
member
Concept ACK
in
src/common/netif.cpp:298
in
219872fc75outdated
296+ if (status == ERROR_BUFFER_OVERFLOW && out_buf.size() < MAX_ADAPTER_ADDR_SIZE) {
297+ // If status == ERROR_BUFFER_OVERFLOW, out_buf_len will contain the needed size.
298+ // Unfortunately, this cannot be fully relied on, because another process may have added interfaces.
299+ // So to avoid getting stuck due to a race condition, double the buffer size at least
300+ // once before retrying (but only up to the maximum allowed size).
301+ do {
Agree, the nested loop here isn’t necessary. It’s already looping.
sipa
commented at 6:01 pm on January 17, 2025:
member
Concept and code review ACK219872fc75e552c87c20b5c1ce7e15fa887eec20, but I am not familiar with the Windows network API, and have not tested it.
DrahtBot requested review from jonatack
on Jan 17, 2025
laanwj
commented at 2:00 pm on January 21, 2025:
member
and have not tested it.
FWIW, i have tested this on a windows machine. Though a virtual one on amazon EC. Both IPv4 and IPv6 addrsses were picked up.
Of course, more testing is very welcome.
achow101 referenced this in commit
c65233230f
on Feb 11, 2025
DrahtBot added the label
Needs rebase
on Feb 12, 2025
laanwj force-pushed
on Feb 12, 2025
laanwj
commented at 11:45 am on February 12, 2025:
member
Rebased for merge of #31022. This got rid of the first commit, which was shared with that PR.
DrahtBot removed the label
Needs rebase
on Feb 12, 2025
in
src/common/netif.cpp:34
in
851085a7f2outdated
30@@ -31,6 +31,30 @@
3132 namespace {
3334+//! Return CNetAddr for the specified OS-level network address. If a length is provided, it is checked for validity.
davidgumberg
commented at 0:41 am on February 13, 2025:
0If a length is provided, it is checked for validity.
Maybe it would be good to mention that it is checked for validity inside of SetSockAddr(), or is this detail that the caller of FromSockAddr() does not need to know?
Right, the logic here has been swapped since this comment was written 😄
i think that sentence is redundant. That the length is checked is implied, why pass it otherwise? The line below already describes which length is used when it’s not passed.
davidgumberg
commented at 0:43 am on February 13, 2025:
contributor
Nice refactor in reusing FromSockAddr() and probably makes local address discovery on Windows less brittle.
Ran this branch and master on a local Windows 10 22H2 machine with multiple network adapters, both Discover()‘ed the same IPv6 addresses from all adapters, which matched up with what ifconfig reported as the machine’s addresses. The device does not have any IPv4 addresses which pass CNetAddr::IsRoutable() so no Discover() messages were logged for IPv4 so I have not tested IPv4.
DrahtBot requested review from sipa
on Feb 13, 2025
laanwj
commented at 9:45 am on February 13, 2025:
member
Thanks for testing!
DrahtBot shouldn’t have re-requested review, i first need to address sipa’s comment in the first place.
maflcko
commented at 9:56 am on February 13, 2025:
member
DrahtBot shouldn’t have re-requested review, i first need to address sipa’s comment in the first place.
For outstanding review comment the bot waits for the next ack to happen, after which it assumes anything outstanding to be addressed. It is not perfect, but I can’t come up with something better, and it seems fine as long any other reviewer notices.
laanwj
commented at 10:04 am on February 13, 2025:
member
For outstanding review comment the bot waits for the next ack to happen, after which it assumes anything outstanding to be addressed. It is not perfect, but I can’t come up with something better, and it seems fine as long any other reviewer notices.
Yes the heuristic’s usually correct, don’t know anything better. Was just noting it, as github has no way to un-request review.
DrahtBot removed review request from sipa
on Feb 13, 2025
laanwj force-pushed
on Feb 13, 2025
laanwj
commented at 12:49 pm on February 13, 2025:
member
Removed the nested loop in GetAdaptersAddresses (@sipa)
Made doc comment for FromSockAddr more concise (@davidgumberg)
DrahtBot added the label
CI failed
on Feb 13, 2025
DrahtBot
commented at 1:30 pm on February 13, 2025:
contributor
Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:
Possibly 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.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.
Leave a comment here, if you need help tracking down a confusing failure.
laanwj force-pushed
on Feb 13, 2025
net: Use GetAdaptersAddresses to get local addresses on Windows
Instead of a `gethostname` hack, use the official way of calling
`GetAdaptersAddresses` to get local network addresses on Windows.
As additional cleanup, move out `FromSockAddr` from MacOS and use it
everywhere appropriate.
Suggested by Ava Chow.
b9d4d5f66a
laanwj force-pushed
on Feb 13, 2025
DrahtBot removed the label
CI failed
on Feb 13, 2025
davidgumberg
commented at 5:11 am on February 14, 2025:
contributor
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: 2025-03-30 09:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me