💬 maflcko commented on pull request "test: switch wallet_crosschain.py to signet and drop testnet4":
(https://github.com/bitcoin/bitcoin/pull/32088#issuecomment-2734205095)
lgtm ACK cec14ee47d71d8dd5ca8f90b760d807c3c8933a5 🌰

<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/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK cec14ee47d71d8dd
...

💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#discussion_r2001653317)
How much resources can a (mempool) transaction waste by violating this rule in the last input / output after making the interpreter do a bunch of hashing work?

And is that worse than existing taproot script allows? And would it be better with a restriction on the number of combinations, e.g. if inputs can't refer to other inputs? Or should these bounds checks be done earlier for the whole transaction?

👍 vasild approved a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2695684854)
ACK 4e265debdc0319bbfcea915d9026b33810b810f8

💬 maflcko commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2734250524)
(According to https://github.com/bitcoin/bitcoin/pull/31866#issuecomment-2691174968 the test commits are the same, so any reviewer here can probably also ack the other pull)

💬 vasild commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r2001668320)
The docs at https://clang.llvm.org/docs/SourceBasedCodeCoverage.html don't mention anything about `-O` flags.

🤔 ajtowns reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2694691922)
Preliminary ACK 72a97c0a07ea6e5a95ab37c8d95e1ea02cff8e92

I think it's okay to merge this code:
* as of this PR, it isn't actually used, so should not introduce bugs immediately
* the fuzz testing is pretty good, so there should not be any catastrophic bugs even if it were used
* while maybe the design could be improved, that can be done incrementally after it's merged
* I'm reasonably confident I understand what's going on and can help find/fix bugs/problems if they occur
* I think
...

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001126453)
Most of the `Cluster` methods require a `TxGraphImpl` to keep the quality/staging/mapping things consistent; feels a bit like it might be simpler just to have almost all this code stay in `TxGraphImpl`?

💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r2001315529)
I think this is a close to accurate diagram of the data structures here:


To me, the cycles in that graph seem like a code smell. The `GraphIndex` backrefs make sense (or are unavoidable), as some stable way to refer to each tx is needed. But, I think it would probably be better for a Cluster not to be aware of its quality level or staging level (removing the `m_setindex/m_quality/m_level`
...

💬 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!