FEE parameter in test/functional/feature_dbcrash.py::generate_small_transaction() is not a fee rate, but an absolute fee. Hence, it doesn’t make sense to replace it with node relay based fee calculation. Get rid of the TODO comment suggesting otherwise.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32058.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | maflcko |
| Concept ACK | yancyribbens, kevkevinpal |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
183@@ -184,7 +184,9 @@ def verify_utxo_hash(self):
184 assert_equal(nodei_utxo_hash, node3_utxo_hash)
185
186 def generate_small_transactions(self, node, count, utxo_list):
187- FEE = 1000 # TODO: replace this with node relay fee based calculation
188+ relay_fee_btc = float(node.rpc.getmempoolinfo()["minrelaytxfee"])
189+ relay_fee_sat = int(relay_fee_btc * COIN)
190+ FEE = relay_fee_sat
not sure about using float and then int again. Why is this needed?
Also, changing FEE may change the test logic, so it seems better to adjust manually
It would be best to just add an assert_equal(FEE, minrelaytxfee)
not sure about using float and then int again. Why is this needed?
The fee in satoshis must be integer, so I added those as a precautionary measure. Looking back at it, they do seem redundant.
Also, changing FEE may change the test logic, so it seems better to adjust manually
Isn’t that the point of the change suggested by the TODO tag though? To make FEE adapt dynamically to a change in minRelayTxFee?
Many other tests do it the same way. See for example test/functional/p2p_1p1c_network.py.
If you wanted to change that, you’d have to go through all tests and check them for applicable changes, but I don’t really see the point to do that without rationale.
Concept ACK 6aff347
This looks to address the TODO correctly
183@@ -184,7 +184,10 @@ def verify_utxo_hash(self):
184 assert_equal(nodei_utxo_hash, node3_utxo_hash)
185
186 def generate_small_transactions(self, node, count, utxo_list):
187- FEE = 1000 # TODO: replace this with node relay fee based calculation
188+ FEE = 1000
189+ relay_fee_btc = node.rpc.getmempoolinfo()["minrelaytxfee"]
190+ relay_fee_sat = relay_fee_btc * COIN
191+ assert_equal(FEE, relay_fee_sat)
Actually, FEE here isn’t a fee rate at all, but an absolute fee.
So it seems fine to just remove the todo and and assert completely.
The 'FEE' parameter in test/functional/feature_dbcrash.py::
generate_small_transaction() is not a fee rate, but an
absolute fee. Hence, it doesn't make sense to replace it
with node relay based fee calculation. Get rid of the TODO
comment suggesting otherwise.