💬 vasild commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1642792214)
Should `s/invalid/valid`?

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2173431291)
Tested faacd264417bd7f3f08c5ff497458030b3a54fbc on Intel macOS 13.6.7, FreeBSD 13.2, Windows and Ubuntu 24.04.

💬 instagibbs commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1642862717)
I'm switching the word to "stored" to be clearer

💬 instagibbs commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1642862845)
good point, I added an assert to make it clear its larger, since I think that's the assertion we really want

💬 instagibbs commented on pull request "test: expand LimitOrphan and EraseForPeer coverage":
(https://github.com/bitcoin/bitcoin/pull/30082#discussion_r1642862912)
considering this resolved

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1642751240)
a8818568a44e02c0346ef37cde9155191f8b3df1: `__FreeBSD_version` can be lowered to `1302000` as per the comment (or `1302001` which I tested on). Although you need `kldload /boot/kernel/netlink.ko`.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1642882599)
3fdc97a2bf7d1ec774d2ffa00502188158c64724: I don't know why this was here in the first place, seems fine to drop.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1642879959)
3fdc97a2bf7d1ec774d2ffa00502188158c64724: this line could be moved to the `miniupnpc` section above:

```
Note: UPnP support will be compiled in, but disabled by default.
```

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1642892799)
57e23f3b4d9cc5bdba421e47def9f3b2335d4cfb: nit, no need to drop this

👍 Sjors approved a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2122801091)
tACK faacd264417bd7f3f08c5ff497458030b3a54fbc

Tested on Intel macOS 13.6.7, FreeBSD 13.2, Windows and Ubuntu 24.04.

💬 ryanofsky commented on issue "RFC: Assumeutxo and large forks and reorgs":
(https://github.com/bitcoin/bitcoin/issues/30288#issuecomment-2173587759)
> > If I can summarize and clarify, neither of you think the current behavior of locking in snapshot block, and temporarily refusing to consider chains that don't include it is a good idea?
>
> I'm not sure I'm following this point exactly: my recollection is that the current observable behavior in this scenario would be to crash [and ...] apart from the terrible UX, this is essentially the right behavior

Whether a node ignores chains that have the most work and don't include the snapshot
...

💬 instagibbs commented on pull request "Ephemeral Anchors, take 2":
(https://github.com/bitcoin/bitcoin/pull/30239#issuecomment-2173636274)
rebased due to s/nVersion/version/ change for transactions causing a silent merge conflict

💬 ryanofsky commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1642951609)
re: https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1638706909

> Some checks are within `ActivateSnapshot()`, others, like the added one, are in the rpc code (which means that they can't be covered in unit tests for that function). Do you know if there is a reason for that?

I think it this is just a mistake, and it makes the checks less reliable because they are done without keeping cs_main locked. I suggested fixing this before in https://github.com/bitcoin/bitcoin/pull/27596#di
...

🤔 marcofleon reviewed a pull request: "fuzz: FuzzedSock::Recv() don't lose bytes from MSG_PEEK read"
(https://github.com/bitcoin/bitcoin/pull/30273#pullrequestreview-2123209479)
ACK 4d81b4de339efbbb68c9785203b699e6e12ecd83. Tested this with the I2P fuzz target and there's no loss in coverage. I think overall this is an improvement in the robustness of `Recv` in `FuzzedSock`.

🤔 glozow reviewed a pull request: "test: fix MiniWallet script-path spend (missing parity bit in leaf version)"
(https://github.com/bitcoin/bitcoin/pull/30076#pullrequestreview-2123358958)
ACK e4b0dabb2115dc74e9c5794ddca3822cd8301c72

Not an expert but: Checked that `test_wallet_tagging` fails without the fix on "mandatory-script-verify-flag-failed (Witness program hash mismatch) (-26)". Reviewed the test by editing the parity check in `VerifyTaprootCommitment` to make it pass on master and examining `bytes(version + negflag)` while running.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2173875634)
Thanks @vasild. I will take your suggestions in a followup or if I retouch this.

🤔 murchandamus reviewed a pull request: "Cluster size 2 package rbf"
(https://github.com/bitcoin/bitcoin/pull/28984#pullrequestreview-2123251625)
utACK 94ed4fbf8e1a396c650b5134d396d6c0be35ce10

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643031435)
I think there may be extraneous words in "too-large package RBF subpackage". How about:

```suggestion
return strprintf("RBF subpackage with tx %s was too large",
wtxid.ToString());
```

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643020251)
```suggestion
- All conflicting clusters (connected components of mempool transactions) must be clusters of up to size 2.
```

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643019636)
```suggestion
- All direct conflicts must signal replacement (or the node must have `-mempoolfullrbf=1` set).
```