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

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643079133)
```suggestion
package_hex5_1, package_txns5_1 = self.create_simple_package(coin, parent_fee=DEFAULT_CHILD_FEE, child_fee=DEFAULT_CHILD_FEE)
pkg_results5_1 = node.submitpackage(package_hex5_1)
self.assert_mempool_contents(expected=package_txns5_1)
self.generate(node, 1)
```

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643027843)
"result replaced" seems like the wrong tense to me, given that the transaction was not accepted. Should this perhaps be:

```suggestion
// Each subpackage is allowed MAX_REPLACEMENT_CANDIDATES replacements (only checking individually here)
if (atmp_result.m_replaced_transactions.size() > MAX_REPLACEMENT_CANDIDATES) {
return strprintf("tx %s would replace too many transactions",
wtxid.ToString());
}
```

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643074768)
Maybe also add a success case in the end to prove that all the reasons for rejection were enumerated?

```suggestion
self.log.info("Check non-heavy child with higher absolute fee will replace")
package_hex3_1, package_txns3_1 = self.create_simple_package(coin, parent_fee=DEFAULT_FEE, child_fee=DEFAULT_CHILD_FEE + Decimal("0.00000001") )
pkg_results3_1 = node.submitpackage(package_hex3_1)
self.assert_mempool_contents(expected=package_txns3_1)
self.ge
...

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643022823)
I understood above comments to mean that this reference to v3 was replaced by a reference to TRUC transactions, but it seems to still mention v3.

💬 murchandamus commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643155693)
I think this comment is now also outdated?

💬 ryanofsky commented on pull request "build: add `-Wundef`":
(https://github.com/bitcoin/bitcoin/pull/29876#issuecomment-2173969308)
> Hmm, this is the first time I've seen the -Wundef warning, and my initial reaction is to be skeptical of it, because it seems like it would require patching a lot of upstream headers

Actually I think I'm wrong about this. It seems like compiler will not trigger warnings in headers if headers are found on an -isystem path instead of a normal include path. So as long as we include capnproto headers with -isystem, maybe there will be no problems. I actually implemented that last week in commit
...

📝 laanwj reopened a pull request: "depends: Remove Qt build-time dependencies"
(https://github.com/bitcoin/bitcoin/pull/29923)
In the spirit of the halving, let's (approximately) halve the number of packages in depends. Remove the following packages:
```diff
bdb.mk
boost.mk
capnp.mk
- expat.mk
- fontconfig.mk
- freetype.mk
libevent.mk
libmultiprocess.mk
libnatpmp.mk
- libXau.mk
- libxcb.mk
- libxcb_util.mk
- libxcb_util_image.mk
- libxcb_util_keysyms.mk
- libxcb_util_render.mk
- libxcb_util_wm.mk
- libxkbcommon.mk
miniupnpc.mk
native_capnp.mk
native_cctools.mk
native_libmultipro
...