Closing weight fix #5004
Merged
Closing weight fix #5004
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ill be. We had an out-by-two error when calculating weights, because we grab weights on unsigned txs. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Compare against lightningd's, and the actual tx. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
Saves us actually creating the witness to measure it. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/closing-weight-fix
branch
from
0fdb8aa to
5aaa03f
Compare
vincenzopalazzo
approved these changes
Jan 12, 2022
|
ACK 5aaa03f |
rustyrussell
force-pushed
the
guilt/closing-weight-fix
branch
from
5aaa03f to
ca593a4
Compare
|
More elements fixes: @cdecker please review! |
And in particular, fix onchaind grinding code which used the actual number of inputs and outputs (which already includes the fee output); that breaks with the next patch which fixes other calculations. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
This fixes lightningd's chronic weight underestimate. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au> Changelog-Fixed: closingd: more accurate weight estimation helps mutual closing near min/max feerates.
Firstly, we were not adding the extra fee output on our dummy tx, because the fee amount was 0. We probably should always do this, even if it's 0. Secondly, there are 6 witnesses, not 1, for elements txs. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
And skip some tests: I'd simply be pasting in the results, which is not very useful. Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
rustyrussell
force-pushed
the
guilt/closing-weight-fix
branch
from
ca593a4 to
d6e603e
Compare
vincenzopalazzo
approved these changes
Jan 26, 2022
There was a problem hiding this comment.
LGTM, but I'm a little bit out of context with elements
ack d6e603e
|
ACK d6e603e |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I was getting regular failures in the #4984 branch. This was caused by the closing fee estimation being wrong in elements, however, AFAICT the flaky retester has been covering it up (prior to this it would sometimes pass).
In debugging this, I discovered that our non-elements closing weight was also wrong, though not enough to trigger an error. New test, more debugging...
Can we remove the flaky stuff please after release? It's just deferring pain, not removing it!