💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1862855947)
> -maxconnections=0 -privatebrodcast and then addnode your trusted peer

Thanks for the idea, but sadly this does not work for this either. It will happily add only my trusted node, but it will not create the private broadcast connections when actually trying to send. It will just wait at 0 connections.

Since we break the maxconnections value already for the addnode exception, could we make an exception for private broadcast connections as well?

🤔 mzumsande reviewed a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112#pullrequestreview-2469192751)
Concept ACK

💬 mzumsande commented on pull request "Improve parallel script validation error debug logging":
(https://github.com/bitcoin/bitcoin/pull/31112#discussion_r1863025707)
comment could be adjusted after the rework:
Maybe something like "Join the execution until completion, if at least one evaluation wasn't successful return its error"?

📝 fanquake unlocked a pull request: "refactor: Use move semantics instead of custom swap functions"
(https://github.com/bitcoin/bitcoin/pull/26749)
This PR makes code more succinct and readable by using move semantics.

📝 maflcko opened a pull request: "refactor: Move GuessVerificationProgress into ChainstateManager"
(https://github.com/bitcoin/bitcoin/pull/31393)
Currently the function is standalone, which means any passed-in data like `TxData` or the block pointer needs to be taken from the `ChainstateManager` and passed in. This is currently verbose and may become even more verbose if the function is reworked in the future. As the function can not be called without a `ChainstateManager` in production code anyway, make it a member function on the class.

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863298196)
so do I understand it correctly that it's fine for other tests to be affected when this one already fails, leaving the directory?

💬 polespinasa commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#issuecomment-2507513442)
I'm not sure why that CI check ,``Win64 native, VS2022 (pull_request)``, fails. Help is appreciated here.

💬 willcl-ark commented on issue "Distribute darknet node addresses via DNS seeds using AAAA records":
(https://github.com/bitcoin/bitcoin/issues/31062#issuecomment-2507563863)
Might it not make more sense to avoid hacking DNS responses for this, or using DNS at all, and simply include some native Tor (/I2P/CJDNS) addresses in the `vSeeds` chainparams list? These hard-coded addresses could still point to a DNS seeder which can respond to addr_fetch requests from its hidden service (or I2P equivalent) with addresses of that type only. @achow101 [dnsseedrs](https://github.com/achow101/dnsseedrs/tree/main) already has onion, I2P and CJDNS results available to query, for e
...

💬 willcl-ark commented on issue "lint: markdown check linting build outputs":
(https://github.com/bitcoin/bitcoin/issues/31044#issuecomment-2507573948)
> The mlc issue is tracked in [github.com/becheran/mlc/pull/96](https://www.github.com/becheran/mlc/pull/96)

This PR has been merged, but we would ideally wait for it to be included in a tagged release so we can download pre-build binaries.

I have asked in the PR if a new tag can be released, and will update here.

🤔 l0rinc requested changes to a pull request: "util: Improve documentation and negation of args"
(https://github.com/bitcoin/bitcoin/pull/31212#pullrequestreview-2469613723)
Thanks for enduring through the struggles the Windows gods bestow upon you.
While there are parts I can't meaningfully review yet (no idea what to do with the cookies yet), I left a few comments about how some restructuring could help me understand the context better: if a commit should not be merged without another one, I would prefer combining them into a single commit - to avoid the bloat, it makes review harder.

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863317001)
Given that `IsArgNegated` returns `GetSetting(strArg).isFalse()` where `isFalse` returns `(typ == VBOOL) && (val != "1")` which contradicts: `bool isNull() const { return (typ == VNULL); }` in `IsArgSet` returning `!GetSetting(strArg).isNull()`, it seems to me we can simplify this to:
```suggestion
if (args.IsArgNegated("-conf"))
```

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863322403)
👍 for changing return false logging to warn!
nit: in other cases we've quoted the file paths

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863320134)
super-nit: redundant `\n`
(and as mentioned, if we don't need to parse the output, I'd prefer a more user-friendly output)

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863319077)
I find these kind of messages to be more readable than `Config file: %s (is directory, not file)` or `"Config file: <disabled>`

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863337310)
My untrained eyes would prefer seeing this in a different PR, I can't meaningfully review these yet :/

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863326293)
nit: "should never happen" -> but it just did, so can we either provide more context or remove the statement that was just invalidated?
```suggestion
assert len(debug_logs) == 1, (
'Found multiple debug.log files, expected only one:\n\t' +
'\n\t'.join(str(f) for f in debug_logs)
)
```

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863338147)
Does it signal the lack of `self.stop_node(0)`? Does this mean the test run order matters now?

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863323186)
same nit: `\n` is likely redundant in these cases as well

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863364323)
it's hard to review 14 commits, as hinted before, could the fix be in the same commit as the test - I personally am lacking a lot of context and wouldn't want this commit to be merged without making sure a test is added as well

💬 l0rinc commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1863392073)
I'm fine with doing it in a different PR - will leave it up to you