[Custom Transactions] Add TxBuilder trait, support fixed additional outputs
#3775
Conversation
|
👋 Thanks for assigning @carlaKC as a reviewer! |
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
c245ca1 to
7fc2092
Compare
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
7fc2092 to
6aa0848
Compare
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
fa3ea56 to
14b83a3
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3775 +/- ##
==========================================
+ Coverage 89.53% 90.37% +0.84%
==========================================
Files 157 160 +3
Lines 125310 133578 +8268
Branches 125310 133578 +8268
==========================================
+ Hits 112192 120721 +8529
+ Misses 10429 10231 -198
+ Partials 2689 2626 -63 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🔔 1st Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 3rd Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 4th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 5th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 6th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 7th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 8th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 9th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 10th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 11th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
|
🔔 12th Reminder Hey @TheBlueMatt! This PR has been waiting for your review. |
lightning/src/ln/chan_utils.rs
Outdated
| @@ -622,6 +622,21 @@ impl HTLCOutputInCommitment { | |||
| && self.cltv_expiry == other.cltv_expiry | |||
| && self.payment_hash == other.payment_hash | |||
| } | |||
|
|
|||
| pub(crate) fn is_dust(&self, feerate_per_kw: u32, broadcaster_dust_limit_sat: u64, features: &ChannelTypeFeatures) -> bool { | |||
There was a problem hiding this comment.
If we want the interface to decide this, shouldn't it just be a new flag in HTLCOutputInCommitment rather than a method?
There was a problem hiding this comment.
I appended a commit below that makes this a flag - hopefully I've understood you :) Assuming for now we don't let users of TxBuilder decide which HTLCs are dust-vs-nondust.
| let mut value_to_self_msat_offset = 0; | ||
|
|
||
| let feerate_per_kw = feerate_per_kw.unwrap_or_else(|| self.get_commitment_feerate(funding, generated_by_local)); | ||
|
|
||
| for htlc in self.pending_inbound_htlcs.iter() { | ||
| if htlc.state.included_in_commitment(generated_by_local) { | ||
| if !htlc.is_dust(local, feerate_per_kw, broadcaster_dust_limit_sat, funding.get_channel_type()) { |
There was a problem hiding this comment.
I assume in the future this will be something the TxBuilder decides?
There was a problem hiding this comment.
Overall LN spec is moving in a direction where whether a HTLC is dust is solely a function of its amount and the dust_limit_satoshis p2p setting - so I thought we could let channel make that decision. Could we require all future HTLC-transactions (including customized ones) to pay exogenous fees ? I find them so superior so I thought this was fair.
I have an alternative branch where TxBuilder makes that decision - but it got thorny quite fast (see get_pending_htlc_stats, get_available_balances_for_scope, next_local/remote_commit_tx_fee_sat), but happy to revisit this.
There was a problem hiding this comment.
Overall LN spec is moving in a direction where whether a HTLC is dust is solely a function of its amount and the dust_limit_satoshis p2p setting - so I thought we could let channel make that decision. Could we require all future HTLC-transactions (including customized ones) to pay exogenous fees ? I find them so superior so I thought this was fair.
Hmm, but we also want to be able to sign for existing channel types, presumably, not only anchor-0FC? Also, I could see a future change to the spec removing the dust_limit_satoshis setting and just fixing it to the network level, which would also make it depend on the scriptPubKey type.
I have an alternative branch where TxBuilder makes that decision - but it got thorny quite fast (see get_pending_htlc_stats, get_available_balances_for_scope, next_local/remote_commit_tx_fee_sat), but happy to revisit this.
Yea, I imagine it will require a larger refactor such that those methods can be implemented as a function of build_commitment_stats, but sadly I just don't think its a reasonable assumption to make. Of course that doesn't mean we have to do that now, we can certainly start with that assumption and relax it later.
There was a problem hiding this comment.
Hmm, but we also want to be able to sign for existing channel types, presumably, not only anchor-0FC? Also, I could see a future change to the spec removing the dust_limit_satoshis setting and just fixing it to the network level, which would also make it depend on the scriptPubKey type.
Certainly - but currently I assume this: "only allow anchor, 0FC channels to be customized, and not static only channels."
I do see your point about removing from channel all branches on channel type. IIUC this is not a blocker for this PR let me know.
There was a problem hiding this comment.
Yea, I imagine it will require a larger refactor such that those methods can be implemented as a function of build_commitment_stats, but sadly I just don't think its a reasonable assumption to make. Of course that doesn't mean we have to do that now, we can certainly start with that assumption and relax it later.
Sounds good I'll push a refactor as a follow-up to this PR is that ok ?
There was a problem hiding this comment.
Certainly - but currently I assume this: "only allow anchor, 0FC channels to be customized, and not static only channels."
Hmm, yea, would be nice to just have a "commitment transactions are not the responsibility of channel.rs" goal. Doesn't have to get there now, but eventually would be nice to not have channel.rs have any concept for commitment transactions, only stats on them.
Yes would love to merge this as part of the 0FC PR sequence ! Also would like to merge #3855 first, to make sure whatever API we settle on here fits in nicely with a splicing world; in that world for a single set of included HTLCs based on their state, we generate multiple commitments, so this is relevant to this PR here. |
Oh! Okay, I thought we were gonna do 0FC first, but happy to do this first. |
|
What's left to get this out of draft, then? Want to make quick progress on 0FC. |
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
14b83a3 to
2541f45
Compare
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
0071eff to
5d6baeb
Compare
lightning/src/ln/channel.rs
Outdated
| if match update_state { | ||
| // Note that these match the inclusion criteria when scanning | ||
| // pending_inbound_htlcs below. | ||
| FeeUpdateState::RemoteAnnounced => { |
There was a problem hiding this comment.
Not that its super critical in this case, but when moving code, please move in a separate commit from rustfmt, such that git show --color-moved --color-moved-ws=ignore-space-change can highlight the move.
lightning/src/ln/channel.rs
Outdated
| remote_balance_before_fee_anchors_msat: u64, // remote balance before fees and anchors *not* considering dust limits | ||
| #[derive(Debug, PartialEq)] | ||
| pub(crate) struct CommitmentStats { | ||
| pub(crate) total_fee_sat: u64, // the total fee included in the transaction |
There was a problem hiding this comment.
Wanna go ahead and make the comments documentation?
lightning/src/ln/channel.rs
Outdated
| #[derive(Debug, PartialEq)] | ||
| pub(crate) struct CommitmentStats { | ||
| pub(crate) total_fee_sat: u64, // the total fee included in the transaction | ||
| pub(crate) local_balance_before_fee_msat: u64, // local balance before fees and anchors *not* considering dust limits |
There was a problem hiding this comment.
Comments are now wrong, as the fields are now post-anchors.
lightning/src/ln/channel.rs
Outdated
| @@ -4146,31 +4172,21 @@ where | |||
| { | |||
| let remote_commit_tx_fee_msat = if funding.is_outbound() { 0 } else { | |||
| let htlc_candidate = HTLCCandidate::new(msg.amount_msat, HTLCInitiator::RemoteOffered); | |||
| self.next_remote_commit_tx_fee_msat(funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations | |||
| self.next_remote_commit_tx_fee_msat(&funding, Some(htlc_candidate), None) // Don't include the extra fee spike buffer HTLC in calculations | |||
There was a problem hiding this comment.
Looks like unnecessary diff to add the & before funding here and below.
lightning/src/ln/channel.rs
Outdated
| // `Some(())` is for the fee spike buffer we keep for the remote. This deviates from | ||
| // the spec because the fee spike buffer requirement doesn't exist on the receiver's | ||
| // side, only on the sender's. Note that with anchor outputs we are no longer as | ||
| // sensitive to fee spikes, so we need to account for them. | ||
| // | ||
| // A `None` `HTLCCandidate` is used as in this case because we're already accounting for | ||
| // the incoming HTLC as it has been fully committed by both sides. | ||
| let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(funding, None, Some(())); | ||
| let mut remote_fee_cost_incl_stuck_buffer_msat = self.next_remote_commit_tx_fee_msat(&funding, None, Some(())); |
There was a problem hiding this comment.
Should we include the extra dust-buffer HTLC in the build_commitment_stats call, above, to match?
There was a problem hiding this comment.
Thank you for the suggestion. I looked into this, here's the major difference between the two paths:
next_remote_commit_tx_fee_msatwill set the dust threshold for HTLCs based oncontext.feerate_per_kw- the number of HTLCs passed to
build_commitment_statsabove come fromget_pending_htlc_stats, which itself will set the HTLC dust threshold according tocontext.get_dust_buffer_feerate(outbound_feerate_update)
Therefore number of nondust HTLCs will differ.
Therefore, commit_tx_fee_sat will differ.
lightning/src/ln/chan_utils.rs
Outdated
| cltv_expiry: cltv_expiry.0.unwrap(), | ||
| payment_hash: payment_hash.0.unwrap(), | ||
| transaction_output_index, | ||
| is_dust: transaction_output_index.is_none(), |
There was a problem hiding this comment.
Oh, should this just be an fn that checks if the output index is none? Would imply passing something other than HTLCOutputInCommitment to build_commitment_transaction (as at that point it doesn't have an output index), but IMO that's a cleaner API anyway.
Thanks I pinged Carla how does that sound ? |
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
5d6baeb to
3f98b95
Compare
lightning/src/sign/tx_builder.rs
Outdated
| value_to_self_after_htlcs: u64, value_to_remote_after_htlcs: u64, | ||
| channel_type: &ChannelTypeFeatures, | ||
| ) -> CommitmentStats { | ||
| let total_fee_sat = commit_tx_fee_sat(feerate_per_kw, nondust_htlc_count, channel_type); |
There was a problem hiding this comment.
Would a goal be to allow implementations of TxBuilder to pick their own feerate (ie, not the value that commit_tx_fee_sat would set?
If yes, transaction_fee_satoshis would be wrong in get_available_balances. Doesn't look like this is actually used, but it would surface an incorrect value to end user.
There was a problem hiding this comment.
Thank you ! In that call, I am wondering whether we can subtract the value of the outputs from some channel_value_satoshis value and get the total fee that way... will think more this afternoon !
lightning/src/ln/channel.rs
Outdated
| total_fee_sat, | ||
| local_balance_before_fee_msat, | ||
| remote_balance_before_fee_msat: _, | ||
| } = SpecTxBuilder {}.build_commitment_stats(true, commitment_feerate, MIN_AFFORDABLE_HTLC_COUNT, value_to_self_msat, push_msat, &channel_type); |
There was a problem hiding this comment.
nit: here and a few others, move parameters onto newlines to save us needing to rustfmt them later
lightning/src/ln/channel.rs
Outdated
| if holder_balance_msat < buffer_fee_msat + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { | ||
| let stats = self.build_commitment_stats(funding, true, true, Some(feerate_per_kw), Some(htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize)); | ||
| let holder_balance_msat = stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat; | ||
| // Note that `stats.total_fee_sat` now accounts for any HTLCs that transition from non-dust to dust under a higher feerate (in the case where HTLC-transactions pay endogenous fees). |
There was a problem hiding this comment.
iiuc previously we were using the old commitment feerate to build our commitment transaction (and thus possibly not trimming some HTLCs that became dust), and now we're using the new proposed feerate so we'll properly trim them to dust and account for them in our fees?
Would be nice to have a test before this commit that demonstrates the htcs not being properly trimmed, and then updated in this commit to show that they are.
lightning/src/ln/channel.rs
Outdated
| funding.get_channel_type(), | ||
| ); | ||
|
|
||
| // Note that this is now the capacity available after we have subtracted any non-zero-value anchors |
There was a problem hiding this comment.
(very) nitpicky: remove "this is now" from comment, here and above. Somebody reading this code from fresh won't be looking at the diff, so the comparison to the past state doesn't help them (commit message seems like a a better home for notes on what's changed).
There was a problem hiding this comment.
Certainly I tend to annotate my code for reviewers using comments like these :) Will move this to the git message thank you
lightning/src/ln/channel.rs
Outdated
| // We don't include AwaitingRemoteRevokeToRemove HTLCs because our next commitment | ||
| // transaction won't be generated until they send us their next RAA, which will mean | ||
| // dropping any HTLCs in this state. |
There was a problem hiding this comment.
nit: Move comment up to above the if-let? Bit out of place here
lightning/src/ln/channel.rs
Outdated
| let extra_htlc_commit_tx_fee_sat = TxBuilder::build_commitment_stats(&SpecTxBuilder {}, funding.is_outbound(), excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, 0, 0, funding.get_channel_type()).total_fee_sat; | ||
|
|
||
| let htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); | ||
| let commit_tx_fee_sat = TxBuilder::build_commitment_stats(&SpecTxBuilder {}, funding.is_outbound(), excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + on_counterparty_tx_offered_nondust_htlcs, 0, 0, funding.get_channel_type()).total_fee_sat; |
There was a problem hiding this comment.
I do wonder whether we've got the right API here when we're making calls like this passing zero values for local/remote amounts (and passing a feerate that we don't need in another place).
Did you consider splitting some of the components of CommitmentStats out into their own methods?
There was a problem hiding this comment.
Yes I did, I agree with you I am not thrilled about this - we had a back and forth with Matt on this issue, will refresh on this this afternoon :)
lightning/src/ln/channel.rs
Outdated
| funding.value_to_self_msat.saturating_sub(local_htlc_total_msat), | ||
| (funding.get_value_satoshis() * 1000).checked_sub(funding.value_to_self_msat).unwrap().saturating_sub(remote_htlc_total_msat), |
There was a problem hiding this comment.
Pull these out into their own vars here and in next_local_commit_tx_fee_msat to help readability a bit?
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
3f98b95 to
a9892a9
Compare
There was a problem hiding this comment.
Major changes:
- Split out
build_commitment_stats, see 660b86e - Introduce
IncludedHTLCtype to enforce the dust status of a HTLC onTxBuilderwithout specifying the particular index, see f736d90- We will let
TxBuilderset this status in the next version of this API, for now we focus on its use in zero-fee commitments.
- We will let
Balance::ClaimableOnChannelClose.transaction_fee_satoshisnow includes the sum of the fee due to dust HTLCs, and non-dust HTLCs getting rounded down, see a9892a9- Introduce standalone commit to remove the last raw cast in
build_commitment_stats, see e26b232
Remaining TODOs:
- Append a commit to remove all
rustfmt::skipin the code we touched. - Write a test to demonstrate how we were previously not accounting for the decrease in weight due to newly trimmed HTLCs when checking whether we can afford a new feerate in
can_send_update_fee, see ebd94c2 - Proper git messages
| let extra_htlc_commit_tx_fee_sat = SpecTxBuilder {}.commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); | ||
| let extra_htlc_htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); |
There was a problem hiding this comment.
Question: do we expand the commit_tx_fee_sat call to also pass information back about the fees on HTLC transactions ?
There was a problem hiding this comment.
It wouldn't be my expectation that a method called commit_tx_fee_sat includes second stage fees - the intuitive question I'm looking to answer from that call is "what is the fee on my commitment transaction?"
Would be more in favor of either a total_fee_spend which adds second stage to commit_tx_fee_sat, or just a dedicated endogenous_fees method.
There was a problem hiding this comment.
I think I'll go with the dedicated method - how about htlc_tx_fee_sat ?
|
🔔 1st Reminder Hey @carlaKC! This PR has been waiting for your review. |
lightning/src/ln/channel.rs
Outdated
| } | ||
| } | ||
|
|
||
| macro_rules! get_included_htlc { |
There was a problem hiding this comment.
macro seems a little unnecessary here - could just be a constructor?
There was a problem hiding this comment.
Ended up deleting it - there is another similar macro above, for now it is useful because $htlc can be two different types :) but will take a closer look in follow-ups.
lightning/src/ln/channel.rs
Outdated
| @@ -450,6 +450,15 @@ impl OutboundHTLCOutput { | |||
| } | |||
| } | |||
|
|
|||
| #[derive(Clone, Debug)] | |||
| pub(crate) struct IncludedHTLC { | |||
There was a problem hiding this comment.
nit: I have a minor quibble with the naming here - IncludedHTLC reads like "included in commitment", but then we've got a is_dust field.
There was a problem hiding this comment.
I agree it's awkward ! I had in mind "included" based on the state of the HTLCs, not based on whether it's dust or nondust. In any case, I've deleted the type and delayed further discussion on a potential new type to follow-up work, see comment below.
lightning/src/ln/channel.rs
Outdated
| htlc_source_table.push((table_htlc, $source)); | ||
| htlcs_included.push(included_htlc); |
There was a problem hiding this comment.
Needing to double-track this is a bit code-smelly to me.
WDYT about tracking in a single spot and using an enum to divide between types:
enum CommitmentHTLC {
Dust{ ...IncludedHLTC fields here... },
NonDust(HTLCOutputInCommitment)
}
There was a problem hiding this comment.
I agree about the smell :) Take a look at the diff below, I've consolidated these two lines, and mostly removed the prior IncludedHTLC commit.
- Eventually
TxBuilderwill make the dust-vs-nondust sorting, so don't want to spend too much time on a type that will get deleted soon. - For now we work with the sub-optimal / ambiguous type
HTLCOutputInCommitment, and delay introducing some new type to the PR that will expand thecommit_tx_fee_satcall to include the full list of HTLCs - these will include both "real" HTLCs, and "candidate" HTLCs.
In preparation for the upcoming `TxBuilder` API, we remove any fields in `CommitmentStats` which will not be set by `TxBuilder`.
The upcoming `TxBuilder` API will sum the msat value of any anchors and
custom outputs on commitment transactions into
`{local, remote}_balance_before_fee_msat`.
We previously were not accounting for the decrease in weight due to newly trimmed HTLCs at a proposed higher feerate, and hence overestimated the total fees required to set such feerate on the commitment transaction. While this only applied to `only_static_remote_key` channels, this could have led us to determine that we could not afford a proposed higher feerate when we in fact could afford it.
When the `TxBuilder` API is made public in the future, this API will let creators of custom commitment transactions tell the lightning state machine the fees required to set a given feerate on a commitment transaction with a given number of nondust HTLCs. In upcoming commits, we plan to expand this call with the full list of HTLCs and let `TxBuilder` decide which HTLCs are dust, and which are nondust.
In addition to the amount set by `commit_tx_fee_sat`, the transaction fee of a commitment transaction may also increase due to HTLC amounts that get burned to fees, and any elided anchors. We now include these amounts in the `transaction_fee_satoshis` field of `Balance::ClaimableOnChannelClose` too. This commit also makes the `transaction_fee_satoshis` field correct for custom transactions not encompassed in `chan_utils::commit_tx_fee_sat`.
When the `TxBuilder` API is made public in the future, this trait method will let creators of custom commitment transactions subtract any additional output amounts from the local and remote balances after the HTLCs have been accounted for. We leverage this API to account for today's 330 sat anchors.
When the `TxBuilder` API is made public in the future, this trait method will let implementers build custom commitment transactions as part of the lightning state machine.
tankyleo
force-pushed
the
fixed-addl-outputs
branch
from
1080b0e to
39cb0a3
Compare
|
Squashed with minor changes:
See https://github.com/tankyleo/rust-lightning/tree/fixed-up-addl-outputs for the branch containing fixups ending at 1080b0e |
|
@TheBlueMatt If that's ok with you I'll come up with a type that is better than the current |
| let buffer_fee_msat = commit_tx_fee_sat(feerate_per_kw, commitment_data.tx.nondust_htlcs().len() + htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize, funding.get_channel_type()) * 1000; | ||
| let holder_balance_msat = commitment_data.stats.local_balance_before_fee_msat - htlc_stats.outbound_holding_cell_msat; | ||
| if holder_balance_msat < buffer_fee_msat + funding.counterparty_selected_channel_reserve_satoshis.unwrap() * 1000 { | ||
| let stats = self.build_commitment_stats(funding, true, true, Some(feerate_per_kw), Some(htlc_stats.on_holder_tx_outbound_holding_cell_htlcs_count as usize + CONCURRENT_INBOUND_HTLC_FEE_BUFFER as usize)); |
There was a problem hiding this comment.
Combining on_holder_tx_outbound_holding_cell_htlcs_count and CONCURRENT_INBOUND_HTLC_FEE_BUFFER in a param called fee_buffer_nondust_htlcs is a little confusing to me - one is a set of pending HTLCs we know we'll need to account for, the other part of fee spike buffer.
Perhaps this can just be num_htlcs like we have it in commit_tx_fee_sat? Or num_pending_htlcs.
There was a problem hiding this comment.
100% agreed on the bad name here - would you take something like extra_htlcs ? To emphasize that these are HTLCs added on top of the ones build_commitment_stats will count by default.
There was a problem hiding this comment.
the other part of fee spike buffer
A clarification here: as far as I see, this is meant to account for the case where the counterparty sends an update_add_htlc at the same time we send update_fee
| let extra_htlc_commit_tx_fee_sat = SpecTxBuilder {}.commit_tx_fee_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1 + on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); | ||
| let extra_htlc_htlc_tx_fees_sat = chan_utils::htlc_tx_fees_sat(excess_feerate, on_counterparty_tx_accepted_nondust_htlcs + 1, on_counterparty_tx_offered_nondust_htlcs, funding.get_channel_type()); |
There was a problem hiding this comment.
It wouldn't be my expectation that a method called commit_tx_fee_sat includes second stage fees - the intuitive question I'm looking to answer from that call is "what is the fee on my commitment transaction?"
Would be more in favor of either a total_fee_spend which adds second stage to commit_tx_fee_sat, or just a dedicated endogenous_fees method.
| let mut removed_outbound_total_msat = 0; | ||
| for htlc in self.pending_outbound_htlcs.iter() { | ||
| if let OutboundHTLCState::AwaitingRemoteRevokeToRemove(OutboundHTLCOutcome::Success(_)) | OutboundHTLCState::AwaitingRemovedRemoteRevoke(OutboundHTLCOutcome::Success(_)) = htlc.state { | ||
| removed_outbound_total_msat += htlc.amount_msat; | ||
| } |
There was a problem hiding this comment.
nit: nice opportunity to change this to filter_map + sum while we're here, likewise below
There was a problem hiding this comment.
Thank you I like that much better !
hi @TheBlueMatt on this branch here i make an attempt at an API that supports "fixed additional outputs" on commitment transactions.
I place today's 330 sat anchors in that bucket, but the API should allow for custom ones.
The API should also let people customize all the existing outputs on the commitment transaction.
Still need to think about any remaining TODOs for additional outputs that are only present for some commitments (eg. DLC outputs), depending on how we'd like to prioritize these.