💬 ShreeHarsha-22 commented on issue "Add `sat_to_btc()` and conversely `btc_to_sat()` util functions in functional tests":
(https://github.com/bitcoin/bitcoin/issues/31345#issuecomment-2506647018)
Hey @rkrux, could you please assign this issue to me? I’d love to take a look and work on it.

💬 TheCharlatan commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1862626447)
This is also a debug only log, and it otherwise does not seem to be hitting any hot code paths, so I am not too concerned either.

💬 hodlinator commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1862652609)
`GetAuthCookieFile()` internally uses `GetPathArg()` and follows it's pattern (predating this PR):
https://github.com/bitcoin/bitcoin/blob/7590e93bc73b3bbac641f05d490fd5c984156b33/src/common/args.cpp#L272-L274

But might change to `optional` if you insist. Tried out changing it in 01fa9d41d12a68f88f6251ae43234dd250a53bd5. Works fairly well, even uncovered a minor pre-existing bug in that if `fs::permissions` fails, we currently print out the wrong filename. With that bug it feels like it coul
...

💬 rebroad commented on issue "Prioritize processing of peers based on their CPU usage":
(https://github.com/bitcoin/bitcoin/issues/31033#issuecomment-2506724582)
> We already keep stats for each and every peer, displayed in the `getpeerinfo` RPC, e.g. `bitcoin-cli getpeerinfo`.

do those stats include CPU time though?

💬 mzumsande commented on pull request "rpc: Remove submitblock pre-checks":
(https://github.com/bitcoin/bitcoin/pull/31175#discussion_r1862673015)
it's unconditional once out of IBD. But allowing untrusted parties access to this rpc is unsafe anyway (can spam the node with low-work blocks since `force_processing` and `min_pow_checked` are true for the `ProcessNewBlock` call).

👍 BrandonOdiwuor approved a pull request: "Remove `src/config` directory"
(https://github.com/bitcoin/bitcoin/pull/31390#pullrequestreview-2468804522)
ACK 935973b315fa3b99f5e79b360bcb66f473bb7b95

💬 theStack commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#issuecomment-2506900085)
Opened a PR for BIP-373 which aims to improve the clarity of some PSBT fields containing pubkeys, which should hopefully also make reviewing this PR a bit easier: https://github.com/bitcoin/bips/pull/1705 (I sometimes had to look at the code first to find out how the specification is really to be interpreted...)

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