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

💬 josibake commented on pull request "refactor, wallet: get serialized size of `CRecipient`s directly":
(https://github.com/bitcoin/bitcoin/pull/30050#issuecomment-2174120310)
Updated to remove the visitor per @S3RK and @furszy 's feedback

💬 kevkevinpal commented on pull request "test: write functional test results to csv":
(https://github.com/bitcoin/bitcoin/pull/30291#issuecomment-2174125844)
tACK [ad06e68](https://github.com/bitcoin/bitcoin/pull/30291/commits/ad06e68399da71c615db0dbf5304d0cd46bc1f40)

Looks good to me!

📝 instagibbs opened a pull request: "NOMERGE #28984 package rbf followups"
(https://github.com/bitcoin/bitcoin/pull/30295)

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643277261)
done in follow-up

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643277352)
done in follow-up

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643277672)
done in follow-up, I had removed V3 in test, not the actual doc

💬 instagibbs commented on pull request "Cluster size 2 package rbf":
(https://github.com/bitcoin/bitcoin/pull/28984#discussion_r1643277948)
done in follow-up