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

📝 dergoegge opened a pull request: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221)

💬 dergoegge commented on pull request "ci: Split out native fuzz jobs for macOS and windows (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31221#issuecomment-2457369883)
cc @maflcko couldn't reopen the other PR after force pushing

📝 dergoegge converted_to_draft a pull request: "ci: Split out native fuzz jobs for macOS and windows (take 2)"
(https://github.com/bitcoin/bitcoin/pull/31221)

💬 instagibbs commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1829485990)
will take language if I touch PR again

💬 Christewart commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#discussion_r1829528839)
I think this was already the case before this PR? It seems that the currently implementation in solver.cpp doesn't handle `OP_0` correctly, while the python codebase does?

https://github.com/bitcoin/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/test/functional/test_framework/script.py#L89

https://github.com/bitcoin/bitcoin/blob/03cff2c1421e5db59963eba1a845ef5dd318c275/src/script/solver.cpp#L61

This goes to the purpose of this PR, handle them consistently across codebases and re
...

💬 Christewart commented on pull request "consensus: Consistently encode and decode `OP_1NEGATE` similar to other small ints in Script":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2457443510)
Thanks for looking at this @mzumsande !

> I didn’t see earlier versions of this PR, but It’s not clear to me what the goal of this PR is from the OP / Title. Should the title be updated, is this still supposed to fix a bug?

Modified the title, hopefully its more clear.

> As mentioned above, `DecodeOP_N` / `EncodeOP_N` currently don't need to handle `OP_1NEGATE` in any of the current use cases - so I think that there should be some explanation why they should be changed to support it.

...