test: get rid of redundant TODO tag #32058

Chand-ra commented at 1:28 PM on March 13, 2025: none

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.

DrahtBot commented at 1:28 PM on March 13, 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/32058.

Reviews

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.

DrahtBot added the label Tests on Mar 13, 2025

yancyribbens commented at 4:53 PM on March 14, 2025: contributor

Concept ACK. The changes appear to address the TODO correctly.

in test/functional/feature_dbcrash.py:189 in 6aff347734 outdated

 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

maflcko commented at 4:20 PM on March 16, 2025:

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)

Chand-ra commented at 5:03 PM on March 16, 2025:

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?

maflcko commented at 7:34 AM on March 25, 2025:

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.

kevkevinpal commented at 11:57 AM on March 21, 2025: contributor

Concept ACK 6aff347

This looks to address the TODO correctly

Chand-ra force-pushed on Mar 25, 2025

in test/functional/feature_dbcrash.py:190 in d99a599090 outdated

 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)

maflcko commented at 12:52 PM on March 25, 2025:

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.

Chand-ra commented at 1:41 PM on March 25, 2025:

Makes sense.

maflcko commented at 2:12 PM on March 25, 2025: member

Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits

test: get rid of redundant TODO tag

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.

d065208f0f

Chand-ra force-pushed on Mar 26, 2025

Chand-ra renamed this:
~~test: replace hardcoded fee with node relay fee based calculation~~
test: get rid of redundant TODO tag
on Mar 26, 2025

maflcko commented at 11:44 AM on March 27, 2025: member

lgtm ACK d065208f0f06309d776114d777bb16b8c38af3f1

fanquake merged this on Mar 28, 2025

fanquake closed this on Mar 28, 2025

sedited referenced this in commit a9c46ce3c3 on Apr 24, 2025

stickies-v referenced this in commit 772a33e052 on May 23, 2025

yuvicc referenced this in commit 069643f094 on Jul 6, 2025

bug-castercv502 referenced this in commit 5b0af4b944 on Sep 28, 2025

Chand-ra deleted the branch on Dec 8, 2025