💬 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

💬 ryanofsky commented on pull request "Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior":
(https://github.com/bitcoin/bitcoin/pull/30529#discussion_r1954970365)
> I agree when it comes to `assert()`s in C/C++, guess I haven't fully transferred that to Python yet, but this discussion helped. What do you think of doing a scripted diff renaming our `assert_*()`-functions and their usage to `check_*()` to signal that they won't be optimized out?

IMO a renaming would not be a good idea just because of all the conflicts it would create. Also I think it overlapping names for assert functions and the assert statement are less of a problem in python than they
...

💬 fanquake commented on pull request "depends: add missing Darwin objcopy":
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2657346395)
> even though it's annoying because it's not actually necessary, I don't see much harm in setting it. @fanquake ?

I'm just not sure of us having to setup/provide tools because CMake thinks they should be available, when we don't use them. iirc one of the issues with `install-name-tool` was that it was only available version-suffixed on certain distros (even when the main llvm install was not suffixed), making it hard to find.

> This required a similar hack for https://github.com/bitcoin/b
...

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#discussion_r1955002067)
Heh, testing shows `GNUInstallDirs` is actually coming in from secp and we don't need it at all in our project. That's pretty gross of CMake...

But still, I'll remove this one and leave the other one there for self-documentation.

💬 ryanofsky commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1955022152)
re: https://github.com/bitcoin/bitcoin/pull/31283#discussion_r1954786986

In commit "Add waitNext() to BlockTemplate interface" (86449088b703d2110edaacbd6d1f1386916a3773)

> I incorporated part of this comment. Note that some callers may just not be interested in fee changes, so I mentioned that as well.

Thanks, I do have two more suggestions:

- In "A caller may not be interested in templates with higher fees, but determining whether fee_threshold is reached is also expensive," can you
...

👍 ryanofsky approved a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283#pullrequestreview-2615870327)
Code review ACK 16b18f845313f25bbcdea122baa753b570add7d0. Just various suggested changes since last review.

> (there seems to be a delay in Github processing my latest push)

Can confirm I see "Processing Updates" "Recent push is still being processed, and will show up in a bit" at the top of this PR.