test: modify weight estimate in functional tests #29502

m3dwards commented at 9:55 AM on February 28, 2024: contributor

The wallet_send functional test has been flaky due to a slightly overestimated weight calculation. This PR makes the weight calculation more accurate, although occasionally, due to how ECDSA signatures can be different lengths it might slightly over estimate. The assertion in the test can handle this slight variation and so should continue passing.

Update:

Because the signature can be shorter that is used in the weight estimation or the final transaction the estimate could be both slightly smaller or slightly larger.

DrahtBot commented at 9:55 AM on February 28, 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
ACK	achow101, S3RK

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29278 (Wallet: Add maxfeerate wallet startup option by ismaelsadeeq)

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.

DrahtBot added the label Tests on Feb 28, 2024

m3dwards force-pushed on Feb 28, 2024

DrahtBot commented at 10:00 AM on February 28, 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.

<sub>Debug: https://github.com/bitcoin/bitcoin/runs/22070837167</sub>

DrahtBot added the label CI failed on Feb 28, 2024

DrahtBot removed the label CI failed on Feb 28, 2024

achow101 added this to the milestone 27.0 on Feb 28, 2024

in test/functional/wallet_send.py:547 in 6c547b3c9a outdated

 542 | @@ -543,11 +543,16 @@ def run_test(self):
 543 |                  break
 544 |          psbt_in = dec["inputs"][input_idx]
 545 |          # Calculate the input weight
 546 | -        # (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
 547 | +        # (prevout + sequence + length of scriptSig + scriptsig) * WITNESS_SCALE_FACTOR + len of num scriptWitness stack items + (length of stack item + stack item) * N stack items
 548 | +        # Note that occasionally this weight estimate may be slightly larger than the real weight

achow101 commented at 11:11 PM on February 28, 2024:

The reverse is also true - the estimate can be a bit smaller than the real weight because the tx that we are using to perform this size calculation could have a shorter signature. So I don't think this actually fixes the problem, it just results in a fee too low error occasionally rather than a fee too high error.

m3dwards commented at 10:55 AM on February 29, 2024:

The assert_fee_amount function allows the estimate to be a bit larger with a 2 vbyte buffer but not smaller.

I can think of a few options:

Replicate the 2 vbyte allowance in the smaller direction
As you already suggested, switch the test to taproot

I'm leaning towards the latter as I don't like the idea of adding an allowance for every test that uses that assert.

As an aside I think it would be better to have two fee assertion functions, one that is precise and another that takes an allowance that can be specified in each tests. This way the intention of the test is clear, at the moment tests benefit from this allowance whether they need it or not. @achow101 what are your thoughts?

achow101 commented at 6:12 PM on February 29, 2024:

Actually, using taproot might result in a little bit less test coverage. IIRC the script chosen for the external utxo in this test is p2sh-segwit because it will have both a scriptSig and scriptWitness. This lets us check that coin control is applying the weight correctly for both legacy and segwit scripts. As Taproot is segwit only, we'd lose the coverage of the legacy side of things.

A different solution we could try is to check that the feerate is as we expected, rather than checking that the fee was expected. Since we are targeting an integer sat/vb feerate, the calculated feerate of the final tx should only ever be higher than the target by less than 1 sat/vb, so we could just calculate the sat/vbyte feerate, rounded down, and check that was what we expected.

m3dwards commented at 10:45 AM on March 1, 2024:

If the final feerate is only ever higher than the expected then the existing assertion should take care of that.

But I understood that the estimate can both be high or low depending on if a signature used for the estimate was smaller or the signature used in the final transaction was smaller.

We can still calculate the feerate in sats/vb but rounding up or down to the nearest integer.

achow101 commented at 11:12 PM on February 28, 2024: member

There is a test in rpc_psbt.py that does basically the same thing. Any changes made here should probably be made to that test as well since it tests the same feature in the psbt workflow.

test: fix weight estimates in functional tests

Updates the weight estimate to be more accurate by removing byte buffers and calculating the length of the count of scriptWitnesses rather than just using the count itself.

3c49e69670

test: fix flaky wallet_send functional test

Rather than asserting that the exact fees are the same, check the fee rate rounded to nearest interger. This will account for small differences in fees caused by variability in ECDSA signature lengths.

e67ab174c9

m3dwards force-pushed on Mar 1, 2024

m3dwards renamed this:
~~test: modify weight estimate in wallet_send~~
test: modify weight estimate in functional tests
on Mar 1, 2024

m3dwards commented at 1:25 PM on March 1, 2024: contributor

Force pushed new version addressing @achow101 's comments.

Switched the assertion in wallet_send.py to calculate the fee rate to the nearest satoshi rather than the precise fee.

achow101 commented at 9:33 PM on March 1, 2024: member

ACK e67ab174c9c04bba7a10724b7e694dd57f732177

in test/functional/wallet_send.py:551 in e67ab174c9

 550 |          len_scriptsig = len(psbt_in["final_scriptSig"]["hex"]) // 2 if "final_scriptSig" in psbt_in else 0
 551 | -        len_scriptsig += len(ser_compact_size(len_scriptsig)) + 1
 552 | -        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
 553 | -        input_weight = ((40 + len_scriptsig) * WITNESS_SCALE_FACTOR) + len_scriptwitness
 554 | +        len_scriptsig += len(ser_compact_size(len_scriptsig))
 555 | +        len_scriptwitness = (sum([(len(x) // 2) + len(ser_compact_size(len(x) // 2)) for x in psbt_in["final_scriptwitness"]]) + len(ser_compact_size(len(psbt_in["final_scriptwitness"])))) if "final_scriptwitness" in psbt_in else 0

S3RK commented at 8:08 AM on March 4, 2024:

Should we also divide len(psbt_in["final_scriptwitness"]) by 2 to get size in bytes?

m3dwards commented at 11:34 AM on March 4, 2024:

The divide by 2 is for turning a hex string into a count of bytes.

The length (read count) of the scriptWitnesses is just that, a count and not a hex string. Further this count is passed into function ser_compact_size that returns bytes so a simple length of this gives us the byte length we are after.

S3RK commented at 8:54 AM on March 5, 2024:

Yes, you're right. I thought witness is also serialised. Got myself confused.

S3RK commented at 8:54 AM on March 5, 2024: contributor

Code review ACK e67ab174c9c04bba7a10724b7e694dd57f732177

fanquake merged this on Mar 5, 2024

fanquake closed this on Mar 5, 2024

m3dwards deleted the branch on Mar 8, 2024

bitcoin locked this on Mar 8, 2025