💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001430348)
`ClusterSet m_main_clusterset; std::optional<ClusterSet> m_staging_clusterset;` might be clearer -- the vector stuff seems more confusing than helpful?

Does it ever make sense to have multiple nested staging levels? Parallel staging levels could make sense (I want to compare three RBFs in parallel on multiple cores, having checked that they're all independent and don't interact with each others' clusters) but seems probably more complicated than would be worthwhile.

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001521035)
Avoid side-effects inside asserts if possible?

💬 Chand-ra commented on pull request "test: replace assert with assert_equal and assert_greater_than":
(https://github.com/bitcoin/bitcoin/pull/32091#discussion_r2001701966)
> What happens if you run the individual test, i.e. $ ./build/test/functional/interface_usdt_net.py

You were right, I didn't compile bitcoind with USDT support. Running the individual test results in the following output:
```
2025-03-18T18:06:36.018000Z TestFramework (INFO): PRNG seed is: 6897053979890084060
2025-03-18T18:06:36.021000Z TestFramework (WARNING): Test Skipped: bitcoind has not been built with USDT tracepoints enabled.
2025-03-18T18:06:36.074000Z TestFramework (INFO): Stoppi
...

💬 instagibbs commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001723093)
I ran into a somewhat contrived use-case, but I have a fairly strong YNGNI feeling of this aspect of the code. That said, I don't think it hurts understanding too much as is.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001731756)
Curious to know this contrived use case, I also think it's a bit of an overkill in a previous comment https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2618143580, but @sipa made a compelling argument for it https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2666866811

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2734449766)
@ajtowns Responding to some of the more conceptual comments

* I'm indeed not entirely happy with the cycles between `TxGraphImpl` and `Cluster`, though there is a design concern that isn't really materialized in the current code yet: `Cluster` may become a virtual class with multiple implementations for the purpose of memory usage (e.g., there could be a specialized `SingletonCluster` which just stores a single transaction, and no DepGraph, linearization, ..., avoiding several vectors and the
...

💬 instagibbs commented on pull request "BIP-119 (OP_CHECKTEMPLATEVERIFY) (no activation)":
(https://github.com/bitcoin/bitcoin/pull/31989#issuecomment-2734450210)
I suggest a regtest-only deployment is defined so functional testing can be re-instated. Currently there is zero consensus coverage at the functional level from what I can tell: https://github.com/bitcoin/bitcoin/pull/31989/commits/4c5790105e7fb3a2cd207403af769fbbccd7296d

⚠️ polespinasa opened an issue: "Migrate from BTC/kvB to sat/vB on RPC and startup options"
(https://github.com/bitcoin/bitcoin/issues/32093)
Most of the RPC and Startup options use BTC/kvB while the "standardized" units by users and other softwares is sats/vB.
Note that updating to sat/vB can be backward incompatible in some cases.

There are two approaches that can be followed:
- Update all to sat/vB even if that is not backward compatible (all in the same release). This has been done before with `bumpfee` and `psbtbumpfee`, before v0.21 were using BTC/kvB and after moved to sats/vB.
- Add an option to use sats/vB but don't remove B
...

💬 yancyribbens commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r2001882084)
The sentence used to end at `they are selected.` Now it's a very long run on sentence. It would be better to break it into two sentences or shorten it.

💬 maflcko commented on pull request "test: avoid disk space warning for non-regtest":
(https://github.com/bitcoin/bitcoin/pull/32057#issuecomment-2734583144)
lgtm ACK 20fe41e9e83d510fd467f5a999d55a614b16ef89

💬 maflcko commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2001937113)
> don't mention anything about `-O` flags.

That is because they don't matter, as explained in the first line of the docs and in https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1993037907.

I also confirmed this locally and couldn't find any "bogus line numbers".

If you find any bugs, it would be good to provide exact steps to reproduce, so that the bug can be reported upstream and fixed.

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#issuecomment-2734675004)
> @hodlinator I'll wait for you to give the ack/review on this, and once you are happy that your feedback is fully addressed, I'll move it out of draft

Can't promise I won't find any more issues, but this turned out surprisingly well IMO. Thanks for taking the time to work on it!

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r1999832708)
nit: Unrelated to this change, but would be nice to avoid "shadowing".
```suggestion
let corpora_dir = Path::new(args.get(2).ok_or(exit_help("Must set fuzz corpora dir"))?);
```

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001916136)
Unrelated, but while looking for faults with this PR and trying to trigger errors, I got distracted into making this run in parallel. Earlier version triggered OOM-killer and this one is still in a super-unbaked state. (Output should be buffered and sequenced, parallization logic should be refined, file naming uniqueness-approach is bad, etc).

<details><summary>diff</summary>

```diff
diff --git a/contrib/devtools/deterministic-fuzz-coverage/src/main.rs b/contrib/devtools/deterministic-fuz
...

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001921947)
This is slightly boilerplate-y but it's a price I'm willing to pay for the rest of the diff.

Might be less annoying to have at the bottom of the file so it's out of the way?

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r1999707442)
info: Would have expected:
```suggestion
_ => return Err(exit_help(&format!("The tool {} is not installed", tool))),
```
`?` feels like returning is an open question, but after `Err()` it's always true... maybe this is still idiomatic though. `?` in other places implies a *possible* `return`, so probably just takes some getting used to.

💬 hodlinator commented on pull request "contrib: Make deterministic-coverage error messages more readable":
(https://github.com/bitcoin/bitcoin/pull/32074#discussion_r2001944320)
Tempting to suggest
```suggestion
struct AppError(String);
```
But it does add a fair amount of noise where it's instantiated, so I understand we might want to go a bit less type-safe for these tools.

🤔 fjahr reviewed a pull request: "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data"
(https://github.com/bitcoin/bitcoin/pull/31910#pullrequestreview-2695499527)
tACK 63b534f97e591d4e107fd5148909852eb2965d27

Reviewed the code and played around with the fuzzer changes a bit.

💬 fjahr commented on pull request "qa: fix an off-by-one in utxo snapshot fuzz target and sanity check its snapshot data":
(https://github.com/bitcoin/bitcoin/pull/31910#discussion_r2001560332)
Hm, this now has the exact same text here as `MineBlock` above and in general it's doesn't explain what happens internally. I think there should be something here or above that makes clear what the difference is. Maybe add something like "Expects the block submitted to have valid PoW." or so.

🤔 ryanofsky reviewed a pull request: "refactor: Improve assumeutxo state representation"
(https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2691157663)
Rebased d09b82156ced5d543d08226e8d7b8fda2e0ec532 -> 467528960689c2913c101ef75bc833e8f04bd0f3 ([`pr/cstate.9`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.9) -> [`pr/cstate.10`](https://github.com/ryanofsky/bitcoin/commits/pr/cstate.10), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/cstate.9-rebase..pr/cstate.10)) due to conflict 31961 and implementing various suggestions.

re: https://github.com/bitcoin/bitcoin/pull/30214#pullrequestreview-2685423528

Thanks for the re
...