✅ dergoegge closed a pull request: "fuzz: Expand script verification flag testing to segwit v0 and tapscript"
(https://github.com/bitcoin/bitcoin/pull/31460)

💬 dergoegge commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-3167427246)
Can be marked up for grabs. Happy to review!

💬 maflcko commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#issuecomment-3167574502)
review ACK a20724d926d5844168c6a13fa8293df8c8927efe 🎰

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

🤔 maflcko reviewed a pull request: "refactor: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/33116#pullrequestreview-3100474378)
(nits are non-blocking)

💬 maflcko commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2262714819)
nit in 5528e0f6e01438f15ed997269bdb4db55873c0ba: Seems a bit odd why this is required. Shouldn't call sites only either put txid or wtxid in here, known at compile time?
Also, it seems a bit confusing to call it txid-hasher, when it hashes the wtxid.

💬 maflcko commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2262738986)
nit: You can use `FromUint256` to retain the compile-time check here.

💬 maflcko commented on pull request "refactor: Convert uint256 to Txid":
(https://github.com/bitcoin/bitcoin/pull/33116#discussion_r2262728298)
doc-nit: you'll also have to adjust the cpp file

🤔 ryanofsky reviewed a pull request: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345#pullrequestreview-3100314172)
Rebased 0a9a1940782fad8dc3494c4aacee0a06c030eb5c -> 2581258ec200efb173ea6449ad09b2e7f1cc02e0 ([`pr/ipc-stop.16`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.16) -> [`pr/ipc-stop.17`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-stop.17), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-stop.16-rebase..pr/ipc-stop.17)) fixing test/bitcoind conflict, improving documentation and pulling in upstream change https://github.com/bitcoin-core/libmultiprocess/pull/192


...

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2262597004)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2258021983

> [6375028](https://github.com/bitcoin/bitcoin/commit/637502894db32a1d532601ecc70d18f9b8a35f19)
>
> nit: comment above should stay attached to line below

Good catch, moved comment

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2262594955)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2197581718

> In [f8fd395](https://github.com/bitcoin/bitcoin/commit/f8fd3959d51f6f80b428828c4fd965e06a8fa19e) _ipc: Use EventLoopRef instead of addClient/removeClient_: if you have to retouch it would be good to document `m_loop` and `m_loop_ref`.

Good idea, documented both

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2262597728)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2258088297

> [6375028](https://github.com/bitcoin/bitcoin/commit/637502894db32a1d532601ecc70d18f9b8a35f19)
>
> Confirmed the test failure by removing these two lines, but that kinda surprised me because I assumed the test environment gets reset between modules anyway...?

This isn't about resetting the test environment between tests, it's more about conflicts between AppInitMain and BasicTestingSetup. AppInitMain was never cal
...

💬 ryanofsky commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2262598536)
re: https://github.com/bitcoin/bitcoin/pull/32345#discussion_r2258111912

> [ff1c927](https://github.com/bitcoin/bitcoin/commit/ff1c9278e7aee8be094c63e90ca37b644f55cc7a)
>
> If there is no parent connection does that mean for sure there are no other incoming connections?

No and in the typical case where bitcoin-node is run with -ipcbind, there will be no parent connection but incoming connections will be allowed. A parent connection only exists when a process is spawned by another, such
...

📝 hebasto opened a pull request: "cmake: Do not require Python to build GUI"
(https://github.com/bitcoin/bitcoin/pull/33156)
This PR fixes https://github.com/bitcoin/bitcoin/issues/33146, which may be considered a regression introduced in https://github.com/bitcoin/bitcoin/pull/31233.

This approach was chosen for its minimal diff.

An alternative approach would be to convert the `translate` build target into a CMake script. This could be justified, as the goal is to update files in the source tree and them commit them to the repository, which is something regular build targets are not intended to do.

On a rela
...

💬 hebasto commented on pull request "cmake: Improve Python robustness and test usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2262899423)
Apologies for the delay in responding to the comments. I've submitted a fix in https://github.com/bitcoin/bitcoin/pull/33156.

💬 sipa commented on pull request "net, validation: don't punish peers for consensus-invalid txs":
(https://github.com/bitcoin/bitcoin/pull/33050#issuecomment-3167852379)
@ajtowns Do you plan to address the feedback here?

🤔 hodlinator reviewed a pull request: "test,refactor: extract script template helpers & widen sigop count coverage"
(https://github.com/bitcoin/bitcoin/pull/32729#pullrequestreview-3094123250)
Concept ACK 748b10bbe864191ef49c32e0963e048b939f6088

Overall incremental improvements, good to add more coverage of sigop-counting considering it is in vogue.

💬 hodlinator commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2258271345)
Why is the *wrapped*-property worth calling out in a P2SH context? Do you mean in contrast to bare multisig? I'm not super clear on this area, so feel a bit uncomfortable reviewing this comment (easier to verify code changes).

💬 hodlinator commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2258225300)
1. nit: Could remove that part from the benchmark-code above too?
2. reflection: Seems like `DataStream::Compact()` is no longer called by anything.

💬 hodlinator commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2258254151)
`nSigOps` => `sig_op_count`?

💬 hodlinator commented on pull request "test,refactor: extract script template helpers & widen sigop count coverage":
(https://github.com/bitcoin/bitcoin/pull/32729#discussion_r2260350800)
Could add a comments about this check not catching invalid prefixes here and below, unless you alter the name to highlight that prefixes are not checked (see other comment in *script.h*).