💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#discussion_r1993185874)
Pushed a test.

💬 Sjors commented on pull request "mining: drop unused -nFees and sigops from CBlockTemplate":
(https://github.com/bitcoin/bitcoin/pull/31897#issuecomment-2720701557)
Rebased and switched the order of commits, so it's more clear this PR doesn't change behavior.

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1993202306)
I've updated code block. Could you please check @maflcko and @davidrobinsonau

💬 Sjors commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2720735149)
The current approach of having the Template Provider communicate via IPC, as described in #31098, indeed does not require Bitcoin Core to change its net code.

Making easy to copy-paste code could still be a motivation for the author, but it would not a good reason to include it.

Additionally, as @theuni pointed out in https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-2702063374, it may be the right design / abstraction for a Template Provider either.

💬 janb84 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720738307)
> @brunoerg @janb84 @Prabhat1308 I expect all of you were running into the issue on macOS? I probably can't fix it myself, but I'd investigate whether the last commit is working:
>
> * Is your result different with or without the last commit?
> * Does `ResetCoverageCounters` work?
> * What is the coverage result for `src/test/util/setup_common.cpp`, especially `g_rng_temp_path_init`? (You can find it in `fuzz_det_cov.show.{run_id}.txt`)

In checking this issue I discovered that due to the
...

💬 maflcko commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720745456)
> I discovered that due to the change in [31161](https://github.com/bitcoin/bitcoin/pull/31161) the tooling is broken. (the path has changed from `src/test/` to `/bin` )

Should be trivial to fix up. I am happy to review a tiny pull to fix the silent merge conflict here.

👍 rkrux approved a pull request: "contrib: Fix `gen-bitcoin-conf.sh`"
(https://github.com/bitcoin/bitcoin/pull/32049#pullrequestreview-2681359199)
tACK a24419f8bed5e1145ce171dbbdad957750585471

I can see it adds the previously excluded options now (starting with `alertnotify`) in the example conf file.

💬 hebasto commented on issue "cmake: (release) version handling is broken":
(https://github.com/bitcoin/bitcoin/issues/31898#issuecomment-2720804983)
> It seems that the first thing to decide is the source of the version information:
>
> * the `CLIENT_VERSION_*` variables set by the build system, or
>
> * a GitHub tag

A related discussion happens in https://github.com/bitcoin-core/bitcoin-maintainer-tools/issues/178.

💬 saikiran57 commented on pull request "removed duplicate call to GetDescriptorScriptPubKeyMan":
(https://github.com/bitcoin/bitcoin/pull/32023#discussion_r1993261238)
My primary goal is to give fix with minor changes to code, that's why I've updated this `throw std::runtime_error(std::string(__func__) + ": " + error);` to `throw std::invalid_argument(error);` so it does its satisfy the test case. During rpc call if we face issue I cached this exception(invalid_argument) return with existing response message to user if any other exception happened I just left with default implementation.

Regarding your suggestion using `util::Result<void>`. Inside AddWallet
...

💬 hebasto commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2720833753)
Rebased to resolve a silent merge conflict with merged bitcoin/bitcoin#31161.

🤔 l0rinc requested changes to a pull request: "refactor: CLI cleanups"
(https://github.com/bitcoin/bitcoin/pull/31887#pullrequestreview-2681370790)
I like these cleanups, ans we should go a step further and modernize the code more when touching them.
I'm also not a fan of the 3rd commit for the inheritance cases and would prefer seeing the last commit in a separate PR, doesn't seem like a "refactor:"

💬 l0rinc commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993237690)
This is better than before, but having a `NETINFO_MAX_LEVEL` and a checking the bounded values in methods and having `DetailsRequested`, but also checking it manually in https://github.com/bitcoin/bitcoin/blob/master/src/bitcoin-cli.cpp#L499-L501 where we're checking the raw `n` value seems quite hacky.

Seems the code is begging for a bounded type - if we're cleaning it up, we might as well try an enum here:

```C++
enum class NetinfoDetailLevel : uint8_t {
BASIC = 0,
PEERS,

...

💬 l0rinc commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993269714)
simple structs are fine, but I'm not a fan of struct inheritance, I like the previous version more in the non-trivial cases

💬 l0rinc commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993256631)
from the usages it seems we can modernize this in a few more ways:
* `UNKNOWN_NETWORK` seems to serve as a value-missing proxy - we could use an optional instead
* `const std::string&` could be modernized to `static std::optional<int8_t> NetworkStringToId(std::string_view network)`


This would allow us to change the usages to something like:
```C++
if (auto network_id{NetworkStringToId(network_name)}) {
++counts.at(*network_id);
}
```

💬 l0rinc commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993272111)
This is a surprising change, the PR states this is a refactor - I would prefer doing this separately

💬 l0rinc commented on pull request "refactor: CLI cleanups":
(https://github.com/bitcoin/bitcoin/pull/31887#discussion_r1993264816)
we're returning an `int8_t`, we might as well iterate over one
```suggestion
for (int8_t i{0}; i < NETWORKS.size(); ++i) {
```

👍 maflcko approved a pull request: "test: avoid treating hash results as integers (part 1)"
(https://github.com/bitcoin/bitcoin/pull/32050#pullrequestreview-2681458911)
review ACK a82829f37e1ed298b6c2b6d2859d4bea65fe3dcc 🐘

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK a82829f37e1e
...

💬 maflcko commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#discussion_r1993286997)
would be nice to remove `calc_sha256(True)` completely in a follow-up, or at least force named args.

Also, if the wtxid is never cached, I wonder what the point is to cache the txid. Either both or none are cached.

But this can be done in a follow-up.

💬 Sjors commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2720862931)
TSan failure is spurious (https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2718471034), but would be good to re-run.

💬 l0rinc commented on pull request "test: avoid treating hash results as integers (part 1)":
(https://github.com/bitcoin/bitcoin/pull/32050#issuecomment-2720869178)
Concept ACK