"Fee too high" CI error in wallet

jonatack commented at 1:39 pm on January 25, 2022: member

wallet_send.py --legacy-wallet line 332

https://cirrus-ci.com/task/6260771733635072

02022-01-25T13:05:06.243000Z TestFramework (INFO): Test setting explicit fee rate
12022-01-25T13:05:06.666000Z TestFramework (ERROR): Assertion failed
2Traceback (most recent call last):
3  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
4    self.run_test()
5  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_send.py", line 332, in run_test
6    assert_fee_amount(fee, Decimal(len(res["hex"]) / 2), Decimal("0.00004531"))
7  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 45, in assert_fee_amount
8    raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))
9AssertionError: Fee of 0.00000993 BTC too high! (Should be 0.00000983 BTC)

Haven’t seen it fail yet locally on the same branch while running:

0(for i in {1..1000}; do test/functional/wallet_send.py --legacy-wallet --pdbonfailure ; done)

This test assertion was added in October 2020 (603c0050837) and the get_fee() test utility helper in October 2021 (80dc829be7f), so if the error is new then it might stem from a more recent change.

jonatack added the label Bug on Jan 25, 2022

MarcoFalke added this to the milestone 23.0 on Jan 25, 2022

jonatack commented at 11:07 am on January 26, 2022: member

Another one, this time wallet_send.py --descriptors and line 559 instead of 332

https://cirrus-ci.com/task/5115109138759680

02022-01-26T10:42:21.513000Z TestFramework (ERROR): Assertion failed 
1Traceback (most recent call last):
2  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 132, in main
3    self.run_test()
4  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_send.py", line 559, in run_test
5    assert_fee_amount(testres["fees"]["base"], testres["vsize"], Decimal(0.0001))
6  File "/tmp/cirrus-ci-build/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/util.py", line 45, in assert_fee_amount
7    raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))
8AssertionError: Fee of 0.00003160 BTC too high! (Should be 0.00003130 BTC)

jonatack renamed this:
~~"Fee too high" CI error in wallet_send.py --legacy-wallet~~
"Fee too high" CI error in wallet_send.py
on Jan 26, 2022

sdaftuar commented at 5:24 pm on January 27, 2022: member

I’m seeing it as well, I think: https://github.com/bitcoin/bitcoin/runs/4969391918 (https://cirrus-ci.com/task/5815596024594432?logs=ci#L4509)

YashPolu commented at 9:36 pm on January 27, 2022: none

@jonatack I would like to work on this. Can I?

jonatack commented at 9:43 pm on January 27, 2022: member

@YashPolu yes, of course, a fix would be great.

YashPolu commented at 9:57 pm on January 27, 2022: none

@jonatack great thank you so much, any suggestions on how to approach this problem

mzumsande commented at 5:15 pm on February 2, 2022: member

See #24239 for a fix for the first failure in wallet_send.py --legacy-wallet line 332.

The other failures in wallet_send.py --descriptors line 559 unfortunately cannot be explained by this: Looking at the numbers, it seems that for some reason the actual tx size deviates by 3 bytes from the one calculated by the wallet (only 2 bytes are tolerated), but I have no idea why.

MarcoFalke referenced this in commit eca694a4e7 on Feb 7, 2022

fanquake commented at 10:21 am on February 22, 2022: member

What is the status of this? Seems like 1 of the 2 issues is fixed, but the other is intermittent, and hasn’t been seen again. Does this issue still need to be one the 23.0 milestone?

MarcoFalke commented at 11:08 am on February 22, 2022: member

I am still running into this issue daily.

https://cirrus-ci.com/task/6564823138828288?logs=ci#L3673

Sjors commented at 12:21 pm on February 22, 2022: member

Seeing it at line 560 now:

https://github.com/bitcoin/bitcoin/blob/8add59d77dd621be57059229f378822e4b707318/test/functional/wallet_send.py#L543-L560

Maybe the input_weight calculation is wrong? Or perhaps too pessimistic, triggering the upper bound in the assert.

The test_send() function picks a new destination address every time its called, which means we can’t predict the length of the ECDSA signature(s) in advance. And this particular test uses multiple inputs. So we should probably increase the tolerance, or use a Taproot wallet so the signature length is fixed.

fanquake commented at 1:47 pm on March 1, 2022: member

After consulting with @achow101 I’m removing this from the 23.0 milestone. This isn’t a blocker, and is a test-only problem.

fanquake removed this from the milestone 23.0 on Mar 1, 2022

fanquake added the label Tests on Mar 1, 2022

MarcoFalke commented at 2:06 pm on March 1, 2022: member

Ehh? This means the tests won’t pass on the release branch. This will be flagging backports and users will be running into issues. But I guess no one is running the test outside the CI?

I presume that this is caused by the “rounding up” thing. So just increasing the range by 1 byte for now might fix this?

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

Maybe the input_weight calculation is wrong? Or perhaps too pessimistic, triggering the upper bound in the assert.

The test_send() function picks a new destination address every time its called, which means we can’t predict the length of the ECDSA signature(s) in advance. And this particular test uses multiple inputs. So we should probably increase the tolerance, or use a Taproot wallet so the signature length is fixed.

I believe this is the case. I think the input_weight calculation is wrong which is putting it at the upper bound of the assert. Then we are occasionally using a signature that is one byte shorter than what the estimator uses, so the final transaction ends up 3 bytes smaller than estimated, which is outside of the assert_approx bounds.

I will have open a PR fixing the calculation.

MarcoFalke commented at 2:09 pm on March 1, 2022: member

This was my fix:

 0diff --git a/test/functional/test_framework/util.py b/test/functional/test_framework/util.py
 1index 210025104e..82a4967e7e 100644
 2--- a/test/functional/test_framework/util.py
 3+++ b/test/functional/test_framework/util.py
 4@@ -40,8 +40,8 @@ def assert_fee_amount(fee, tx_size, feerate_BTC_kvB):
 5     target_fee = get_fee(tx_size, feerate_BTC_kvB)
 6     if fee < target_fee:
 7         raise AssertionError("Fee of %s BTC too low! (Should be %s BTC)" % (str(fee), str(target_fee)))
 8-    # allow the wallet's estimation to be at most 2 bytes off
 9-    high_fee = get_fee(tx_size + 2, feerate_BTC_kvB)
10+    # allow the wallet's estimation to be at most 3 bytes off
11+    high_fee = get_fee(tx_size + 3, feerate_BTC_kvB)
12     if fee > high_fee:
13         raise AssertionError("Fee of %s BTC too high! (Should be %s BTC)" % (str(fee), str(target_fee)))
14

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

#24454 should fix this.

fanquake deleted a comment on Apr 9, 2022

jonatack commented at 4:47 pm on April 12, 2022: member

Keep seeing this, even several times today like https://cirrus-ci.com/task/5823757662027776.

jonatack commented at 4:40 pm on April 21, 2022: member

Still seeing this fail in CI: https://cirrus-ci.com/task/4565158969212928

fanquake closed this on Apr 25, 2022

DrahtBot locked this on Apr 25, 2023

“Fee too high” CI error in wallet_send.py #24151