👍 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

💬 Sjors commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2720869273)
cc orhpanage fans @glozow, @sipa, @mzumsande

📝 janb84 opened a pull request: "contrib: Fix deterministic-unittest-coverage tool path"
(https://github.com/bitcoin/bitcoin/pull/32055)
Fix for the tooling introduced/modified in #31901 but the tool path is broken due to silent merge conflict introduced by #31161.

The `deterministic-unittest-coverage` and `deterministic-fuzz-coverage` tools uses the `fuzz` and `test_bitcoind` binaries, for which the location was modified in #31161. This patch updates the location to align with that change.

💬 janb84 commented on pull request "contrib: Add deterministic-unittest-coverage":
(https://github.com/bitcoin/bitcoin/pull/31901#issuecomment-2720894341)
Fix created in https://github.com/bitcoin/bitcoin/pull/32055

💬 Sjors commented on pull request "Have createNewBlock() wait for tip, make rpc handle shutdown during long poll and wait methods":
(https://github.com/bitcoin/bitcoin/pull/31785#issuecomment-2720894982)
Rebased after #31283. There was no conflict, but the PR description makes reference to `waitNext()` so this should make it easier to (re-)review with that context in mind.

💬 TheCharlatan commented on pull request "refactor: Improve assumeutxo state representation":
(https://github.com/bitcoin/bitcoin/pull/30214#issuecomment-2720898462)
> Maybe what you are are suggesting could be an improvement, through, and it's been a while since I thought about this and reasons I listed above are speculative. Could try implementing the suggestion and see if it simplifies the code.

This is what I had in mind: https://github.com/TheCharlatan/bitcoin/commit/9c89548c670e7b807a969e13408ff03df15143a2 (also rebased this branch to get cmake) . It is very broken in its current state though. There are a few problems with it, but I guess the fundam
...

💬 maflcko commented on pull request "ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#discussion_r1993309811)
I don't understand this. Don't the docs claim that `set -e` is set for bash/sh?

And if `&& '` is needed here, wouldn't it be required in all other `run`s as well that span over more than one line?