👍 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-2525417419)
re-ACK a32002f2d5cff72639c9782d70ae52ec162d9ef8

- Only added commit that fuzzes the max length parameter of `DecodeBase58`&`DecodeBase58Check`.

Re-ran the 2 modified fuzz-targets 30 seconds each.

💬 hodlinator commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1899441448)
nit: As `random_string` is currently `0-1000` long, and `max_ret_len` is `1-300`, most of the time `DecodeBase58()` is just returning `false` since `max_ret_len` is too small. But maybe that is intentional to stress that aspect?

Might be better to have more similar lengths, and also to include `0` as `max_ret_len` since it might occur in places where that parameter is calculated.

```suggestion
const std::string random_string{provider.ConsumeRandomLengthString(1000)};
const auto m
...

💬 l0rinc commented on pull request "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests":
(https://github.com/bitcoin/bitcoin/pull/30746#discussion_r1899492703)
Good point, and even negative values theoretically - thanks, [pushed](https://github.com/bitcoin/bitcoin/compare/a32002f2d5cff72639c9782d70ae52ec162d9ef8..6983d82c84d4ca351a7cd9d1e0e20ab878da6475)

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

💬 l0rinc commented on pull request "RPC: Add reserve member function to `UniValue` and use it in `getblock` RPC":
(https://github.com/bitcoin/bitcoin/pull/31179#issuecomment-2565453331)
@ismaelsadeeq https://github.com/bitcoin/bitcoin/pull/20999 by @martinus also contains a few optimization attempts (a lot of them were already applied in other PRs since, but a few of the changes can still be done).
If RPC speed is important to you, the above suggestions can result in a measurable speedups.

💬 fjahr commented on pull request "wallet, desc spkm: Return SigningProvider only if we have the privkey":
(https://github.com/bitcoin/bitcoin/pull/31242#issuecomment-2565453438)
utACK 493656763f73e5ef1cfb979a513c12983dca99dd

Test coverage would be nice but I suppose it might be skipped if this get coverage as part of wider coverage of the musig2 code paths.

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