💬 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

💬 mzumsande commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2628242525)
`assert_on_connection_open` seems like a redundant arg, could call one or the other based on the existing `outbound` arg.

💬 mzumsande commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2628081222)
I would suggest to rework or remove this before merge, first person language and `print()` don't seem appropriate at that stage anymore, the mismatch print could be an assert if it's not expected to ever happen.