Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Incl tx fee when calcing inbound+outbound HTLC limits on channels #577

Merged
merged 3 commits into from
Jun 15, 2020
Merged

Incl tx fee when calcing inbound+outbound HTLC limits on channels #577

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
283c947
Refactor: move channel checks for HTLC adds into Channel
valentinewallace Jun 5, 2020
c9926b9
Add fee spike buffer + incl commit tx fee in chan reserve calculation
valentinewallace May 6, 2020
17ccab4
Update chanmon fuzzer to include small payment actions.
valentinewallace May 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 30 additions & 12 deletions fuzz/src/chanmon_consistency.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {

loop {
macro_rules! send_payment {
($source: expr, $dest: expr) => { {
($source: expr, $dest: expr, $amt: expr) => { {
let payment_hash = Sha256::hash(&[payment_id; 1]);
payment_id = payment_id.wrapping_add(1);
if let Err(_) = $source.send_payment(&Route {
Expand All @@ -417,15 +417,15 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
node_features: NodeFeatures::empty(),
short_channel_id: $dest.1,
channel_features: ChannelFeatures::empty(),
fee_msat: 5000000,
fee_msat: $amt,
cltv_expiry_delta: 200,
}]],
}, PaymentHash(payment_hash.into_inner()), &None) {
// Probably ran out of funds
test_return!();
}
} };
($source: expr, $middle: expr, $dest: expr) => { {
($source: expr, $middle: expr, $dest: expr, $amt: expr) => { {
let payment_hash = Sha256::hash(&[payment_id; 1]);
payment_id = payment_id.wrapping_add(1);
if let Err(_) = $source.send_payment(&Route {
Expand All @@ -441,7 +441,7 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
node_features: NodeFeatures::empty(),
short_channel_id: $dest.1,
channel_features: ChannelFeatures::empty(),
fee_msat: 5000000,
fee_msat: $amt,
cltv_expiry_delta: 200,
}]],
}, PaymentHash(payment_hash.into_inner()), &None) {
Expand Down Expand Up @@ -644,12 +644,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
});
for event in events.drain(..) {
match event {
events::Event::PaymentReceived { payment_hash, payment_secret, .. } => {
events::Event::PaymentReceived { payment_hash, payment_secret, amt } => {
if claim_set.insert(payment_hash.0) {
if $fail {
assert!(nodes[$node].fail_htlc_backwards(&payment_hash, &payment_secret));
} else {
assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, 5_000_000));
assert!(nodes[$node].claim_funds(PaymentPreimage(payment_hash.0), &payment_secret, amt));
}
}
},
Expand Down Expand Up @@ -691,12 +691,12 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
nodes[2].channel_monitor_updated(&chan_2_funding, *id);
}
},
0x09 => send_payment!(nodes[0], (&nodes[1], chan_a)),
0x0a => send_payment!(nodes[1], (&nodes[0], chan_a)),
0x0b => send_payment!(nodes[1], (&nodes[2], chan_b)),
0x0c => send_payment!(nodes[2], (&nodes[1], chan_b)),
0x0d => send_payment!(nodes[0], (&nodes[1], chan_a), (&nodes[2], chan_b)),
0x0e => send_payment!(nodes[2], (&nodes[1], chan_b), (&nodes[0], chan_a)),
0x09 => send_payment!(nodes[0], (&nodes[1], chan_a), 5_000_000),
0x0a => send_payment!(nodes[1], (&nodes[0], chan_a), 5_000_000),
0x0b => send_payment!(nodes[1], (&nodes[2], chan_b), 5_000_000),
0x0c => send_payment!(nodes[2], (&nodes[1], chan_b), 5_000_000),
0x0d => send_payment!(nodes[0], (&nodes[1], chan_a), (&nodes[2], chan_b), 5_000_000),
0x0e => send_payment!(nodes[2], (&nodes[1], chan_b), (&nodes[0], chan_a), 5_000_000),
0x0f => {
if !chan_a_disconnected {
nodes[0].peer_disconnected(&nodes[1].get_our_node_id(), false);
Expand Down Expand Up @@ -781,6 +781,24 @@ pub fn do_test<Out: test_logger::Output>(data: &[u8], out: Out) {
},
0x22 => send_payment_with_secret!(nodes[0], (&nodes[1], chan_a), (&nodes[2], chan_b)),
0x23 => send_payment_with_secret!(nodes[2], (&nodes[1], chan_b), (&nodes[0], chan_a)),
0x25 => send_payment!(nodes[0], (&nodes[1], chan_a), 10),
0x26 => send_payment!(nodes[1], (&nodes[0], chan_a), 10),
0x27 => send_payment!(nodes[1], (&nodes[2], chan_b), 10),
0x28 => send_payment!(nodes[2], (&nodes[1], chan_b), 10),
0x29 => send_payment!(nodes[0], (&nodes[1], chan_a), (&nodes[2], chan_b), 10),
0x2a => send_payment!(nodes[2], (&nodes[1], chan_b), (&nodes[0], chan_a), 10),
0x2b => send_payment!(nodes[0], (&nodes[1], chan_a), 1_000),
0x2c => send_payment!(nodes[1], (&nodes[0], chan_a), 1_000),
0x2d => send_payment!(nodes[1], (&nodes[2], chan_b), 1_000),
0x2e => send_payment!(nodes[2], (&nodes[1], chan_b), 1_000),
0x2f => send_payment!(nodes[0], (&nodes[1], chan_a), (&nodes[2], chan_b), 1_000),
0x30 => send_payment!(nodes[2], (&nodes[1], chan_b), (&nodes[0], chan_a), 1_000),
0x31 => send_payment!(nodes[0], (&nodes[1], chan_a), 100_000),
0x32 => send_payment!(nodes[1], (&nodes[0], chan_a), 100_000),
0x33 => send_payment!(nodes[1], (&nodes[2], chan_b), 100_000),
0x34 => send_payment!(nodes[2], (&nodes[1], chan_b), 100_000),
0x35 => send_payment!(nodes[0], (&nodes[1], chan_a), (&nodes[2], chan_b), 100_000),
0x36 => send_payment!(nodes[2], (&nodes[1], chan_b), (&nodes[0], chan_a), 100_000),
// 0x24 defined above
_ => test_return!(),
}
Expand Down
184 changes: 167 additions & 17 deletions lightning/src/ln/channel.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ pub struct ChannelValueStat {
pub pending_inbound_htlcs_amount_msat: u64,
pub holding_cell_outbound_amount_msat: u64,
pub their_max_htlc_value_in_flight_msat: u64, // outgoing
pub their_dust_limit_msat: u64,
}

enum InboundHTLCRemovalReason {
Expand Down Expand Up @@ -1663,8 +1664,83 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
cmp::min(self.value_to_self_msat as i64 - self.get_outbound_pending_htlc_stats().1 as i64, 0) as u64)
}

pub fn update_add_htlc(&mut self, msg: &msgs::UpdateAddHTLC, pending_forward_state: PendingHTLCStatus) -> Result<(), ChannelError> {
if (self.channel_state & (ChannelState::ChannelFunded as u32 | ChannelState::RemoteShutdownSent as u32)) != (ChannelState::ChannelFunded as u32) {
// Get the fee cost of a commitment tx with a given number of HTLC outputs.
// Note that num_htlcs should not include dust HTLCs.
fn commit_tx_fee_msat(&self, num_htlcs: usize) -> u64 {
// Note that we need to divide before multiplying to round properly,
// since the lowest denomination of bitcoin on-chain is the satoshi.
(COMMITMENT_TX_BASE_WEIGHT + num_htlcs as u64 * COMMITMENT_TX_WEIGHT_PER_HTLC) * self.feerate_per_kw / 1000 * 1000
}

// Get the commitment tx fee for the local (i.e our) next commitment transaction
// based on the number of pending HTLCs that are on track to be in our next
// commitment tx. `addl_htcs` is an optional parameter allowing the caller
// to add a number of additional HTLCs to the calculation. Note that dust
// HTLCs are excluded.
fn next_local_commit_tx_fee_msat(&self, addl_htlcs: usize) -> u64 {
assert!(self.channel_outbound);

let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
for ref htlc in self.pending_outbound_htlcs.iter() {
if htlc.amount_msat / 1000 <= self.our_dust_limit_satoshis {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be preferable to multiple self.our_dust_limit_satoshis by 1000 instead to avoid losing precision?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC we want to be checking the rounded version since millisats are meaningless on-chain. Extrapolating from #577 (comment) that when we're dealing with amounts that relate to on-chain, always round down

continue
}
match htlc.state {
OutboundHTLCState::Committed => their_acked_htlcs += 1,
OutboundHTLCState::RemoteRemoved {..} => their_acked_htlcs += 1,
OutboundHTLCState::LocalAnnounced {..} => their_acked_htlcs += 1,
_ => {},
}
}

for htlc in self.holding_cell_htlc_updates.iter() {
match htlc {
&HTLCUpdateAwaitingACK::AddHTLC { .. } => their_acked_htlcs += 1,
_ => {},
}
}

self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
}

// Get the commitment tx fee for the remote's next commitment transaction
// based on the number of pending HTLCs that are on track to be in their
// next commitment tx. `addl_htcs` is an optional parameter allowing the caller
// to add a number of additional HTLCs to the calculation. Note that dust HTLCs
// are excluded.
fn next_remote_commit_tx_fee_msat(&self, addl_htlcs: usize) -> u64 {
assert!(!self.channel_outbound);

// When calculating the set of HTLCs which will be included in their next
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
// commitment_signed, all inbound HTLCs are included (as all states imply it will be
// included) and only committed outbound HTLCs, see below.
let mut their_acked_htlcs = self.pending_inbound_htlcs.len();
for ref htlc in self.pending_outbound_htlcs.iter() {
if htlc.amount_msat / 1000 <= self.their_dust_limit_satoshis {
continue
}
// We only include outbound HTLCs if it will not be included in their next
// commitment_signed, i.e. if they've responded to us with an RAA after announcement.
match htlc.state {
OutboundHTLCState::Committed => their_acked_htlcs += 1,
OutboundHTLCState::RemoteRemoved {..} => their_acked_htlcs += 1,
_ => {},
}
}

self.commit_tx_fee_msat(their_acked_htlcs + addl_htlcs)
}

pub fn update_add_htlc<F>(&mut self, msg: &msgs::UpdateAddHTLC, mut pending_forward_status: PendingHTLCStatus, create_pending_htlc_status: F) -> Result<(), ChannelError>
where F: for<'a> Fn(&'a Self, PendingHTLCStatus, u16) -> PendingHTLCStatus {
// We can't accept HTLCs sent after we've sent a shutdown.
let local_sent_shutdown = (self.channel_state & (ChannelState::ChannelFunded as u32 | ChannelState::LocalShutdownSent as u32)) != (ChannelState::ChannelFunded as u32);
if local_sent_shutdown {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why assign product of channel state comparison and then test this assignment instead of testing channel state directly ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to make it readable and clear what's being checked. The variable makes it explicit that we're trying to check whether the local has sent a shutdown msg

pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x1000|20);
}
// If the remote has sent a shutdown prior to adding this HTLC, then they are in violation of the spec.
let remote_sent_shutdown = (self.channel_state & (ChannelState::ChannelFunded as u32 | ChannelState::RemoteShutdownSent as u32)) != (ChannelState::ChannelFunded as u32);
if remote_sent_shutdown {
return Err(ChannelError::Close("Got add HTLC message when channel was not in an operational state"));
}
if self.channel_state & (ChannelState::PeerDisconnected as u32) == ChannelState::PeerDisconnected as u32 {
Expand Down Expand Up @@ -1708,9 +1784,55 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
removed_outbound_total_msat += htlc.amount_msat;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@TheBlueMatt

I don't get the rational between the difference in the way we account inbound HTLCs and outbound HTLCs. For any inbound HTLCs we evaluate as part of our balance independently of its state. We presume its resolution is favorable to us, even if it maybe not the case, which is reasonable without more tracking complexity.

Instead for outbound HTLC we account them in counterparty balance only after its removal announcement, independently of a success or failure. Why we don't also presume outbound HTLCs resolution are always favorable to them ? I.e accounting in their balance once we announce outbound HTLCs

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont really get your question. We should not be assuming any kind of HTLC resolution here, nor do I think we do - we only ever consider HTLCs which have begun the removal process.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we talked about this offline but after reading conv again it was a still a misunderstood. I think we're making a different assumption for outbound, namely we don't consider that a committed outbound will resolve favorably for them.

Why we don't account OutboundHTLCState::Committed in part of their balance, removed_outbound_total_msat ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still a bit confused. Note that the "None" here means "there is no HTLCFailReason and the claim has resolved in our favor"

}
}
if htlc_inbound_value_msat + msg.amount_msat + self.value_to_self_msat > (self.channel_value_satoshis - Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis)) * 1000 + removed_outbound_total_msat {
return Err(ChannelError::Close("Remote HTLC add would put them under their reserve value"));

let pending_value_to_self_msat =
self.value_to_self_msat + htlc_inbound_value_msat - removed_outbound_total_msat;
let pending_remote_value_msat =
self.channel_value_satoshis * 1000 - pending_value_to_self_msat;
if pending_remote_value_msat < msg.amount_msat {
return Err(ChannelError::Close("Remote HTLC add would overdraw remaining funds"));
}

// Check that the remote can afford to pay for this HTLC on-chain at the current
// feerate_per_kw, while maintaining their channel reserve (as required by the spec).
let remote_commit_tx_fee_msat = if self.channel_outbound { 0 } else {
// +1 for this HTLC.
self.next_remote_commit_tx_fee_msat(1)
};
if pending_remote_value_msat - msg.amount_msat < remote_commit_tx_fee_msat {
return Err(ChannelError::Close("Remote HTLC add would not leave enough to pay for fees"));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the spec deviation right ? Please at least add a comment for it and modify commit message to say we fail channel. Anyway that may not conjugate well with anchor output, where once implemented a initiator-sender consider it's always good to send a HTLC if above reserve because you can always bump ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, don't believe this is a deviation. Curious why you think it's a deviation?
From the BOLT 2:

update_add_htlc
A sending node:
    if it is responsible for paying the Bitcoin fee:
        MUST NOT offer amount_msat if, after adding that HTLC to its commitment transaction, it cannot pay the fee for either the local or remote commitment transaction at the current feerate_per_kw while maintaining its channel reserve

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAICT, this is a requirement on the sender but here we're at HTLC reception. It makes sense as that way you avoid to close channel even if sender violates spec. I think channel can still be unstuck if this HTLC is failed and sender-initiator balance is increased again without waiting feerate decrease.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
     receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
            SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah but there's no fee spike buffer check here?

Here's the deviating fee spike buffer check: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1811-R1818

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe misunderstood is due to the spec saying

update_add_htlc
A receiving node:
     receiving an amount_msat that the sending node cannot afford at the current `feerate_per_kw` (while maintaining its channel reserve):
            SHOULD fail the channel

It doesn't require to enforce fee buffer spikes.

Right, this check is intended to purely perform the check that you quote above here ^

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After IRC discussion, this is right, I confused myself with what this check was enforcing, but still you should have point out the receiver spec-side :p

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fair, lol

Copy link
Contributor Author

@valentinewallace valentinewallace Jun 12, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw the IRC discussion -- added clarifying comment https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1796

};

let chan_reserve_msat =
Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis) * 1000;
if pending_remote_value_msat - msg.amount_msat - remote_commit_tx_fee_msat < chan_reserve_msat {
return Err(ChannelError::Close("Remote HTLC add would put them under remote reserve value"));
}

if !self.channel_outbound {
// `+1` for this HTLC, `2 *` and `+1` fee spike buffer we keep for the remote. This deviates from the
// spec because in the spec, the fee spike buffer requirement doesn't exist on the receiver's side,
// only on the sender's.
// Note that when we eventually remove support for fee updates and switch to anchor output fees,
// we will drop the `2 *`, since we no longer be as sensitive to fee spikes. But, keep the extra +1
// as we should still be able to afford adding this HTLC plus one more future HTLC, regardless of
// being sensitive to fee spikes.
let remote_fee_cost_incl_stuck_buffer_msat = 2 * self.next_remote_commit_tx_fee_msat(1 + 1);
if pending_remote_value_msat - msg.amount_msat - chan_reserve_msat < remote_fee_cost_incl_stuck_buffer_msat {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why we don't check that sender-initiator can also afford this new HTLC with regards to our local commitment transaction fee ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what this is supposed to be doing: https://github.com/rust-bitcoin/rust-lightning/pull/577/files#diff-79ec46664bd0cf85b6be0ddb567527a7R1830. Though it also takes into account the channel reserve, which we discuss further below.

// Note that if the pending_forward_status is not updated here, then it's because we're already failing
// the HTLC, i.e. its status is already set to failing.
pending_forward_status = create_pending_htlc_status(self, pending_forward_status, 0x1000|7);
}
} else {
// Check that they won't violate our local required channel reserve by adding this HTLC.

// +1 for this HTLC.
let local_commit_tx_fee_msat = self.next_local_commit_tx_fee_msat(1);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you set this to 0 the tests all still pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, when I set this to 0 test_chan_reserve_violation_inbound_htlc_outbound_channel fails (as is the intention of the test). Would you mind double checking?

if self.value_to_self_msat < self.local_channel_reserve_satoshis * 1000 + local_commit_tx_fee_msat {
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
return Err(ChannelError::Close("Cannot receive value that would put us under local channel reserve value"));
}
}

if self.next_remote_htlc_id != msg.htlc_id {
return Err(ChannelError::Close("Remote skipped HTLC ID"));
}
Expand All @@ -1719,7 +1841,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
}

if self.channel_state & ChannelState::LocalShutdownSent as u32 != 0 {
if let PendingHTLCStatus::Forward(_) = pending_forward_state {
if let PendingHTLCStatus::Forward(_) = pending_forward_status {
panic!("ChannelManager shouldn't be trying to add a forwardable HTLC after we've started closing");
}
}
Expand All @@ -1731,7 +1853,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
amount_msat: msg.amount_msat,
payment_hash: msg.payment_hash,
cltv_expiry: msg.cltv_expiry,
state: InboundHTLCState::RemoteAnnounced(pending_forward_state),
state: InboundHTLCState::RemoteAnnounced(pending_forward_status),
});
Ok(())
}
Expand Down Expand Up @@ -3036,6 +3158,7 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
res
},
their_max_htlc_value_in_flight_msat: self.their_max_htlc_value_in_flight_msat,
their_dust_limit_msat: self.their_dust_limit_satoshis * 1000,
}
}

Expand Down Expand Up @@ -3546,40 +3669,67 @@ impl<ChanSigner: ChannelKeys> Channel<ChanSigner> {
return Err(ChannelError::Ignore("Cannot send value that would put us over the max HTLC value in flight our peer will accept"));
}

if !self.channel_outbound {
// Check that we won't violate the remote channel reserve by adding this HTLC.

let remote_balance_msat = self.channel_value_satoshis * 1000 - self.value_to_self_msat;
let remote_chan_reserve_msat = Channel::<ChanSigner>::get_remote_channel_reserve_satoshis(self.channel_value_satoshis);
// 1 additional HTLC corresponding to this HTLC.
let remote_commit_tx_fee_msat = self.next_remote_commit_tx_fee_msat(1);
if remote_balance_msat < remote_chan_reserve_msat + remote_commit_tx_fee_msat {
return Err(ChannelError::Ignore("Cannot send value that would put them under remote channel reserve value"));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this scenario tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added here: f26a2d2 though not certain it's the best way of testing, same with the other tests that I added.

}
}

let pending_value_to_self_msat = self.value_to_self_msat - htlc_outbound_value_msat;
if pending_value_to_self_msat < amount_msat {
return Err(ChannelError::Ignore("Cannot send value that would overdraw remaining funds"));
}

// The `+1` is for the HTLC currently being added to the commitment tx and
// the `2 *` and `+1` are for the fee spike buffer.
let local_commit_tx_fee_msat = if self.channel_outbound {
2 * self.next_local_commit_tx_fee_msat(1 + 1)
} else { 0 };
if pending_value_to_self_msat - amount_msat < local_commit_tx_fee_msat {
return Err(ChannelError::Ignore("Cannot send value that would not leave enough to pay for fees"));
}

// Check self.local_channel_reserve_satoshis (the amount we must keep as
// reserve for the remote to have something to claim if we misbehave)
if self.value_to_self_msat < self.local_channel_reserve_satoshis * 1000 + amount_msat + htlc_outbound_value_msat {
let chan_reserve_msat = self.local_channel_reserve_satoshis * 1000;
if pending_value_to_self_msat - amount_msat - local_commit_tx_fee_msat < chan_reserve_msat {
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
return Err(ChannelError::Ignore("Cannot send value that would put us under local channel reserve value"));
}

// Now update local state:
if (self.channel_state & (ChannelState::AwaitingRemoteRevoke as u32)) == (ChannelState::AwaitingRemoteRevoke as u32) {
self.holding_cell_htlc_updates.push(HTLCUpdateAwaitingACK::AddHTLC {
amount_msat: amount_msat,
payment_hash: payment_hash,
cltv_expiry: cltv_expiry,
amount_msat,
valentinewallace marked this conversation as resolved.
Show resolved Hide resolved
payment_hash,
cltv_expiry,
source,
onion_routing_packet: onion_routing_packet,
onion_routing_packet,
});
return Ok(None);
}

self.pending_outbound_htlcs.push(OutboundHTLCOutput {
htlc_id: self.next_local_htlc_id,
amount_msat: amount_msat,
amount_msat,
payment_hash: payment_hash.clone(),
cltv_expiry: cltv_expiry,
cltv_expiry,
state: OutboundHTLCState::LocalAnnounced(Box::new(onion_routing_packet.clone())),
source,
});

let res = msgs::UpdateAddHTLC {
channel_id: self.channel_id,
htlc_id: self.next_local_htlc_id,
amount_msat: amount_msat,
payment_hash: payment_hash,
cltv_expiry: cltv_expiry,
onion_routing_packet: onion_routing_packet,
amount_msat,
payment_hash,
cltv_expiry,
onion_routing_packet,
};
self.next_local_htlc_id += 1;

Expand Down
Loading