👍 ryanofsky approved a pull request: "miner: drop dummy extraNonce in coinbase scriptSig for templates requested via IPC"
(https://github.com/bitcoin/bitcoin/pull/32420#pullrequestreview-3588945400)
Code review ACK 9832a9aa2bffb5c0e14eb67ce22d9ce900106304. Just rebased since last review and tweaked comment

💬 l0rinc commented on pull request "ci: run s390x job":
(https://github.com/bitcoin/bitcoin/pull/33436#issuecomment-3666537730)
That sounds like a good idea, that's the most painful part.
Is cross compilation stable cross-endianness? I guess it has to be, has anyone tried it?

👍 ryanofsky approved a pull request: "mining: getCoinbase() returns struct instead of raw tx"
(https://github.com/bitcoin/bitcoin/pull/33819#pullrequestreview-3588964247)
Code review ACK 1bd6cae51fedeff62755e03ddcb9346b868f6d83. Just rebasing again since last review to avoid minor conflict

💬 maflcko commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3666546750)
review ACK 73439910a02c2c07de94ad3a78ac3421ba7218d9 💣

<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 73439910a02c
...

💬 maflcko commented on issue "GUI crashes in regtest":
(https://github.com/bitcoin-core/gui/issues/918#issuecomment-3666633855)
Is this really gui-specific? Does it not happen with `bitcoind`?

✅ maflcko closed a pull request: "Defer transaction signing until user clicks Send"
(https://github.com/bitcoin-core/gui/pull/915)

📝 maflcko reopened a pull request: "Defer transaction signing until user clicks Send"
(https://github.com/bitcoin-core/gui/pull/915)
Fixes [#30070](https://github.com/bitcoin/bitcoin/issues/30070)

When creating an unsigned PSBT from the GUI, the transaction was already signed during preparation, causing legacy inputs to have non-empty scriptSig fields. The PSBT parser then rejects them.

This defers signing until the user clicks "Send" instead of signing during preparation. Fee calculation still works since transactions can be created without signing.

Follows the approach suggested by @achow101 in the issue comments.

💬 maflcko commented on pull request "fuzz: doc: remove any mention to `address_deserialize_v2`":
(https://github.com/bitcoin/bitcoin/pull/34091#issuecomment-3666666963)
review ACK caf4843a59a9d2512d69f8fd88a9672112bd80ac 🎾

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

💬 fjahr commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#issuecomment-3666699027)
Concept ACK

💬 fjahr commented on pull request "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation":
(https://github.com/bitcoin/bitcoin/pull/33604#discussion_r2628258598)
nit: Could clarify the behavior in the comment here a bit more.

🤔 ryanofsky reviewed a pull request: "util: Add some more Unexpected and Expected methods"
(https://github.com/bitcoin/bitcoin/pull/34032#pullrequestreview-3589136730)
Code review fa874c9f050500dc521ef8d9c81b043687f0dcc7. Looks mostly good and thanks for considering the previous suggestions. One thing that I think may be a problem is use of throwing `value()` method in operator methods declared `noexcept` but I left a more detailed comment below.

Various changes were made since last review: renaming m_err, getting rid of Assume() in value(), testing void value() specialization, adding noexcepts, adding method `&` qualifiers, reformatting and using `std::get_i
...

💬 ryanofsky commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2628245089)
re: https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2609835030

> This just brings back the problem we are trying to solve

I don't think it does. The problem I want the void specialization to solve is leaking the std::monostate values to code that isn't otherwise using it and is expecting void, making it harder to use std::expected in places like template functions or generic lambdas. I don't see std::expected accepting (not returning) `std::monostate{}` as a comparable problem, o
...

💬 ryanofsky commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2628289525)
In commit "util: Implement Expected::operator*()&&" (fa9aad738a9bfd3969aa4e1f19f87edadc69f455)

Would be good for commit message to mention it is also adding noexcept to other overloads

💬 ryanofsky commented on pull request "util: Add some more Unexpected and Expected methods":
(https://github.com/bitcoin/bitcoin/pull/34032#discussion_r2628209646)
In commit "util: Make Expected::value() throw" (fa5a49c988c065526105f7d30c53298305f6e8c5)

The change seems unsafe because none of the other functions currently calling `value()` are switched to use `operator*` instead, so they will now throw instead of assert if the value is not set. This would make it dangerous to swap the `util::Expected` class with `std::expected` in the future.

I'd recommend reconsidering the suggestion from https://github.com/bitcoin/bitcoin/pull/34032#discussion_r260
...

💬 hebasto commented on pull request "net: Fix `-Wmissing-braces`":
(https://github.com/bitcoin/bitcoin/pull/34090#issuecomment-3666789241)
> https://github.com/bitcoin/bitcoin/actions/runs/20307057596/job/58327346295?pr=34090#step:11:495:

CI is green now.

👍 pinheadmz approved a pull request: "Broadcast own transactions only via short-lived Tor or I2P connections"
(https://github.com/bitcoin/bitcoin/pull/29415#pullrequestreview-3589291068)
ACK 89372213048adf37a47427112a1ff836ee84c50e

Minimal diff since last ACK, a few small logic changes and mostly just rebasing logging statements. Built and tested this commit on macos and also in Warnet with 100 nodes, all very cool!

I am indifferent on the [std::optional question](https://github.com/bitcoin/bitcoin/pull/29415#discussion_r2620864832). My instincts are to leave it as-is because it consumes minimal resources when deactivated, but there is a question about the burden on net_pr
...

👍 ryanofsky approved a pull request: "ci: detect outbound internet traffic generated while running tests"
(https://github.com/bitcoin/bitcoin/pull/31349#pullrequestreview-3589303104)
Code review ACK 47f4f65d0c8ba4680bab45b085939ace9624f3a2. Since last review just rebased to avoid conflicts, added comments to commit message and test, added `local` to some bash variables

🤔 janb84 reviewed a pull request: "kernel: Remove non-kernel module includes"
(https://github.com/bitcoin/bitcoin/pull/34079#pullrequestreview-3586973614)
Concept ACK 065639200505035428df01584ff43172c4b2dd90

Think the changes are correct. Have added some NITS** to remove some more unused includes, this to optimally use the file churn.

**Small disclaimer on those removals, have tried my best to research if they are really unused (code review, compile, test, IWYU, Guix build) but not sure if I have hit all the edges.

💬 janb84 commented on pull request "kernel: Remove non-kernel module includes":
(https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2626416456)
NIT: I think `#include <consensus/validation.h>` can also be removed, IWYU does not complain after removal and compiles + tests fine.

💬 janb84 commented on pull request "kernel: Remove non-kernel module includes":
(https://github.com/bitcoin/bitcoin/pull/34079#discussion_r2628339958)
NIT: Also ` #include <util/strencodings.h>` should be fine to remove compiles, tests etc. fine.

👍 ryanofsky approved a pull request: "refactor: Avoid copies by using const references or by move-construction"
(https://github.com/bitcoin/bitcoin/pull/31650#pullrequestreview-3589326320)
Code review ACK fa9b7f0630691cf6b0add88f646bbcae6466eeaa. Looks like this needs to be rebased again but latest version does look good, and this would be a nice change