💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473041954)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)

It seems like there are a number of things here making this code more rigid and complicated than it needs to be which aren't explained. It seems like it would be more straightforward to:

- Make `BlockTemplateCache::GetBlockTemplate` method non-virtual
- Drop `MakeBlockTemplateCache` function, and just give `GetBlockTemplate` class a constructor.
- Drop `BlockTemplateCacheImpl` class and just im
...

💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473250518)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)

`Template` name doesn't seem very descriptive. I'd consider calling it `CachedBlockTemplate` to be unambiguous, and also making it a top level struct instead of an inner struct so it is less entwined with the `BlockTemplateCacheImpl` class, and could be forward declared, accessed from tests, etc.

💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473275740)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)

This doesn't seem to be right because it is forward declaring a global `::BlockTemplateCache` class not in the `::node` namespace. Probably this should just be dropped.

💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2472926339)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)

Not very important, but would be good to call UnregisterValidationInterface in Shutdown to unregister this. Otherwise, in theory if Init/Shutdown were called multiple times (e.g. in unit tests) there would be a memory leak. `node.block_template_cache` can be handled the same way as `node.fee_estimator` in `Shutdown`.

💬 ryanofsky commented on pull request "node: add `BlockTemplateCache`":
(https://github.com/bitcoin/bitcoin/pull/33421#discussion_r2473282308)
In commit "node: add block template cache to miner" (4de7887ca47197654f480471db04750d44e405de)

`DEFAULT_CACHE_SIZE` is a very generic name for a global, maybe `DEFAULT_BLOCK_TEMPLATE_CACHE_SIZE` would be better.

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#discussion_r2474162538)
Fixed the no-op issue where `getblockstats` was called twice and compared against itself. Now the RPC method is called once, and the test validates consistency between different query methods (hash vs height, single vs multiple stats) rather than checking if the method returns identical results to itself.

Also reverted changes that introduced the untested path problem.

💬 fanquake commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2474188422)
> include-what-you-use/boost-1.75-all.imp

I guess this is still correct-enough to use here, but are we making any effort to usptream updates, given it's 14 Boost releases out of date (current release is 1.89.0)? Seems possible that this could lead to incorrect IWYU suggestions, given any recent Boost refactorings?

💬 umrashrf commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3462594921)
@maflcko ran configure again and here's the output https://gist.github.com/umrashrf/6a63476cce762e00a4f45603837c5bda

I just opened XCode and it asked me to install it. Still fails to build after installation.

New logs: https://gist.github.com/umrashrf/0714372957165e0264c8854d0060f660

XCode version: 26.0.1 (17A400)

💬 umrashrf commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3462647625)
Boost libs https://gist.github.com/umrashrf/15bfff6425025902a7f310d867929f14

🚀 fanquake merged a pull request: "build: Bump clang minimum supported version to 17"
(https://github.com/bitcoin/bitcoin/pull/33555)

💬 ajtowns commented on issue "Batch tx broadcast RPC":
(https://github.com/bitcoin/bitcoin/issues/33700#issuecomment-3462702682)
I'd contend that if you could make a p2p connection, announce `INV tx1 tx2 ...` and reply to `GETDATA` requests (or BIP331 messages or whatever we add in future), and get your txs added to the node's mempool and relayed onwards, then you should be able to get the same result with a simple RPC call.

For errors, I think it would be preferable to have a parameter that decides whether the behaviour should be "only accept anything if you can accept everything" or "accept as much as you can and tell
...

💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2474293335)
Fixed

💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#discussion_r2474293676)
Fixed

💬 enirox001 commented on pull request "test: Replace legacy wallet with MiniWallet in rpc_getblockstats.py":
(https://github.com/bitcoin/bitcoin/pull/33184#issuecomment-3462730169)
In commit ac7f694

Fixed the no-op issue raised by @maflcko and @Eunovo. The test was calling getblockstats twice and comparing results against itself. Now it calls getblockstats once and validates consistency

💬 ismaelsadeeq commented on pull request "fees: enable `CBlockPolicyEstimator` return sub 1 sat/vb fee rate estimates":
(https://github.com/bitcoin/bitcoin/pull/33199#issuecomment-3462730845)
Rebased and fixed nits.

💬 fanquake commented on pull request "fuzz: don't pass (maybe) nullptr to memcpy()":
(https://github.com/bitcoin/bitcoin/pull/33644#discussion_r2474301667)
@vasild did you want to follow up here?

💬 furszy commented on pull request "rpc: add "ischange: true" to decoded tx outputs in wallet gettransaction response":
(https://github.com/bitcoin/bitcoin/pull/32517#issuecomment-3462765091)
ACK [060bb55](https://github.com/bitcoin/bitcoin/pull/32517/commits/060bb55508245776bb6a39c8b7849769ee588d69)

💬 hebasto commented on issue "Can I compile on OSX Tahoe?":
(https://github.com/bitcoin/bitcoin/issues/33733#issuecomment-3462777867)
@umrashrf

Could you please post the output of the following command:
```
rm -rf build && cmake -B build -DENABLE_IPC=OFF -DBUILD_GUI=OFF -DWITH_QRENCODE=OFF -LA 2>&1 | grep -i boost
```
?

💬 ryanofsky commented on pull request "init: Split file path handling out of -asmap option":
(https://github.com/bitcoin/bitcoin/pull/33631#issuecomment-3462799469)
> If reading a file by default or embedding it into the binary gets rid of that, that's fine with me too. But i guess even in that case, one might want to either disable the functionality, or override the path?

Yes, these things are done with `-noasmap` and `-asmap=<filename>` and covered in existing `feature_asmap.py` tests.

> It seems partly orthogonal to these options.

I do think the issues are pretty directly related, not orthogonal. The basic problem here is that this option's defa
...

💬 hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#issuecomment-3462818493)
> > this pull request conflicts with the following ones:
> > #33629 (Cluster mempool by sdaftuar)
> > #33591 (Cluster mempool followups by sdaftuar)
>
> I think if you want to continue these refactors, it might be better if you picked code that doesn't conflict with Cluster Mempool or Kernel work, or things we are trying to ship in `v31`.

I agree, and when I noticed these conflicts, I checked that those PRs don't have any ACKs yet.

> Could instead do directories that rarely change, li
...