rpc: followups for 33106 #33189

glozow · 2025-08-14T18:12:56Z

Followups from #33106:

policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)
policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106 (comment)

DrahtBot · 2025-08-14T18:13:00Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33189.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	davidgumberg, instagibbs, ajtowns

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

No conflicts as of last run.

DrahtBot · 2025-08-14T21:12:18Z

🚧 At least one of the CI tasks failed.
_{Task multiprocess, i686, DEBUG: https://github.com/bitcoin/bitcoin/runs/48115164025}
_{LLM reason (✨ experimental): The CI failure is caused by a compilation error in mining.cpp due to an unimplemented pure virtual method in an abstract class marked 'final', leading to build failure.}

Hints

Try to run the tests locally, according to the documentation. However, a CI failure may still
happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being
incompatible with the current code in the target branch). If so, make sure to rebase on the latest
commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the
affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

fanquake · 2025-08-15T13:53:25Z

https://github.com/bitcoin/bitcoin/pull/33189/checks?check_run_id=48115164025:

[14:51:10.997] In file included from /ci_container_base/src/ipc/capnp/mining-types.h:11:
[14:51:10.997] /ci_container_base/ci/scratch/build-i686-pc-linux-gnu/src/ipc/capnp/mining.capnp.proxy.h:317:8: error: abstract class is marked 'final' [-Werror,-Wabstract-final-class]
[14:51:10.997]   317 | struct ProxyClient<ipc::capnp::messages::Mining> final : public ProxyClientCustom<ipc::capnp::messages::Mining, interfaces::Mining>
[14:51:10.997]       |        ^
[14:51:10.997] /ci_container_base/src/interfaces/mining.h:137:22: note: unimplemented pure virtual method 'GetBlockMinFee' in 'ProxyClient'
[14:51:10.997]   137 |     virtual CFeeRate GetBlockMinFee() const = 0;
[14:51:10.997]       |                      ^
[14:51:10.997] 1 error generated.

Clarify that the purpose of some parameters are to ensure identical transactions are not created. Also, strengthen the test to catch these cases.

achow101 · 2025-08-15T22:36:40Z

test/functional/feature_rbf.py

@@ -594,7 +594,7 @@ def test_incremental_relay_feerates(self):
        node = self.nodes[0]
        for incremental_setting in (0, 5, 10, 50, 100, 234, 1000, 5000, 21000):
            incremental_setting_decimal = incremental_setting / Decimal(COIN)
-            self.log.info(f"-> Test -incrementalrelayfee={incremental_setting_decimal:.8f}sat/kvB...")
+            self.log.info(f"-> Test -incrementalrelayfee={incremental_setting:.8f}sat/kvB...")


In 636fa21 "test fixups"

Can drop the .8f, otherwise these are printed with 8 decimal places of 0's that are not necessary.

personal opinion only but printing 8 places always to easily eyeball the rate is better, so -0 on this suggestion

None of the settings have decimals though

-> Test -incrementalrelayfee=100.00000000sat/kvB... doesn't seem very useful indeed.

mining_basic reports -> Test -blockmintxfee=0.00000500 (500 sat/kvB)... which seems strictly better, as it gives a value that can actually be used with the config param as well as a sats figure?

davidgumberg

ACK daa40a3

davidgumberg · 2025-08-15T18:57:33Z

src/rpc/mining.cpp

@@ -429,6 +429,7 @@ static RPCHelpMan getmininginfo()
                        {RPCResult::Type::STR_HEX, "target", "The current target"},
                        {RPCResult::Type::NUM, "networkhashps", "The network hashes per second"},
                        {RPCResult::Type::NUM, "pooledtx", "The size of the mempool"},
+                        {RPCResult::Type::STR_AMOUNT, "blockmintxfee", "Minimum feerate of packages selected for block inclusion in " + CURRENCY_UNIT + "/kvB"},


Maybe a release note?

small note with explanation on how miners can use it would be nice since some miners are opting out of mining such low feerates for now

davidgumberg · 2025-08-15T19:01:43Z

test/functional/feature_rbf.py

@@ -583,7 +583,7 @@ def test_replacement_relay_fee(self):
        tx = self.wallet.send_self_transfer(from_node=self.nodes[0])['tx']

        # Higher fee, higher feerate, different txid, but the replacement does not provide a relay
-        # fee conforming to node's `incrementalrelayfee` policy of 1000 sat per KB.
+        # fee conforming to node's `incrementalrelayfee` policy of 100 sat per KB.


nit:

Suggested change

# fee conforming to node's `incrementalrelayfee` policy of 100 sat per KB.

# fee conforming to node's `incrementalrelayfee` policy of 100 sat per kvB.

instagibbs

ACK daa40a3

instagibbs · 2025-08-20T12:30:32Z

test/functional/feature_rbf.py

@@ -594,7 +594,7 @@ def test_incremental_relay_feerates(self):
        node = self.nodes[0]
        for incremental_setting in (0, 5, 10, 50, 100, 234, 1000, 5000, 21000):
            incremental_setting_decimal = incremental_setting / Decimal(COIN)
-            self.log.info(f"-> Test -incrementalrelayfee={incremental_setting_decimal:.8f}sat/kvB...")
+            self.log.info(f"-> Test -incrementalrelayfee={incremental_setting:.8f}sat/kvB...")


personal opinion only but printing 8 places always to easily eyeball the rate is better, so -0 on this suggestion

instagibbs · 2025-08-20T12:38:31Z

src/rpc/mining.cpp

@@ -429,6 +429,7 @@ static RPCHelpMan getmininginfo()
                        {RPCResult::Type::STR_HEX, "target", "The current target"},
                        {RPCResult::Type::NUM, "networkhashps", "The network hashes per second"},
                        {RPCResult::Type::NUM, "pooledtx", "The size of the mempool"},
+                        {RPCResult::Type::STR_AMOUNT, "blockmintxfee", "Minimum feerate of packages selected for block inclusion in " + CURRENCY_UNIT + "/kvB"},


small note with explanation on how miners can use it would be nice since some miners are opting out of mining such low feerates for now

l0rinc · 2025-08-21T17:34:38Z

Not everyone knows what "33106" is. To avoid it being interpreted as trying to hide something, could you please detail the changes in the PR title and description to help the reviewers and provide full transparency?

dergoegge · 2025-08-22T10:36:01Z

Not everyone knows what "33106" is

"33106" refers to the pull request with that number: #33106, which is also linked in the PR description (only 12 times). It's common practice to link to a related PR like this (e.g. for a followup) in the PR title. Hope this helps.

l0rinc · 2025-08-22T19:28:44Z

Hope this helps.

Why do you think that exactly? You didn't say anything new, my comment was about making it obvious from the PR's title what it's about (without having to open it) - and not only to ones who already know what 33106 was about.

maflcko · 2025-08-28T14:04:56Z

Could remove the outdated comment as well, after 3eab8b7?

diff --git a/test/functional/test_framework/mempool_util.py b/test/functional/test_framework/mempool_util.py
index 9ffd934f5a..3c4609c0b4 100644
--- a/test/functional/test_framework/mempool_util.py
+++ b/test/functional/test_framework/mempool_util.py
@@ -57,8 +57,7 @@ def fill_mempool(test_framework, node, *, tx_sync_fun=None):
     """Fill mempool until eviction.
 
     Allows for simpler testing of scenarios with floating mempoolminfee > minrelay
-    Requires -maxmempool=5 and assumes -minrelaytxfee
-    is 1 sat/vbyte.
+    Requires -maxmempool=5.
     To avoid unintentional tx dependencies, the mempool filling txs are created with a
     tagged ephemeral miniwallet instance.
     """

ajtowns · 2025-08-28T16:45:10Z

"33106" refers to the pull request with that number: #33106, which is also linked in the PR description (only 12 times). It's common practice to link to a related PR like this (e.g. for a followup) in the PR title.

Can we please make this not common practice anymore? Memorising PR numbers is no fun. "Followups for min fee changes" would have been fine.

ajtowns · 2025-08-28T16:54:36Z

ACK daa40a3 ; cursory review, seems reasonable

glozow added the RPC/REST/ZMQ label Aug 14, 2025

glozow mentioned this pull request Aug 14, 2025

policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee #33106

Merged

DrahtBot added the CI failed label Aug 14, 2025

DrahtBot mentioned this pull request Aug 15, 2025

wallet, rpc: add v3 transaction creation and wallet support #32896

Merged

9 tasks

glozow force-pushed the 2025-08-minrelay branch 2 times, most recently from a0ccaf7 to 0dba9eb Compare August 15, 2025 15:29

glozow marked this pull request as ready for review August 15, 2025 16:52

glozow force-pushed the 2025-08-minrelay branch from 0dba9eb to debb06c Compare August 15, 2025 17:27

glozow added 4 commits August 15, 2025 13:36

[rpc] expose blockmintxfee via getmininginfo

9169a50

test fixups

636fa21

test fixup for incremental feerate

c568511

Clarify that the purpose of some parameters are to ensure identical transactions are not created. Also, strengthen the test to catch these cases.

doc fixups for 33106

daa40a3

glozow force-pushed the 2025-08-minrelay branch from debb06c to daa40a3 Compare August 15, 2025 17:36

DrahtBot removed the CI failed label Aug 15, 2025

achow101 reviewed Aug 15, 2025

View reviewed changes

davidgumberg approved these changes Aug 15, 2025

View reviewed changes

fanquake added this to the 30.0 milestone Aug 20, 2025

instagibbs approved these changes Aug 20, 2025

View reviewed changes

achow101 requested review from darosior, ajtowns and ismaelsadeeq August 28, 2025 16:32

fanquake merged commit 6ff2d42 into bitcoin:master Aug 28, 2025
19 checks passed

kevkevinpal mentioned this pull request Aug 28, 2025

rpc: followups for 33106 kevkevinpal/bitcoin#136

Open

	# fee conforming to node's `incrementalrelayfee` policy of 100 sat per KB.
	# fee conforming to node's `incrementalrelayfee` policy of 100 sat per kvB.

rpc: followups for 33106 #33189

rpc: followups for 33106 #33189

Uh oh!

Conversation

glozow commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Aug 14, 2025

Uh oh!

fanquake commented Aug 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidgumberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc commented Aug 21, 2025

Uh oh!

dergoegge commented Aug 22, 2025

Uh oh!

l0rinc commented Aug 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Aug 28, 2025

Uh oh!

ajtowns commented Aug 28, 2025

Uh oh!

ajtowns commented Aug 28, 2025

Uh oh!

Uh oh!

Uh oh!

glozow commented Aug 14, 2025 •

edited

Loading

DrahtBot commented Aug 14, 2025 •

edited

Loading

l0rinc commented Aug 22, 2025 •

edited

Loading