💬 andremralves commented on issue "Discover() will not run if listening on any address with an explicit bind=0.0.0.0":
(https://github.com/bitcoin/bitcoin/issues/31293#issuecomment-2488958085)
Is `Discover()` working properly? I tried running `test_runner.py --ihave1111and2222 feature_bind_port_discover.py` and the test didn't find the `1.1.1.1` and `2.2.2.2` addresses. In the commit where `feature-bind_port_discover.py` was added, the test works as expected and passes. Actually, I think, now, only ipv6 addresses are being discovered.

🤔 l0rinc reviewed a pull request: "kernel: make m_tip_block std::optional"
(https://github.com/bitcoin/bitcoin/pull/31325#pullrequestreview-2449193292)
Concept ACK, please see my simplification nits.

💬 l0rinc commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850680750)
Since C++17 we can [safely compare](https://en.cppreference.com/w/cpp/utility/optional/operator_cmp) an `optional<T>` with another `T`:
```C++
template< class T, class U >
constexpr bool operator!=(const optional<T>& opt, const U& value);
(23) (since C++17)
```
If you modify again, consider simplifying to:
```suggestion
return (notifications().m_tip_block && notifications().m_tip_block != current_tip) || chainman().m_interrupt;
```

💬 l0rinc commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850685295)
Does the `BOOST_REQUIRE` serve here as a better error message than what `m_tip_block.value()` would already throw (i.e. `bad_optional_access()`)?

💬 l0rinc commented on pull request "kernel: make m_tip_block std::optional":
(https://github.com/bitcoin/bitcoin/pull/31325#discussion_r1850668343)
I've noticed that we're checking whether the optional has a value in different ways here and in `interfaces.cpp`.
If you change the PR again, please consider unifying:
```suggestion
return kernel_notifications.m_tip_block || ShutdownRequested(node);
```

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-2489128571)
Updated 6c9121f7907262b2bf065a7ceeb8bca620060a7f -> 97fe2b25af31ca612c1f8d9f3de739fa3dee3902 ([kernelApi_1](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_1) -> [kernelApi_2](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_2), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_1..kernelApi_2))

* Added @stickies-v's [suggestion](https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1757015877), implementing variadic args for nonnull attribute macro.

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r1850697300)
Thanks, decided to take this. I was a bit careful here, because I lifted the check from secp, which also does not use variadic args: https://github.com/bitcoin-core/secp256k1/blob/master/include/secp256k1.h#L174. But thinking a bit more about it, I could not come up with a good reason not to, so took your suggestion.

⚠️ andremralves opened an issue: "Functional tests: `feature_bind_port_discover.py` is failing"
(https://github.com/bitcoin/bitcoin/issues/31336)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

The test is failing due to `Discover()` not being able to finding the local addresses "1.1.1.1" and "2.2.2.2".

### Expected behaviour

The test should pass. Addr1 and Addr2 should be found.

### Steps to reproduce

Run the following commands:
```bash
sudo ifconfig lo:0 1.1.1.1/32 up && sudo ifconfig lo:1 2.2.2.2/32 up
./build/test/functional/test_runner.py --ihave1111and2222 feature_bi
...

🤔 l0rinc reviewed a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2449278047)
I love that you search for these inconsistencies and attack them head on.
+1 for adding representative tests (which I didn't review in detail yet, will do in the next round of reviews).

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850720954)
slightly unrelated: Checking the different options, it's not always trivial to assess what negation would mean, but
```C++
if (gArgs.IsArgSet("-color")) {
const std::string color{gArgs.GetArg("-color", DEFAULT_COLOR_SETTING)};
if (color == "always") {
should_colorize = true;
} else if (color == "never") {
should_colorize = false;
} else if (color != "auto") {
throw std::runtime_error("Invalid value for -color option
...

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850737479)
in `scripted-diff: Avoid printing version information for -noversion`:

Nit: the replacement doesn't need any escapes, if you edit again, consider simplifying:
```bash
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's/\b(gArgs|args)\.IsArgSet\("-version"\)/\1.GetBoolArg("-version", false)/g' $(git grep -l '-version')
-END VERIFY SCRIPT-
```

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850740240)
nit: since we're using all values later (here and in a few other examples below), we might as well leave this as `IsArgSet` at the top - it would simplify the PR diff, but make the scripted diff more complicated. Will leave it for you to decide, don't have strong feelings either way.

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850750700)
Do these same arguments apply to `blocksdir` & `walletdir`?

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850760249)
Since you edit these already, could you please check if the trailing newline is still needed?

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850768609)
You gotta' love these 3-state-booleans :)

nit: To reduce the noise, consider simplified to either
```suggestion
if (std::optional result{GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)}) {
```
or
```suggestion
if (auto result{GenerateAuthCookie(&strRPCUserColonPass, cookie_perms)}) {
```

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850771750)
this seems like a slight behavior change: previously when `GenerateAuthCookie` returned `true`, this was logged, now it seems to me it may not be (haven't tried the code)

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850774218)
nit: we might as well use f-strings here:
```suggestion
assert_equal(BLOCKS, self.nodes[0].cli('-norpccookiefile', f'-rpcuser={user}', f'-rpcpassword={password}').getblockcount())
```

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1850762988)
super-nit: formatting seems a bit off (whitespace and missing trailing full stop in first line)

🤔 furszy reviewed a pull request: "fuzz: Implement G_TEST_GET_FULL_NAME"
(https://github.com/bitcoin/bitcoin/pull/31333#pullrequestreview-2449405248)
utACK 92d3d691f097ead8e5ae571eb9bf691133a6aa49

💬 TheCharlatan commented on pull request "validation: fetch block inputs on parallel threads ~17% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/31132#discussion_r1848485522)
Should this rather say "optionally marking the emplaced coin as not dirty", since the default is always dirty?