🤔 fjahr reviewed a pull request: "test: Avoid intermittent error in assert_equal(pruneheight_new, 248)"
(https://github.com/bitcoin/bitcoin/pull/31468#pullrequestreview-2525753584)
Code review ACK fa0998f0a028161fe7264d0e81af36ffdcb43384

I think adding detail to the comment would be good but it's not a blocker.

💬 fjahr commented on pull request "test: descriptor: fix test for `MaxSatisfactionWeight`":
(https://github.com/bitcoin/bitcoin/pull/31570#issuecomment-2565679863)
utACK b29d68f942e333d2cfdd6be8d49fe484e0d15e11

🤔 jonatack reviewed a pull request: "test: Remove non-portable IPv6 test"
(https://github.com/bitcoin/bitcoin/pull/31580#pullrequestreview-2525772119)
Is there a way to skip the test for Illumos-based platforms?

📝 Sjors opened a pull request: "test: have miner_tests use Mining interface and fewer cs_main locks"
(https://github.com/bitcoin/bitcoin/pull/31581)
Needed for both #31283 and #31564.

By using the Mining interface in `miner_tests.cpp` we increase its coverage in unit tests.

We mainly call the `createNewBlock()` method, but also `getTip()->height` and `submitSolution()`. Calls to the latter are alternated with direct calls to `Chainman`'s `ProcessNewBlock`, because e.g. `net_processing.cpp` uses that.

#31564 proposes an alternative to `TestBlockValidity` which requires that `cs_main` is _not_ held. Prepare for this by not holding `cs
...

💬 Sjors commented on pull request "Add waitNext() to BlockTemplate interface":
(https://github.com/bitcoin/bitcoin/pull/31283#issuecomment-2565696340)
I extracted the first commit into its own PR #31581. Review on the second commit is still welcome here, since I don't expect it to change. Marking draft.

📝 Sjors converted_to_draft a pull request: "Add waitNext() to BlockTemplate interface"
(https://github.com/bitcoin/bitcoin/pull/31283)
Alternative approach to #31003, suggested in https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2451942362.

This PR introduces `waitNext()`. It waits for either the tip to update or for fees at the top of the mempool to rise sufficiently. It then returns a new template, with which the caller can rinse and repeat.

💬 instagibbs commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#issuecomment-2565706580)
> is there any reason you went for a multiplier?

nothing off the top of my head

💬 Sjors commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#issuecomment-2565707144)
I refactored `checkblock` to take a `target` argument rather than a multiplier. In order to make this easier to implement and test, I also introduced a `gettarget` RPC and `DeriveTarget()` helper.

I then dropped the original `TestBlockValidity` and renamed `CheckNewBlock` to it. The `generateblock` and `getblocktemplate` RPC calls, as well
as `BlockAssembler::CreateNewBlock` use the new method.

If people prefer I could rewrite the git history to directly refactor `TestBlockValidity`, thou
...

💬 Sjors commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#discussion_r1899676636)
This also applies to the new `DeriveTarget` method. I added the motivation for not changing `CheckProofOfWorkImpl` to the commit message in: https://github.com/bitcoin/bitcoin/pull/31564/commits/d3710b0ffe416337cd2c21ba871348b0e7b4aa9e

💬 fjahr commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-2565723488)
Concept ACK

The changes in the tests look good but I need a little more time with the CI stuff. Maybe the PR could have been split there to get the critical fixes in faster but I guess it's a bit late now.

💬 hebasto commented on pull request "test: Remove non-portable IPv6 test":
(https://github.com/bitcoin/bitcoin/pull/31580#issuecomment-2565734613)
> Is there a way to skip the test for Illumos-based platforms?

I've verified both OpenIndiana and OmniOS. The `__illumos__` macro is defined to `1` in both.

💬 achow101 commented on pull request "descriptor: Add proper Clone function to miniscript::Node":
(https://github.com/bitcoin/bitcoin/pull/30866#issuecomment-2565759517)
> Looks like you didn't.

Oops, pushed it.

📝 achow101 opened a pull request: "[28.x] Finalize 28.1"
(https://github.com/bitcoin/bitcoin/pull/31582)

💬 achow101 commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#issuecomment-2565774931)
ACK b9766c9977e58a9ebc358d9879576376e76a72b1

💬 achow101 commented on pull request "Remove unused variable assignment":
(https://github.com/bitcoin/bitcoin/pull/31497#issuecomment-2565775604)
ACK b9766c9977e58a9ebc358d9879576376e76a72b1

💬 achow101 commented on pull request "rpc: Add signet_challenge field to getblockchaininfo and getmininginfo":
(https://github.com/bitcoin/bitcoin/pull/31531#issuecomment-2565782475)
ACK ecaa786cc103cf7cc63ae899ec13d81a54e2fd1e

📝 Sjors converted_to_draft a pull request: "Add checkblock RPC and checkBlock() to Mining interface"
(https://github.com/bitcoin/bitcoin/pull/31564)
## Rationale

### Verifying block templates (no PoW)

Stratum v2 and DATUM allow miners to generate their own block template. Pools may wish (or need) to verify these templates. This typically involves comparing mempools, asking miners to providing missing transactions and then reconstructing the proposed block.[^0] This is not sufficient to ensure a proposed block is actually valid. In some schemes miners could take advantage of incomplete validation[^1].

The Stratum Reference Implementa
...

💬 achow101 commented on pull request "refactor: std::span compat fixes":
(https://github.com/bitcoin/bitcoin/pull/31540#issuecomment-2565786650)
ACK fa494a1d53f3f030fafe7b533d72b2200428a0fd

🚀 achow101 merged a pull request: "Remove unused variable assignment"
(https://github.com/bitcoin/bitcoin/pull/31497)

💬 Sjors commented on pull request "Add checkblock RPC and checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31564#issuecomment-2565793245)
I might make a separate PR to consider whether to drop `AssertLockHeld(cs_main)` from the original `TestBlockValidity`.

If that ends up rejected, then I'll keep the existing `TestBlockValidity` calls as they are, i.e. drop the last two commits from this PR and leave them as a followup.