test: miner: add coverage for `-blockmintxfee` setting

theStack commented at 7:20 pm on May 10, 2023: contributor

This PR adds missing test coverage for the -blockmintxfee option, which can be used by miners to specify the lowest fee-rate for transactions to be included in blocks. The setting was introduced in PR #9380 (commit daec955fd68bd0da036a5b446b54ffb01108adcd), with the rationale to decouple different minimum fees from -minrelaytxfee. According to the PR description it “should be set by miners to reflect their marginal cost of transmitting extra bytes.”.

On each iteration, the test creates and submits two txs using MiniWallet: one with the the minimum fee-rate as specified for -blockmintxfee and a second one with a fee-rate a little below that (-0.01 sats/vbyte). Then it checks that only the first one is picked for the block template and accordingly also only exists in the block that is mined after. This is repeatedly done for a fixed (but obviously somewhat arbitrary) list of different -blockmintxfee settings on a single node, including the default and zero (i.e. no minimum fee a.k.a. “include even zero-fee txs”) settings.

DrahtBot commented at 7:20 pm on May 10, 2023: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, brunoerg, glozow

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

DrahtBot added the label Tests on May 10, 2023

brunoerg commented at 8:09 pm on May 10, 2023: contributor

Concept ACK

glozow requested review from brunoerg on Jun 2, 2023

in test/functional/mining_basic.py:97 in 989fa37670 outdated

92+                self.restart_node(0, extra_args=[blockmintxfee_parameter, '-minrelaytxfee=0'])
93+
94+            # submit one tx with exactly the min tx-fee rate, and one slightly below (if possible)
95+            tx_with_min_feerate = self.wallet.send_self_transfer(from_node=node, fee_rate=blockmintxfee_btc_kvb)
96+            assert_equal(tx_with_min_feerate["fee"], get_fee(tx_with_min_feerate["tx"].get_vsize(), blockmintxfee_btc_kvb))
97+            if blockmintxfee_btc_kvb != 0:  # can't go below zero-fee

brunoerg commented at 8:05 pm on June 2, 2023:

nit:

0            if blockmintxfee_btc_kvb > 0:  # can't go below zero-fee

I think > fits better with the comment “can’t go below zero-fee” then !=

theStack commented at 5:19 pm on June 4, 2023:

Thanks, done.

brunoerg approved

brunoerg commented at 8:29 pm on June 2, 2023: contributor

crACK 989fa37670cbb386873a765799fd6db8ea6986f7

Nice test coverage!

Just a thought that came to my mind while reviewing it:

Seems like these test coverage uses just 1 node, if we’d stop node1 right before line 81, it wouldn’t make the test to fail. So, it seems that -minrelaytxfee=0 is just to make send_self_transfer - which calls sendrawtransaction - not fail (even if we might not have any connection).

theStack force-pushed on Jun 4, 2023

theStack commented at 5:38 pm on June 4, 2023: contributor

Thanks for reviewing! Addressed the nit.

Just a thought that came to my mind while reviewing it:

Seems like these test coverage uses just 1 node, if we’d stop node1 right before line 81, it wouldn’t make the test to fail. So, it seems that -minrelaytxfee=0 is just to make send_self_transfer - which calls sendrawtransaction - not fail (even if we might not have any connection).

Yes indeed, this sub-test works with a single node (it follows the design of previously introduced tests mempool_dust.py and mempool_sigoplimit.py, see #26631, #27171), i.e. with a self.stop_node(1) call at the beginning it would still succeed – just verified that. Decided to not stop node1 though, as I think it makes sense to keep all nodes alive for potential future sub-tests. Note that the -minrelaytxfee=0 setting is needed in any case, in order to get low/zero-fee-rate txs into the mempool.

glozow added the label Good First Review on Jun 7, 2023

in test/functional/mining_basic.py:111 in 7286efece4 outdated

106+            block = node.getblock(node.getbestblockhash(), verbosity=2)
107+            block_txids = [tx['txid'] for tx in block['tx']]
108+
109+            assert tx_with_min_feerate['txid'] in block_template_txids
110+            assert tx_with_min_feerate['txid'] in block_txids
111+            if blockmintxfee_btc_kvb != 0:

nberlin1980 commented at 9:12 pm on July 1, 2023:

0            if blockmintxfee_btc_kvb > 0: # can't go below zero-fee

To match the previous suggestion?

theStack commented at 1:50 pm on July 7, 2023:

~~Changed to use the comparison operator. Note that the “can’t go below zero-fee” comment from the above was removed, so I’m also not using it here.~~ //EDIT: With the latest force-push, this condition is gone.

glozow requested review from brunoerg on Jul 5, 2023

brunoerg approved

brunoerg commented at 5:46 pm on July 5, 2023: contributor

reACK 7286efece4442c5afe4e19fa6a7d2420444c904c

in test/functional/mining_basic.py:94 in 7286efece4 outdated

89+            else:
90+                blockmintxfee_parameter = f"-blockmintxfee={blockmintxfee_btc_kvb:.8f}"
91+                self.log.info(f"-> Test {blockmintxfee_parameter} ({blockmintxfee_sat_kvb} sat/kvB)...")
92+                self.restart_node(0, extra_args=[blockmintxfee_parameter, '-minrelaytxfee=0'])
93+
94+            # submit one tx with exactly the min tx-fee rate, and one slightly below (if possible)

glozow commented at 10:00 am on July 6, 2023:

nit: given the numerous minimum feerates that exist, imo it’s good to use specific names to disambiguate. For example the config option name -blockmintxfee vs -minrelaytxfee, or the RPC result name “mempoolminfee” vs “minrelaytxfee” vs “incrementalrelayfee”

In this case

0            # submit one tx with exactly the blockmintxfee rate, and one slightly below (if possible)

theStack commented at 1:50 pm on July 7, 2023:

Agree, done.

in test/functional/mining_basic.py:105 in 7286efece4 outdated

 96+            assert_equal(tx_with_min_feerate["fee"], get_fee(tx_with_min_feerate["tx"].get_vsize(), blockmintxfee_btc_kvb))
 97+            if blockmintxfee_btc_kvb > 0:  # can't go below zero-fee
 98+                lowerfee_btc_kvb = blockmintxfee_btc_kvb - Decimal(10)/COIN  # 0.01 sat/vbyte lower
 99+                tx_below_min_feerate = self.wallet.send_self_transfer(from_node=node, fee_rate=lowerfee_btc_kvb)
100+                assert_equal(tx_below_min_feerate["fee"], get_fee(tx_below_min_feerate["tx"].get_vsize(), lowerfee_btc_kvb))
101+

glozow commented at 10:51 am on July 6, 2023:

For the ==0 case, you could add some coverage for the fact that modified fees are used?

0        else:
1                tx_below_min_feerate = self.wallet.send_self_transfer(from_node=node, fee_rate=blockmintxfee_btc_kvb)
2                node.prioritisetransaction(tx_modified_below_min["txid"], 0, -1) 
3

theStack commented at 1:50 pm on July 7, 2023:

Nice idea, done.

in test/functional/mining_basic.py:92 in 7286efece4 outdated

87+            if blockmintxfee_sat_kvb == DEFAULT_BLOCK_MIN_TX_FEE:
88+                self.log.info(f"-> Default -blockmintxfee setting ({blockmintxfee_sat_kvb} sat/kvB)...")
89+            else:
90+                blockmintxfee_parameter = f"-blockmintxfee={blockmintxfee_btc_kvb:.8f}"
91+                self.log.info(f"-> Test {blockmintxfee_parameter} ({blockmintxfee_sat_kvb} sat/kvB)...")
92+                self.restart_node(0, extra_args=[blockmintxfee_parameter, '-minrelaytxfee=0'])

glozow commented at 4:21 pm on July 6, 2023:

Since the below-blockmintxfee transactions don’t get mined, they hang out in the mempool across iterations of the for loop and after the test. I think it’d be cleaner to clear it across restarts, as it’ll help if another subtest added after this one

0                self.restart_node(0, extra_args=[blockmintxfee_parameter, '-minrelaytxfee=0', '-persistmempool=0'])

theStack commented at 1:50 pm on July 7, 2023:

Agree, done. Note that I had to trigger an UTXO set rescan for MiniWallet after restart, otherwise it would sometimes try to spend UTXOs from mempool transactions (which are now not available anymore).

glozow commented at 1:53 pm on July 7, 2023:

Ah good thinking!

in test/functional/mining_basic.py:109 in 7286efece4 outdated

100+                assert_equal(tx_below_min_feerate["fee"], get_fee(tx_below_min_feerate["tx"].get_vsize(), lowerfee_btc_kvb))
101+
102+            # check that tx below specified fee-rate is neither in template nor in the actual block
103+            block_template = node.getblocktemplate(NORMAL_GBT_REQUEST_PARAMS)
104+            block_template_txids = [tx['txid'] for tx in block_template['transactions']]
105+            self.generate(self.wallet, 1, sync_fun=self.no_op)

glozow commented at 8:42 am on July 7, 2023:

Question: what is the purpose of using both getblocktemplate and generate for this test?

theStack commented at 1:54 pm on July 7, 2023:

The idea was to check that in both RPC code-paths that result in assembling blocks (generateto* and getblocktemplate, AFAICT there aren’t any others), the setting has the same effect. Happy to remove one of them though, if that’s seen as redundant.

glozow commented at 1:58 pm on July 7, 2023:

No need to remove, I was just wondering

in test/functional/mining_basic.py:84 in 7286efece4 outdated

75@@ -73,6 +76,42 @@ def mine_chain(self):
76         self.restart_node(0)
77         self.connect_nodes(0, 1)
78 
79+    def test_blockmintxfee_parameter(self):
80+        self.log.info("Test -blockmintxfee setting")
81+        self.restart_node(0, extra_args=['-minrelaytxfee=0'])
82+        node = self.nodes[0]
83+
84+        # test default (no parameter), disabled (=0) and a bunch of arbitrary minimum tx fee rates [sat/kvB]

glozow commented at 8:52 am on July 7, 2023:

Are you sure =0 means disabled? AFAICT it is still enforced, but you can only hit it using prioritisetransaction to make the fee negative. “Disabled” suggests to me that the block assembler would include packages at any feerate.

theStack commented at 1:50 pm on July 7, 2023:

You’re right. I think the “disabled” string originated from sloppily copy/pasting the test loop structure from mempool_dust.py. Replaced by “zero” now.

theStack force-pushed on Jul 7, 2023

in test/functional/mining_basic.py:117 in 95465cdb49 outdated

112+
113+            assert tx_with_min_feerate['txid'] in block_template_txids
114+            assert tx_with_min_feerate['txid'] in block_txids
115+            if blockmintxfee_btc_kvb > 0:
116+                assert tx_below_min_feerate['txid'] not in block_template_txids
117+                assert tx_below_min_feerate['txid'] not in block_txids

glozow commented at 1:55 pm on July 7, 2023:

No need for this condition anymore

0            assert tx_below_min_feerate['txid'] not in block_template_txids
1            assert tx_below_min_feerate['txid'] not in block_txids

theStack commented at 1:57 pm on July 7, 2023:

Oh right, done 👌

test: miner: add coverage for `-blockmintxfee` setting

Co-authored-by: glozow <gloriajzhao@gmail.com>

bbbb89d238

theStack force-pushed on Jul 7, 2023

ryanofsky approved

ryanofsky commented at 1:53 pm on July 20, 2023: contributor

Code review ACK bbbb89d238e9bdaa9f426d55b0a3b714dac1d39b, nice test

DrahtBot requested review from brunoerg on Jul 20, 2023

brunoerg approved

brunoerg commented at 2:06 pm on July 20, 2023: contributor

reACK bbbb89d238e9bdaa9f426d55b0a3b714dac1d39b

changes since my last review looks good!

glozow commented at 2:07 pm on July 20, 2023: member

ACK bbbb89d238e9bdaa9f426d55b0a3b714dac1d39b, sorry for the late re-review!

fanquake merged this on Jul 20, 2023

fanquake closed this on Jul 20, 2023

theStack deleted the branch on Jul 20, 2023

sidhujag referenced this in commit fc3b2eb053 on Jul 21, 2023

bitcoin locked this on Jul 19, 2024

test: miner: add coverage for -blockmintxfee setting #27620

Reviews

test: miner: add coverage for `-blockmintxfee` setting #27620