👍 ryanofsky approved a pull request: "node: add `BlockTemplateCache`"
(https://github.com/bitcoin/bitcoin/pull/33421#pullrequestreview-3411154210)
Code review ACK 084bfbc1ec7f8f64f54d231bb641285622311b59 reviewing first commit in detail, but reviewed fuzz test in second commit pretty lightly. Changes here all seem very straightforward and implmenetation seems simpler than it was initially.

I do still suspect specifying template age in seconds instead of using a more granular measure may be a mistake if this caching is supposed to be useful for mining, but others would know more about this than me. It would seem straightfoward to add anoth
...

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

I know I suggested `operator==` previously but since this is not checking direct equality maybe it is better to call this method something like `CanReuse` or `ReusableTemplate`. Just an idea. Keeping operator== or using any method name seems ok.

Would also suggest moving detailed comments about specific fields inisde the method implementation. Could just have a general comment like "intentionally
...

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

Probably should use `uint32_t` instead of `const uint32_t&`. Const reference parameter are usually only preferred when parameter types are much bigger.

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

Should mempool be a reference rather if this class can't really function when it is null?

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

Maybe use `auto block_copy = std::make_shared<const CBlock>(m_block_template->block);` here to avoid needing to make a shared_ptr and a second copy of the block below.

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

Maybe s/maximum_/max_/

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486699478)
It doesn't make sense to check the skip for block index 0, that's why 0 is excluded.

👍 hebasto approved a pull request: "depends: disable variables, rules and suffixes."
(https://github.com/bitcoin/bitcoin/pull/33045#pullrequestreview-3411327796)
ACK 3651344fedfd75ea1fd178e53de2607a3bdfb558.

> I was going to suggest:
>
> * Using `--no-builtin-rules` rather than `-r` to be a little more self-documenting
>
> ...
>
> But...
>
> BSD Make supports `-r`, so that pretty much moots both points. Also, I'm pretty sure we require GNU Make anyway.

We do [require](https://github.com/bitcoin/bitcoin/blob/master/depends/README.md) GNU Make, so I also support using the long option names for better self-documentation.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486708184)
If it's inlined, it can't be used as extern from the test :(

💬 hebasto commented on issue "depends: Evaluate potential improvements":
(https://github.com/bitcoin/bitcoin/issues/18821#issuecomment-3480893699)
> 2. Eliminating builtin rules and variables e.g. `MAKEFLAGS += --no-builtin-variables`: I've tried this, and it breaks libminiupnpc because it relies on builtin rules.

See https://github.com/bitcoin/bitcoin/pull/33045.

👍 hebasto approved a pull request: "ci: Fix lint runner selection (and docker cache)"
(https://github.com/bitcoin/bitcoin/pull/33744#pullrequestreview-3411360968)
ACK 0b3b8a3be1a0db0dfc634acca1d9305dc0fbfae6, I have reviewed the code and it looks OK.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486733227)
I check the avg, which is computationally easier to obtain. It would make sense to check the median as well, and maybe compare the median with the average. However, I prefer to avoid the O(N log N) complexity of the sorting.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486736452)
Yes, nontheless I reduce the count from 30000 to 10000, that's also enough for the test.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486781519)
Removed

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486783350)
I've adjusted the numbers used and added comments

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486784728)
Changed to `BOOST_CHECK_EQUAL`, added comments

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#discussion_r2486787928)
Done

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3480999335)
Review comments addressed, mostly minor:

- Reduce the number of iterations to 10000 (from 30000), for faster test execution.
- Add comments on choice of test inputs.
- Use BOOST_REQUIRE_EQUAL or BOOST_CHECK_GT where possible.

💬 optout21 commented on pull request "test: Add test on skip heights in CBlockIndex":
(https://github.com/bitcoin/bitcoin/pull/33661#issuecomment-3481005723)
@sipa, as this is triggered by #33515 of yours, I would kindly ask for a review.

💬 willcl-ark commented on pull request "ci: Fix lint runner selection (and docker cache)":
(https://github.com/bitcoin/bitcoin/pull/33744#issuecomment-3481075031)
Sorry for the slow turnaround.

I pushed an additional commit to address (half of) @maflcko's nits. He is correct that GHA apparently does not validate inputs (even if you set `type: choice`), so it makes sense to remove the `options`. In order that we not miss if the wrong option is passed here in the future I added a sanity check which will annotate the CI run with a `Warning` when an unknown input is detected. See the bottom of the annotations of this run for what that looks like: https://g
...