💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512255248)
Redid this and `CalculateDescendantData` as you suggested.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512255248)
Redid this and `CalculateDescendantData` as you suggested.
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512255792)
Should be fixed now in that commit.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512255792)
Should be fixed now in that commit.
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512256794)
Done.
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512256794)
Done.
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512270361)
I rewrote the commit message -- should be a bit better now.
(Also, it's worth noting that mini_miner is only used by the wallet, and we may want the wallet's behavior to change more slowly than with the next release, as it will take time for new software to be widely deployed.)
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512270361)
I rewrote the commit message -- should be a bit better now.
(Also, it's worth noting that mini_miner is only used by the wallet, and we may want the wallet's behavior to change more slowly than with the next release, as it will take time for new software to be widely deployed.)
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3514262272)
I believe I've responded to all of @sipa's comments, with the exception of the open issue around where to lock the mempool in `validation.cpp`.
(https://github.com/bitcoin/bitcoin/pull/33629#issuecomment-3514262272)
I believe I've responded to all of @sipa's comments, with the exception of the open issue around where to lock the mempool in `validation.cpp`.
💬 sipa commented on pull request "doc: corrected lockunspent rpc quoting":
(https://github.com/bitcoin/bitcoin/pull/31275#issuecomment-3514327614)
I wanted to verify if all invocations of `HelpExampleRpc` produce valid JSON, so I tested by adding a `UniValue::read` inside the function. The following RPCs all fail it still (with this PR):
* `getblockfrompeer`
* `importmempool`
* `getindexinfo`
* `addnode`
* `addconnection`
* `sendmsgtopeer`
* `createwalletdescriptor`
* `restorewallet`
* `listlabels`
* `loadwallet`
* `unloadwallet`
I think this means we either need to:
* Get rid of the `curl` example codes in the RPC help text
...
(https://github.com/bitcoin/bitcoin/pull/31275#issuecomment-3514327614)
I wanted to verify if all invocations of `HelpExampleRpc` produce valid JSON, so I tested by adding a `UniValue::read` inside the function. The following RPCs all fail it still (with this PR):
* `getblockfrompeer`
* `importmempool`
* `getindexinfo`
* `addnode`
* `addconnection`
* `sendmsgtopeer`
* `createwalletdescriptor`
* `restorewallet`
* `listlabels`
* `loadwallet`
* `unloadwallet`
I think this means we either need to:
* Get rid of the `curl` example codes in the RPC help text
...
💬 sdaftuar commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512305384)
See also https://github.com/bitcoin/bitcoin/pull/33591/commits/7e715bc2749378f92284a1924f2079492adccf3f
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512305384)
See also https://github.com/bitcoin/bitcoin/pull/33591/commits/7e715bc2749378f92284a1924f2079492adccf3f
📝 ryanofsky opened a pull request: "kernel: Improve logging API"
(https://github.com/bitcoin/bitcoin/pull/33847)
Simplify kernel logging API and try to connect it to the rest of the kernel API by letting log options be set on `Logger` objects directly and `Logger` objects to be associated with `Context` objects directly. Also drop `logging_disable()` function and avoid having library accumulate log messages in a 1MB buffer by default.
Changes are intended to make the API a little less surprising and more future proof. Rationale is described in some more detail the commit message.
This change is not b
...
(https://github.com/bitcoin/bitcoin/pull/33847)
Simplify kernel logging API and try to connect it to the rest of the kernel API by letting log options be set on `Logger` objects directly and `Logger` objects to be associated with `Context` objects directly. Also drop `logging_disable()` function and avoid having library accumulate log messages in a 1MB buffer by default.
Changes are intended to make the API a little less surprising and more future proof. Rationale is described in some more detail the commit message.
This change is not b
...
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512026907)
in 6b791cc4ee9: this arg is not necessary
```suggestion
self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000', f'-limitancestorcount={CUSTOM_ANCESTOR_COUNT}', f'-limitdescendantcount={CUSTOM_DESCENDANT_COUNT}', "-limitclustersize=1000"]]
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512026907)
in 6b791cc4ee9: this arg is not necessary
```suggestion
self.extra_args = [['-limitdescendantsize=1000', '-limitancestorsize=1000', f'-limitancestorcount={CUSTOM_ANCESTOR_COUNT}', f'-limitdescendantcount={CUSTOM_DESCENDANT_COUNT}', "-limitclustersize=1000"]]
```
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512035648)
nitty nity in 6b791cc4ee9eda5cccc310e3efb869af36057935: it feels nice and future-proof to have annotations like
```suggestion
m_txgraph->AddDependency(/*parent=*/*it, /*child=*/*childIter);
```
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512035648)
nitty nity in 6b791cc4ee9eda5cccc310e3efb869af36057935: it feels nice and future-proof to have annotations like
```suggestion
m_txgraph->AddDependency(/*parent=*/*it, /*child=*/*childIter);
```
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511991832)
nit in cf1bcbbb8e1a4cb72cd37a44dcd8811db8a1958b: `clustercount` would be a better name. Looks like `*size` is used to mean aggregate size here in `ancestorsize`
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2511991832)
nit in cf1bcbbb8e1a4cb72cd37a44dcd8811db8a1958b: `clustercount` would be a better name. Looks like `*size` is used to mean aggregate size here in `ancestorsize`
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512031063)
in 6b791cc4ee9eda5cccc310e3efb869af36057935
```suggestion
/** Check if any cluster limits are exceeded. Returns true if pass, false if fail.*/
bool CheckMemPoolPolicyLimits();
````
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512031063)
in 6b791cc4ee9eda5cccc310e3efb869af36057935
```suggestion
/** Check if any cluster limits are exceeded. Returns true if pass, false if fail.*/
bool CheckMemPoolPolicyLimits();
````
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512203125)
187f78991fad3c521e55508a9d1901d3767c2f89: I'm not sure if there is coverage for the case we were trying to prevent with the old Rule 2:
```diff
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index 288a501d53e..f29e7a77803 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -13,6 +13,7 @@ from test_framework.messages import (
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512203125)
187f78991fad3c521e55508a9d1901d3767c2f89: I'm not sure if there is coverage for the case we were trying to prevent with the old Rule 2:
```diff
diff --git a/test/functional/feature_rbf.py b/test/functional/feature_rbf.py
index 288a501d53e..f29e7a77803 100755
--- a/test/functional/feature_rbf.py
+++ b/test/functional/feature_rbf.py
@@ -13,6 +13,7 @@ from test_framework.messages import (
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import (
...
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512379818)
Good catch 👍
I'm very very happy to save this for a followup, but since the repeated locking and cleanups aren't ideal, I thought I'd give another approach a shot: https://github.com/glozow/bitcoin/tree/2025-10-mempoolaccept-layers
It keeps two routes, `AcceptSingleTransaction` and `AcceptPackage` (which allows packages of size 1). I had to make `m_pool` a public member of `CTxMemPool` (that felt bad at first, but the caller needs to provide it, so it seems reasonable to have access?).
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512379818)
Good catch 👍
I'm very very happy to save this for a followup, but since the repeated locking and cleanups aren't ideal, I thought I'd give another approach a shot: https://github.com/glozow/bitcoin/tree/2025-10-mempoolaccept-layers
It keeps two routes, `AcceptSingleTransaction` and `AcceptPackage` (which allows packages of size 1). I had to make `m_pool` a public member of `CTxMemPool` (that felt bad at first, but the caller needs to provide it, so it seems reasonable to have access?).
...
💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512390118)
Good catch 👍
I'm very very happy to save this for a followup, but since the repeated locking and cleanups aren't ideal, I thought I'd give another approach a shot: https://github.com/glozow/bitcoin/tree/2025-10-mempoolaccept-layers
It keeps two routes, AcceptSingleTransaction and AcceptPackage (which allows packages of size 1). I had to make m_pool a public member of CTxMemPool (that felt bad at first, but the caller needs to provide it, so it seems reasonable to have access?).
One
...
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2512390118)
Good catch 👍
I'm very very happy to save this for a followup, but since the repeated locking and cleanups aren't ideal, I thought I'd give another approach a shot: https://github.com/glozow/bitcoin/tree/2025-10-mempoolaccept-layers
It keeps two routes, AcceptSingleTransaction and AcceptPackage (which allows packages of size 1). I had to make m_pool a public member of CTxMemPool (that felt bad at first, but the caller needs to provide it, so it seems reasonable to have access?).
One
...
🤔 polespinasa reviewed a pull request: "rpc: Optionally print feerates in sat/vb"
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3445589098)
Rebased following @w0xlt suggestions :)
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3445589098)
Rebased following @w0xlt suggestions :)
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2512314414)
Yes maybe it's a better approach.
Moved the function into `core_write.cpp` and `core_io.h` similar to function `ValueFromAmount`.
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2512314414)
Yes maybe it's a better approach.
Moved the function into `core_write.cpp` and `core_io.h` similar to function `ValueFromAmount`.
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2512303937)
True!
I thought that there was the intention to add all values and not only `relayfee` and `incrementalfee` here as the comment ` // Those fields can be deprecated, to be replaced by the getmempoolinfo fields` seems suggest.
But seeing https://github.com/bitcoin/bitcoin/pull/25648/files#r935563077 I think I just misunderstood and the intention was to maybe remove them in the future and just use `getmempool` rpc to know that info.
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2512303937)
True!
I thought that there was the intention to add all values and not only `relayfee` and `incrementalfee` here as the comment ` // Those fields can be deprecated, to be replaced by the getmempoolinfo fields` seems suggest.
But seeing https://github.com/bitcoin/bitcoin/pull/25648/files#r935563077 I think I just misunderstood and the intention was to maybe remove them in the future and just use `getmempool` rpc to know that info.
💬 polespinasa commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2512312634)
yup, good catch tanks! :)
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2512312634)
yup, good catch tanks! :)
💬 151henry151 commented on pull request "Check required interfaces before generating manpages":
(https://github.com/bitcoin/bitcoin/pull/33828#issuecomment-3514498164)
Thanks for taking a look, fanquake.
I noticed #33085 had gone quiet for a little over three weeks with the review concerns still open, so I put this together.
In #33085 the script pulls component flags from `test/config.ini` (adding checks for CLI, chainstate, fuzz, USDT tracepoints, etc.), but that file only exists when tests are configured and most of those toggles don’t affect the manpages, so the script breaks in release-style builds. This branch reads `CMakeCache.txt`, the authorita
...
(https://github.com/bitcoin/bitcoin/pull/33828#issuecomment-3514498164)
Thanks for taking a look, fanquake.
I noticed #33085 had gone quiet for a little over three weeks with the review concerns still open, so I put this together.
In #33085 the script pulls component flags from `test/config.ini` (adding checks for CLI, chainstate, fuzz, USDT tracepoints, etc.), but that file only exists when tests are configured and most of those toggles don’t affect the manpages, so the script breaks in release-style builds. This branch reads `CMakeCache.txt`, the authorita
...