💬 ajtowns commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2308952120)
Keeping the `block_unspendables_*` as cumulative totals would be more flexible, and would be nice to be able to directly query. (dropping `total_unspendable_amount` would be possible in that case, considering it's just the sum of these totals)

Likewise keeping `total_subsidy`.

With those values, `total_subsidy = total_amount + total_unspendable_amount` should be an accounting identity, with `total_amount` also matching a manual sum of values in the utxo set, and `total_subsidy` being preci
...

💬 ajtowns commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2308983931)
I also have this question. Adding `{0}` default initializers in the `struct DBVal` declaration seems sensible?

💬 ajtowns commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2308987847)
This could be a reference rather than a copy: `const Coin& coin{tx_undox.vprevout[j]};`

💬 ajtowns commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2309010798)
Both `prevout_spent_amount` and `new_outputs_ex_coinbase_amount` can overflow an int64_t in extreme cases -- filling a block with 15000 65-byte transactions (975kvB) repeatedly spending the same 7M BTC will hit 105 billion BTC, which is 1.05e19 sats which is about 13% more than 2**63-1 which is the max value a CAmount can store, and this is undefined behaviour in C++. (You'd need to cycle spends of ~12M BTC or more to overflow a uint64_t, though that is not undefined behaviour)

Even if we don
...

💬 ajtowns commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#issuecomment-3235597424)
When running this index over signet, I end up with ~700 55kB ldb files, and a single 8.8MB ldb file. That seems like something is probably suboptimal?

💬 151henry151 commented on pull request "build: Remove deprecated CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#issuecomment-3235780122)
There was a TODO still in the src/CMakeLists.txt file; I cleaned it up and squashed the commits just now and think that it is now all correct.

📝 maflcko opened a pull request: "test: Fixup fill_mempool docstring"
(https://github.com/bitcoin/bitcoin/pull/33269)
The assumption was removed in commit
3eab8b724044dc321f70e5eed66b149713158a04.

💬 maflcko commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2309281175)
It is less code this way:

```diff
diff --git a/src/test/fuzz/difference_formatter.cpp b/src/test/fuzz/difference_formatter.cpp
index 49e859ffdc..38b54352c1 100644
--- a/src/test/fuzz/difference_formatter.cpp
+++ b/src/test/fuzz/difference_formatter.cpp
@@ -4,25 +4,13 @@

#include <blockencodings.h>
#include <streams.h>
-#include <test/fuzz/FuzzedDataProvider.h>
+#include <random.h>
#include <test/fuzz/fuzz.h>

#include <vector>

namespace {
-// Test struct for VectorFo
...

💬 maflcko commented on pull request "test: p2p block malleability":
(https://github.com/bitcoin/bitcoin/pull/33172#issuecomment-3235915404)
lgtm ACK d0e1bbad016cc4949094daea2934712f92dfeecd

💬 frankomosh commented on pull request "p2p: add `DifferenceFormatter` fuzz target and invariant check":
(https://github.com/bitcoin/bitcoin/pull/33252#discussion_r2309383866)
I think this is more cleaner and simpler to follow. thanks

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#issuecomment-3236024582)
Are there any stats on increased coverage, or is this just checking more values in the same paths?

lgtm ACK 84927d37745920412b270ad3502308f0ad8cb7ca 📠

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/
...

🤔 maflcko reviewed a pull request: "fuzz: enhance wallet_fees by mocking mempool stuff"
(https://github.com/bitcoin/bitcoin/pull/33210#pullrequestreview-3167583982)
.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309317896)
nit in 0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: just `CMutableTransaction mtx{};` is enough, no need to list the type twice.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309376831)
nit in https://github.com/bitcoin/bitcoin/commit/79b28c0740c57aac5b91fd3aff20bc46b9229376: Seems to pass right now, even though it runs into fs path errors. Seems fine for now, but in the future there could be a pure abstract base class below `CBlockPolicyEstimator` (used by wallet_fees) and `CBlockPolicyEstimator` as well as `FuzzedBlockPolicyEstimator` could derive from it (`final`). But this can be done in a follow-up, if there is need to.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309336307)
nit in https://github.com/bitcoin/bitcoin/commit/0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: Why only assert `>0` and not `>fee`, or even `==fee+min_inc_fee`?

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309337870)
nit in https://github.com/bitcoin/bitcoin/commit/0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: Can just be a default-constructed CScript?

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309371283)
also clang-format for new code?

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309370885)
nit in 79b28c0740c57aac5b91fd3aff20bc46b9229376: Could drop the `C` for new code? `FuzzedBlockPolicyEstimator`.

💬 maflcko commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2309394350)
or maybe just move `MockMempoolMinFee` to the src/test/util/mempool utils and use it? (Skip if the target fee is less than inc relay fee or min relay fee)?

💬 jsarenik commented on pull request "wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet":
(https://github.com/bitcoin/bitcoin/pull/33268#issuecomment-3236117393)
Tested ACK 7d1c3ce7f0f0

Thanks!