💬 jasonribble commented on pull request "Refactor BnB tests":
(https://github.com/bitcoin/bitcoin/pull/29532#discussion_r1815773346)
nit: perhaps the comment on line 144 can be more descriptive for this part. (or the comments in general can be improved). It's the strangest test in the group.

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#issuecomment-2436507075)
> > > Concept ACK Thanks, this is useful! I'm uncertain if this check is better suited for `p2p_orphan_handling` or `rpc_getorphantxs` (or for a `feature_orphanage` or `mempool_orphanage`). If `p2p_orphan_handling` is the preferred location, I'm happy to include your commit (preserving you as author) in #31037.
> > > https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#naming-guidelines
> > > Can also check unit tests (https://github.com/bitcoin/bitcoin/blob/master/src/test
...

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815804685)
I think the @cleanup function when moving the test to `p2p_orphan_handling.py` in [3bc3bcc](https://github.com/bitcoin/bitcoin/pull/31040/commits/3bc3bcce546581f450748b8106dc36dfc3e71b26) should be handling this now

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815804921)
This is now removed after moving to `p2p_orphan_handling.py` in [3bc3bcc](https://github.com/bitcoin/bitcoin/pull/31040/commits/3bc3bcce546581f450748b8106dc36dfc3e71b26)

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815805677)
I've just defined it at the top of the test file should that be fine?

💬 kevkevinpal commented on pull request "test: Assert that when we add the max orphan amount that we cannot add anymore and that a random orphan gets dropped":
(https://github.com/bitcoin/bitcoin/pull/31040#discussion_r1815805874)
updated in [f2b920c](https://github.com/bitcoin/bitcoin/pull/31040/commits/f2b920c31cc1254c510dcb9cdc14b73d50f800a2)

💬 kevkevinpal commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1815813480)
Thanks! I moved it more towards the start of the function in [34c6643](https://github.com/bitcoin/bitcoin/pull/31139/commits/34c6643c2fe0e73d73edb2ddddf0900e4256de57)

💬 kevkevinpal commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1815815759)
Ya that makes sense!

What I did in [34c6643](https://github.com/bitcoin/bitcoin/pull/31139/commits/34c6643c2fe0e73d73edb2ddddf0900e4256de57)

Is I used a proper hex value and then had the second value malformed so that we can cover more bases

let me know if that looks good to you I can modify it other ways aswell!

👍 sdaftuar approved a pull request: "functional test: Additional package evaluation coverage"
(https://github.com/bitcoin/bitcoin/pull/31152#pullrequestreview-2393878103)
Looks good, just some comment nits.

💬 sdaftuar commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815810879)
"belo"?

💬 sdaftuar commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815817176)
This comment isn't right, is it? We don't expect the package to be evicted?

💬 instagibbs commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1815820994)
> it only plays with non-hex values and do not exercise the hex parsing.

an empty string is valid hex, it's just an invalid tx, no? If you want to test invalid hex that's another thing.

> if the first tx is invalid, the second one has no effect.

You need two transactions currently for it to get far enough to try and deserialize. I don't really care which one(s) are valid except for ease of reading.

> let me know if that looks good to you I can modify it other ways aswell!

imo havi
...

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815822938)
incomplete sent, fixed

💬 instagibbs commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815823046)
fixed thanks

💬 instagibbs commented on pull request "test: added test to assert TX decode rpc error on submitpackage rpc":
(https://github.com/bitcoin/bitcoin/pull/31139#discussion_r1815824499)
would be good to double-check that the first tx doesn't make it into the mempool even if it otherwise would

💬 sdaftuar commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#issuecomment-2436556487)
ACK b318959bf8c2b9b02f718649adec03f7e07ac79d

💬 glozow commented on pull request "validation: fix coins disappearing mid-package evaluation":
(https://github.com/bitcoin/bitcoin/pull/28251#issuecomment-2436567273)
> As I understand it, I think there are two key behaviors introduced in this PR

> However, the specific issue that @glozow described above (https://github.com/bitcoin/bitcoin/pull/28251#issuecomment-1688170869), where we need to ensure that package validation fails if an input is evicted mid-validation, seems to be completely mitigated by behavior (1).

> As far as I can tell from reviewing [3ea71fe (#28251)](https://github.com/bitcoin/bitcoin/pull/28251/commits/3ea71feb11c261f002ed918f91f3
...

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1815842865)
After looking at this a little more, I don't think we need `m_all_conflicts` to live in `SubPackageState` anymore -- `FinalizeSubpackage` can grab this information from the changeset and then the other two places are just local uses within a function.

Pushed a commit to get rid of this extra state.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1815846450)
Added docs in 7bc0f1f48c079b019f1d7a3fef00ae711f25d624

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1815846892)
I think this is all good now (your test in #31152 passes for me locally), so marking this as resolved.