👍 vasild approved a pull request: "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip"
(https://github.com/bitcoin/bitcoin/pull/30111#pullrequestreview-2122762881)
ACK 0e0c422aedd4009ab34eca127e4904d15e81f5be

Changes like this are inherently difficult to review:

One has to check all variables protected by `cs_main` and ensure that none of them are required to be in sync with the ones that are removed from `cs_main` protection like `m_txrequest`.

Also, one has to check that all mutexes locked before the new one are never locked after the new one anywhere else in the code and that mutexes locked after the new one are never locked before it anywhere
...

💬 vasild commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1642728827)
Since now `cs_main` need not be held when accessing `m_txrequest` better not hold it. For clarity and performance.

```diff
{
LOCK(cs_main); // For m_node_states
m_node_states.emplace_hint(m_node_states.end(), std::piecewise_construct, std::forward_as_tuple(nodeid), std::forward_as_tuple(node.IsInboundConn()));
+ }
+ {
LOCK(m_tx_download_mutex);
assert(m_txrequest.Count(nodeid) == 0);
}

💬 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());
```