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

1. 
   ismaelsadeeq commented at 2:20 pm on June 22, 2024: member

   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.
2. 
   DrahtBot commented at 2:20 pm on June 22, 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
   Stale ACK	rkrux

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

   Conflicts

   No conflicts as of last run.

3. in test/functional/test_framework/wallet.py:129 in 9acd7b150a outdated

   125@@ -126,7 +126,7 @@ def _bulk_tx(self, tx, target_weight):
   126         """
   127         tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
   128         # determine number of needed padding bytes by converting weight difference to vbytes
   129-        dummy_vbytes = (target_weight - tx.get_weight() + 3) // 4
   130+        dummy_vbytes = (target_weight - tx.get_weight() + 3) // WITNESS_SCALE_FACTOR
   

   ---

   ---

   

   rkrux commented at 10:36 am on June 23, 2024:

   Always nice to see getting rid of magic numbers!
4. in test/functional/feature_framework_miniwallet.py:23 in 3a248172b1 outdated

   19@@ -20,7 +20,7 @@ def set_test_params(self):
   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."""
   24+           works accurately with all modes."""
   

   ---

   ---

   

   rkrux commented at 10:38 am on June 23, 2024:

   Thanks for this change, I recall highlighting in the previous PR.
5. in test/functional/test_framework/wallet.py:126 in 3a248172b1 outdated

   121@@ -121,19 +122,19 @@ def _create_utxo(self, *, txid, vout, value, height, coinbase, confirmations):
   122         return {"txid": txid, "vout": vout, "value": value, "height": height, "coinbase": coinbase, "confirmations": confirmations}
   123 
   124     def _bulk_tx(self, tx, target_weight):
   125-        """Pad a transaction with extra outputs until it reaches a target weight (or higher).
   126-        returns the tx
   127+        """Pad a transaction with extra outputs until it reaches a target weight (or higher
   128+        at most BULK_TX_PADDING_OFFSET WU).
   

   ---

   ---

   

   rkrux commented at 10:39 am on June 23, 2024:

   0"""Pad a transaction with extra outputs until it reaches a target weight (or higher by
   1        at most BULK_TX_PADDING_OFFSET WU).
   

6. in test/functional/test_framework/wallet.py:133 in 3a248172b1 outdated

   128+        at most BULK_TX_PADDING_OFFSET WU).
   129         """
   130         tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
   131         # determine number of needed padding bytes by converting weight difference to vbytes
   132-        dummy_vbytes = (target_weight - tx.get_weight() + 3) // WITNESS_SCALE_FACTOR
   133+        dummy_vbytes = (target_weight - tx.get_weight() + BULK_TX_PADDING_OFFSET) // WITNESS_SCALE_FACTOR
   

   ---

   ---

   

   rkrux commented at 10:40 am on June 23, 2024:

   ➕
7. in test/functional/test_framework/wallet.py:129 in 2bae40e7fd outdated

   134-        tx.vout[-1].scriptPubKey = CScript([OP_RETURN] + [OP_1] * dummy_vbytes)
   135-        # Actual weight should be at most BULK_TX_PADDING_OFFSET higher than target weight
   136-        assert_greater_than_or_equal(tx.get_weight(), target_weight)
   137-        assert_greater_than_or_equal(target_weight + BULK_TX_PADDING_OFFSET, tx.get_weight())
   138+        # Prevent bulking tx when target weight is below the sum of tx weight and BULK_TX_PADDING_OFFSET
   139+        if (target_weight + BULK_TX_PADDING_OFFSET) > tx.get_weight():
   

   ---

   ---

   

   rkrux commented at 10:48 am on June 23, 2024:

   The condition seems correct but the corresponding comment doesn’t, it was a bit difficult for me to understand.

   How about the following?

   0Prevent bulking tx when target weight (along with offset) is less than or equal to the tx weight
   

   ---

   

   ismaelsadeeq commented at 12:46 pm on June 23, 2024:

   Yes, updated.
8. in test/functional/feature_framework_miniwallet.py:31 in 58655c0e00 outdated

   25@@ -26,9 +26,14 @@ def test_tx_padding(self):
   26             utxo = wallet.get_utxo(mark_as_spent=False)
   27             for target_weight in [100, 300, 500, 900, 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"-> tx weight: {tx.get_weight()} is always greater than or equal target weight: {target_weight}")
   31+                created_tx = wallet.create_self_transfer(utxo_to_spend=utxo, target_weight=target_weight)
   32+                tx = created_tx["tx"]
   33+                self.log.debug(f"-> tx weight: {target_weight} is always greater than or equal target weight: {tx.get_weight()}")
   

   ---

   ---

   

   rkrux commented at 10:50 am on June 23, 2024:

   Isn’t this debug log incorrect?

   ---

   

   rkrux commented at 10:51 am on June 23, 2024:

   0                self.log.debug(f"-> tx weight: {tx.get_weight()} is always greater than or equal target weight: {target_weight}")
   

   ---

   

   ismaelsadeeq commented at 12:47 pm on June 23, 2024:

   good catch, updated thanks.
9. in test/functional/test_framework/wallet.py:388 in d0a0408fcf outdated

   382@@ -383,7 +383,8 @@ def create_self_transfer(
   383             max_actual_weight = target_weight + BULK_TX_PADDING_OFFSET
   384             fee = get_fee(math.ceil(max_actual_weight / WITNESS_SCALE_FACTOR), fee_rate)
   385         send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))
   386-
   387+        if send_value <= 0:
   388+            raise Exception(f"UTXO value {utxo_to_spend['value']} is too small to cover fees {(fee or (fee_rate * vsize / 1000))}")
   

   ---

   ---

   

   rkrux commented at 10:52 am on June 23, 2024:

   Good addition imo.
10. rkrux changes_requested
11. 
    rkrux commented at 10:55 am on June 23, 2024: none

    tACK d0a0408

    make, test/functional are successful.

    Thanks @ismaelsadeeq for picking this up, I like the introduction of few assertions here. Added couple comment nits, and suggested one debug log change.

12. ismaelsadeeq force-pushed on Jun 23, 2024
13. rkrux approved
14. 
    rkrux commented at 7:21 am on June 24, 2024: none

    reACK 7d5afbb @ismaelsadeeq Thanks for quickly addressing my comments, these multiple small commits helped in thorough review of the PR.
15. in test/functional/test_framework/wallet.py:129 in 7d5afbb562 outdated

    136-        tx.vout[-1].scriptPubKey = CScript([OP_RETURN] + [OP_1] * dummy_vbytes)
    137-        # Actual weight should be at most 3 higher than target weight
    138-        assert_greater_than_or_equal(tx.get_weight(), target_weight)
    139-        assert_greater_than_or_equal(target_weight + 3, tx.get_weight())
    140+        # Prevent bulking tx when target weight (along with BULK_TX_PADDING_OFFSET) is less than or equal to the tx weight
    141+        if (target_weight + BULK_TX_PADDING_OFFSET) > tx.get_weight():
    

    ---

    ---

    

    maflcko commented at 10:33 am on June 25, 2024:

    Is there a reason to allow (skip) this if the condition is false? Does it not seem like a code error, which should throw?

    ---

    

    ismaelsadeeq commented at 10:49 am on June 25, 2024:

    yes, should throw! I will update this.

    ---

    

    ismaelsadeeq commented at 11:45 am on June 25, 2024:

    updated.
16. ismaelsadeeq force-pushed on Jun 25, 2024
17. ismaelsadeeq force-pushed on Jun 25, 2024
18. ismaelsadeeq force-pushed on Jun 25, 2024
19. ismaelsadeeq force-pushed on Jun 25, 2024
20. ismaelsadeeq force-pushed on Jun 25, 2024
21. DrahtBot added the label CI failed on Jun 25, 2024
22. 
    DrahtBot commented at 11:47 am on June 25, 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/26650226304

23. ismaelsadeeq renamed this:
    [test]: prevent `create_self_transfer` failure when target weight is below tx weight
    [test]: raise exception in `_bulk_tx` when target weight is below tx weight and #30162 followups
    on Jun 25, 2024
24. ismaelsadeeq renamed this:
    [test]: raise exception in `_bulk_tx` when target weight is below tx weight and #30162 followups
    [test]: raise exception in `_bulk_tx` when target weight is below tx weight
    on Jun 25, 2024
25. ismaelsadeeq renamed this:
    [test]: raise exception in `_bulk_tx` when target weight is below tx weight
    [test]: raise an exception in `_bulk_tx_` when `target_weight` is too low.
    on Jun 25, 2024
26. DrahtBot removed the label CI failed on Jun 25, 2024
27. in test/functional/test_framework/wallet.py:130 in 5e19ff8e29 outdated

    127+        """Pad a transaction with extra outputs until it reaches a target weight (or higher
    128+        at most BULK_TX_PADDING_OFFSET WU).
    129         """
    130+        padded_target_weight = target_weight + BULK_TX_PADDING_OFFSET
    131+        if padded_target_weight < tx.get_weight():
    132+            raise Exception(f"target_weight + BULK_TX_PADDING_OFFSET {padded_target_weight} is less than transaction weight {tx.get_weight()}")
    

    ---

    ---

    

    maflcko commented at 7:40 am on June 26, 2024:

    style nit: Could use the more specific RuntimeError to clarify this is a programming error?

    ---

    

    ismaelsadeeq commented at 10:43 pm on October 1, 2024:

    Done, thanks.
28. in test/functional/test_framework/wallet.py:388 in 5e19ff8e29 outdated

    386+            max_actual_weight = target_weight + BULK_TX_PADDING_OFFSET
    387             fee = get_fee(math.ceil(max_actual_weight / WITNESS_SCALE_FACTOR), fee_rate)
    388         send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))
    389-
    390+        if send_value <= 0:
    391+            raise Exception(f"UTXO value {utxo_to_spend['value']} is too small to cover fees {(fee or (fee_rate * vsize / 1000))}")
    

    ---

    ---

    

    maflcko commented at 7:40 am on June 26, 2024:

    same

    ---

    

    ismaelsadeeq commented at 10:44 pm on October 1, 2024:

    Done
29. maflcko approved
30. 
    maflcko commented at 7:40 am on June 26, 2024: member

    lgtm
31. ismaelsadeeq requested review from rkrux on Jul 3, 2024
32. glozow added the label Tests on Jul 4, 2024
33. rkrux approved
34. 
    rkrux commented at 9:53 am on July 7, 2024: none

    reACK 5e19ff8e

    Ran make and functional tests again, all successful.

35. DrahtBot added the label CI failed on Sep 18, 2024
36. DrahtBot removed the label CI failed on Sep 22, 2024
37. DrahtBot added the label Needs rebase on Sep 30, 2024
38. ismaelsadeeq force-pushed on Oct 1, 2024
39. ismaelsadeeq renamed this:
    [test]: raise an exception in `_bulk_tx_` when `target_weight` is too low.
    [test]: raise an error in `_bulk_tx_` when `target_vsize` is too low.
    on Oct 1, 2024
40. ismaelsadeeq renamed this:
    [test]: raise an error in `_bulk_tx_` when `target_vsize` is too low.
    test: raise an error in `_bulk_tx_` when `target_vsize` is too low
    on Oct 1, 2024
41. test: remove duplicate asserts that verify tx vsize 960fc3a209
42. test: test that `create_self_transfer_multi` respects `target_vsize` 0f1b964c22
43. test: raise an error if output value is <= 0 in `create_self_transfer` 48b709896d
44. test: raise an error when target_vsize is below tx virtual size 5e024b12db
45. ismaelsadeeq force-pushed on Oct 1, 2024
46. DrahtBot added the label CI failed on Oct 1, 2024
47. 
    DrahtBot commented at 10:42 pm on October 1, 2024: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/30942987582

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly 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.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

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

48. 
    ismaelsadeeq commented at 10:59 pm on October 1, 2024: member

    Rebased after #30718

    The rationale behind this PR was to improve the _bulk_tx method. Some of the commits here are no longer relevant after #30718; however, I believe the ones included in the latest push are still significant.

    The PR now consists of four trivial but useful commits, IMO:

    • 960fc3a20947a6282ff065e98db63f3ef0bdfc36: Removes the duplicate assert statement in test_tx_padding since it would be dead code in case of failure.
    • 0f1b964c22fee180542fd72de6001ef350fee29b: Ensures that create_self_transfer_multi respects the passed target_vsize.
    • 48b709896d6da5bdb1cb8be112611af7b36dc0d4: Raises an error when the output is insufficient to cover the transaction fee.
    • 5e024b12db64607fc009817a239a07025092a5d4: Raises a runtime error if the target_vsize passed to create_self_transfer* is less than the transaction weight.

    Before this PR, when a low vsize value was passed, the following error would occur:

    0
    1  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/wallet.py", line 132, in _bulk_tx
    2    dummy_vbytes -= len(ser_compact_size(dummy_vbytes)) - 1
    3                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    4  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/messages.py", line 100, in ser_compact_size
    5    r = l.to_bytes(1, "little")
    6        ^^^^^^^^^^^^^^^^^^^^^^^
    7OverflowError: can't convert negative int to unsigned
    

    Now, it will produce a more readable error message:

    0  File "/Users/abubakarismail/Desktop/Work/bitcoin-dev/bitcoin/test/functional/test_framework/wallet.py", line 125, in _bulk_tx
    1    raise RuntimeError(f"target_vsize {target_vsize} is less than the transaction virtual size {tx.get_vsize()}")
    2RuntimeError: target_vsize 80 is less than transaction virtual size 104
    

49. DrahtBot removed the label CI failed on Oct 1, 2024
50. DrahtBot removed the label Needs rebase on Oct 2, 2024

Contributors
ismaelsadeeq DrahtBot rkrux maflcko

Labels
Tests