💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2433686891)
It would be nice to expose these through the `getmempoolinfo` RPC.
```diff
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index 492693c9cef..1b62c74f0b3 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -812,6 +812,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
ret.pushKV("fullrbf", true);
ret.pushKV("permitbaremultisig", pool.m_opts.permit_bare_multisig);
ret.pushKV("maxdatacarriersize", pool.m_opts.max_datacarrier_bytes.value_or(0));
+
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2433686891)
It would be nice to expose these through the `getmempoolinfo` RPC.
```diff
diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp
index 492693c9cef..1b62c74f0b3 100644
--- a/src/rpc/mempool.cpp
+++ b/src/rpc/mempool.cpp
@@ -812,6 +812,8 @@ UniValue MempoolInfoToJSON(const CTxMemPool& pool)
ret.pushKV("fullrbf", true);
ret.pushKV("permitbaremultisig", pool.m_opts.permit_bare_multisig);
ret.pushKV("maxdatacarriersize", pool.m_opts.max_datacarrier_bytes.value_or(0));
+
...
💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2434014043)
I don't know why we sometimes return a reference and sometimes just copy it. But I think I'll try to return a view here where possible.
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2434014043)
I don't know why we sometimes return a reference and sometimes just copy it. But I think I'll try to return a view here where possible.
🤔 mzumsande reviewed a pull request: "p2p: Allow block downloads from peers without snapshot block after assumeutxo validation"
(https://github.com/bitcoin/bitcoin/pull/33604#pullrequestreview-3342475616)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/33604#pullrequestreview-3342475616)
Concept ACK
🤔 instagibbs reviewed a pull request: "Cluster mempool"
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3340512731)
reviewed through 8cdd7bb11adeaeb4709c670bef4b57362cfbebcb
focusing on logic this time around, only glanced at tests
(https://github.com/bitcoin/bitcoin/pull/33629#pullrequestreview-3340512731)
reviewed through 8cdd7bb11adeaeb4709c670bef4b57362cfbebcb
focusing on logic this time around, only glanced at tests
💬 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
(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?
(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`
(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?
(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.
```
(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.
```
(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
...
(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
(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
(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.
(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
(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
(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!
(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?
(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.)
(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.
(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.