👍 ryanofsky approved a pull request: "move-only: Extract common/args from util/system"
(https://github.com/bitcoin/bitcoin/pull/27419#pullrequestreview-1392362907)
Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.

Confirmed move-only except for making InterpretValue function nonstatic, and moving some symbols from util to common namespace.

💬 ryanofsky commented on pull request "move-only: Extract common/args from util/system":
(https://github.com/bitcoin/bitcoin/pull/27419#discussion_r1171524879)
In commit "move-only: Extract common/args and common/config.cpp from util/system" (be55f545d53d44fdcf2d8ae802e9eae551d120c6)

Not important, but there is still some extra whitespace on this line

💬 MarcoFalke commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171530477)
And given that the CI fails here, maybe add it to the msan task in a follow-up and only set it on the fuzz msan task?

🤔 pablomartin4btc reviewed a pull request: "blockstorage: do not flush block to disk if it is already there"
(https://github.com/bitcoin/bitcoin/pull/27039#pullrequestreview-1392374264)
Concept ACK. (went thru notes and other related PRs mentioned on the [Review club meeting preparation webpage](https://bitcoincore.reviews/27039) - Please add label when possiblel)

💬 fanquake commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#discussion_r1171548431)
> Forgot to set this for the fuzz msan task?

I didn't bother as that isn't run here, and wouldn't compile rn (master) in any case.

> maybe add it to the msan task in a follow-up and only set it on the fuzz msan task?

I think I'd rather defer to adding both, at the same time, in a follow up, if you're alright with that. Can drop the test commit from this PR.

🤔 darosior reviewed a pull request: "wallet: cache IsMine scriptPubKeys to improve performance of descriptor wallets"
(https://github.com/bitcoin/bitcoin/pull/26008#pullrequestreview-1392406641)
I really like the performance gains here, however i feel like this is making the spks management bleed into the wallet from the spkman. But there is no other way for the wallet to efficiently locate the spkman(s)..

I was looking into how to get rid of the callbacks and avoid duplicating the scriptpubkeys, and thought we might as well share a single mapping owned by `CWallet` across all spkmans. This would also have the nice property that we don't need to make sure moving forward we don't intr
...

💬 MarcoFalke commented on pull request "ci: build libc++ in DEBUG mode in MSAN jobs":
(https://github.com/bitcoin/bitcoin/pull/27448#issuecomment-1514991825)
lgtm ACK 4de9c2a65f6770405f167c7392cd8371111bc4e8

🤔 jamesob reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1392348713)
Thanks to all the dedicated reviewers and my apologies for the delay. Lot of good feedback addressed.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171527318)
Great catch, done.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171531164)
Fixed.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171554198)
Fixed.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171516957)
Done.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171514903)
Yep, that's a good point. Will do, thanks!

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171552243)
Reworded.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171549876)
Good point, added.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171551677)
Good point, done.

💬 jamesob commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1171552108)
Thanks, done.

💬 jonatack commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#discussion_r1171560928)
> Seems like a lot of these includes are unnecessary and can be removed... With the following diff it still compiles

A way to verify is to add the file to `ci/test/06_script_b.sh` and look at the `lint` CI output after repushing.

💬 jonatack commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#discussion_r1171566094)
>> the tests added in this PR can be done without ... touching the source code at all

> Yes, that works. Here it is:

Nice!

Virtualization/dynamic dispatch has performance and overhead costs in addition to increased class/code size and complexity, and there are places in this codebase where it seems to have been avoided, and still is, for that reason. For an example, see the discussion in #25349.

If virtualization is still considered to be worth the overhead here, on its own merits ab
...

💬 jonatack commented on pull request "net, test: Virtualise CConnman and add initial PeerManager unit tests":
(https://github.com/bitcoin/bitcoin/pull/25515#issuecomment-1515003669)
Concept ACK on adding testing modulo the comments above and https://github.com/bitcoin/bitcoin/pull/25515#discussion_r1171566094. Built and ran a green make check on each commit. Needs rebase.