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

🤔 mzumsande reviewed a pull request: "net_processing: make any misbehavior trigger immediate discouragement"
(https://github.com/bitcoin/bitcoin/pull/29575#pullrequestreview-2123526970)
Code Review ACK 6eecba475efd025eb011400af58621ad5823994e

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2174002062)
Force-push: rebased to master for qt version bump. Trivial changes to depends, didn't need to change qt patch.

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1643213495)
@S3RK , @furszy you're right: `get_if` works here, as well. For some reason I was thinking `std::get_if` doesn't check if the type exists in the `std::variant` at compile time, hence using a visitor was better. But `std::get_if` does check that the type is included in the variant; its just that the visitor can actually do compile time checks to ensure all the cases are covered. But in the silent payments example, we don't need exhaustive coverage so the visitor is likely overkill. I've updated t
...

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1643215137)
IIRC, I tried to move `CRecipient` out of `wallet.h` awhile back and it ended up being more complicated than I expected? But in general I agree.

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#discussion_r1643215401)
Fixed.

👍 rkrux approved a pull request: "test: expand LimitOrphan and EraseForPeer coverage"
(https://github.com/bitcoin/bitcoin/pull/30082#pullrequestreview-2123601945)
reACK [172c1ad](https://github.com/bitcoin/bitcoin/pull/30082/commits/172c1ad026cc38c6f52679e74c14579ecc77c48e)

Thanks for very quickly addressing my comments @instagibbs, appreciate it!