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 & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/30322.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	theStack, rkrux, glozow, achow101

   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:26 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. ismaelsadeeq force-pushed on Oct 1, 2024
42. DrahtBot added the label CI failed on Oct 1, 2024
43. 
    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.

44. 
    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 relevant.

    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
    

45. DrahtBot removed the label CI failed on Oct 1, 2024
46. DrahtBot removed the label Needs rebase on Oct 2, 2024
47. 
    theStack commented at 0:54 am on January 7, 2025: contributor

    Concept ACK, thanks for following up!

    LGTM, but would prefer to keep the vsize check in the MiniWallet functional framework test (first commit). It’s true that this is currently unreachable, but a potential bug where e.g. the logic would change in a way that _bulk_tx isn’t even called would then remain undetected by the test.

48. test: test that `create_self_transfer_multi` respects `target_vsize` f6e88931f0
49. test: raise an error if output value is <= 0 in `create_self_transfer` a8780c937f
50. test: raise an error when target_vsize is below tx virtual size 92787dd52c
51. ismaelsadeeq force-pushed on Jan 8, 2025
52. 
    ismaelsadeeq commented at 2:49 pm on January 8, 2025: member

    but would prefer to keep the vsize check in the MiniWallet functional framework test (first commit). It’s true that this is currently unreachable, but a potential bug where e.g. the logic would change in a way that _bulk_tx isn’t even called would then remain undetected by the test.

    Reverted. thanks for review.

53. DrahtBot added the label CI failed on Jan 8, 2025
54. DrahtBot removed the label CI failed on Jan 8, 2025
55. in test/functional/test_framework/wallet.py:382 in a8780c937f outdated

    377@@ -378,7 +378,8 @@ def create_self_transfer(
    378         if target_vsize and not fee:  # respect fee_rate if target vsize is passed
    379             fee = get_fee(target_vsize, fee_rate)
    380         send_value = utxo_to_spend["value"] - (fee or (fee_rate * vsize / 1000))
    381-
    382+        if send_value <= 0:
    383+            raise RuntimeError(f"UTXO value {utxo_to_spend['value']} is too small to cover fees {(fee or (fee_rate * vsize / 1000))}")
    

    ---

    ---

    

    theStack commented at 1:51 pm on January 9, 2025:

    nit: could deduplicate the expression for calculating the final fee in sats
56. theStack approved
57. 
    theStack commented at 1:51 pm on January 9, 2025: contributor

    ACK 92787dd52cd4ddc378cf1bcb7e92a649916a0f42

    Tested the new error raising code paths manually by introducing create_self_transfer calls (in an arbitrary functional test), one with too-high-fee and another one with too-low-target-vsize:

    0wallet.create_self_transfer(fee=Decimal("50.00000001"))
    1...
    2=> RuntimeError: UTXO value 50.00000000 is too small to cover fees 50.00000001
    3
    4
    5wallet.create_self_transfer(utxo_to_spend=utxo, target_vsize=100)
    6...
    7=> RuntimeError: target_vsize 100 is less than transaction virtual size 104
    

58. DrahtBot requested review from rkrux on Jan 9, 2025
59. rkrux approved
60. 
    rkrux commented at 2:21 pm on January 10, 2025: none

    reACK 92787dd52cd4ddc378cf1bcb7e92a649916a0f42

    Nit: The PR title can be updated to account for output value check as well.

    Simulated the errors for the 2 checks in feature_framework_miniwallet.py by passing too high fees and too low target_vsize:

    0RuntimeError: UTXO value 50.00000000 is too small to cover fees 100.00000001
    1RuntimeError: target_vsize 10 is less than transaction virtual size 104
    

61. in test/functional/feature_framework_miniwallet.py:34 in f6e88931f0 outdated

    28@@ -29,8 +29,11 @@ def test_tx_padding(self):
    29             utxo = wallet.get_utxo(mark_as_spent=False)
    30             for target_vsize in [250, 500, 1250, 2500, 5000, 12500, 25000, 50000, 1000000,
    31                                  248, 501, 1085, 3343, 5805, 12289, 25509, 55855,  999998]:
    32-                tx = wallet.create_self_transfer(utxo_to_spend=utxo, target_vsize=target_vsize)["tx"]
    33-                assert_equal(tx.get_vsize(), target_vsize)
    34+                tx = wallet.create_self_transfer(utxo_to_spend=utxo, target_vsize=target_vsize)
    35+                assert_equal(tx['tx'].get_vsize(), target_vsize)
    36+                child_tx = wallet.create_self_transfer_multi(utxos_to_spend=[tx["new_utxo"]], target_vsize=target_vsize)
    

    ---

    ---

    

    glozow commented at 4:07 pm on January 23, 2025:

    f6e88931f071f06cb8aefeac61cf3bce662eea74

    Is the purpose to test that the padding works using both functions? This still uses just one utxo. Is it significant that this is a child?

    ---

    

    ismaelsadeeq commented at 4:29 pm on January 23, 2025:

    Thanks for review!

    Is the purpose to test that the padding works using both functions?

    Yes, that is the purpose.

    This still uses just one UTXO.

    Yes, create_self_transfer_multi uses one UTXO or more. You add additional UTXOs when the UTXO output value is not sufficient to cover the fee for the target size. The UTXOs are 50, 25, and 25 BTC, which are enough, so there is no need to add more UTXOs.

    Is it significant that this is a child?

    I think It does not matter whether the UTXO is a child or the initial UTXO, as we are just creating it, not sending it.

62. 
    glozow commented at 4:10 pm on January 23, 2025: member

    ACK 92787dd52cd4ddc378cf1bcb7e92a649916a0f42
63. 
    achow101 commented at 11:03 pm on January 24, 2025: member

    ACK 92787dd52cd4ddc378cf1bcb7e92a649916a0f42
64. achow101 merged this on Jan 24, 2025
65. achow101 closed this on Jan 24, 2025

    ---

Contributors
ismaelsadeeq DrahtBot rkrux maflcko theStack glozow achow101

Labels
Tests