💬 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?

🤔 l0rinc reviewed a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2449445449)
utACK f4df783f1ca22d96476d52ec5d1929547691ba13

💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1850825553)
leaving as is

💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1850825631)
removed

💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1850825725)
done

💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1850825828)
took modified version of file-local constant

💬 instagibbs commented on pull request "policy: ephemeral dust followups":
(https://github.com/bitcoin/bitcoin/pull/31279#discussion_r1850825989)
IIUC I'd have needed to at least make the RHS an optional to do that?

Either way, this goes away in a later commit, so I'm going to leave as-s.