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

💬 theuni commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2657416328)
Addressed @stickies-v's review.

Only change of interest is changing the name of the `Kernel` component to `bitcoinkernel` rather than `libbitcoinkernel` as it was before.

💬 theuni commented on pull request "depends: add missing Darwin objcopy":
(https://github.com/bitcoin/bitcoin/pull/31840#issuecomment-2657423789)
> > 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.

Ah right, I'd forgotten that bit. Blah.

Yeah, ok, never
...

💬 ryanofsky commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2657453951)
Rebased 47a872236e070814ad74922d3f8a653e1c6af968 -> d2ceb2e0735a2c8343f8316b55fac55323aba62c ([`pr/libexec.2`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.2) -> [`pr/libexec.3`](https://github.com/ryanofsky/bitcoin/commits/pr/libexec.3), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/libexec.2-rebase..pr/libexec.3)) due to conflict with #31834

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2657455048)
It seems Github never around to my last push. The new one was picked up immediately. It incorporates the two suggestions.