👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1885543942)
Code review ACK d5228efb5391b31a9a0673019e43e7fa2cd4ac07. Just comment change since last review.

👍 hebasto approved a pull request: "[26.1] final changes for 26.1rc1"
(https://github.com/bitcoin/bitcoin/pull/29440#pullrequestreview-1885558510)
ACK 3d789657a8bf97089051348d472d7143a37ee988.

💬 hebasto commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#discussion_r1492723273)
nit: Why not the same `help2man` version?

💬 vasild commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492732536)
> this seems like an argument to always use shared_ptr or unique_ptr everywhere and never use plain pointers or references.

Raw pointers and references are ok to be used for arguments to functions. Or as automatic variables that obviously don't outlive the referenced object (on-stack local variables inside the body of a function).

> If you would not want a plain reference to be stored here, is there any case where you would want one to be stored?

If you store raw pointers or references
...

📝 remyers opened a pull request: "wallet: Target a pre-defined utxo set composition by adjusting change outputs"
(https://github.com/bitcoin/bitcoin/pull/29442)
This PR is designed for the use case of a Lightning node that provides liquidity of predefined amounts via [liquidity ads](https://github.com/lightning/bolts/pull/878).

Coin selection is currently optimized to reduce the size of the utxo set and create change optimized for privacy. A liquidity provider instead needs to service multiple liquidity requests by spending confirmed utxos of known sizes.

Ideally most liquidity transactions would be funded by a single input or a small set of input
...

💬 glozow commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#discussion_r1492742188)
I suppose that's because fanquake and I use different machines

👍 vasild approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1885604077)
ACK d5228efb5391b31a9a0673019e43e7fa2cd4ac07

I would be happy to re-review if this is changed to use automatic memory management everywhere (https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489457095 and https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1489527335). Or at least add a comment next to the raw pointers/references like "this points to NodeContext::validation_signals and should not be used after that is destroyed".

💬 instagibbs commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1492766771)
this is odd, should we just `Assume()` `replacement_vsize>0` in `CalculateFeerateDiagramsForRBF`?

💬 vasild commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#discussion_r1492773923)
Yes! See the first two commits in https://github.com/bitcoin/bitcoin/pull/26812:

bee6bdf0a5 `test: put the generic parts from StaticContentsSock into a separate class`
f42e4f3b3b `test: add a mocked Sock that allows inspecting what has been Send() to it`

and then how to use that in the last commit of the same PR:

8b10990aa0 `test: add unit tests exercising full call chain of CConnman and PeerManager`

With those it is possible to send/receive raw bytes to/from the (mocked) socket, or
...

💬 glozow commented on pull request "[26.1] final changes for 26.1rc1":
(https://github.com/bitcoin/bitcoin/pull/29440#issuecomment-1948954776)
Added backport of #29195

💬 theuni commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#issuecomment-1948959711)
It's not clear to me what the benefit of using IWYU is here. The missing defines will (by definition) vary per-platform, so I'm not sure what we're chasing here exactly.

Or is it more-so just to keep IWYU usable without warnings for our c-i configs specifically?

💬 theuni commented on pull request "refactor: bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1948961800)
PR title an description updated.

💬 Sjors commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#issuecomment-1948964294)
Concept ACK. The ability to (better) mock a socket would be very helpful in my Stratum v2 Template Provider work #29432. I found out the hard way that using a real socket makes the test rather brittle.

💬 ryanofsky commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492799393)
> With a few shared_ptrs you simply don't care and the code is safe from "use after free" bugs. That's automatic lifetime management.

Of course, thanks for clarifying. C++ is a very flexible language and you can certainly use it in the style you are suggesting where everything is basically garbage collected, and use after free bugs never happen. There are benefits to doing this, but also drawbacks. IMO, the main drawbacks of using shared_ptr are (1) that it can make lifetime of objects unpred
...

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492836170)
I think it's fine as `LegacyScriptPubKeyMan` is designed to be able to handle unknown scripts gracefully. But I've added a `CanProvide` check just for consistency.

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492836266)
Done as suggested

🤔 furszy reviewed a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1885740077)
diff ACK d5228ef

💬 furszy commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1492837763)
> A good place to move it would be to the ValidationSignals constructor
I would rather place it above the imp class declaration, but just a tiny styling nit.

👍 BrandonOdiwuor approved a pull request: "ci: Avoid CI failures from temp env file reuse"
(https://github.com/bitcoin/bitcoin/pull/29441#pullrequestreview-1885770449)
ACK fa91bf2559d2e839592bf1dc1d423d5fb1c3573e

Looks good to me

👋 Sjors's pull request is ready for review: "Stratum v2 Noise Protocol"
(https://github.com/bitcoin/bitcoin/pull/29346)