💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2432836478)
6c73e4744837a7dc138a9177df3a48f30a1ba6c1

Had to double-check the claim on space usage, seems to hold

https://www.boost.org/doc/libs/1_73_0/libs/multi_index/doc/performance.html#:~:text=On%20the%20other%20hand%2C%20the,N%2D1)p%2C%20where

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2432642810)
51430680ecb722e1d4ee4a26dac5724050f41c9e

The why of this is unclear to me through 8cdd7bb11adeaeb4709c670bef4b57362cfbebcb , worth reordering or expanding commit message?

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2433860250)
ae1ac5410383e57ed16867580a5fd355a46953de

Seems more self-contained to start/stop the builder inside `addChunks`

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2433884234)
ae1ac5410383e57ed16867580a5fd355a46953de

if we're essentially only using this, why not inline `chunk_feerate` instead?

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434013019)
2aad9f01e514b77538eb780138a8e52484eaf5a8

is this right? If so this warrants a test (note to self and others)
```Suggestion
// If we can't calculate a feerate, it's because the cluster size limits were hit, and we may want to try package RBF.
```

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2433318113)
5a388c0d595b2318fea4b1dce977e2d5ff1abc48

```Suggestion
// Check if the transactions would exceed the cluster size limit.
```

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2433925096)
ae1ac5410383e57ed16867580a5fd355a46953de

Was getting confused with the two vectors, are they even needed?

```
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index 52103bda82..1bfe771444 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -206,8 +206,8 @@ bool BlockAssembler::TestPackage(FeePerWeight package_feerate, int64_t packageSi
// Perform transaction-level checks before adding to block:
// - transaction finality (locktime)
-bool BlockAssembler::TestPackageTran
...

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434078457)
2aad9f01e514b77538eb780138a8e52484eaf5a8

Didn't carefully look at this test case, but we should make sure we are testing the case of package RBF conflicting against clusters of up to count 64

Looks like the test is still only conflicting with size 2

💬 instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2433939831)
ae1ac5410383e57ed16867580a5fd355a46953de

nit: I think renaming `TestPackage` to `TestPackageBlockLimits` or something might help readability going forward

💬 willcl-ark commented on pull request "doc: document capnproto and libmultiprocess deps in 29.x":
(https://github.com/bitcoin/bitcoin/pull/33623#discussion_r2434100605)
Nice, thanks!

I double-checked the tag points to the correct commit and tested a depends build using your package suggestion which all worked fine.

Have therefore gone ahead with updating the depends package and _dependencies.md_ in bf236d7a8af0826efc39a04113b79f96fe05f0c7 as you suggest.

🤔 5girlstaken-arch reviewed a pull request: "ci: Properly include $FILE_ENV in DEPENDS_HASH"
(https://github.com/bitcoin/bitcoin/pull/33581#pullrequestreview-3342555102)
00000020
a6f5e4d3c2b1a0f9e8d7c6b5a4938271605f4e3d2c1b0a000000000000000000
04f15068366fba1de4c323bb6ca4fe58c7d968d81688abf68380bddf46fd4ef0
a08fee68
6a5d0f17
00010007

💬 5girlstaken-arch commented on pull request "ci: Properly include $FILE_ENV in DEPENDS_HASH":
(https://github.com/bitcoin/bitcoin/pull/33581#issuecomment-3408539754)
3L3WW6b3c9ArSc35geweMZan2YpYkfCXAP

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434195677)
Looks good, will include -- thank you for the test coverage!

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434207347)
Ok it's been a really long time since I've thought about this, but I believe the issue was that I needed epoch markers to be movable so that I could make CTxMemPoolEntry movable.

I don't exactly remember why CTxMemPoolEntry needs to be movable but I believe it has something to do with inheriting from TxGraph::Ref. I guess Ref's can't be copied, but they can be moved?

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434211174)
Actually, in fairness I think there might be some platforms where the space usage may be different -- I think I ran into that at some point in the history of this PR with one of our CI jobs, but I don't recall the details now. (Even still, I think saving the map lookup is likely worth it.)

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434216116)
I think the reason I didn't is probably because `addChunks()` has multiple places where it returns, so it seemed a little messy to have to invoke `StopBlockBuilding()` within that function. But if you think it's worth it, I can rework the function and push the calls down into it.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434220844)
In this commit, `chunk_feerate.fee` is used further down, but this gets cleaned up further in #33591 with the introduction of `CFeeRate::GetFeePerVSize`, at which point this could be inlined. I'll make that change in #33591.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434234855)
Will take this doc change in #33591.

📝 fjahr opened a pull request: "wallet: Expand MuSig test coverage and follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33636)
This is a follow-up to #29675 and primarily adds test coverage for some of the most prominent failure cases in the first commit.

The following commits address a few left-over nit comments that didn't make it in before merge.

💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2434241333)
Good point! Taking your change.