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

👍 tdb3 approved a 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#pullrequestreview-2393934421)
ACK f2b920c31cc1254c510dcb9cdc14b73d50f800a2
Thanks for moving the test to `p2p_orphan_handling`. Left some non-blocking recommendations.

💬 tdb3 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_r1815846025)
nit: Could put it in `test/functional/test_framework/mempool_util.py` as `DEFAULT_MAX_ORPHAN_TRANSACTIONS` to match https://github.com/bitcoin/bitcoin/blob/947f2925d55f363cf1880315d467fc2edac04545/src/net_processing.h#L27

💬 tdb3 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_r1815848810)
Generating 160 blocks might be overkill. The following generates one and moves `node = self.nodes[0]` to just under the opening log (to match other functions).

```diff
diff --git a/test/functional/p2p_orphan_handling.py b/test/functional/p2p_orphan_handling.py
index 0a88b4c9587..5276814a087 100755
--- a/test/functional/p2p_orphan_handling.py
+++ b/test/functional/p2p_orphan_handling.py
@@ -572,9 +572,9 @@ class OrphanHandlingTest(BitcoinTestFramework):
@cleanup
def test_max_o
...

🤔 glozow reviewed a pull request: "functional test: Additional package evaluation coverage"
(https://github.com/bitcoin/bitcoin/pull/31152#pullrequestreview-2393934991)
ACK b318959bf8c2b9b02f718649adec03f7e07ac79d, tested that adding a trim at the end of subpackage evaluation causes this to fail. Happy to reack if you take the nits.

💬 glozow commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815846424)
forgot to delete?

💬 glozow commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815859993)
nit: strictly speaking...
```suggestion
max_parent_feerate = max([entry["fees"]["modified"] / (Decimal(entry["vsize"]) / 1000) for entry in parent_entries])
```

💬 glozow commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815846947)
nit: to be possibly be
```suggestion
# Last parent is higher feerate causing other parents to possibly
# be evicted if trimming was allowed, which would cause the package to end up failing
```

💬 glozow commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815863021)
nit: relying on the fact that mempool min feerate isn't persisted seems a little bit brittle.

💬 glozow commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1815870276)
Comment could be clearer: This test is specifically trying to test that intra-package trimming isn't happening. We already know that a transaction with a high feerate descendant shouldn't be evicted; we specifically want to make sure that the eviction decision doesn't happen until the package is all the way in. So we check effective-includes to ensure that the transactions were evaluated separately.

💬 glozow commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#discussion_r1815873284)
Fixed, thanks :)

💬 glozow 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_r1815876255)
nit: why is this a `wait_until` instead of an assert?

💬 glozow commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1815883458)
nice catch

💬 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_r1815888974)
Thanks! Updated in [5c299ec](https://github.com/bitcoin/bitcoin/pull/31040/commits/5c299ecafe6f336cffa145d28036b04b87e26712)

💬 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_r1815889024)
Thanks! Updated in [5c299ec](https://github.com/bitcoin/bitcoin/pull/31040/commits/5c299ecafe6f336cffa145d28036b04b87e26712)

💬 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_r1815889280)
Thanks! Updated in [5c299ec](https://github.com/bitcoin/bitcoin/pull/31040/commits/5c299ecafe6f336cffa145d28036b04b87e26712)

💬 ariard commented on pull request "Halt processing of unrequested transactions v2":
(https://github.com/bitcoin/bitcoin/pull/30572#issuecomment-2436659077)
> This looks like a separate issue. I was talking about orphan resolution removing the tx, you are talking about TxRequestTracker expiration removing it.

Yes, see my comments above. I got it there are current issues in the orphan resolution removing the transaction with a `ForgetTxHash` before we received it again, and wrongly see it as unsolicited.

> I wasn't changing a random line of code: the added code is guarded by a future P2P version upgrade and therefore unreachable and untestable
...