💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1422813069)
nit
```suggestion
MockedTxPool& tx_pool = *static_cast<MockedTxPool*>(tx_pool_.get());
```

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1423923859)
nit: I'd intuitively expect `Make` to return a (wrapped) ` CTxMemPool`. Would it make sense to name this method `MakeUnique`?

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1424090165)
Wdyt about this approach to fix the fuzzer (this impl requires rebase to use c++20 for `std::string::starts_with`)? This should be efficient without relying too much (but still somewhat) on the implementation of `CTxMemPool::Make`?

After #25665 is merged, could have a `Make` return an enum failure value and avoid the errorstring checking.

<details>
<summary>git diff on 3aa41a3169</summary>

```diff
diff --git a/src/test/fuzz/package_eval.cpp b/src/test/fuzz/package_eval.cpp
index ec7
...

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1422850484)
nit: could limit scope of `mempool`:

<details>
<summary>git diff on 3aa41a3169</summary>

```diff
diff --git a/src/init.cpp b/src/init.cpp
index eb88a3fa73..1fa34b77e8 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1476,11 +1476,11 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)
}

for (bool fLoaded = false; !fLoaded && !ShutdownRequested();) {
- auto mempool{CTxMemPool::Make(mempool_opts)};
- if (!mempool) {
+
...

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1423915340)
nit: unnecessary newline?

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1422862314)
nit: is there a meaningful use for `Assert` here (and in most other places in this pull) since we already use `util::Result::value()` and there are no side effects?

💬 stickies-v commented on pull request "[refactor] Check CTxMemPool options in ctor":
(https://github.com/bitcoin/bitcoin/pull/28830#discussion_r1424003014)
nit: if we're okay dropping the `Assert`, would just one-line this ([+here ](https://github.com/bitcoin/bitcoin/pull/28830/files#diff-d324f8d2ac7811c1d9b47d79833154afa6e1e41261170573968ac2ebfde9f478R55-R56), [here](https://github.com/bitcoin/bitcoin/pull/28830/files#diff-77738eb901e87bd186e09ee7d817a0989f1a1a86d045bc2a8a4ffbe35f71145dR36-R37)). Otherwise, not a fan of using `mempool` (or `tx_pool`) as var names, when the difference with `pool` is one being a ptr to the other. `ppool` or `pool_pt
...

💬 maflcko commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1424103755)
Yes, but I guess it is not too useful, so I've dropped the commit for now.

💬 furszy commented on issue "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection crashes due to #25273":
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852182222)
The issue is the change output index. Since #25273, needs to be `std::nullopt`, not -1 (which now throws "out of range").

💬 furszy commented on issue "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection crashes due to #25273":
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852187709)
Also, this will happen at the GUI level too. We are still passing -1 at the wallet controller level too.
Will work on both things, add coverage and push the fix in few hours.

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424116143)
did you want to use `CheckMempoolInvariants` here as well?

🤔 jamesob reviewed a pull request: "fuzz: p2p: Detect peer deadlocks"
(https://github.com/bitcoin/bitcoin/pull/29009#pullrequestreview-1777731296)
ACK 9f265d88253ed464413dea5614fa13dea0d8cfd5 ([`jamesob/ackr/29009.1.maflcko.fuzz_p2p_detect_peer_dea`](https://github.com/jamesob/bitcoin/tree/ackr/29009.1.maflcko.fuzz_p2p_detect_peer_dea))

Using the suggested buggy diff, I was able to reproduce a fuzz timeout locally:

```
SUMMARY: libFuzzer: timeout
================== Job 4 exited with exit code 70 ============
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3004856548
INFO: Loaded 1 modules (549836 inline 8-b
...

💬 BrandonOdiwuor commented on pull request "doc: explain what the wallet password does":
(https://github.com/bitcoin/bitcoin/pull/28974#discussion_r1424167602)
Fixed

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424169715)
Taken

💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1424176890)
I don't think we should do that; we'd want to look at the impact on all related transaction's descendant scores. This transaction's modified fee might be negative, but it could have high-feerate descendants.

💬 Sjors commented on pull request "Stratum v2 Template Provider (take 2)":
(https://github.com/bitcoin/bitcoin/pull/28983#issuecomment-1852266713)
Pleased clang-tidy. Fixed memory issue by disabling a broken test, added `Assume` to `noise.cpp` to check the message size. Added a (possibly overkill) destructor to `Sv2Template`.

💬 achow101 commented on pull request "wallet: skip BnB when SFFO is enabled":
(https://github.com/bitcoin/bitcoin/pull/28994#issuecomment-1852286863)
ACK 576bee88fd36e207b7288077626947a1fce0fc33

💬 fanquake commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#issuecomment-1852300861)
Guix build (aarch64):
```bash
1415d765a10d1686e0a37f550aaf83fadad17d2d94c17688ae298ececcca17e2 guix-build-bde8d63b1763/output/aarch64-linux-gnu/SHA256SUMS.part
7e6f42f033469d23300b93c3541babae3d60a7e21615ea3ba08f243131339e5c guix-build-bde8d63b1763/output/aarch64-linux-gnu/bitcoin-bde8d63b1763-aarch64-linux-gnu-debug.tar.gz
3eba206402f4a6f7bcbe73c53f1b95f01944e35b7c3a94cf0cb1daf2db08afc8 guix-build-bde8d63b1763/output/aarch64-linux-gnu/bitcoin-bde8d63b1763-aarch64-linux-gnu.tar.gz
571ba0
...

👍 TheCharlatan approved a pull request: "refactor: Print verbose serialize compiler error messages"
(https://github.com/bitcoin/bitcoin/pull/29056#pullrequestreview-1777851574)
Re-ACK fa45567e030272d34845e6b563a6b626b9eda739

🚀 achow101 merged a pull request: "wallet: skip BnB when SFFO is enabled"
(https://github.com/bitcoin/bitcoin/pull/28994)