💬 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

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1829434993)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (a902af6fb195fae97a89876b88d5034162ab8366)

Should this include a space between that value and flags? Otherwise if test fails it looks like amount and flags will run together and might not be possible to distinguish.

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1829440915)
Thanks, no this is just a note to help myself and other reviewers.

💬 ryanofsky commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1829431415)
In commit "test: Migrate GetCoinsMapEntry to return MaybeCoin" (a902af6fb195fae97a89876b88d5034162ab8366)

May want to add "refactor" to commit subject or otherwise mention that this commit is not adding or removing any test coverage. Otherwise it's hard to know looking at the commit what it's intended to do.

Same comment also applies to commit "test: Remove remaining unbounded flags from coins_tests" (e4db49949c55470950a206172ee53f6baacf3767) and commit "test: Compact ccoins_access and cco
...

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1829477742)
In a next PR we could add another DynamicUsage which accepts a vector of vectors and does the iteration instead of the call sites

💬 brunoerg commented on pull request "fuzz: wallet: add target for spkm migration":
(https://github.com/bitcoin/bitcoin/pull/29694#discussion_r1829481247)
Will do it.

💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#issuecomment-2457364731)
ACK d22a234ed270286b483aec2db1e2f716b9756231

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1829483491)
That is changing the API somewhat unnecessarily for this PR. We can properly remove it with deprecation flag in follow-up PR, or in a following release.