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

💬 stickies-v commented on pull request "log: reduce excessive "rolling back/forward" messages during block replay":
(https://github.com/bitcoin/bitcoin/pull/33443#issuecomment-3481078520)
Approach ACK 1fc7a81f1f5f30ba3ebe305ac2a520c7b4afb596, except for progress indication I don't see the need to unconditionally log every roll{back,forward} operation, so I strongly prefer logging it every n iterations over adding another non-ratelimited unconditional site.

💬 fanquake commented on pull request "prevector: simplify implementation of comparison operators and match behavior of `std::vector`":
(https://github.com/bitcoin/bitcoin/pull/33772#issuecomment-3481100244)
As indicated by the CI, the macOS SDK we are targeting doesn't (yet) support `lexicographical_compare_three_way`. A Guix build will fail the same way:
```bash
In file included from /distsrc-base/distsrc-21318e8b5df5-arm64-apple-darwin/src/chainparams.cpp:6:
In file included from /distsrc-base/distsrc-21318e8b5df5-arm64-apple-darwin/src/chainparams.h:9:
In file included from /distsrc-base/distsrc-21318e8b5df5-arm64-apple-darwin/src/kernel/chainparams.h:11:
In file included from /distsrc-base
...

💬 fanquake commented on pull request "depends: disable variables, rules and suffixes.":
(https://github.com/bitcoin/bitcoin/pull/33045#issuecomment-3481126835)
Pushed up the long form for both options.