💬 aureleoules commented on issue "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection due to #25273":
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852155662)
> Why was this not caught by CI?

Yeah, I don't believe all the benchmarks are being run. I think they should all be executed with at least -min-time=1000 as well.
I've noticed while working on my bench tool (context https://github.com/bitcoin/bitcoin/issues/27284#issuecomment-1852136094) that a few pulls almost introduced these bugs.
And some benchs only crash with `-min-time=1000` or `-min-time=10000` for some reasons.

📝 BrandonOdiwuor opened a pull request: "Calculate used balance from GetBalance(...)"
(https://github.com/bitcoin/bitcoin/pull/29062)
Compute `used` balance from GetBalance(...)

_Check https://github.com/bitcoin/bitcoin/pull/28776#discussion_r1407875236 by @achow101_

> However, I think it would be better to move all of this into `GetBalance` itself and just have it always compute the used balance for us rather than having the caller do this extra computation.

💬 fanquake commented on issue "Benchmark WalletCreateTxUsePresetInputsAndCoinSelection crashes due to #25273":
(https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852171394)
> Unfortunately no, it still crashes on master as of https://github.com/bitcoin/bitcoin/commit/a7484be65f7617d77aff92ecf6f5fb26015d27a8

cc @achow101 @furszy

🤔 stickies-v reviewed a pull request: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830#pullrequestreview-1775679380)
LGTM 3aa41a31690da2ae22b6a15a9a5de0de78a01757 when fuzzer is fixed

💬 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