tests: Fix calculation of external input weights #24454

achow101 · 2022-03-01T14:18:52Z

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

test/functional/rpc_psbt.py

jonatack

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

41 + len_scriptsig + 2

with

40 + (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

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

test/functional/rpc_psbt.py

DrahtBot · 2022-03-02T06:26:00Z

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.

jonatack · 2022-03-02T23:28:27Z

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

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

The usual values I'm seeing on this branch are (see #24454 (review) above for more detail)

len_scriptSig: 133
len_scriptwitness: 0
input_weight: 692

achow101 · 2022-03-03T12:03:22Z

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.

test/functional/rpc_psbt.py

test/functional/wallet_send.py

jonatack

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 db4f737

test/functional/rpc_psbt.py

maflcko · 2022-03-16T14:27:48Z

What is the status here? It looks like there is a comment, which may need to be replied to before merge?

achow101 · 2022-03-16T14:33:38Z

I've replied to the comment and will not be changing this unless there are more comments.

test/functional/wallet_send.py

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.

achow101 · 2022-03-24T15:51:26Z

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.

Sjors · 2022-03-29T10:40:12Z

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?

jonatack · 2022-03-29T10:53:40Z

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 25e03ba.

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

achow101 · 2022-03-29T22:21:42Z

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.

jonatack

ACK 9a8cf70

This looks considerably improved since my review above at #24454 (review).

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

"final_scriptSig" in psbt_in -> True
len(psbt_in["final_scriptSig"]["hex"]) -> 70
len_scriptsig -> 35, then 37
"final_scriptwitness" in psbt_in -> True
len_scriptwitness -> 136
input_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.

test/functional/rpc_psbt.py

test/functional/wallet_send.py

test/functional/rpc_psbt.py

jonatack · 2022-04-22T08:17:43Z

re-ACK 9f5ab67

glozow

code review ACK 9f5ab67

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. Github-Pull: bitcoin#24454 Rebased-From: 8a04a38

Github-Pull: bitcoin#24454 Rebased-From: 9f5ab67

fanquake · 2022-06-09T11:26:58Z

Backported in #25316.

4ebf6e3 p2p: always set nTime for self-advertisements (Martin Zumsande) 039ef21 tests: Use descriptor that requires both legacy and segwit (Andrew Chow) 5fd25eb tests: Calculate input weight more accurately (Andrew Chow) bd6d3ac windeploy: Renewed windows code signing certificate (Andrew Chow) 32fa522 test: ensure createmultisig and addmultisigaddress are not returning any warning for expected cases (brunoerg) 7658055 rpc: fix inappropriate warning for address type p2sh-segwit in createmultisig and addmultisigaddress (brunoerg) Pull request description: Backports: - #24454 - #25201 - #25220 - #25314 ACKs for top commit: LarryRuane: re-utACK 4ebf6e3 achow101: ACK 4ebf6e3 Tree-SHA512: add3999d0330b3442f3894fce38ad9b5adc75da7d681c949e1d052bac5520c2c6fb06eba98bfbeb4aa9a560170451d24bf00d08dddd4a3d080030ecb8ad61882

achow101 mentioned this pull request Mar 1, 2022

"Fee too high" CI error in wallet_send.py #24151

Closed

jonatack reviewed Mar 1, 2022

View reviewed changes

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

fanquake added this to the 23.0 milestone Mar 1, 2022

achow101 force-pushed the fix-input-weight-test branch from 2c09ad6 to 765b138 Compare March 1, 2022 15:05

DrahtBot added the Tests label Mar 1, 2022

jonatack reviewed Mar 1, 2022

View reviewed changes

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

achow101 force-pushed the fix-input-weight-test branch 2 times, most recently from 6f6529a to 108935a Compare March 2, 2022 11:07

DrahtBot mentioned this pull request Mar 2, 2022

tests: Run both descriptor and legacy tests within a single test invocation #20892

Closed

achow101 force-pushed the fix-input-weight-test branch from 108935a to db4f737 Compare March 3, 2022 12:02

prusnak suggested changes Mar 3, 2022

View reviewed changes

test/functional/rpc_psbt.py Show resolved Hide resolved

test/functional/wallet_send.py Show resolved Hide resolved

jonatack reviewed Mar 3, 2022

View reviewed changes

test/functional/rpc_psbt.py Show resolved Hide resolved

fanquake added the Needs backport (23.x) label Mar 4, 2022

fanquake requested a review from instagibbs March 8, 2022 11:58

maflcko assigned achow101 and unassigned achow101 Mar 9, 2022

glozow reviewed Mar 24, 2022

View reviewed changes

test/functional/wallet_send.py Outdated Show resolved Hide resolved

achow101 force-pushed the fix-input-weight-test branch from db4f737 to 97bfa82 Compare March 24, 2022 15:38

achow101 force-pushed the fix-input-weight-test branch from 97bfa82 to 9a8cf70 Compare March 24, 2022 15:51

jonatack approved these changes Mar 31, 2022

View reviewed changes

fanquake removed this from the 23.0 milestone Apr 8, 2022

jonatack mentioned this pull request Apr 12, 2022

Sanity assert GetAncestor() != nullptr where appropriate #24804

Merged

achow101 changed the title ~~tests: Calculate input weight more accurately~~ tests: Fix calculation of external input weights Apr 21, 2022

glozow reviewed Apr 22, 2022

View reviewed changes

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

test/functional/wallet_send.py Show resolved Hide resolved

test/functional/rpc_psbt.py Show resolved Hide resolved

tests: Use descriptor that requires both legacy and segwit

9f5ab67

achow101 force-pushed the fix-input-weight-test branch from 9a8cf70 to 9f5ab67 Compare April 22, 2022 01:01

glozow reviewed Apr 22, 2022

View reviewed changes

maflcko added this to the 23.1 milestone Apr 22, 2022

fanquake merged commit aa54132 into bitcoin:master Apr 25, 2022

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 26, 2022

Merge bitcoin#24454: tests: Fix calculation of external input weights

78f517d

fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jun 9, 2022

tests: Use descriptor that requires both legacy and segwit

039ef21

Github-Pull: bitcoin#24454 Rebased-From: 9f5ab67

fanquake mentioned this pull request Jun 9, 2022

23.x backports #25316

Merged

fanquake removed the Needs backport (23.x) label Jun 9, 2022

bitcoin locked and limited conversation to collaborators Jun 9, 2023

tests: Fix calculation of external input weights #24454

tests: Fix calculation of external input weights #24454

Uh oh!

Conversation

achow101 commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jonatack commented Mar 2, 2022

Uh oh!

achow101 commented Mar 3, 2022

Uh oh!

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

maflcko commented Mar 16, 2022

Uh oh!

achow101 commented Mar 16, 2022

Uh oh!

Uh oh!

achow101 commented Mar 24, 2022

Uh oh!

Sjors commented Mar 29, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack commented Mar 29, 2022

Uh oh!

achow101 commented Mar 29, 2022

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonatack commented Apr 22, 2022

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

fanquake commented Jun 9, 2022

Uh oh!

Uh oh!

achow101 commented Mar 1, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading

DrahtBot commented Mar 2, 2022 •

edited

Loading

Sjors commented Mar 29, 2022 •

edited

Loading

jonatack left a comment •

edited

Loading