💬 sipa commented on pull request "Update secp256k1 subtree to v0.6.0":
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2457195648)
Another included benefit is that the default test iteration count for the secp256k1 `tests` binary has been reduced, which means Bitcoin Core's `ctest` run should speed up (the `tests` binary is the single longest running task currently).

👍 tdb3 approved a pull request: "test: cover edge case for lockunspent rpc"
(https://github.com/bitcoin/bitcoin/pull/31209#pullrequestreview-2415647691)
ACK e58c4307ef576915200c4eda57d6d6ff10088667

Thank you.
Coverage report seems to indicate coverage was added.

Sanity checked the change by executing functional tests (without legacy-wallet, but with `wallet_basic.py --descriptors`) with the following update.
```diff
diff --git a/src/wallet/rpc/coins.cpp b/src/wallet/rpc/coins.cpp
index f1430a3c601..81fd75db0fd 100644
--- a/src/wallet/rpc/coins.cpp
+++ b/src/wallet/rpc/coins.cpp
@@ -320,7 +320,7 @@ RPCHelpMan lockunspent()

...

👍 ryanofsky approved a pull request: "Prune mining interface"
(https://github.com/bitcoin/bitcoin/pull/31196#pullrequestreview-2415644803)
Code review 6d46e067b71901308433afd7c70d4ea64c2a1d1b, but the rpc_generate.py test is currently broken (https://cirrus-ci.com/task/4929149096165376?logs=ci#L4665). Suggested a fix below

💬 ryanofsky commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#discussion_r1829361447)
In commit "Remove testBlockValidity() from mining interface" (4d4a1822ba8eef2ff857885c5253156dacff8d3c)

This change seem to be causing the rpc_generate.py test to fail. Can be fixed with:

```diff
--- a/test/functional/rpc_generate.py
+++ b/test/functional/rpc_generate.py
@@ -87,7 +87,7 @@ class RPCGenerateTest(BitcoinTestFramework):
txid1 = miniwallet.send_self_transfer(from_node=node)['txid']
utxo1 = miniwallet.get_utxo(txid=txid1)
rawtx2 = miniwallet.crea
...

💬 ryanofsky commented on pull request "Prune mining interface":
(https://github.com/bitcoin/bitcoin/pull/31196#discussion_r1829368566)
In commit "Remove testBlockValidity() from mining interface" (4d4a1822ba8eef2ff857885c5253156dacff8d3c)

Review note: it seemed like a potential problem that this check is being dropped, but looking more closely it should be ok. In the `getblocktemplate` "proposal" call this condition is already checked in, and in the `generateblock` call the check wouldn't reliable because cs_main isn't hold, and more reliable checking happens later when the block is connected.

👍 ryanofsky approved a pull request: "refactor: mining interface 30955 followups"
(https://github.com/bitcoin/bitcoin/pull/31197#pullrequestreview-2415686066)
Code review ACK 058862581085316927287817b2af01e8f4765a1d. Looks good, thanks for the cleanups!

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829410358)
Fixed in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829410806)
Fixed in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5

💬 maflcko commented on pull request "test: cover edge case for lockunspent rpc":
(https://github.com/bitcoin/bitcoin/pull/31209#issuecomment-2457266568)
Not sure about this. This seems to be adding coverage for code that shouldn't exist in the first place.

The integer is passed to `COutPoint`, which only takes unsigned integers, so parsing the integer as such would be more consistent. The check would then be redundant and could be removed.

💬 Eunovo commented on pull request "wallet: fix unrelated parent conflict doesn't cause child tx to be marked as conflict":
(https://github.com/bitcoin/bitcoin/pull/29680#discussion_r1829412307)
Updated comment in https://github.com/bitcoin/bitcoin/pull/29680/commits/3c893c51426b2bd9b9b504d00614f3f1742322d5

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829420178)
that's a mistake, I'll fix if I touch the PR

💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1829420263)
will fixup if I end up touching things

💬 hebasto commented on pull request "Update secp256k1 subtree to v0.6.0":
(https://github.com/bitcoin/bitcoin/pull/31216#issuecomment-2457287984)
> Another included benefit is that the default test iteration count for the secp256k1 `tests` binary has been reduced, which means Bitcoin Core's `ctest` run should speed up (the `tests` binary is the single longest running task currently); see [bitcoin-core/secp256k1#1581](https://github.com/bitcoin-core/secp256k1/pull/1581).

Additionally, the subtree tests can now match `ctest` regex options, such as `-E` or `-R`:
```
$ ctest --test-dir build -j 16 -R secp256k1
```

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2457295003)
Rebased due to the conflict with the merged bitcoin/bitcoin#31191.

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2457295681)
ACK 131bed19bdfc922328cad9781fa9122b6810a8fd

🤔 marcofleon reviewed a pull request: "test: cover base32/base58/base64 with symmetric roundtrip fuzz (and padding) tests"
(https://github.com/bitcoin/bitcoin/pull/30746#pullrequestreview-2415779821)
Code review ACK 6fd185c035c1cc4dd961cf14a2087e97fb069440

Nice improvements to the fuzz test. The separation into different targets for each encoding looks good to me. I guess in principle, each roundtrip should be separated as well. But seems fine as is for these simple targets.

💬 Christewart commented on pull request "test: Add `leaf_version` parameter to `taproot_tree_helper()`":
(https://github.com/bitcoin/bitcoin/pull/29371#issuecomment-2457328118)
> Concept ACK
>
> Please cleanup your commit message, it contains message fragments from squashed commits.

Done in 6ffdabd

👍 ryanofsky approved a pull request: "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests"
(https://github.com/bitcoin/bitcoin/pull/30906#pullrequestreview-2415709430)
Code review ACK aa2f3139529c054b011a0f75ff314e6d63f0d977. Thanks for the updates! Left a few more comments that are not important and you can feel free to ignore.

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1829401813)
In commit "coins, refactor: Split up AddFlags to remove invalid states" (a6921049fcc3de4067dc3f88fef57884450d25a1)

Not important, but instead of SetFresh and SetClean() are being introduced as non-static methods in this commit and then changing them to static in the next commit, wouldn't it make more sense to just introduce them as static methods in this commit? That should reduce churn and make the next commit simpler, because only AddFlags will be changing.

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1829417295)
In commit "coins, refactor: Assume state after SetClean in AddFlags to prevent dangling pointers" (2d691b50d3729445a60898981b78c4239f2f0dd7)

I think this could also check the converse, that pointers are valid if flags were set. It could simplify check that pointers are non-null if flags are set, or go further and check prev->next == self, and next->prev == self