-
Notifications
You must be signed in to change notification settings - Fork 412
Add missing pending FundingScope
checks
#3811
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
Add missing pending FundingScope
checks
#3811
Conversation
👋 Thanks for assigning @wpaulino as a reviewer! |
👋 The first review has been submitted! Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer. |
bc3ede2
to
30ac751
Compare
Good to squash |
30ac751
to
c6b2d01
Compare
Oops, sorry more fixups to address lint checks. |
@wpaulino Any thoughts on moving the helpers to |
Hm yeah I guess we could, let's do the move in its own commit then. Feel free to squash also. |
If there are any pending splices when an update_add_htlc message is received, it must be validated against each pending FundingScope. Otherwise, the HTLC could be invalid once the splice is locked.
If there are any pending splices when a revoke_and_ack message is received, FundingScope::value_to_self_msat needs to be updated for each. Otherwise, the promoted FundingScope will be invalid when the splice is locked.
If there are any pending splices when an update_fee message is received, it must be validated against each pending FundingScope. Otherwise, it may be invalid once the splice is locked.
If there are any pending splices when an sending an update_fee message, the new fee rate must be validated against each pending FundingScope. Otherwise, it may be invalid once the splice is locked.
If there are any pending splices when an accepting an incoming HTLC, the HTLC needs to be validated against each pending FundingScope. Otherwise, once the splice is locked, the HTLC could have been failed when it should have been forwarded / claimed, or vice versa, under the promoted FundingScope.
c6b2d01
to
b6a92da
Compare
Made a separate commit. Squashed fixups except for the last one in case the second reviewer disagrees on dropping that check. |
🔔 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. |
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.
LGTM
|
||
for funding in core::iter::once(&mut self.funding).chain(self.pending_funding.iter_mut()) { | ||
funding.value_to_self_msat = (funding.value_to_self_msat as i64 + value_to_self_msat_diff) as u64; | ||
} | ||
|
||
if let Some((feerate, update_state)) = self.context.pending_update_fee { | ||
match update_state { |
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.
Can't comment there, but a few lines down funding.is_outbound()
is asserted around the pending fee update. I presume is_outbound
should move out of funding
as its something that now must remain static across splices (and it being in funding
implies that we can change that).
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.
It actually comes from ChannelTransactionParameters::is_outbound_from_holder
and is further needed in the DirectedChannelTransactionParameters::is_outbound
method. Not sure how involved it would be to refactor it out into ChannelContext
. May not be much work but we'd need to worry about serialization compatibility. Perhaps best to leave as a TODO / follow-up?
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.
Yea, it doesn't have to happen here. There's lots of cleanups to do on channel.rs
...
@@ -8080,70 +8207,10 @@ impl<SP: Deref> FundedChannel<SP> where | |||
} | |||
|
|||
let dust_exposure_limiting_feerate = self.context.get_dust_exposure_limiting_feerate(&fee_estimator); |
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.
nit: seems somewhat wasteful to to pass this in to can_accept_incoming_htlc_for_funding
instead of just fetching it from context there?
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.
Hmm... I guess it would depend on how the FeeEstimator
is implemented or any timing concerns. By passing it as a parameter, we are guaranteed to use the same value for all FundingScope
s.
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.
Ah, right, makes sense.
Feel free to squash the squash commit i think. |
This case is already covered by LocalHTLCFailureReason::HTLCMaximum, so is not needed.
Previous commits refactored validation checks in FundedChannel to work on a specific FundingScope. However, keeping these helpers in FundedChannel doesn't prevent them from using self.funding inadvertently instead of the passed in FundingScope. Move these helpers to ChannelContext to avoid this problem, as we done with similar helpers.
b6a92da
to
368e41b
Compare
Squashed fixup. |
When sending or receiving
update_add_htlc
,update_fee
, orrevoke_and_ack
messages, check that the messages (or amount or fee rates, as is appropriate) are valid for any pendingFundingScope
. Otherwise, the promotedFundingScope
will be invalid when the splice is locked.This assumes
FundingScope::is_outbound
is the same across allFundingScope
s. This PR does not fix similar issues for funding negotiation and funding confirmation, which should be handled in #3736 and #3741, respectively.