clarification: when does max_accepted_htlcs apply?

[Crypt-iQ]

This came up in lightningnetwork/lnd#3910. If we have two parties Alice and Bob who exchange the following asynchronous state updates:

  Alice                          Bob
1.       ------add----->
2.       <--settle/fail--- (covers a locked-in add)
3.       <-----sig------
4.       ------sig-----> (covers the add in step 1, Bob's commitment has max htlcs now)
5.       <-----rev------
6.       ------rev----->
7.       ------add----->

where the add Alice sends in step 1 gives Bob the max htlcs once locked-in, then is the add that Alice sends in step 7 allowed? My rationale for allowing it is that the settle/fail in step 2 will make room for the add in step 7. Alice's next signature for Bob that covers the add in step 7 must also cover the settle/fail and thus make room on Bob's commitment transaction (in the end Bob will have max htlcs).

There are two places this is mentioned in the spec:

if result would be offering more than the remote's max_accepted_htlcs HTLCs, in the remote commitment transaction:

    MUST NOT add an HTLC.

and

if a sending node adds more than receiver max_accepted_htlcs HTLCs to its local commitment transaction [...]:

    SHOULD fail the channel.

Since both passages are concerned with the effect on the commitment transaction, which is the hard constraint here, I think allowing the add in step 7 is appropriate and therefore the correct behavior. If an add is sent in step 8, however, that should be rejected as this would overflow Bob's commitment. Thoughts?

[alevchuk]

fwiw, seems you addressed it in LND by running validateCommitmentSanity earlier lightningnetwork/lnd#3910

[Crypt-iQ]

fwiw, seems you addressed it in LND by running validateCommitmentSanity earlier lightningnetwork/lnd#3910

Yup, just need to see that everyone agrees

Ping @t-bast , I think this could be a topic of discussion at a spec meeting. Of course if this is the obvious behavior, then there's nothing to talk about. But I think we should all agree on exactly when it applies.

[t-bast]

Ping @t-bast , I think this could be a topic of discussion at a spec meeting.

Agreed, we've all spent some time on this recently, so it would be nice to summarize that.
I won't be able to attend tonight's meeting though, but if you can attend it feel free to raise the topic, otherwise I'll add this to the next meeting agenda.

[Crypt-iQ]

@t-bast Forgot to attend the meeting, could be added to the next meeting agenda

[ariard]

where the add Alice sends in step 1 gives Bob the max htlcs once locked-in, then is the add that Alice sends in step 7 allowed?

That's my understanding too that Alice's RAA in step 6 irremediably drops settle/fail HTLC from her commitment transaction and as such Bob is free to forget about it. At least this is what RL is doing : https://github.com/rust-bitcoin/rust-lightning/blob/84967faf52b992206176e58020dfed80b1093c35/lightning/src/ln/channel.rs#L2562.

Since both passages are concerned with the effect on the commitment transaction, which is the hard constraint here, I think allowing the add in step 7 is appropriate and therefore the correct behavior. If an add is sent in step 8, however, that should be rejected as this would overflow Bob's commitment. Thoughts?

+1 to clarify the spec on this point. AFAICT, the two excerpts you're mentioning aren't saying when a counterparty is supposed to drop a settled/failed HTLC otherwise a HTLC sender could prune out the number of HTLCs announced to remote as soon as update_fail/commitment_signed are received but without waiting to send a RAA ?

[Crypt-iQ]

That's my understanding too that Alice's RAA in step 6 irremediably drops settle/fail HTLC from her commitment transaction and as such Bob is free to forget about it. At least this is what RL is doing : https://github.com/rust-bitcoin/rust-lightning/blob/84967faf52b992206176e58020dfed80b1093c35/lightning/src/ln/channel.rs#L2562.

Just to make sure we're on the same page -- Bob must validate that Alice's next sig "covers" this removed HTLC otherwise invalid commitment sig occurs.

+1 to clarify the spec on this point. AFAICT, the two excerpts you're mentioning aren't saying when a counterparty is supposed to drop a settled/failed HTLC otherwise a HTLC sender could prune out the number of HTLCs announced to remote as soon as update_fail/commitment_signed are received but without waiting to send a RAA ?

Yeah so I guess the spec doesn't specifically mention that.
In lnd the following is not allowed (but is technically valid):

6. ---add---> (max_accepted_htlcs on commitment + 1 in-memory)
7. ---rev--->
8. ---sig---> (must have max_accepted_htlcs since rev effectively dropped an htlc)

We do some pre-validation when receiving an htlc to make sure it doesn't bypass max_accepted_htlcs, but we may need to rework this because atm we assume that a RAA won't occur after an add but before a sig in this specific case of pre-commitment validating max_accepted_htlcs.

[rustyrussell]

Yes, this is my understanding too. If you ever end up constructing a commitment tx which has more than max_accepted_htlcs, your peer has violated the constraint.

Thus, you could choose a more conservative approach when sending, if you wanted, but not when receiving.

Side note: option_simplified_commitment fixes this, of course :)

[rustyrussell]

Yeah so I guess the spec doesn't specifically mention that.
In lnd the following is not allowed (but is technically valid):

6. ---add---> (max_accepted_htlcs on commitment + 1 in-memory)
7. ---rev--->
8. ---sig---> (must have max_accepted_htlcs since rev effectively dropped an htlc)

I don't think step 6 is valid? You haven't acked the removal yet, so you'd be over. You're allows in the spec to validate either as-you-go or defer to when you receive commitment_signed, but it has to be valid both ways!

[TheBlueMatt]

I don't think step 6 is valid?

At least we will reject it, so I'm gonna call it invalid :)

[Crypt-iQ]

Yeah so I guess the spec doesn't specifically mention that.
In lnd the following is not allowed (but is technically valid):

6. ---add---> (max_accepted_htlcs on commitment + 1 in-memory)
7. ---rev--->
8. ---sig---> (must have max_accepted_htlcs since rev effectively dropped an htlc)

I don't think step 6 is valid? You haven't acked the removal yet, so you'd be over. You're allows in the spec to validate either as-you-go or defer to when you receive commitment_signed, but it has to be valid both ways!

Ah I see, then step 6 would be invalid and lnd has the correct behavior for this case

[Crypt-iQ]

Yeah so I guess the spec doesn't specifically mention that.
In lnd the following is not allowed (but is technically valid):

6. ---add---> (max_accepted_htlcs on commitment + 1 in-memory)
7. ---rev--->
8. ---sig---> (must have max_accepted_htlcs since rev effectively dropped an htlc)

I don't think step 6 is valid? You haven't acked the removal yet, so you'd be over. You're allows in the spec to validate either as-you-go or defer to when you receive commitment_signed, but it has to be valid both ways!

Is the validate as-you-go point explicit or implicit in the spec right now? If it's implicit, might be good to have it explicit?

[rustyrussell]

Actually, I was wrong: we have that language for update_fee (...but MAY delay this check until the update_fee is committed.) but the other checks are immediate:

### Adding an HTLC: `update_add_htlc`
...
A receiving node:
...
  - if a sending node adds more than receiver `max_accepted_htlcs` HTLCs to
    its local commitment transaction, OR adds more than receiver `max_htlc_value_in_flight_msat` worth of offered HTLCs to its local commitment transaction:
    - SHOULD fail the channel.

Which AFAICT is where everyone checks anyway.