💬 dergoegge commented on pull request "fuzz: make FuzzedDataProvider usage deterministic":
(https://github.com/bitcoin/bitcoin/pull/29043#issuecomment-1852151987)
> I don't know how stable the ordering is for llvm. If it stays like that it's not a big deal I'd say, but it might change in future versions or with optimization levels?

Good point, I guess since the spec is unspecified they have the right to change it but I would still be surprised if they do. I'm not sure if there is a trivial way to check if they ever have.

> in a different project I'm working on we run the fuzzing targets with the minimized corpus as unit tests, and this is done in th
...

👍 TheCharlatan approved a pull request: "refactor: Print verbose serialize compiler error messages"
(https://github.com/bitcoin/bitcoin/pull/29056#pullrequestreview-1777459272)
`concept` ACK fa180c523a173550ab4e549788cb21d4e7994118 :tada:

This:
```
./serialize.h:763:6: note: candidate template ignored: constraints not satisfied [with Stream = DataStream, T = Span<int>]
void Serialize(Stream& os, const T& a)
^
./serialize.h:762:14: note: because 'Serializable<Span<int>, DataStream>' evaluated to false
requires Serializable<T, Stream>
^
./serialize.h:760:52: note: because 'a.Serialize(s)' would be invalid: no member named 'Serialize' in
...

💬 TheCharlatan commented on pull request "refactor: Print verbose serialize compiler error messages":
(https://github.com/bitcoin/bitcoin/pull/29056#discussion_r1423986205)
Nit: Should this rather be in `assumptions.h` if it counts for the entire codebase?

💬 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