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

💬 mzumsande commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2628279820)
nit: **an** outbound

💬 mzumsande commented on pull request "test: address self-announcement":
(https://github.com/bitcoin/bitcoin/pull/34039#discussion_r2628375476)
note: I first thought that there might be a race condition (node receives verack first, and getaddr with some delay, sends out self-advertisment in between) but this isn't actually true because in this case we won't self-advertise due to #21528 (`SetupAddressRelay` not called yet) . So this is fine for now (whether that's good is another issue)

🤔 furszy reviewed a pull request: "qa: Avoid knock-on exception in assert_start_raises_init_error"
(https://github.com/bitcoin/bitcoin/pull/32929#pullrequestreview-3589512416)
Code ACK 356883f0e48be59bcb154096cef82cbf3f0dd9d8

💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3667066005)
@maflcko

Thank you for the review. The PR has been reworked per your feedback.

💬 hebasto commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#discussion_r2628562127)
> edit: The full patch would be:

[Taken](https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3667066005).

💬 jb55 commented on issue "sqlite legacy descriptor wallet migration fails":
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3667104351)
oh nice I instinctually tried that and ran into the checksum error, didn't realize I could replace the checksum. will try

💬 jb55 commented on issue "sqlite legacy descriptor wallet migration fails":
(https://github.com/bitcoin/bitcoin/issues/33468#issuecomment-3667153801)
that works! thanks boss.

✅ jb55 closed an issue: "sqlite legacy descriptor wallet migration fails"
(https://github.com/bitcoin/bitcoin/issues/33468)