💬 fjahr commented on pull request "wallet: Be able to receive and spend inputs involving MuSig2 aggregate keys":
(https://github.com/bitcoin/bitcoin/pull/29675#issuecomment-2565454379)
Concept ACK

🤔 furszy reviewed a pull request: "test: descriptor: fix test for `MaxSatisfactionWeight`"
(https://github.com/bitcoin/bitcoin/pull/31570#pullrequestreview-2525657833)
utACK b29d68f942e

💬 furszy commented on pull request "test: descriptor: fix test for `MaxSatisfactionWeight`":
(https://github.com/bitcoin/bitcoin/pull/31570#discussion_r1899587183)
nit: could use `BOOST_CHECK_LE`.

👍 hodlinator approved a pull request: "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests"
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2525711019)
re-ACK 6983d82c84d4ca351a7cd9d1e0e20ab878da6475

Responding to my previous feedback + testing negative max return values.

Retested modified targets.

📝 hebasto opened a pull request: "test: Remove non-portable IPv6 test"
(https://github.com/bitcoin/bitcoin/pull/31580)
On Illumos-based systems, such as OpenIndiana and SmartOS, the assumption that "the default zone ID of 0 can be omitted for the default scope" is incorrect. As a result, `getaddrinfo("fe80::1%0", ...)` returns the `EAI_NONAME` error instead of resolving to "fe80::1".

See: https://www.illumos.org/man/3SOCKET/getaddrinfo.

This PR removes the problematic code introduced in https://github.com/bitcoin/bitcoin/pull/19951.

💬 hebasto commented on pull request "test: Remove non-portable IPv6 test":
(https://github.com/bitcoin/bitcoin/pull/31580#issuecomment-2565627902)
cc @jonatack @laanwj @ryanofsky @vasild

💬 hebasto commented on pull request "net, test: CNetAddr scoped ipv6 test coverage, rename scopeId to m_scope_id":
(https://github.com/bitcoin/bitcoin/pull/19951#discussion_r1899626430)
Apparently, this test is not portable. See: https://github.com/bitcoin/bitcoin/pull/31580.

💬 fjahr commented on pull request "Add `contrib/justfile` containing useful development workflow commands.":
(https://github.com/bitcoin/bitcoin/pull/31292#issuecomment-2565635915)
This would require some additional documentation in order to be merged but I also think this doesn't belong into this repo per the rationale given by @achow101 . Such files would probably add a lot of noise to the repo even from well meaning contributors. The approach @willcl-ark suggests sounds like a good compromise. Maybe we could have a page in the wiki that maintains a list of productivity tools and resources like this. I wasn't aware of Will's repo myself until now.

💬 fjahr commented on pull request "test: skip test if any of the needed release binaries is missing":
(https://github.com/bitcoin/bitcoin/pull/31462#issuecomment-2565652805)
I agree that the output isn't ideal, I always forget what to run to get the missing binaries when I run into this. But I also think the test should still fail.

💬 fjahr commented on pull request "test: Avoid intermittent error in assert_equal(pruneheight_new, 248)":
(https://github.com/bitcoin/bitcoin/pull/31468#discussion_r1899646488)
I think a little more detail would be helpful since it may not be obvious what the issue is.

``` suggestion
# Linear sync over RPC, because P2P sync may lead to blocks being received and saved to disk out of order which means prune heights would non-deterministic.
```

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