🤔 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

💬 maflcko commented on pull request "wallet, rpc: Settxfeerate":
(https://github.com/bitcoin/bitcoin/pull/31278#discussion_r1863408856)
Parsing it twice and casting to double in-between seems highly fragile. It would be better to directly parse into a fraction.