💬 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)

💬 stickies-v commented on issue "RFC: separate kernel logging infrastructure":
(https://github.com/bitcoin/bitcoin/issues/34062#issuecomment-3667169341)
I have implemented a new approach ("Layering") in https://github.com/bitcoin/bitcoin/compare/master...stickies-v:bitcoin:2025-12/kernel-logging-layering. The Layering approach turned out much less invasive than my initial attempts showed, and now has my preference.

In a nutshell:
- we distinguish between a (low-level) logger that produces log statements (`util::log::Logger`) and (higher-level) log sinks (`BCLog::Sink`, `btck_LoggingConnection`) that consume the logs by registering a callback on
...

👍 ryanofsky approved a pull request: "refactor: unify container presence checks"
(https://github.com/bitcoin/bitcoin/pull/33192#pullrequestreview-3589700908)
Code review ACK d9319b06cf82664d55f255387a348135fd7f91c7. I manually reviewed the full change, and it seems there are a lot of positive comments about this and no more very significant conflicts, so I will merge it shortly.

I am wondering if there are plans to turn on the clang-tidy check and follow up with other replacements?