test: MiniWallet: respect passed feerate for padded txs (using `target_weight`) #30162

theStack · 2024-05-23T18:09:33Z

MiniWallet allows to create padded transactions that are equal or slightly above a certain target_weight (first introduced in PR #25379, commit 1d6b438), which can be useful especially for mempool-related tests, e.g. for policy limit checks or scenarios to trigger mempool eviction. Currently the target_weight parameter doesn't play together with fee_rate though, as the fee calculation is incorrectly based on the tx vsize before the padding output is added, so the fee-rate is consequently far off. This means users are forced to pass an absolute fee, which can be quite inconvenient and leads to lots of duplicated "calculate absolute fee from fee-rate and vsize" code with the pattern fee = (feerate / 1000) * (weight // 4) on the call-sites.

This PR first improves the tx padding itself to be more accurate, adds a functional test for it, and fixes the fee_rate treatment for the {create,send}_self_transfer methods. (Next step would be to enable this also for the _self_transfer_multi methods, but those currently don't even offer a fee_rate parameter). Finally, the ability to pass both target_weight and fee_rate is used in the mempool_limit.py functional test. There might be more use-cases in other tests, that could be done in a follow-up.

DrahtBot · 2024-05-23T18:09:37Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	rkrux, ismaelsadeeq, glozow

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30157 (Fee Estimation via Fee rate Forecasters by ismaelsadeeq)
#30076 (test: fix MiniWallet script-path spend (missing parity bit in leaf version) by theStack)
#15218 (validation: sync chainstate to disk after syncing to tip by andrewtoth)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot · 2024-05-23T18:14:20Z

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the
documentation.

Possibly this is 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.

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

_{Debug: https://github.com/bitcoin/bitcoin/runs/25345334548}

glozow

Concept ACK!

rkrux

Concept ACK 93527b8

Make is successful, so are all functional tests.

I am in support of the changes in this PR because it fixes the tx fee calculation and improves the caller code in various places. Asked few questions and provided suggestion, would like to take another look later and provide ACK.

test/functional/test_framework/wallet.py

test/functional/feature_framework_miniwallet.py

test/functional/test_framework/wallet.py

test/functional/mempool_limit.py

rkrux · 2024-05-27T08:58:59Z

test/functional/mempool_limit.py

-        parent_fee = (mempoolmin_feerate / 1000) * (target_weight_each // 4) - Decimal("0.00001")
+        parent_feerate = mempoolmin_feerate - Decimal("0.000001")  # 0.1 sats/vbyte below min feerate


The previous implementation is 10^-5, and the newer one is 10^-6.
IMO, it's worth creating it a constant with an expressive name to make reading this piece easier.

Good idea. Leaving that as a follow-up, as the introduction of that constant could likely be used in much more places than only the ones I touch (maybe even other functional tests).

rkrux · 2024-05-27T09:00:16Z

test/functional/mempool_limit.py

-        child_fee = (worst_feerate_btcvb) * (target_weight_each // 4) - Decimal("0.00001")
+        child_feerate = (worst_feerate_btcvb * 1000) - Decimal("0.000001")  # 0.1 sats/vbyte below worst feerate


Preference style: Although this is not a fresh test, but IMHO it makes it far easier for the reviewer to read when the variable name contains the unit as well such as child_feerate_satvb or child_fee_sat.

Agree, leaving that as a follow-up as it likely needs more instances to adapt that are not touched here.

I can pick it up in a follow-up.

test/functional/test_framework/wallet.py

ismaelsadeeq

Concept ACK

This change is really useful and needed.

I came across this issue while working on another PR and had to fix it in my PR #30157. You can see the relevant commit 9314692.

Third commit 93527b8 looks good to me.

I will do a thorough review of the first two commits.

ismaelsadeeq · 2024-05-30T14:39:55Z

test/functional/test_framework/wallet.py

+        if target_weight and not fee:  # respect fee_rate if target weight is passed
+            # the actual weight might be off by 3 WUs, so calculate based on that (see self._bulk_tx)
+            max_actual_weight = target_weight + 3
+            fee = get_fee((max_actual_weight + 3) // 4, fee_rate)


Why are you incrementing by 3 twice
Please use WITNESS_SCALE_FACTOR const instead of just 4.

Magic numbers makes my head hurts 😃

Should be worth it to define 3 in wallet as PADDING_OFFSET ?

Why are you incrementing by 3 twice

The first increment by 3 is done to find the worst-case weight (that could result from ._bulk_tx), the second is part of a weight-to-vsize conversion. I've changed the latter to use float division and rounding up, as that should reflect better what this calculation actually achieves (the same way it's done e.g. in CTransaction.get_vsize()). It might be worth to introduce a weight_to_vsize helper in the util module in a follow-up, which could probably be used in a lots of functional tests and framework modules.

Please use WITNESS_SCALE_FACTOR const instead of just 4.

Sure, done :)

Should be worth it to define 3 in wallet as PADDING_OFFSET ?

Leaving that as a follow-up.

ismaelsadeeq · 2024-05-30T14:41:00Z

test/functional/test_framework/wallet.py

+        if target_weight and not fee:  # respect fee_rate if target weight is passed
+            # the actual weight might be off by 3 WUs, so calculate based on that (see self._bulk_tx)
+            max_actual_weight = target_weight + 3
+            fee = get_fee((max_actual_weight + 3) // 4, fee_rate)
        send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))


Suggested change

send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))

send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))

# Ensure UTXO is enough to account for the required fee

assert_greater_than_or_equal(send_value, 0)

This sounds like a very good idea for a follow-up, as it's not directly related to this PR (though one could argue that it's more likely to happen with large target_weight values). I think it's even worth it to throw an exception with an explicit message here like "UTXO is too small to cover for fees" (maybe even including details like utxo value, absolute fee, fee-rate, vsize) rather than only an assertion on the send value, which could still be confusing for users.

Agreed. However if it had not been for the absolute fee, we could come up with a more generic function get_effective_unspent_value that calculates based on the fee_rate and the vsize of the unspent.

Can't include the absolute fee in this because it is paid for the whole transaction and not just one unspent - still valid for transactions spending only one unspent.

…mprove accuracy

…limit.py

theStack · 2024-05-30T22:29:45Z

@rkrux @ismaelsadeeq Thanks for your reviews, much appreciated! I tried to tackle the comments, though for some of them I suggested that they would be a better fit for a follow-up PR, to not increase the size this PR (focused on functional change) with too many refactoring commits that would likely make most sense with changes also in other functional tests and framework modules.

rkrux

tACK 39d135e

@theStack Thanks for addressing the comments and answering my questions!

rkrux · 2024-05-31T06:56:57Z

test/functional/feature_framework_miniwallet.py

+
+    def test_tx_padding(self):
+        """Verify that MiniWallet's transaction padding (`target_weight` parameter)
+           works accurately enough (i.e. at most 3 WUs higher) with all modes."""


IMO, this 3 deserves a constant soon because it has started showing up in a comment in the functional test as well now besides being in few other places too in this diff. Otherwise, the mind of the code reader wanders off to figuring out where this 3 comes from.

ismaelsadeeq · 2024-06-04T14:23:20Z

test/functional/test_framework/wallet.py

@@ -119,13 +121,16 @@ def _bulk_tx(self, tx, target_weight):
        """Pad a transaction with extra outputs until it reaches a target weight (or higher).
        returns the tx
        """
-        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN, b'a'])))
+        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
+        # determine number of needed padding bytes by converting weight difference to vbytes
        dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4


nit:

Suggested change

dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4

dummy_vbytes = (target_weight - tx.get_weight() + 3) // WITNESS_SCALE_FACTOR

ismaelsadeeq · 2024-06-04T14:57:26Z

test/functional/feature_framework_miniwallet.py

+                self.log.debug(f"-> target weight: {target_weight}, actual weight: {tx.get_weight()}")
+                assert_greater_than_or_equal(tx.get_weight(), target_weight)
+                assert_greater_than_or_equal(target_weight + 3, tx.get_weight())


In case of failure these lines wont be reached as we will fail early in _bulk_tx.

I think delegating the weight verification to __bulk_tx is more elegant, else It will be messy for all create_self_transfer callers to verify the tx weight and the target weight.

It will be cleaner to just delete this and move the debugging to __bulk_tx.

Suggested change

self.log.debug(f"-> target weight: {target_weight}, actual weight: {tx.get_weight()}")

assert_greater_than_or_equal(tx.get_weight(), target_weight)

assert_greater_than_or_equal(target_weight + 3, tx.get_weight())

ismaelsadeeq

Code Review ACK 39d135e 🚀

I've tested this locally
c17550b ensures that the target weight is calculated correctly, at most we get 3 off.
b2f0a9f fails without the first commit.
39d135e fixes the issue of respecting fee rate when target weight is provided, as large tx now has correct fee rate.

My comments are not blockers, just suggestions for improvement, can come in a follow-up, but happy to reACK when fixed.

glozow

light review ACK 39d135e

Would be nice to have this work for create_self_transfer_multi as well

ismaelsadeeq · 2024-06-13T19:58:32Z

Would be nice to have this work for create_self_transfer_multi as well

I will pick this up and the outstanding review comments.

ismaelsadeeq · 2024-06-22T14:23:23Z

Would be nice to have this work for create_self_transfer_multi as well

I will pick this up and the outstanding review comments.

Opened #30322 that addresses most comments here and also prevent create_self_transfer failure when target weight is below tx weight, you can test this by passing a low target_weight to create_self_transfer.

hebasto · 2024-07-14T06:55:47Z

Ported to the CMake-based build system in hebasto#264.

940edd6 test: refactor: introduce and use `TRUC_CHILD_MAX_VSIZE` constant (Sebastian Falbesoner) c16ae71 test: switch MiniWallet padding unit from weight to vsize (Sebastian Falbesoner) Pull request description: This PR is a late follow-up for #30162, where I retrospectively consider the padding unit of choice as a mistake. The weight unit is merely a consensus rule detail and is largely irrelevant from a user's perspective w.r.t. fee-rate calculations and mempool policy rules (e.g. for package relay and TRUC limits), so there doesn't seem to be any value of using a granularity that we can't even guarantee to reach exactly anyway. Switch to the more natural unit of vsize instead, which simplifies both the padding implementation (no "round up to the next multiple of 4" anymore) and the current tests that take use of this padding. The rather annoying multiplications by `WITNESS_SCALE_FACTOR` can then be removed and weird-looking magic numbers like `4004` can be replaced by numbers that are more connected to actual policy limit constants from the codebase, e.g. `1001` for exceeding `TRUC_CHILD_MAX_VSIZE` by one. The second commits introduces a constant for that. ACKs for top commit: glozow: reACK 940edd6 via range-diff instagibbs: reACK 940edd6 maflcko: re-ACK 940edd6 🍷 achow101: ACK 940edd6 Tree-SHA512: 35325f22bbe548664273051b705059b8f2f4316215be116c71b8c21dc87d190b3e8fcc4a48f04deaba2f3632a9c809d272b0bae654cf74d7492759554c0f0d14

… is too low 92787dd test: raise an error when target_vsize is below tx virtual size (ismaelsadeeq) a8780c9 test: raise an error if output value is <= 0 in `create_self_transfer` (ismaelsadeeq) f6e8893 test: test that `create_self_transfer_multi` respects `target_vsize` (ismaelsadeeq) Pull request description: This is a simple test PR that does two things: 1. Raise an exception in `_bulk_tx_` when `target_vsize` is too low, i.e., below the tx vsize. 2. Addresses some review comments from #30162, which are: - Raise an error if the output value is less than or equal to zero in `create_self_transfer`. This prevents creating transactions with a value of 0 or less. - Add a test to verify that `create_self_transfer_multi` also respects the passed `target_vsize`. ACKs for top commit: achow101: ACK 92787dd theStack: ACK 92787dd rkrux: reACK 92787dd glozow: ACK 92787dd Tree-SHA512: 1f2767f2cf715ed65074c5fff347eec160b142685777d833d5e872cfef364d3dc1916b52ee442e99c7b9a8d514ff62bc67a9899d8854f65a4b93ac3ae300d18e

DrahtBot added the Tests label May 23, 2024

theStack force-pushed the 202405-test-MiniWallet_target_weight_improvements branch from 8c7d90b to 2be1e86 Compare May 23, 2024 18:14

DrahtBot added the CI failed label May 23, 2024

theStack force-pushed the 202405-test-MiniWallet_target_weight_improvements branch from 2be1e86 to 93527b8 Compare May 23, 2024 18:17

DrahtBot mentioned this pull request May 23, 2024

test: fix MiniWallet script-path spend (missing parity bit in leaf version) #30076

Merged

DrahtBot removed the CI failed label May 24, 2024

glozow reviewed May 24, 2024

View reviewed changes

DrahtBot mentioned this pull request May 24, 2024

validation: sync chainstate to disk after syncing to tip #15218

Closed

rkrux reviewed May 27, 2024

View reviewed changes

DrahtBot mentioned this pull request May 29, 2024

Fee Estimation via Fee rate Forecasters #30157

Draft

rkrux reviewed May 30, 2024

View reviewed changes

test/functional/test_framework/wallet.py Show resolved Hide resolved

ismaelsadeeq reviewed May 30, 2024

View reviewed changes

theStack added 3 commits May 31, 2024 00:11

test: MiniWallet: fix tx padding (target_weight) for large sizes, i…

c17550b

…mprove accuracy

test: add framework functional test for MiniWallet's tx padding

b2f0a9f

test: MiniWallet: respect fee_rate for target_weight, use in mempool_…

39d135e

…limit.py

theStack force-pushed the 202405-test-MiniWallet_target_weight_improvements branch from 93527b8 to 39d135e Compare May 30, 2024 22:22

rkrux approved these changes May 31, 2024

View reviewed changes

DrahtBot requested review from glozow and ismaelsadeeq May 31, 2024 06:58

ismaelsadeeq reviewed Jun 4, 2024

View reviewed changes

DrahtBot requested a review from ismaelsadeeq June 4, 2024 15:19

ismaelsadeeq approved these changes Jun 4, 2024

View reviewed changes

glozow reviewed Jun 7, 2024

View reviewed changes

glozow merged commit e6e4c18 into bitcoin:master Jun 11, 2024

theStack deleted the 202405-test-MiniWallet_target_weight_improvements branch June 11, 2024 12:42

ismaelsadeeq mentioned this pull request Jun 22, 2024

test: raise an error in _bulk_tx_ when target_vsize is too low #30322

Merged

hebasto added the Needs CMake port label Jun 30, 2024

hebasto mentioned this pull request Jul 14, 2024

cmake: Regular rebasing of the cmake-staging branch hebasto/bitcoin#264

Closed

hebasto removed the Needs CMake port label Jul 14, 2024

theStack mentioned this pull request Aug 26, 2024

test: switch MiniWallet padding unit from weight to vsize #30718

Merged

bitcoin locked and limited conversation to collaborators Jul 14, 2025

		parent_fee = (mempoolmin_feerate / 1000) * (target_weight_each // 4) - Decimal("0.00001")
		parent_feerate = mempoolmin_feerate - Decimal("0.000001") # 0.1 sats/vbyte below min feerate

		child_fee = (worst_feerate_btcvb) * (target_weight_each // 4) - Decimal("0.00001")
		child_feerate = (worst_feerate_btcvb * 1000) - Decimal("0.000001") # 0.1 sats/vbyte below worst feerate

	dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4
	dummy_vbytes = (target_weight - tx.get_weight() + 3) // WITNESS_SCALE_FACTOR

	self.log.debug(f"-> target weight: {target_weight}, actual weight: {tx.get_weight()}")
	assert_greater_than_or_equal(tx.get_weight(), target_weight)
	assert_greater_than_or_equal(target_weight + 3, tx.get_weight())

test: MiniWallet: respect passed feerate for padded txs (using target_weight) #30162

test: MiniWallet: respect passed feerate for padded txs (using target_weight) #30162

Uh oh!

Conversation

theStack commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

DrahtBot commented May 23, 2024

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

rkrux left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ismaelsadeeq 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theStack commented May 30, 2024

Uh oh!

rkrux 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!

ismaelsadeeq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

ismaelsadeeq commented Jun 13, 2024

Uh oh!

ismaelsadeeq commented Jun 22, 2024

Uh oh!

hebasto commented Jul 14, 2024

Uh oh!

Uh oh!

test: MiniWallet: respect passed feerate for padded txs (using `target_weight`) #30162

test: MiniWallet: respect passed feerate for padded txs (using `target_weight`) #30162

theStack commented May 23, 2024 •

edited

Loading

DrahtBot commented May 23, 2024 •

edited

Loading

ismaelsadeeq left a comment •

edited

Loading