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

💬 maflcko commented on issue "wallet: rpc: `settxfee` sets the wallet feerate not fee":
(https://github.com/bitcoin/bitcoin/issues/31088#issuecomment-2507658805)
On a greater scope, I wonder if it is a good approach to one-by-one deprecate one unit and add another.

If the goal is to only support one unit, it would be better to adjust all places to return that unit, so that it can be used in round-trips. Otherwise, users will have to convert values they receive from the RPC to pass them to another RPC of the same program.