👍 stickies-v approved a pull request: "cmake: add a component for each binary"
(https://github.com/bitcoin/bitcoin/pull/31844#pullrequestreview-2615433035)
tACK 686ebcfe93efa9ecca9b94e78da23a36c3b01f90

Being able to specify install components for all targets is helpful, and this implementation seems very clean. Part of me annoyed is that the consistency isn't there for kernel (`bitcoinkernel` target and `libbitcoinkernel` component), but it seems like that's conventional for libraries so I'll suck it up.

💬 stickies-v commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#discussion_r1954741365)
I think this is now duplicated and can be removed?

🤔 danielabrozzoni reviewed a pull request: "test: deduplicates p2p_tx_download constants"
(https://github.com/bitcoin/bitcoin/pull/31758#pullrequestreview-2615727315)
Whoops, I had forgotten about this one :)

re-ACK 0a02e7fdeaca26455b3710ef76b11101ac816e53

💬 hodlinator commented on pull request "test: Remove stale gettime test":
(https://github.com/bitcoin/bitcoin/pull/31846#issuecomment-2657262496)
Would probably have leaned towards just updating the comment and leaving the test just out of paranoia, even though so much is different with the `std::chrono` implementation that anything similar is unlikely to re-occur. Getting rid of lines to maintain has value too.

💬 theuni commented on pull request "depends: add missing Darwin objcopy":
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2657264201)
@hebasto Agreed as long as it's trivial. From what I can tell `llvm-install-name-tool` is just as available as the rest of the tools, so even though it's annoying because it's not actually necessary, I don't see much harm in setting it. @fanquake ?

This required a similar hack for https://github.com/bitcoin/bitcoin/pull/30434 as well.

💬 brunoerg commented on issue "wallet: Branch and Bound producing change":
(https://github.com/bitcoin/bitcoin/issues/31830#issuecomment-2657278221)
> q: isn't the test just wrong?. `min_viable_change` is initialized to the future input fee without increasing the window by one as we do in the actual code. Thats why there is an overlap here. See the wallet code:
>
> [bitcoin/src/wallet/spend.cpp](https://github.com/bitcoin/bitcoin/blob/c242fa5be358150d83c2446896b6f4c45c6365e9/src/wallet/spend.cpp#L1131-L1132)
>
> Lines 1131 to 1132 in [c242fa5](/bitcoin/bitcoin/commit/c242fa5be358150d83c2446896b6f4c45c6365e9)
>
> const auto change_spend_f
...

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2657285774)
Rebased c6658d675d0bb945f53dc62f7e9b95c7bba973bb -> b2108240dec5d858d17af798fdc79037f894786d ([`pr/subtree.15`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.15) -> [`pr/subtree.16`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.16), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.15-rebase..pr/subtree.16)) to fix conflict with #31834. Also added another fix for #30975 and documented & unhid CAPNP_EXECUTABLE variable as suggested

re: https://github.com/bi
...

🚀 glozow merged a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495)

💬 ryanofsky commented on pull request "ci: build multiprocess on most jobs":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2657295215)
@Sjors Note if you rebase this on #31741 you should be able to drop ab831be2a93d9c30408192ca4eaa1552ac6bc3dc and 19f693e6b81e5e0e75b820be5ba7a53911b4918c workarounds since a more direct fix for the fuzz build issues was added (96d3b2a0bb0c267569162b055ca306f709ec0b4e). Also of course can drop be8abf9c55c5279ca36cd080626b8071733ac6c1 since #31834 was merged

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657299519)
@stickies-v Thanks for pointing that out, that was accidental! I actually didn't notice the difference. I think we should fix it so that they're aligned instead of having a single outlier.

Though.. would it make sense to rename the target to `libbitcoinkernel` instead of changing the component to `bitcoinkernel`? I think the former would be more obvious?

💬 brunoerg commented on pull request "test: check `scanning` field from `getwalletinfo`":
(https://github.com/bitcoin/bitcoin/pull/31768#discussion_r1954941746)
Good suggestion, but I will leave as is for now to not invalidate the reviews. Thanks.

💬 theStack commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954923344)
nit:
```suggestion
LogPrintf("%s\n", STR_INTERNAL_BUG("Error: Some output scripts were not migrated."));
```
(the expansion in `StrFormatInternalBug` already includes a new-line after the message)

🤔 theStack reviewed a pull request: "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases"
(https://github.com/bitcoin/bitcoin/pull/31495#pullrequestreview-2615748798)
post-merge code-review ACK af76664b12d8611b606a7e755a103a20542ee539

nit for functional test commit c39b3cfcd1bc5002aa06d1b79c948ce94f3ec8dc: there are still instances of `self.migrate_and_get_rpc` calls with is-descriptor/sqlite-checks after that can now be removed (e.g. in the `test_no_privkeys` subtest).

💬 theStack commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1954942761)
nit: rather than calculating the candidate scriptPubKey set a second time, could create a non-modifiable copy the first time above at ```std::unordered_set<CScript, SaltedSipHasher> spks{GetScriptPubKeys()};``` (probably only relevant for huge wallets, if it's noticable at all)

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#issuecomment-2657314884)
Rebased eb325bc0efd3f999189e155ba836be92e6cc9af7 -> a85e8c0e6158fad2408bda5cb1e36da707eb081b ([`pr/listset.9`](https://github.com/ryanofsky/bitcoin/commits/pr/listset.9) -> [`pr/listset.10`](https://github.com/ryanofsky/bitcoin/commits/pr/listset.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/listset.9-rebase..pr/listset.10)) due to conflict with #31767

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#discussion_r1954948470)
Hmm, IIUC it's always been superfluous as it should've been needed for the .pc install above, so it must've always coming in from the top level anyway.

Will remove.

💬 davidgumberg commented on pull request "build: Enhance Ccache performance across worktrees and build trees":
(https://github.com/bitcoin/bitcoin/pull/30861#discussion_r1954948701)
Is making this a `configure_warning` compatible with #31665? Maybe it would be a benefit if CI complained when unintentionally skipping ccache.

✅ brunoerg closed an issue: "wallet: Branch and Bound producing change"
(https://github.com/bitcoin/bitcoin/issues/31830)

💬 theuni commented on issue "build: RFC Coverage build type":
(https://github.com/bitcoin/bitcoin/issues/31047#issuecomment-2657324634)
Will do.

🤔 danielabrozzoni reviewed a pull request: "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior"
(https://github.com/bitcoin/bitcoin/pull/30529#pullrequestreview-2615816888)
re-ACK a85e8c0e6158fad2408bda5cb1e36da707eb081b