net: remove useless call to IsReachable() from CConnman::Bind() #25735

pull vasild wants to merge 1 commits into bitcoin:master from vasild:remove_IsReachable_from_Bind changing 1 files +5 −9

vasild commented at 11:41 AM on July 29, 2022: contributor

CConnman::Bind() is called without BF_EXPLICIT only when passed either 0.0.0.0 or ::. For those addresses IsReachable() is always true (regardless of the -onlynet= setting!), meaning that the if condition never evaluates to true.

IsReachable() is always true for the "any" IPv4 and IPv6 addresses because CNetAddr::GetNetwork() returns NET_UNROUTABLE instead of NET_IPV4 or NET_IPV6 and the network NET_UNROUTABLE is always considered reachable.

It follows that BF_EXPLICIT is unnecessary, remove it too.
fanquake added the label P2P on Jul 29, 2022
DrahtBot commented at 9:57 PM on July 29, 2022: contributor


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #22729 (Make it possible to disable Tor binds and abort startup on bind failure 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.
jonatack commented at 10:55 AM on July 30, 2022: contributor

Code review ACK d7c1d9f133961320af6df9db76860772ef44de95 this seems correct, though due to the logic spread among various methods I wonder if the check should stay as belt-and-suspenders against future changes
shaavan commented at 12:12 PM on July 30, 2022: contributor
Code review ACK d7c1d9f133961320af6df9db76860772ef44de95
- I verified that the explanation for the change in the description is sound.
- And I verified that the BF_EXPLICIT variable is redundant and can be safely removed.
aureleoules commented at 2:08 PM on August 15, 2022: member

ACK d7c1d9f133961320af6df9db76860772ef44de95. Verified that the statements in the PR descriptions are correct. BF_EXPLICIT is only passed with a 0.0.0.0 or :: address and IsReachable is always true for these addresses.
in src/net.cpp:94 in d7c1d9f133 outdated
```
  90 | @@ -91,7 +91,6 @@ static constexpr auto FEELER_SLEEP_WINDOW{1s};
  91 |  /** Used to pass flags to the Bind() function */
  92 |  enum BindFlags {
  93 |      BF_NONE         = 0,
  94 | -    BF_EXPLICIT     = (1U << 0),
```
mzumsande commented at 2:29 PM on August 15, 2022:

Should the other enum entries be moved down so that there isn't a gap?

vasild commented at 1:18 PM on August 18, 2022:

These are stored in memory only, so it is irrelevant. They would have to stay the same if they were written on disk. The gap could cause somebody to ask themselves "why is there a gap" in the future.

There are already 3 ACKs, should I change it?

aureleoules commented at 8:02 AM on August 22, 2022:

Missed this, I think you should change it for clarity. I will ACK again.

vasild commented at 12:20 PM on August 22, 2022:

Changed, thanks!

net: remove useless call to IsReachable() from CConnman::Bind()

`CConnman::Bind()` is called without `BF_EXPLICIT` only when passed
either `0.0.0.0` or `::`. For those addresses `IsReachable()` is always
true (regardless of the `-onlynet=` setting!), meaning that the `if`
condition never evaluates to true.

`IsReachable()` is always true for the "any" IPv4 and IPv6 addresses
because `CNetAddr::GetNetwork()` returns `NET_UNROUTABLE` instead of
`NET_IPV4` or `NET_IPV6` and the network `NET_UNROUTABLE` is always
considered reachable.

It follows that `BF_EXPLICIT` is unnecessary, remove it too.

9cbfe40d8a

vasild force-pushed on Aug 22, 2022
vasild commented at 12:19 PM on August 22, 2022: contributor

d7c1d9f133...9cbfe40d8a: address suggestion

Invalidates ACKs from @jonatack, @shaavan, @aureleoules
aureleoules commented at 12:24 PM on August 22, 2022: member

ACK 9cbfe40d8af8567682284890c080b0c3cf434490
naumenkogs commented at 9:02 AM on August 24, 2022: member

ACK 9cbfe40d8af8567682284890c080b0c3cf434490
fanquake requested review from mzumsande on Oct 2, 2022
mzumsande commented at 4:48 PM on October 3, 2022: contributor

ACK 9cbfe40d8af8567682284890c080b0c3cf434490

I agree that the change is correct.

Although I find it strange that addresses that are unroutable are always reachable, which we now explicitly rely on (and probably did in other parts of the codebase before?) Is there any intuitive explanation for that? Just from the naming, I would have expected reachable addresses to be a subset of routable addresses.
fanquake merged this on Oct 3, 2022
fanquake closed this on Oct 3, 2022
sidhujag referenced this in commit 708236490f on Oct 4, 2022
vasild deleted the branch on Oct 4, 2022
vasild commented at 7:43 AM on October 4, 2022: contributor

@mzumsande I find it strange too and it is plain wrong. Maybe the idea was that e.g. 127.0.0.1 is unroutable but always reachable. However !CNetAddr::IsRoutable() includes much more addresses that are not necessary guaranteed to be reachable. I even started changing that at some point but left it for another time because the (risk + effort) / importance ratio seemed too high (that is subjective).
bitcoin locked this on Oct 4, 2023

Contributors

Labels

net: remove useless call to IsReachable() from CConnman::Bind() #25735

Conflicts