rpc: followups for 33106

glozow commented at 6:12 pm on August 14, 2025: member

Followups from #33106:

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

DrahtBot commented at 6:13 pm on August 14, 2025: contributor

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

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 added the label CI failed on Aug 14, 2025

DrahtBot commented at 9:12 pm on August 14, 2025: contributor

🚧 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.

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 commented at 1:53 pm on August 15, 2025: member

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

0[14:51:10.997] In file included from /ci_container_base/src/ipc/capnp/mining-types.h:11:
1[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]
2[14:51:10.997]   317 | struct ProxyClient<ipc::capnp::messages::Mining> final : public ProxyClientCustom<ipc::capnp::messages::Mining, interfaces::Mining>
3[14:51:10.997]       |        ^
4[14:51:10.997] /ci_container_base/src/interfaces/mining.h:137:22: note: unimplemented pure virtual method 'GetBlockMinFee' in 'ProxyClient'
5[14:51:10.997]   137 |     virtual CFeeRate GetBlockMinFee() const = 0;
6[14:51:10.997]       |                      ^
7[14:51:10.997] 1 error generated.

glozow force-pushed on Aug 15, 2025

glozow marked this as ready for review on Aug 15, 2025

glozow force-pushed on Aug 15, 2025

[rpc] expose blockmintxfee via getmininginfo 9169a50d52

test fixups 636fa219d3

test fixup for incremental feerate

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

c568511e8c

doc fixups for 33106 daa40a3ff9

glozow force-pushed on Aug 15, 2025

in src/rpc/mining.cpp:432 in daa40a3ff9

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

davidgumberg commented at 6:57 pm on August 15, 2025:

Maybe a release note?

instagibbs commented at 12:38 pm on August 20, 2025:

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

in test/functional/feature_rbf.py:586 in daa40a3ff9

582@@ -583,7 +583,7 @@ def test_replacement_relay_fee(self):
583         tx = self.wallet.send_self_transfer(from_node=self.nodes[0])['tx']
584 
585         # Higher fee, higher feerate, different txid, but the replacement does not provide a relay
586-        # fee conforming to node's `incrementalrelayfee` policy of 1000 sat per KB.
587+        # fee conforming to node's `incrementalrelayfee` policy of 100 sat per KB.

davidgumberg commented at 7:01 pm on August 15, 2025:

nit:

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

DrahtBot removed the label CI failed on Aug 15, 2025

in test/functional/feature_rbf.py:597 in 636fa219d3 outdated

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

achow101 commented at 10:36 pm on August 15, 2025:

In 636fa219d37f86067d996c86fada286cedc0d78e “test fixups”

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

instagibbs commented at 12:30 pm on August 20, 2025:

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

achow101 commented at 9:32 pm on August 20, 2025:

None of the settings have decimals though

davidgumberg approved

davidgumberg commented at 10:54 pm on August 15, 2025: contributor

ACK https://github.com/bitcoin/bitcoin/pull/33189/commits/daa40a3ff97346face9dcc64564010a66c91ccb2

fanquake added this to the milestone 30.0 on Aug 20, 2025

instagibbs approved

instagibbs commented at 1:01 pm on August 20, 2025: member

ACK daa40a3ff97346face9dcc64564010a66c91ccb2

l0rinc commented at 5:34 pm on August 21, 2025: contributor

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 commented at 10:36 am on August 22, 2025: member

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 commented at 7:28 pm on August 22, 2025: contributor

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.

rpc: followups for 33106 #33189

Code Coverage & Benchmarks

Reviews

Conflicts