tests: Fix calculation of external input weights #24454

1. 
   achow101 commented at 2:18 pm on March 1, 2022: member

   The external input tests with specifying input weight would sometimes result in a test failure because it would add 2 to the calculated byte size in order to account for some of the variation in signature and script sizes. However 1 in 128 signatures are actually 1 byte smaller than we expect, so the difference between the actual signature size and our calculated size becomes 3 bytes which is outside of the tolerance of assert_fee_amount and would thus cause the test failure.

   To resolve this, the 2 byte buffer is reduced to 1 byte, so in the above scenario, the difference is 2 bytes which is within the tolerance of assert_fee_amount. Additionally, instead of putting a fixed size that we assume is the correct size for the length of the compact size length prefix of data, we actually get the length of the compact size uint.

   Lastly, the size calculation for a scriptWitness was simply incorrect and used fields that did not exist. This is fixed, and the test slightly modified so that it also produces a scriptWitness.

   Fixes #24151

2. in test/functional/rpc_psbt.py:664 in 2c09ad698f outdated

   654@@ -655,10 +655,11 @@ def test_psbt_input_keys(psbt_input, keys):
   655                 break
   656         psbt_in = dec["inputs"][input_idx]
   657         # Calculate the input weight
   658-        # (prevout + sequence + length of scriptSig + 2 bytes buffer) * 4 + len of scriptwitness
   659+        # (prevout + sequence + length of scriptSig + scriptsig) * 4 + num scriptWitness stack items + (length of stack item + stack item) * N stack items
   660         len_scriptsig = len(psbt_in["final_scriptSig"]["hex"]) // 2 if "final_scriptSig" in psbt_in else 0
   661-        len_scriptwitness = len(psbt_in["final_scriptwitness"]["hex"]) // 2 if "final_scriptwitness" in psbt_in else 0
   662-        input_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitness
   663+        len_scriptsig += len(ser_compact_size(len_scriptsig))
   

   ---

   ---

   

   jonatack commented at 3:03 pm on March 1, 2022:

   Linter unhappy

   0$ test/lint/lint-python.sh 
   1test/functional/rpc_psbt.py:660:30: F821 undefined name 'ser_compact_size'
   2test/functional/rpc_psbt.py:661:51: F821 undefined name 'ser_compact_size'
   

   Fix:

   0--- a/test/functional/rpc_psbt.py
   1+++ b/test/functional/rpc_psbt.py
   2@@ -10,6 +10,7 @@ from itertools import product
   3 
   4 from test_framework.descriptors import descsum_create
   5 from test_framework.key import ECKey
   6+from test_framework.messages import ser_compact_size
   7 from test_framework.test_framework import BitcoinTestFramework
   8 from test_framework.util import (
   

3. fanquake added this to the milestone 23.0 on Mar 1, 2022
4. achow101 force-pushed on Mar 1, 2022
5. DrahtBot added the label Tests on Mar 1, 2022
6. in test/functional/rpc_psbt.py:659 in 765b1387f1 outdated

   655@@ -655,10 +656,11 @@ def test_psbt_input_keys(psbt_input, keys):
   656                 break
   657         psbt_in = dec["inputs"][input_idx]
   658         # Calculate the input weight
   659-        # (prevout + sequence + length of scriptSig + 2 bytes buffer) * 4 + len of scriptwitness
   660+        # (prevout + sequence + length of scriptSig + scriptsig) * 4 + num scriptWitness stack items + (length of stack item + stack item) * N stack items
   

   ---

   ---

   

   jonatack commented at 4:55 pm on March 1, 2022:

   Maybe in both tests use WITNESS_SCALE_FACTOR if I’m not confused

    0 from test_framework.key import ECKey
    1-from test_framework.messages import ser_compact_size
    2+from test_framework.messages import (
    3+    ser_compact_size,
    4+    WITNESS_SCALE_FACTOR,
    5+)
    6 from test_framework.test_framework import BitcoinTestFramework
    7@@ -656,11 +659,13 @@ class PSBTTest(BitcoinTestFramework):
    8         # Calculate the input weight
    9-        # (prevout + sequence + length of scriptSig + scriptsig) * 4 + num scriptWitness stack items + (length of stack item + stack item) * N stack items
   10+        # (prevout + sequence + length of scriptSig + scriptsig) * WITNESS_SCALE_FACTOR + num scriptWitness stack items + (length of stack item + stack item) * N stack items
   11         len_scriptsig = len(psbt_in["final_scriptSig"]["hex"]) // 2 if "final_scriptSig" in psbt_in else 0
   12         len_scriptsig += len(ser_compact_size(len_scriptsig))
   13         len_scriptwitness = (sum([(len(x) // 2) + ser_compact_size(len(x) // 2) for x in psbt_in["final_scriptwitness"]]) + len(psbt_in["final_scriptwiness"])) if "final_scriptwitness" in psbt_in else 0
   14-        input_weight = ((40 + len_scriptsig) * 4) + len_scriptwitness
   15+        input_weight = ((40 + len_scriptsig) * WITNESS_SCALE_FACTOR) + len_scriptwitness
   

   ---

   

   achow101 commented at 10:53 am on March 2, 2022:

   Done
7. 
   jonatack commented at 5:11 pm on March 1, 2022: member

   If I’m parsing correctly, the tangible change here is replacing

   041 + len_scriptsig + 2
   

   with

   040 + (len_scriptsig += len(ser_compact_size(len_scriptsig)))
   

   In both tests "final_scriptwitness" in psbt_in is currently false and so len_scriptwitness is 0

   0"final_scriptSig" in psbt_in -> True
   1len(psbt_in["final_scriptSig"]["hex"]) -> 264
   2len_scriptsig -> 133
   3"final_scriptwitness" in psbt_in -> False
   4len_scriptwitness -> 0
   5input_weight -> (40 + 133) * 4 -> 692
   

8. 
   DrahtBot commented at 6:26 am on March 2, 2022: member

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #20892 (tests: Run both descriptor and legacy tests within a single test invocation 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.

9. achow101 force-pushed on Mar 2, 2022
10. achow101 force-pushed on Mar 2, 2022
11. 
    jonatack commented at 11:28 pm on March 2, 2022: member

    Running wallet_send.py on loop with this branch and some pprints to see the values, saw this error after a few dozen runs

     02022-03-02T22:44:14.061000Z TestFramework (INFO): External outputs
     1len_scriptSig: 132
     2len_scriptwitness: 0
     3input_weight: 688
     42022-03-02T22:44:14.866000Z TestFramework (ERROR): Assertion failed
     5Traceback (most recent call last):
     6  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 132, in main
     7    self.run_test()
     8  File "/home/jon/projects/bitcoin/bitcoin/test/functional/wallet_send.py", line 569, in run_test
     9    assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001))
    10  File "/home/jon/projects/bitcoin/bitcoin/test/functional/test_framework/util.py", line 42, in assert_fee_amount
    11    raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)" % (str(fee), str(target_fee)))
    12AssertionError: Fee of 0.00003130 BTC too low! (Should be 0.0000314 BTC)
    

    The usual values I’m seeing on this branch are (see #24454#pullrequestreview-896573285 above for more detail)

    0len_scriptSig: 133
    1len_scriptwitness: 0
    2input_weight: 692
    

12. achow101 force-pushed on Mar 3, 2022
13. 
    achow101 commented at 12:03 pm on March 3, 2022: member

    Hmm. I guess that error is now the same problem in the other direction. I’ve added a + 1 to the calculation as an additional buffer. Running this repeatedly shows no failures in 200+ runs.
14. in test/functional/rpc_psbt.py:666 in db4f73730c outdated

    664         len_scriptsig = len(psbt_in["final_scriptSig"]["hex"]) // 2 if "final_scriptSig" in psbt_in else 0
    665-        len_scriptwitness = len(psbt_in["final_scriptwitness"]["hex"]) // 2 if "final_scriptwitness" in psbt_in else 0
    666-        input_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitness
    667+        len_scriptsig += len(ser_compact_size(len_scriptsig)) + 1
    668+        len_scriptwitness = (sum([(len(x) // 2) + ser_compact_size(len(x) // 2) for x in psbt_in["final_scriptwitness"]]) + len(psbt_in["final_scriptwiness"]) + 1) if "final_scriptwitness" in psbt_in else 0
    669+        input_weight = ((40 + len_scriptsig) * WITNESS_SCALE_FACTOR) + len_scriptwitness
    

    ---

    ---

    

    prusnak commented at 6:27 pm on March 3, 2022:

    0        input_weight = (40 + len_scriptsig) * WITNESS_SCALE_FACTOR + len_scriptwitness
    

    ---

    

    jonatack commented at 8:46 pm on March 3, 2022:

    In both tests, for grep-ability any reason to not keep the current value of 41 instead of adding + 1 two lines earlier? and/or add “41 bytes” to the comments.

    0$ git grep "41 bytes"
    1src/test/data/tx_valid.json:445:["41 bytes push should not be considered a witness scriptPubKey"],
    2src/test/transaction_tests.cpp:915:    t.vin.resize(2438); // size per input (empty scriptSig): 41 bytes
    3src/validation.cpp:4907:        // If our average Coin size is roughly 41 bytes, checking every 120,000 coins
    4src/wallet/rpc/spend.cpp:574:                throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid parameter, weight cannot be less than 165 (41 bytes (size of outpoint + sequence + empty scriptSig) * 4 (witness scaling factor)) + 1 (empty witness)");
    5test/functional/test_framework/script_util.py:27:# Blank Input: 32 [PrevTxHash] + 4 [Index] + 1 [scriptSigLen] + 4 [SeqNo] = 41 bytes
    

    ---

    

    achow101 commented at 2:32 pm on March 16, 2022:

    I prefer to keep the parentheses so that terms are more clearly grouped together.

    ---

    41 is the length of an input with anempty scriptSig, but the scriptSig here is not empty. I don’t think it is useful to make it 41 here for grep-ability since you would be looking at the same number but with two different meanings.

    The + 1 here is as a buffer, not the length of an empty scriptSig.

    ---

    

    jonatack commented at 10:14 am on March 17, 2022:

    Thanks
15. in test/functional/wallet_send.py:537 in db4f73730c outdated

    535         len_scriptsig = len(psbt_in["final_scriptSig"]["hex"]) // 2 if "final_scriptSig" in psbt_in else 0
    536-        len_scriptwitness = len(psbt_in["final_scriptwitness"]["hex"]) // 2 if "final_scriptwitness" in psbt_in else 0
    537-        input_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitness
    538+        len_scriptsig += len(ser_compact_size(len_scriptsig)) + 1
    539+        len_scriptwitness = (sum([(len(x) // 2) + ser_compact_size(len(x) // 2) for x in psbt_in["final_scriptwitness"]]) + len(psbt_in["final_scriptwiness"]) + 1) if "final_scriptwitness" in psbt_in else 0
    540+        input_weight = ((40 + len_scriptsig) * WITNESS_SCALE_FACTOR) + len_scriptwitness
    

    ---

    ---

    

    prusnak commented at 6:28 pm on March 3, 2022:

    0        input_weight = (40 + len_scriptsig) * WITNESS_SCALE_FACTOR + len_scriptwitness
    

    ---

    

    achow101 commented at 2:33 pm on March 16, 2022:

    I prefer to keep the parentheses so that terms are more clearly grouped together.
16. prusnak changes_requested
17. 
    jonatack commented at 8:47 pm on March 3, 2022: member

    I haven’t been able to make the latest push fail with wallet_send.py on loop. That said, I wasn’t able to reproduce the original issue locally either.

    Light ACK db4f73730ce9c5f5fab8effe1746c53af065106b

18. fanquake added the label Needs backport (23.x) on Mar 4, 2022
19. fanquake requested review from instagibbs on Mar 8, 2022
20. MarcoFalke assigned achow101 on Mar 9, 2022
21. MarcoFalke unassigned achow101 on Mar 9, 2022
22. 
    MarcoFalke commented at 2:27 pm on March 16, 2022: member

    What is the status here? It looks like there is a comment, which may need to be replied to before merge?
23. 
    achow101 commented at 2:33 pm on March 16, 2022: member

    I’ve replied to the comment and will not be changing this unless there are more comments.
24. in test/functional/wallet_send.py:536 in db4f73730c outdated

    534+        # (prevout + sequence + length of scriptSig + scriptsig + 1 byte buffer) * WITNESS_SCALE_FACTOR + num scriptWitness stack items + (length of stack item + stack item) * N stack items + 1 byte buffer
    535         len_scriptsig = len(psbt_in["final_scriptSig"]["hex"]) // 2 if "final_scriptSig" in psbt_in else 0
    536-        len_scriptwitness = len(psbt_in["final_scriptwitness"]["hex"]) // 2 if "final_scriptwitness" in psbt_in else 0
    537-        input_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitness
    538+        len_scriptsig += len(ser_compact_size(len_scriptsig)) + 1
    539+        len_scriptwitness = (sum([(len(x) // 2) + ser_compact_size(len(x) // 2) for x in psbt_in["final_scriptwitness"]]) + len(psbt_in["final_scriptwiness"]) + 1) if "final_scriptwitness" in psbt_in else 0
    

    ---

    ---

    

    glozow commented at 12:44 pm on March 24, 2022:

    This is a typo, no? Surprised this runs fine - must mean it’s always 0. Same with rpc_psbt.py

    0        len_scriptwitness = (sum([(len(x) // 2) + ser_compact_size(len(x) // 2) for x in psbt_in["final_scriptwitness"]]) + len(psbt_in["final_scriptwitness"]) + 1) if "final_scriptwitness" in psbt_in else 0
    

    ---

    

    achow101 commented at 3:39 pm on March 24, 2022:

    Oops, fixed.

    These transactions are non-segwit, so this always ends up being 0. However I think it is still important to keep these for when the address type changes.

25. achow101 force-pushed on Mar 24, 2022
26. tests: Calculate input weight more accurately

    The external input test with specifying input weight would make a
    pessimistic estimate of the input weight. However this would result in a
    test failure as it is sometimes too pessimistic when an ECDSA signature
    ends up being smaller than usual. To correct this, we can calculate the
    input weight more accurately.

    8a04a386f7
27. achow101 force-pushed on Mar 24, 2022
28. 
    achow101 commented at 3:51 pm on March 24, 2022: member

    I’ve added a commit that changes the descriptor to also have a segwit component so that we can the weight of final_scriptwitness will be calculated.
29. 
    Sjors commented at 10:40 am on March 29, 2022: member

    Some background reviewers may find useful: https://bitcoin.stackexchange.com/a/77192/4948. Valid signatures were up to 73 bytes (since BIP 66). But our wallet only produces standard signatures, which are up to 72 bytes because they require a low S value (https://bitcoin.org/en/release/v0.11.1#test-for-lows-signatures-before-relaying). With R grinding in #13666 our signatures are always 71 bytes or less.

    It’s the “or less” part that’s the problem. DER requires using the shortest possible notation for a number, which means 1 in 256 times the R value will be 1 byte shorter. Ditto for the S value. See high school math for the odds of either a short R or S value. (Thank goodness BIP 340 Schnorr signatures get rid of DER encoding). So occasionally we produce signatures of 70 or even 69 bytes.

    At the same time, we can’t expect co-signers to go beyond what’s required by consensus (ditto for external signers). So they might give us a signature of up 73 bytes. (or 72 if we do insist on standardness)

    All that said, I’m still confused how this applies to our tests. Are we actually calculating the weight more “accurately” or are we assuming in the test that the external signature uses Bitcoin Core’s convention? In the latter case, shouldn’t the buffer be negative?

30. 
    jonatack commented at 10:53 am on March 29, 2022: member

    Thanks @Sjors. You reminded me that I added a similar comment to test_dust_to_fee() in wallet_bumpfee.py a couple years ago in 25e03ba1ff18ca06954786e512000648941b4dfb.

    0    # The DER formatting used by Bitcoin to serialize ECDSA signatures means that signatures can have a
    1    # variable size of 70-72 bytes (or possibly even less), with most being 71 or 72 bytes. The signature
    2    # in the witness is divided by 4 for the vsize, so this variance can take the weight across a 4-byte
    3    # boundary. Thus expected transaction size (p2wpkh, 1 input, 2 outputs) is 140-141 vbytes, usually 141.
    

31. 
    achow101 commented at 10:21 pm on March 29, 2022: member

    All that said, I’m still confused how this applies to our tests. Are we actually calculating the weight more “accurately” or are we assuming in the test that the external signature uses Bitcoin Core’s convention? In the latter case, shouldn’t the buffer be negative?

    I suppose the more accurately part refers to the use of ser_compact_size and actually calculating the scriptWitness length correctly.

    But the part that actually fixes the problem is really reducing the size of the buffer from 2 to 1. This has the desired effect for the test as assert_fee_amount can accept a 2 byte discrepancy, but a shorter signature would result in a 3 byte discrepancy previously.

32. jonatack approved
33. 
    jonatack commented at 7:05 pm on March 31, 2022: member

    ACK 9a8cf704e51961894f8d687c58195dbb31cf53f5

    This looks considerably improved since my review above at #24454#pullrequestreview-896573285.

    The values in the tests have changed since then as well and in my runs are always the same/stable.

    0"final_scriptSig" in psbt_in -> True
    1len(psbt_in["final_scriptSig"]["hex"]) -> 70
    2len_scriptsig -> 35, then 37
    3"final_scriptwitness" in psbt_in -> True
    4len_scriptwitness -> 136
    5input_weight -> ((40 + 37) * 4) + 136 -> 444
    

    Ran each of the two test files 100 times on a build of current master.

    I’m still seeing these test failures in the CI.

34. fanquake removed this from the milestone 23.0 on Apr 8, 2022
35. achow101 renamed this:
    tests: Calculate input weight more accurately
    tests: Fix calculation of external input weights
    on Apr 21, 2022
36. in test/functional/rpc_psbt.py:622 in 9a8cf704e5 outdated

    619@@ -620,7 +620,7 @@ def test_psbt_input_keys(psbt_input, keys):
    620         wallet = self.nodes[1].get_wallet_rpc("extfund")
    621 
    622         # Make a weird but signable script. sh(pkh()) descriptor accomplishes this
    

    ---

    ---

    

    glozow commented at 0:00 am on April 22, 2022:

    9a8cf704e51961894f8d687c58195dbb31cf53f5: comment should be updated to say sh(wsh(pkh())) since you’ve changed the descriptor type

    ---

    

    achow101 commented at 1:01 am on April 22, 2022:

    Done
37. in test/functional/wallet_send.py:496 in 9a8cf704e5 outdated

    492@@ -493,7 +493,7 @@ def run_test(self):
    493         ext_fund = self.nodes[1].get_wallet_rpc("extfund")
    494 
    495         # Make a weird but signable script. sh(pkh()) descriptor accomplishes this
    496-        desc = descsum_create("sh(pkh({}))".format(privkey))
    

    ---

    ---

    

    glozow commented at 0:00 am on April 22, 2022:

    same here

    ---

    

    achow101 commented at 1:01 am on April 22, 2022:

    Done
38. in test/functional/rpc_psbt.py:665 in 8a04a386f7 outdated

    663+        # (prevout + sequence + length of scriptSig + scriptsig + 1 byte buffer) * WITNESS_SCALE_FACTOR + num scriptWitness stack items + (length of stack item + stack item) * N stack items + 1 byte buffer
    664         len_scriptsig = len(psbt_in["final_scriptSig"]["hex"]) // 2 if "final_scriptSig" in psbt_in else 0
    665-        len_scriptwitness = len(psbt_in["final_scriptwitness"]["hex"]) // 2 if "final_scriptwitness" in psbt_in else 0
    666-        input_weight = ((41 + len_scriptsig + 2) * 4) + len_scriptwitness
    667+        len_scriptsig += len(ser_compact_size(len_scriptsig)) + 1
    668+        len_scriptwitness = (sum([(len(x) // 2) + len(ser_compact_size(len(x) // 2)) for x in psbt_in["final_scriptwitness"]]) + len(psbt_in["final_scriptwitness"]) + 1) if "final_scriptwitness" in psbt_in else 0
    

    ---

    ---

    

    glozow commented at 0:07 am on April 22, 2022:

    Question, why is the // 2 needed?

    ---

    

    achow101 commented at 1:01 am on April 22, 2022:

    2 because it’s a hex string. // because I want ints as the result.

    ---

    

    glozow commented at 6:03 pm on April 22, 2022:

    Ah understood, thanks!
39. tests: Use descriptor that requires both legacy and segwit 9f5ab670e7
40. achow101 force-pushed on Apr 22, 2022
41. 
    jonatack commented at 8:17 am on April 22, 2022: member

    re-ACK 9f5ab670e7c8165f161ec44dbd95778c5515ece0
42. 
    glozow commented at 6:03 pm on April 22, 2022: member

    code review ACK 9f5ab670e7c8165f161ec44dbd95778c5515ece0
43. MarcoFalke added this to the milestone 23.1 on Apr 22, 2022
44. fanquake merged this on Apr 25, 2022
45. fanquake closed this on Apr 25, 2022

    ---

46. sidhujag referenced this in commit 78f517d14e on Apr 26, 2022
47. fanquake referenced this in commit 5fd25eb9cb on Jun 9, 2022
48. fanquake referenced this in commit 039ef215bc on Jun 9, 2022
49. fanquake removed the label Needs backport (23.x) on Jun 9, 2022
50. 
    fanquake commented at 11:26 am on June 9, 2022: member

    Backported in #25316.
51. MarcoFalke referenced this in commit a33ec8a693 on Jul 8, 2022
52. DrahtBot locked this on Jun 9, 2023

Contributors
achow101 jonatack DrahtBot prusnak MarcoFalke glozow Sjors fanquake

Review Requested
instagibbs

Labels
Tests

Milestone
23.1