-
Notifications
You must be signed in to change notification settings - Fork 969
Fix test_peer_anchor_push error due to variable signature length #8797
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| # create 71-byte sigs always! | ||
| def did_short_sig(node): | ||
| # This can take a moment to appear in the log! | ||
| time.sleep(1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be unrelated. The signal that we can check is rather fuzzy, being a timeout. It'd be more stable if we had something concrete to wait for, such as a log, or we could poll rather than checking once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I increased it from 1s to 2s because the log message "overgrind: short signature length" sometimes appears with a small delay, and 1s wasn't always enough to catch it reliably.
Would it be better to use wait_for() to poll the log instead? Something like:
def did_short_sig(node): return wait_for()
|
|
||
| def check_feerate(nodes, actual_feerate, expected_feerate): | ||
| # Feerate can't be lower. | ||
| assert actual_feerate > expected_feerate - 2 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is pytest.approx() we could use to check if a number if close-ish to another number. The widening to tolerance=10 seems a bit much, and may actually be covering a real issue. Can you share what you are seeing and why this is addressing it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test failed with - actual_feerate (14006.1) > expected_feerate (14000) + 2, this happens because ECDSA signatures have variable byte length 71-72 bytes, but sometimes shorter. And when we get a shorter signature - the transaction weight decreases - and that causes higher feerate for the same fee!
For example with fee = 32150 sat:
Normal weight 2294 - 32150 * 1000 / 2294 = 14000 sat/kw, but
short sig weight 2293 - 32150 * 1000 / 2293 = 14006.1 sat/kw!
So the 1-2 weight difference can cause 4-8 sat.kw difference, and i've thought that tolerance = 10 can cover this natural difference.
Would pytest.approx(rel=0.001) be better? Or should I calculate the exact tolerance based on expected weight variance?
d9e74ec to
96c8d5b
Compare
…th (Fixes ElementsProject#8493) Changelog-None
0c392c8 to
4cdfc21
Compare
(Fixes #8493)
Problem
The test_peer_anchor_push test was failing with AssertionError when the actual feerate exceeded the expected by more than 2 sat/kw. This occurred because ECDSA signatures have variable length, typically 71-72 bytes, but can be shorter. A shorter signature means less transaction weight, which bumps up the feerate even though the fee stays the same!
Solution
Important
26.04 FREEZE March 11th: Non-bugfix PRs not ready by this date will wait for 26.06.
RC1 is scheduled on March 23rd
The final release is scheduled for April 15th.
Checklist
Before submitting the PR, ensure the following tasks are completed. If an item is not applicable to your PR, please mark it as checked:
tools/lightning-downgrade