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

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1949144664)
With https://github.com/stratum-mining/stratum/pull/737, https://github.com/stratum-mining/stratum/pull/752, https://github.com/stratum-mining/stratum/pull/729 and https://github.com/stratum-mining/stratum/pull/722 merged this PR is now compatible with SRI's `main` branch. This should make testing a lot easier.

🤔 furszy reviewed a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1885842494)
Reviewed. Need to update the second commit description.

💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492891474)
In 65c7997221:

This is not needed. The benchmark uses a db mock.

💬 furszy commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492899741)
In bf898af0190d:

It was removed on the last push: `LoadDescriptorScriptPubKeyMan` now returns the spkm but no function utilizes it.

💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1949200230)
Possibly relevant: the test introduced here, and not modified later, failed at least once in the parent PR: https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-1948481906

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

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

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#discussion_r1492911131)
Oops, somehow that usage got dropped. Fixed.

💬 achow101 commented on pull request "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets":
(https://github.com/bitcoin/bitcoin/pull/26008#issuecomment-1949209816)
> Need to update the second commit description.

Done

💬 ismaelsadeeq 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_r1492915210)
Yes, I can't think of a scenario where that will happen!