Anchor outputs fee fixes #2587
Anchor outputs fee fixes #2587
Conversation
wpaulino
force-pushed
the
anchors-fee-fixes
branch
from
200e802
to
308ea2f
Compare
Codecov ReportPatch coverage:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #2587 +/- ##
==========================================
- Coverage 90.61% 88.77% -1.85%
==========================================
Files 113 113
Lines 59251 84489 +25238
Branches 59251 84489 +25238
==========================================
+ Hits 53693 75007 +21314
- Misses 5558 7256 +1698
- Partials 0 2226 +2226
☔ View full report in Codecov by Sentry. |
wpaulino
force-pushed
the
anchors-fee-fixes
branch
from
308ea2f
to
f1ad5f1
Compare
Since we don't know the total input amount of an external claim (those which come anchor channels), we can't limit our feerate bumps by the amount of funds we have available to use. Instead, we choose to limit it by a margin of the new feerate estimate.
We'd previously ignore the existing amount transactions were already attempting to spend when deciding whether we should add more inputs throughout coin selection. This would result in us attaching more inputs than necessary to satisfy our target amount. In the case of HTLC transactions, we'd burn the HTLC amount completely, since the pre-signed transaction has zero fee (input amount == output amount). Along the way, we also fix the slight overpayment in anchor transactions. We now properly account for the fees the transaction already paid for, simply by pretending the fees are part of the anchor input amount.
We add a few debug assertions to ensure we don't overpay fees by 5% more than expected.
wpaulino
force-pushed
the
anchors-fee-fixes
branch
from
f1ad5f1
to
9736c70
Compare
| // estimate. | ||
| let previous_feerate = self.feerate_previous.try_into().unwrap_or(u32::max_value()); | ||
| let mut new_feerate = previous_feerate.saturating_add(previous_feerate / 4); | ||
| if new_feerate > feerate_estimate * 5 { |
There was a problem hiding this comment.
From my understanding, the new code now upper bounds our effective new_feeratee to a multiplicand of feerate_estimate. If FeeEstimator is broken or our underlying inbound transaction-relay has been partitioned from the rest of the network and the sat_per_1000_weight we got is 1 sat-per-byte, we might never confirm our transaction ?
There was a problem hiding this comment.
Yep. There's not really anything else we can do - I hit a case where a commitment transaction didn't get into the mempool because of its low feerate, and the package logic here ended up reaching a feerate of u32::max_value(), which would have spent all available funds on one transaction the second the commitment transaction got into the mempool. Even after package relay a similar issue exists if the user is offline for some time. Trusting the fee estimator sucks, indeed, but spending all available user funds on a single transaction sucks more. We could bump the minimum here to add a constant and ignore the fee estimator if the constant is > feerate_estimate * 5, but that's really the best we can do.
This PR attempts to address a few bugs discovered by @TheBlueMatt while using anchor outputs channels.
Fixes #2586.