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

💬 ariard commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#issuecomment-2436662747)
if one can go and publish on the mailing list in which core release this is expected to land and indicates the expected deployment timelines for the downstream softwares. learning a bit from the ecosystem controversies raised when `mempoolfullrbf` was actually introduced back in 24.0.1.

💬 pythcoiner commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2436768523)
concept ACK

💬 maflcko commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2437086690)
rfm?

🤔 hebasto reviewed a pull request: "doc: replace `-?` with `-h` and `-help`"
(https://github.com/bitcoin/bitcoin/pull/31118#pullrequestreview-2394439589)
Post-merge ACK 33a28e252a7349c0aa284005aee97873b965fcfe.

💬 i-am-yuvi commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437131445)
Concept ACK!!

🚀 fanquake merged a pull request: "fuzz: wallet: add target for `CreateTransaction`"
(https://github.com/bitcoin/bitcoin/pull/29936)

👍 maflcko approved a pull request: "Drop miniupnp dependency"
(https://github.com/bitcoin/bitcoin/pull/31130#pullrequestreview-2394558598)
Almost lgtm. Didn't review the gui changes and the rebase was borked.

ACK 2a541e93cf13362e52c62e4d01d34a602843471e 📏

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU
...

💬 maflcko commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#discussion_r1816223837)
3c0fe09cc2fac18175aac35a54d64c9f6cd4482b: This merge conflict was solved incorrectly. Please be careful when rebasing. See also diff3, which should make rebasing easier, if used correctly: https://github.com/bitcoin/bitcoin/blob/master/doc/productivity.md#rebasingmerging-code