💬 willcl-ark commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3513567925)
My build:

```
x86_64
05fc5cae41b19bbfdec5942e334e7b7d78ee0406ba69c221381b0b07b0a9e886 guix-build-f06c6e189831/output/aarch64-linux-gnu/SHA256SUMS.part
961613408c4d0b3b169488b43edc838135be0b712740b4015457f4f2808e103b guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu-debug.tar.gz
906437c316d50e1f60fdfe2098fe72f917be109a68a2103e915183ed4fe01947 guix-build-f06c6e189831/output/aarch64-linux-gnu/bitcoin-f06c6e189831-aarch64-linux-gnu.tar.gz
73a257f64eb6
...

👍 willcl-ark approved a pull request: "guix: build for Linux HOSTS with `-static-libgcc`"
(https://github.com/bitcoin/bitcoin/pull/33181#pullrequestreview-3444844067)
ACK f06c6e18983139dd63873b3537d2f87b8c6ec752

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511014559)
In commit "Allow moving CTxMemPoolEntry objects, disallow copying"

Ultranit: `push_back` instead of `emplace_back` (no real downside to using emplace_back, but push_back makes it more clear that nothing is really being constructed in-place).

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511137437)
In commit "Add sigops adjusted weight calculator"

This can be done more accurately (avoiding the rounding):
```c++
return {feerate.fee * WITNESS_SCALE_FACTOR, feerate.size};
```

Would that be desirable? It's used inside `BlockAssembler::addChunks` where it would be useful I think, but also in `TxMempool` where it's less clear to me if sticking to the old rounding-up behavior may be necessary.

One potential pitfall is that the resulting `size` member variable of the returned `FeePerVS
...

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511123334)
In commit "Create a txgraph inside CTxMemPool"

Based on the `Linearize64TxWorstCase15000Iters` and `Linearize64TxWorstCase5000Iters` benchmarks, it seems one iteration costs around:
* Ryzen 5950X: ~41 ns
* Ryzen 9980X: ~30 ns

So 1200-1700 is probably a more reasonable range here to get to the "~50 μs per cluster" number.

In #32545 the cost metric is changed, and aims for ~2 ns per "iteration", so then 25000 may be more realistic.

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511204657)
In commit "squashme: fix locking so that mempool lock is held through subpackage destruction"

It seems strange to use the `AndCleanup` version here (both for `MultipleTransactions` here and `SingleTransaction` below), for two reasons:
* The `AndCleanup` version grabs its own `m_pool.cs` lock, but is called here in a place that already holds that lock.
* The call here in `AcceptSubPackage` is followed by a `ClearSubPackageState();` below, which is already invoked inside the `AndCleanup` vers
...

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511170420)
In commit "Add transactions to txgraph, but without cluster dependencies"

There should always exist a txgraph staging when a `ChangeSet` is destroyed, right?

```c++
Assume(m_pool->m_txgraph->HaveStaging());
Assume(m_pool->m_have_changeset);
m_pool->m_txgraph->AbortStaging();
m_pool->m_have_changeset = false;
```

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511144640)
In commit "Add transactions to txgraph, but without cluster dependencies"

Could this repeat of the adjustment be avoided by swapping? I.e.,
```c++
mapTx.modify(it, [&nFeeDelta](CTxMemPoolEntry& e) { e.UpdateModifiedFee(nFeeDelta); });
m_txgraph->SetTransactionFee(*it, it->GetModifiedFee());
```

If so, that would seem more natural.

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511253385)
In commit "Do not allow mempool clusters to exceed configured limits"

Given the `Assume(!m_dependencies_process);` above, is the **re**process comment correct here? It seems they're expected to not have been processed at all here.

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511307703)
In commit "Select transactions for blocks based on chunk feerate"

This ought to be possible without a conversion to sats, something like:
```c++
if (chunk_feerate << m_options.blockMinFeeRate.GetFeePerVSize()) {
```
(where `FeePerVSize CFeeRate::GetFeePerVSize()` is a new member function to get the underlying fraction object)

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511155021)
In commit "Add transactions to txgraph, but without cluster dependencies"

Possibly the same can be done here as I suggested above, but with a separate call:
```c++
TxGraph::Ref ref(m_pool->m_txgraph->AddTransaction(FeePerWeight(fee, GetSigOpsAdjustedWeight(GetTransactionWeight(*tx), sigops_cost, ::nBytesPerSigOp))));
auto newit = m_to_add.emplace(std::move(ref), tx, fee, time, entry_height, entry_sequence, spends_coinbase, sigops_cost, lp).first;
if (delta) {
m_to_add.modify(newit, [
...

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511560296)
In commit "Reimplement GetTransactionAncestry() to not rely on cached data"

Nit, it seems cleaner to make these outputs rather than pass-by-reference inputs:
```c++
std::tuple<size_t, size_t, CAmount> CalculateAncestorData(const CTxMemPoolEntry& entry) const
```

which can then by used with structured bindings like (next commit):
```c++
auto [ancestor_count, ancestor_size, ancestor_fees] = CalculateAncestorData(e);
```

💬 willcl-ark commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3513602175)
ACK f06c6e18983139dd63873b3537d2f87b8c6ec752

> i'm wondering if this build uses more than 38 GiB, my guix build just failed for this PR. it just has slightly more than 38 GiB available so it passes the check but just failed on no more space on disk.
>
> Will do another run to check :)

I don't know exactly how much space it needs, but my `/gnu` partition is at 36.8GiB total after this build, which certainly seems close to your figure...

💬 waketraindev commented on pull request "Prevent re-execution of sensitive commands from console history":
(https://github.com/bitcoin-core/gui/pull/909#issuecomment-3513665716)
Extended the blocking filter to include transaction-related RPCs `send`, `sendall`, `sendmany` and
`sendtoaddress` as these can also cause unintended effects.

Test covereage added for them

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511867396)
Maybe misunderstanding, but this just means it's purely an optimzation?

If it's not in the main graph, it still is able to gather ancestors via more expensive mempool queries.

💬 JeremyRubin commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r2511884930)
I would change this to read "Unknown Signet Challenge Version %d, versions [0x01] supported"

and read the first field as a VarInt or CompactSize.

💬 JeremyRubin commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r2511885878)
this should come _after_ the version check

💬 sipa commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511906918)
@instagibbs Oh, you're right - the mutation just causes the code to always use the fallback path, which needs to exist for non-mempool transactions anyway.

💬 JeremyRubin commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#discussion_r2511906920)
i would probably make parsing work by doing a SERIALIZE_METHODS on a struct, and have any extensions in the future be done by failing to deserialize unknown versions...

💬 JeremyRubin commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-3513831449)
Concept ACK.

We use something like this for testing and it's quite helpful.

it's nice being able to ship a parameter pack in a single hex for a signet.

I think the future version flexibility can be simplified, reducing @Sjors concern of the wrapper complexity. A good ol' serialize_methods chunk can probably make this trivial :)

e.g.:


```
SERIALIZE_METHODS(WrappedSignetChallenge, obj)
{
READWRITE(obj.version);
if (obj.version == 0) {
READW
...