[test]: raise an exception in _bulk_tx_ when target_weight 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_weight is too low, i.e., below the tx weight.
   2. Addresses some review comments from #30162, which are:
      • Use WITNESS_SCALE_FACTOR instead of the magic number 4.
      • Use PADDING_OFFSET instead of the magic number 3.
      • Raise an exception 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_weight."
2. [test]: use `WITNESS_SCALE_FACTOR` const in `_bulk_tx` instead 4 9acd7b150a
3. 
   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.

4. 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!
5. in test/functional/feature_framework_miniwallet.py:20 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.
6. 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).
   

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

   ➕
8. 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.
9. 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.
10. 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.
11. rkrux changes_requested
12. 
    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.

13. ismaelsadeeq force-pushed on Jun 23, 2024
14. rkrux approved
15. 
    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.
16. 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.
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. [test]: remove duplicate asserts that verify target weight 2786048e6e
22. [test]: use `BULK_TX_PADDING_OFFSET` constant instead of 3

    - Also clarify the docstring of `_bulk_tx` instead of clarifying in `test_tx_padding`.

    b47d14c340
23. [test]: raise exception when `target_weight` + `BULK_TX_PADDING_OFFSET` is below tx weight 62cf1fd8be
24. [test]: test that `create_self_transfer_multi` respects `target_weight` 623990b91c
25. [test]: raise exception if output value is <= 0 in `create_self_transfer` 5e19ff8e29
26. ismaelsadeeq force-pushed on Jun 25, 2024
27. DrahtBot added the label CI failed on Jun 25, 2024
28. 
    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

29. 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
30. 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
31. 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
32. DrahtBot removed the label CI failed on Jun 25, 2024
33. in test/functional/test_framework/wallet.py:130 in 5e19ff8e29

    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?
34. in test/functional/test_framework/wallet.py:388 in 5e19ff8e29

    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
35. maflcko approved
36. 
    maflcko commented at 7:40 am on June 26, 2024: member

    lgtm

Contributors
ismaelsadeeq DrahtBot rkrux maflcko