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

💬 maflcko commented on pull request "contrib: Fix deterministic-unittest-coverage tool path":
(https://github.com/bitcoin/bitcoin/pull/32055#issuecomment-2720906097)
lgtm ACK 893ca5458503b432b055f0cf8a9a5aa2cfe7f6e4

Haven't tested this, but it looks plausible.

💬 hebasto commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2720917354)
The recent [feedback](https://github.com/bitcoin/bitcoin/pull/32019#discussion_r1988501276) regarding verbosity has been addressed.

Rebased on top of bitcoin/bitcoin#31161.

💬 hebasto commented on pull request "cmake: Check for `makensis` and `zip` tools before using them for optional `deploy` targets":
(https://github.com/bitcoin/bitcoin/pull/32019#discussion_r1993318514)
Thanks! [Reworked](https://github.com/bitcoin/bitcoin/pull/32019#issuecomment-2720917354).

💬 l0rinc commented on pull request "[IBD] specialize CheckBlock's input & coinbase checks":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1993324156)
By the way, the distributions of inputs lengths in `block413567` used in `CheckBlockBench ` are:

size | count
|---:|-----:|
| 1 | 1052
| 2 | 255
| 3 | 113
| 4 | 26
| 5 | 41
| 6 | 22
| 7 | 14
| >=8 | 33

<details>
<summary>>=8</summary>

```bash
8
8
8
8
8
9
9
10
10
12
12
12
14
17
17
21
24
25
26
27
31
32
35
45
56
80
90
120
200
200
200
274
357
442
```

</details>

💬 Sjors commented on pull request "signet: omit commitment for some trivial challenges":
(https://github.com/bitcoin/bitcoin/pull/29032#issuecomment-2720929755)
Occasional rebase.

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

But it is not `bash` or `sh` in the Windows environment.