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

...

💬 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_r1829540702)
In f2f8796 I modified `IsSmallInteger()` to handle `OP_1NEGATE` _and_ `OP_0` correctly. Previously only the python codebase handled `OP_0` correctly.

💬 mzumsande commented on issue "Listen on random port by default (not 8333)":
(https://github.com/bitcoin/bitcoin/issues/31036#issuecomment-2457459639)
> What about having P2P-seeds, an alternative to DNS-seeds, which are serving the P2P protocol and are used as addrfetch peers and they return only high-quality addresses from the crawler?

Yes, I think that could work.
Also, I wouldn't say that this particular downside I mentioned would be a blocker - even if it would take on average 1 minute instead of 20 seconds to find the initial 10 peers, that wouldn't be the end of the world, since it's just a one-time event for an empty addrman.

💬 darosior 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-2457482792)
Concept NACK. This introduces dead code and i don't think the justification for this meets the bar for touching consensus code.

💬 mzumsande 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_r1829569059)
I don't think that "correct" is the right term here, it's more about what these functions are intended to be used for. In this case, the comment `Test for "small positive integer"` indicates that the function wasn't designed to deal with `OP_0`, so if just the name could just rename it to `IsSmallPositiveInteger` to not introduce a change in behavior.

💬 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-2457529378)
Looks like using the matrix feature works for this