📝 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

💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3666835355)
> Is the local failure you observe reproducible or sporadic?

Maybe 50%, see my previous comment: https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3410342332.

I guess this makes bisect a bit harder, but it should be possible by running 10 times, or so.

🤔 janb84 reviewed a pull request: "[30.x] Finalise v30.1"
(https://github.com/bitcoin/bitcoin/pull/34092#pullrequestreview-3589334408)
ACK 2a21824b1190968ee8ba3120400c00e56be10591

Did a code review and dev-mode compile all oke.

💬 ryanofsky commented on pull request "A few followups after introducing `/rest/blockpart/` endpoint":
(https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2628395741)
re: https://github.com/bitcoin/bitcoin/pull/34074#discussion_r2626269231

> Regarding including the `reason`, it seems fairly hard coded: "Bad Request", "OK", etc. So only useful to avoid having to know/look up the HTTP status codes.

Haven't tested this but it would be surprising if true. There are lots of specific reasons returned in the src/rest.cpp file like specific block hashes not found, undo data missing, output format not supported, pruned data errors, etc

🤔 w0xlt reviewed a pull request: "[30.x] Finalise v30.1"
(https://github.com/bitcoin/bitcoin/pull/34092#pullrequestreview-3589401465)
Code Review ACK https://github.com/bitcoin/bitcoin/pull/34092/commits/2a21824b1190968ee8ba3120400c00e56be10591

💬 l0rinc commented on pull request "qa: Avoid knock-on exception in assert_start_raises_init_error":
(https://github.com/bitcoin/bitcoin/pull/32929#issuecomment-3666916582)
Only reviewed the diff [since my last ack](https://github.com/bitcoin/bitcoin/compare/6b0eac750f3aecb29446d70c039559143e43a011..356883f0e48be59bcb154096cef82cbf3f0dd9d8) which only contains the rename and a description update - and some commit structure and message changes which I was already fine with before.

> Might also suggest renaming PR to "qa: Make assert_start_raises_init_error output more readable"

The PR has a PR problem

ACK 356883f0e48be59bcb154096cef82cbf3f0dd9d8

🤔 mzumsande reviewed a pull request: "test: address self-announcement"
(https://github.com/bitcoin/bitcoin/pull/34039#pullrequestreview-3588983004)
Concept ACK