test: refactor: introduce and use `calculate_input_weight` helper #29777

1. 
   theStack commented at 1:07 PM on April 1, 2024: contributor

   Rather than manually estimating an input's weight by adding up all the involved components (fixed-size skeleton, compact-serialized lengths, and the actual scriptSig / witness stack items) we can simply take use of the serialization classes CTxIn / CTxInWitness instead, to achieve the same with significantly less code.

   The new helper is used in the functional tests rpc_psbt.py and wallet_send.py, where the previous manual estimation code was duplicated. Unit tests are added in the second commit.

2. 
   DrahtBot commented at 1:07 PM on April 1, 2024: contributor

   <!--e57a25ab6845829454e8d69fc972939a-->

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

   <!--006a51241073e994b41acfe9ec718e94-->

   Code Coverage

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

   <!--021abf342d371248e50ceaed478a90ca-->

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	AngusP, rkrux, kevkevinpal, QureshiFaisal, achow101
   Stale ACK	naiyoma

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

   <!--174a7506f384e20aa4161008e828411d-->

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #29771 (test: Run framework unit tests in parallel by tdb3)
   • #21283 (Implement BIP 370 PSBTv2 by achow101)

   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.

3. DrahtBot added the label Tests on Apr 1, 2024
4. theStack force-pushed on Apr 1, 2024
5. in test/functional/wallet_send.py:545 in b1674d688a outdated

    542 | @@ -544,16 +543,9 @@ def run_test(self):
    543 |                  break
    544 |          psbt_in = dec["inputs"][input_idx]
    545 |          # Calculate the input weight
   

   ---

   ---

   

   kevkevinpal commented at 2:46 AM on April 3, 2024:

   nit: not sure if this comment is redundant now since the function name is calculate_input_weight

   ---

   

   theStack commented at 11:10 PM on April 4, 2024:

   removed

6. in test/functional/rpc_psbt.py:754 in b1674d688a outdated

    751 | @@ -753,16 +752,9 @@ def test_psbt_input_keys(psbt_input, keys):
    752 |                  break
    753 |          psbt_in = dec["inputs"][input_idx]
    754 |          # Calculate the input weight
   

   ---

   ---

   

   kevkevinpal commented at 2:47 AM on April 3, 2024:

   nit: not sure if this comment is redundant now since the function name is calculate_input_weight

   ---

   

   theStack commented at 11:10 PM on April 4, 2024:

   removed

7. in test/functional/test_framework/wallet_util.py:175 in b1674d688a outdated

    168 | +        SMALL_LEN_BYTES = 1  # bytes needed for encoding scriptSig / witness item lenghts < 253
    169 | +        LARGE_LEN_BYTES = 3  # bytes needed for encoding scriptSig / witness item lengths >= 253
    170 | +
    171 | +        # empty scriptSig, no witness
    172 | +        self.assertEqual(calculate_input_weight("", None),
    173 | +                         (SKELETON_BYTES + SMALL_LEN_BYTES) * WITNESS_SCALE_FACTOR)
   

   ---

   ---

   

   kevkevinpal commented at 2:52 AM on April 3, 2024:

   It might make sense to add another test that does not explicitly set witness_stack_hex equal to None

   self.assertEqual(calculate_input_weight(""),
                            (SKELETON_BYTES + SMALL_LEN_BYTES) * WITNESS_SCALE_FACTOR)
   

   ---

   

   theStack commented at 11:11 PM on April 4, 2024:

   done

8. in test/functional/test_framework/wallet_util.py:183 in b1674d688a outdated

    178 | +        # large scriptSig, no witness
    179 | +        scriptSig_large = "00"*253
    180 | +        self.assertEqual(calculate_input_weight(scriptSig_large, None),
    181 | +                         (SKELETON_BYTES + LARGE_LEN_BYTES + 253) * WITNESS_SCALE_FACTOR)
    182 | +        # large scriptSig, empty witness stack
    183 | +        scriptSig_large = "00"*253
   

   ---

   ---

   

   kevkevinpal commented at 3:26 AM on April 3, 2024:

   nit: we only need to define this once

   ---

   

   naiyoma commented at 11:38 AM on April 3, 2024:

   nit: would it be beneficial to add an assertion for a small scriptSig with an empty witness stack?

   # small scriptSig, empty witness stack
   self.assertEqual(calculate_input_weight(scriptSig_small, []),
                           (SKELETON_BYTES + SMALL_LEN_BYTES + 252) * WITNESS_SCALE_FACTOR)
   
   

   ---

   

   theStack commented at 11:12 PM on April 4, 2024:

   removed the duplicated scriptSig_large and added the assertion (small scriptSig, empty witness stack) as suggested

9. 
   naiyoma commented at 11:08 AM on April 3, 2024: contributor

   CONCEPT ACK , Using calculate_input_weight is a better approach than manual estimations, also increases readability.

10. test: introduce and use `calculate_input_weight` helper

    Rather than manually estimating an input's weight by adding up all the
    involved components (fixed-size skeleton, compact-serialized lengths,
    and the actual scriptSig / witness stack items) we can simply take use
    of the serialization classes `CTxIn` / `CTxInWitness` instead, to
    achieve the same with significantly less code.
    
    The new helper is used in the functional tests rpc_psbt.py and
    wallet_send.py, where the previous manual estimation code was
    duplicated.

    f81fad5e0f
11. test: add unit tests for `calculate_input_weight` 6d91cb781c
12. theStack force-pushed on Apr 4, 2024
13. 
    theStack commented at 11:14 PM on April 4, 2024: contributor

    Thanks for your reviews @kevkevinpal, @naiyoma, I force-pushed with all of your suggestions taken!

14. 
    naiyoma commented at 8:49 PM on April 6, 2024: contributor

    tACK https://github.com/bitcoin/bitcoin/pull/29777/commits/f81fad5e0f3be1f7aed59f9da00396c75c2a6406 I have tested both call sites for the helper function locally (test/functional/wallet_send.py and test/functional/rpc_psbt.py) — tests run ok.

15. AngusP approved
16. 
    AngusP commented at 12:17 PM on April 7, 2024: contributor

    tACK 6d91cb781c30966963f28e7577c7aa3829fa9390

    All functional tests pass for me including affected rpc_psbt and wallet_send

17. DrahtBot requested review from naiyoma on Apr 7, 2024
18. rkrux approved
19. 
    rkrux commented at 8:35 AM on April 8, 2024: contributor

    tACK 6d91cb7

    1. Successful Make
    2. Successful all Functional Tests including rpc_psbt and wallet_send.

    Thanks for extracting out the common function, and reducing code duplication.

20. 
    kevkevinpal commented at 1:51 AM on April 10, 2024: contributor

    tACK 6d91cb7

21. 
    QureshiFaisal commented at 3:11 PM on April 11, 2024: none

    tACK 6d91cb7

    Functional tests passed for rpc_psbt.py and wallet_send.py

22. maflcko approved
23. 
    maflcko commented at 6:36 AM on April 12, 2024: member

    lgtm

24. 
    achow101 commented at 10:45 PM on April 22, 2024: member

    ACK 6d91cb781c30966963f28e7577c7aa3829fa9390

25. achow101 merged this on Apr 22, 2024
26. achow101 closed this on Apr 22, 2024

    ---

27. theStack deleted the branch on Apr 23, 2024
28. bitcoin locked this on Apr 23, 2025

Contributors

theStackDrahtBotkevkevinpalnaiyomaAngusPrkruxQureshiFaisalmaflckoachow101

Review Requested
naiyoma