test: MiniWallet: respect passed feerate for padded txs (using `target

theStack commented at 6:09 pm on May 23, 2024: contributor

MiniWallet allows to create padded transactions that are equal or slightly above a certain target_weight (first introduced in PR #25379, commit 1d6b438ef0ccd05e1522ac38b44f847c1d93e72f), 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 commented at 6:09 pm on May 23, 2024: contributor

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 added the label Tests on May 23, 2024

theStack force-pushed on May 23, 2024

DrahtBot added the label CI failed on May 23, 2024

DrahtBot commented at 6:14 pm on May 23, 2024: contributor

🚧 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

theStack force-pushed on May 23, 2024

DrahtBot removed the label CI failed on May 24, 2024

glozow commented at 4:15 pm on May 24, 2024: member

Concept ACK!

in test/functional/test_framework/wallet.py:124 in 0dda80b759 outdated

120@@ -119,13 +121,14 @@ def _bulk_tx(self, tx, target_weight):
121         """Pad a transaction with extra outputs until it reaches a target weight (or higher).
122         returns the tx
123         """
124-        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN, b'a'])))
125+        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
126         dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4
127-        tx.vout[-1].scriptPubKey = CScript([OP_RETURN, b'a' * dummy_vbytes])

rkrux commented at 8:34 am on May 27, 2024:

In the previous implementation, it seemed a little unusual that b'a' was added first, and then more padding was added later. The newer approach seems cleaner where padding is added only once.

theStack commented at 10:39 pm on May 30, 2024:

One thing that’s maybe worth mentioning here is that the method of padding is changed from “one push of N bytes” to “N single pushes” (using repeated OP_1 instructions). The former method could introduce additional bytes due to the length encoding of the pushed data (using either single-byte length bytes for sizes <= 75, and OP_PUSHDATA{1,2,4} for larger ones, see https://en.bitcoin.it/wiki/Script#Constants), making exact padding even harder, which is avoided with the new way.

rkrux commented at 6:48 am on May 31, 2024:

I see, nice!

in test/functional/test_framework/wallet.py:129 in 0dda80b759 outdated

120@@ -119,13 +121,14 @@ def _bulk_tx(self, tx, target_weight):
121         """Pad a transaction with extra outputs until it reaches a target weight (or higher).
122         returns the tx
123         """
124-        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN, b'a'])))
125+        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
126         dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4

rkrux commented at 8:36 am on May 27, 2024:

Though it’s older implementation, for my understanding, why is the actual weight more than the target_weight. Doesn’t target_weight become a misnomer with this?

theStack commented at 1:35 pm on May 27, 2024:

Each byte in the output script counts as four weight units, so with the padding method used here, we can only ever increase the weight by multiples of four. If the weight of the unpadded tx is not a multiple of four already (i.e. tx.get_weight() % 4 != 0), it will also not be after the padding and the best we can do is to be not more off than 3 WUs in the worst case.

It would be nice if we could somehow do exact padding with weight unit precision, but to my knowledge there is no general way to add extra witness data to transactions without making them non-standard. (For example, the test feature_fastprune.py uses an annex in the witness for padding, but that is only consensus-valid, not policy-valid).

rkrux commented at 8:10 am on May 30, 2024:

Hmm from the way I understand this, addition of 3 is helpful in the case when (target_weight - tx.get_weight() < 4 because otherwise the floor division by 4 will cause no padding to be added and the transaction will end up being same in vbytes. Is this the right way to understand this?

Tying this to an earlier comment of mine - if target_weight had been called target_weight_wu, the code would become more self explanatory.

I feel this is worth adding as a comment for reference in the code, and thanks for the explanation!

theStack commented at 10:23 pm on May 30, 2024:

What is done here is a weight-to-vsize conversion, like also done in e.g. CTransaction.get_vsize(), so it’s essentially dividing by 4 and rounding up. Added a comment to clarify.

Tying this to an earlier comment of mine - if target_weight had been called target_weight_wu, the code would become more self explanatory.

To my knowledge, weight is always measured in weight units, so I don’t think adding the _wu suffix here would add any new information.

rkrux commented at 6:23 am on May 31, 2024:

To my knowledge, weight is always measured in weight units, so I don’t think adding the _wu suffix here would add any new information.

Makes sense if that’s the case. I’m new to this piece and will keep this in mind from now on.

in test/functional/feature_framework_miniwallet.py:32 in 372e5a7492 outdated

27+            for target_weight in [1000, 2000, 5000, 10000, 20000, 50000, 100000, 200000, 4000000,
28+                                  989,  2001, 4337, 13371, 23219, 49153, 102035, 223419, 3999989]:
29+                tx = wallet.create_self_transfer(utxo_to_spend=utxo, target_weight=target_weight)["tx"]
30+                self.log.debug(f"-> target weight: {target_weight}, actual weight: {tx.get_weight()}")
31+                assert_greater_than_or_equal(tx.get_weight(), target_weight)
32+                assert_greater_than_or_equal(target_weight + 3, tx.get_weight())

rkrux commented at 8:38 am on May 27, 2024:

Non blocking nit: This seems to be testing for a specific piece and if you don’t anticipate it to be updated later, WDYT about either adding this test in an existing functional test, or adding this in the unit tests of the functional test framework?

theStack commented at 1:36 pm on May 27, 2024:

We need a running node to create a MiniWallet instance, so unfortunately doing these checks in an unit test wouldn’t work. I considered putting it into an existing functional test but wouldn’t know which one to pick, so creating a new MiniWallet-specific test where features can tested independently seemed to make the most sense.

ismaelsadeeq commented at 2:57 pm on June 4, 2024:

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.

in test/functional/test_framework/wallet.py:378 in 93527b82e7 outdated

370@@ -370,6 +371,10 @@ def create_self_transfer(
371             vsize = Decimal(168)  # P2PK (73 bytes scriptSig + 35 bytes scriptPubKey + 60 bytes other)
372         else:
373             assert False
374+        if target_weight and not fee:  # respect fee_rate if target weight is passed

rkrux commented at 8:40 am on May 27, 2024:

Should there also be an assertion disallowing passing both target_weight and fee?

theStack commented at 1:36 pm on May 27, 2024:

Passing target_weight with an absolute fee should still work fine. There is arguably less reason now to do so as passing a fee_rate is likely more convenient for most use-cases, but I don’t think we have to disallow padding txs with an absolute fee.

in test/functional/test_framework/wallet.py:379 in 93527b82e7 outdated

370@@ -370,6 +371,10 @@ def create_self_transfer(
371             vsize = Decimal(168)  # P2PK (73 bytes scriptSig + 35 bytes scriptPubKey + 60 bytes other)
372         else:
373             assert False
374+        if target_weight and not fee:  # respect fee_rate if target weight is passed
375+            # the actual weight might be off by 3 WUs, so calculate based on that (see self._bulk_tx)

rkrux commented at 8:41 am on May 27, 2024:

As per the code in _bulk_tx, the actual weight is indeed off by 3 WUs, consider getting rid of might be here, it adds a bit of uncertainty imo.

theStack commented at 1:36 pm on May 27, 2024:

The actual weight can be larger by at most 3 WUs, see comment above.

in test/functional/mempool_limit.py:62 in 93527b82e7 outdated

58@@ -59,7 +59,7 @@ def test_rbf_carveout_disallowed(self):
59         mempoolmin_feerate = node.getmempoolinfo()["mempoolminfee"]
60         tx_A = self.wallet.send_self_transfer(
61             from_node=node,
62-            fee=(mempoolmin_feerate / 1000) * (A_weight // 4) + Decimal('0.000001'),
63+            fee_rate=mempoolmin_feerate,

rkrux commented at 8:42 am on May 27, 2024:

Nice to see the removal of many of these repetitive fee calculation code!

in test/functional/mempool_limit.py:317 in 93527b82e7 outdated

313@@ -314,18 +314,20 @@ def run_test(self):
314         target_weight_each = 200000
315         assert_greater_than(target_weight_each * 2, node.getmempoolinfo()["maxmempool"] - node.getmempoolinfo()["bytes"])
316         # Should be a true CPFP: parent's feerate is just below mempool min feerate
317-        parent_fee = (mempoolmin_feerate / 1000) * (target_weight_each // 4) - Decimal("0.00001")
318+        parent_feerate = mempoolmin_feerate - Decimal("0.000001")  # 0.1 sats/vbyte below min feerate

rkrux commented at 8:58 am on May 27, 2024:

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.

theStack commented at 10:23 pm on May 30, 2024:

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

in test/functional/mempool_limit.py:319 in 93527b82e7 outdated

313@@ -314,18 +314,20 @@ def run_test(self):
314         target_weight_each = 200000
315         assert_greater_than(target_weight_each * 2, node.getmempoolinfo()["maxmempool"] - node.getmempoolinfo()["bytes"])
316         # Should be a true CPFP: parent's feerate is just below mempool min feerate
317-        parent_fee = (mempoolmin_feerate / 1000) * (target_weight_each // 4) - Decimal("0.00001")
318+        parent_feerate = mempoolmin_feerate - Decimal("0.000001")  # 0.1 sats/vbyte below min feerate
319         # Parent + child is above mempool minimum feerate
320-        child_fee = (worst_feerate_btcvb) * (target_weight_each // 4) - Decimal("0.00001")
321+        child_feerate = (worst_feerate_btcvb * 1000) - Decimal("0.000001")  # 0.1 sats/vbyte below worst feerate

rkrux commented at 9:00 am on May 27, 2024:

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.

theStack commented at 10:23 pm on May 30, 2024:

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

rkrux commented at 6:48 am on May 31, 2024:

I can pick it up in a follow-up.

in test/functional/test_framework/wallet.py:132 in 0dda80b759 outdated

125+        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
126         dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4
127-        tx.vout[-1].scriptPubKey = CScript([OP_RETURN, b'a' * dummy_vbytes])
128-        # Lower bound should always be off by at most 3
129+        # compensate for the compact-size encoded script length
130+        dummy_vbytes -= len(ser_compact_size(dummy_vbytes)) - 1

rkrux commented at 9:02 am on May 27, 2024:

Nice catch here that fixes the calculation!

rkrux commented at 9:04 am on May 27, 2024: none

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.

in test/functional/test_framework/wallet.py:132 in 93527b82e7 outdated

126+        tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
127         dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4
128-        tx.vout[-1].scriptPubKey = CScript([OP_RETURN, b'a' * dummy_vbytes])
129-        # Lower bound should always be off by at most 3
130+        # compensate for the compact-size encoded script length
131+        dummy_vbytes -= len(ser_compact_size(dummy_vbytes)) - 1

rkrux commented at 8:16 am on May 30, 2024:

I’ve been trying to understand why 1 is subtracted here, but could not come up with a proper reason. Is it somehow related to excluding the already serialised length of the output script before padding?

theStack commented at 10:23 pm on May 30, 2024:

I’ve been trying to understand why 1 is subtracted here, but could not come up with a proper reason. Is it somehow related to excluding the already serialised length of the output script before padding?

Yes, exactly. The encoded length of the unpadded output script fits into one byte, and we want to find how much the length encoding increases with the padding, so we subtract by one. I added a comment to (hopefully) reduce the confusion for code readers.

rkrux commented at 6:21 am on May 31, 2024:

Thanks for the confirmation, appreciate it!

in test/functional/test_framework/wallet.py:377 in 93527b82e7 outdated

370@@ -370,6 +371,10 @@ def create_self_transfer(
371             vsize = Decimal(168)  # P2PK (73 bytes scriptSig + 35 bytes scriptPubKey + 60 bytes other)
372         else:
373             assert False
374+        if target_weight and not fee:  # respect fee_rate if target weight is passed
375+            # the actual weight might be off by 3 WUs, so calculate based on that (see self._bulk_tx)
376+            max_actual_weight = target_weight + 3
377+            fee = get_fee((max_actual_weight + 3) // 4, fee_rate)

ismaelsadeeq commented at 2:39 pm on May 30, 2024:

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 ?

theStack commented at 10:23 pm on May 30, 2024:

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.

in test/functional/test_framework/wallet.py:382 in 93527b82e7 outdated

370@@ -370,6 +371,10 @@ def create_self_transfer(
371             vsize = Decimal(168)  # P2PK (73 bytes scriptSig + 35 bytes scriptPubKey + 60 bytes other)
372         else:
373             assert False
374+        if target_weight and not fee:  # respect fee_rate if target weight is passed
375+            # the actual weight might be off by 3 WUs, so calculate based on that (see self._bulk_tx)
376+            max_actual_weight = target_weight + 3
377+            fee = get_fee((max_actual_weight + 3) // 4, fee_rate)
378         send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))

ismaelsadeeq commented at 2:41 pm on May 30, 2024:

0        send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))
1        # Ensure UTXO is enough to account for the required fee
2        assert_greater_than_or_equal(send_value, 0)

theStack commented at 10:24 pm on May 30, 2024:

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.

rkrux commented at 6:37 am on May 31, 2024:

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.

ismaelsadeeq commented at 2:47 pm on May 30, 2024: member

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 https://github.com/bitcoin/bitcoin/pull/30157/commits/93146927acf8ce7d929598b15224ef91ece6768c.

Third commit 93527b82e70c8e19d7317ce5287b0ee2a0020f1b looks good to me.

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

test: MiniWallet: fix tx padding (`target_weight`) for large sizes, improve accuracy c17550bc3a

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

test: MiniWallet: respect fee_rate for target_weight, use in mempool_limit.py 39d135e79f

theStack force-pushed on May 30, 2024

theStack commented at 10:29 pm on May 30, 2024: contributor

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

in test/functional/feature_framework_miniwallet.py:23 in 39d135e79f

18+    def set_test_params(self):
19+        self.num_nodes = 1
20+
21+    def test_tx_padding(self):
22+        """Verify that MiniWallet's transaction padding (`target_weight` parameter)
23+           works accurately enough (i.e. at most 3 WUs higher) with all modes."""

rkrux commented at 6:56 am on May 31, 2024:

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.

rkrux approved

rkrux commented at 6:58 am on May 31, 2024: none

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

DrahtBot requested review from glozow on May 31, 2024

DrahtBot requested review from ismaelsadeeq on May 31, 2024

in test/functional/test_framework/wallet.py:126 in c17550bc3a outdated

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

ismaelsadeeq commented at 2:23 pm on June 4, 2024:

nit:

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

DrahtBot requested review from ismaelsadeeq on Jun 4, 2024

ismaelsadeeq approved

ismaelsadeeq commented at 3:31 pm on June 4, 2024: member

Code Review ACK 39d135e79f3f0c40dfd8fad2c53723d533cd19b4 🚀

I’ve tested this locally c17550bc3a6 ensures that the target weight is calculated correctly, at most we get 3 off. b2f0a9f8b07 fails without the first commit. 39d135e79f3f0c40dfd8fad2c53723d533cd19b4 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 commented at 2:42 pm on June 7, 2024: member

light review ACK 39d135e79f3f0c40dfd8fad2c53723d533cd19b4

Would be nice to have this work for create_self_transfer_multi as well

glozow merged this on Jun 11, 2024

glozow closed this on Jun 11, 2024

theStack deleted the branch on Jun 11, 2024

ismaelsadeeq commented at 7:58 pm on June 13, 2024: member

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 commented at 2:23 pm on June 22, 2024: member

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.

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

Code Coverage

Reviews

Conflicts

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