Correct handling of added OP_RETURN outputs
#3285
Merged
Conversation
This file contains hidden or 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
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3285 +/- ##
========================================
Coverage 89.82% 89.82%
========================================
Files 126 126
Lines 103134 103449 +315
Branches 103134 103449 +315
========================================
+ Hits 92643 92926 +283
- Misses 7772 7800 +28
- Partials 2719 2723 +4 ☔ View full report in Codecov by Sentry. |
jkczyz
reviewed
Sep 3, 2024
While these are consensus-valid, they have been nonstandard for quite some time and will not relay nor confirm.
TheBlueMatt
force-pushed
the
2024-08-tx-too-small
branch
from
299b9bd to
4cd9a86
Compare
When we do coin selection for channel close anchor spends, we may do coin selection targeting exactly the input values we need. However, if coin selection does not include a change output, we may add an OP_RETURN output, which may cause us to end up with less fee than we wanted on the resulting package. Here we address this issue by running coin selection twice - first without seeking the extra weight of the OP_RETURN output, and again if we find that we under-selected.
We should always select at least as many coins as is required to meet the feerate target, but its perfectly fine if we overshoot. Specifically, we may overshoot deliberately if we choose to burn change to fee instead.
This adds a single test for coin selection which exercises the issues fixed in the past three commits.
TheBlueMatt
force-pushed
the
2024-08-tx-too-small
branch
from
4cd9a86 to
5a2372c
Compare
dunxen
previously approved these changes
Sep 3, 2024
`ANCHOR_INPUT_WITNESS_WEIGHT` is too high by two weight units, likely it was calculated to include the SegWit marker bytes, but it is used to describe an `Input::satisfaction_weight`, which does not expect the marker bytes. This corrects that oversight, reducing the constant by two and adding the marker bytes back in our own internal weight calculations. It also fixes a second issue where the constant was too low by one when `grind_signatures` is not set, as that may result in a signature being one byte longer than we expect.
|
Ugh, build failed with |
|
Note that I assume there is a similar bug in |
dunxen
approved these changes
Sep 3, 2024
jkczyz
approved these changes
Sep 3, 2024
This was referenced Sep 3, 2024
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.
This fixes two bugs in handling of coin selections that don't contain change, both of them pretty minor (in part cause we'll always re-select soon anyway and that selection might well contain change) but also important.