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.

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

Implement channel_type negotiation #1078

Merged
merged 4 commits into from
Nov 3, 2021

Conversation

TheBlueMatt
Copy link
Collaborator

@TheBlueMatt TheBlueMatt commented Sep 17, 2021

This implements channel_type negotiation as defined at lightning/bolts#880, though it does not currently send the proposed feature bit from lightning/bolts#906. It is likely to be required for turbo channels, but if not will be required for anchor channels anyway.

It could use a test of some form, at least of the fallback behavior.

@codecov
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

Merging #1078 (0a31c12) into main (0a31c12) will not change coverage.
The diff coverage is n/a.

❗ Current head 0a31c12 differs from pull request most recent head db2e7e7. Consider uploading reports for the commit db2e7e7 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1078   +/-   ##
=======================================
  Coverage   90.46%   90.46%           
=======================================
  Files          68       68           
  Lines       35176    35176           
=======================================
  Hits        31822    31822           
  Misses       3354     3354           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0a31c12...db2e7e7. Read the comment docs.

@TheBlueMatt TheBlueMatt force-pushed the 2021-09-chan-types branch 2 times, most recently from 200c607 to 74930bd Compare September 18, 2021 01:45
Copy link
Contributor

@arik-so arik-so left a comment

Choose a reason for hiding this comment

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

Looks good with the exception of minor nits.

lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
lightning/src/ln/channel.rs Outdated Show resolved Hide resolved
@@ -153,7 +153,7 @@ impl BackgroundProcessor {
/// functionality implemented by other handlers.
/// * [`NetGraphMsgHandler`] if given will update the [`NetworkGraph`] based on payment failures.
///
/// [top-level documentation]: Self
/// [top-level documentation]: BackgroundProcessor
Copy link
Contributor

Choose a reason for hiding this comment

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

not really part of this PR, but whatever

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤷‍♂️ not worth a separate pr for it.

@@ -4204,6 +4204,7 @@ impl<Signer: Sign> Channel<Signer> {
Some(script) => script.clone().into_inner(),
None => Builder::new().into_script(),
}),
channel_type: None,
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this diff, but if the message is to obtain an unprompted message to send, "generate" is probably a better prefix than "get"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yea, 🤷‍♂️, I mean its getting the open channel? I'm not gonna bother changing it unless you feel strongly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair point

@@ -2052,7 +2059,7 @@ mod tests {
do_encoding_channel_update(true, true, true, true);
}

fn do_encoding_open_channel(random_bit: bool, shutdown: bool) {
fn do_encoding_open_channel(random_bit: bool, shutdown: bool, chan_type: bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

channel_type is a bitmap or feature set. The boolean should be named encode_channel_type or should_encode_channel_type or sth. along those lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

went with "incl" ie "include".

lightning/src/ln/features.rs Show resolved Hide resolved
lightning/src/ln/channel.rs Show resolved Hide resolved
lightning/src/ln/features.rs Show resolved Hide resolved
Copy link
Contributor

@valentinewallace valentinewallace left a comment

Choose a reason for hiding this comment

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

Not 100% sure on some of these but didn't want to hold up feedback too long

Comment on lines 790 to 819
if channel_type.supports_unknown_bits() {
return Err(ChannelError::Close("Channel Type field contained optional bits - this is not allowed".to_owned()));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Atm, this would still allow an optional bit if the bit were static_remotekey, correct? Bit of an inexact error msg if so

Moreover, I think supports_unknown_bits needs to be modified for ChannelTypeFeatures in case the optional static_remotekey bit is set (in which case we'll still want to return true iiuc)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh, right, I was trying to be too clever by half. I rewrote it.

@@ -776,6 +784,23 @@ impl<Signer: Sign> Channel<Signer> {
where K::Target: KeysInterface<Signer = Signer>,
F::Target: FeeEstimator
{
// First check the channel type is known, failing before we do anything else if we don't
// support this channel type.
let channel_type = if let Some(channel_type) = &msg.channel_type {
Copy link
Contributor

Choose a reason for hiding this comment

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

From the spec for accept_channel:

if channel_type is set, and channel_type was set in open_channel, and they are not equal types:
    MUST reject the channel.

I don't think we do this?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We kinda do indirectly, but I made it explicit.

}
channel_type.clone()
} else {
ChannelTypeFeatures::from_counterparty_init(&their_features)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we're to include this line, and their Init supports but doesn't require static_remotekey, we should translate their features to require static_remotekey, since bit 13 should never be set in a ChannelTypeFeatures iiuc

lightning/src/ln/features.rs Show resolved Hide resolved
@TheBlueMatt
Copy link
Collaborator Author

Squashed without diff, will land after CI:

$ git diff-tree -U1 105a6aba8 33dd6a224
$

Apparently at least rustc 1.48 doesn't support `Self` in doc links,
so we make it explicit.
Its semantics are somewhat different from existing features,
however not enough to merit a different struct entirely.
Specifically, it only supports required features (if you send a
channel_type, the counterparty has to accept it wholesale or try
again, it cannot select only a subset of the flags) and it is
serialized differently (only appearing in TLVs).
This implements the channel type negotiation, though as we currently
only support channels with only static_remotekey set, it doesn't
implement the negotiation explicitly.
@TheBlueMatt
Copy link
Collaborator Author

Rebased on upstream, minor diff in the last commit to tweak serialization order, but realized I'd also missed the write side of the TLV entry -

$ git range-diff 801d6e5256d6ac91d5d5668da1fa5a2b55303246..33dd6a224900cb5a1580cce26fc57a993529000b 0a31c12f85b55dd2b3a85e929a5c92086bc8842b..db2e7e75c451bba0f1d0b75aa62fa6212d008215
1:  54f37ac9e = 1:  d0c3fb745 Fix `cargo doc` on older rustc
2:  85ed83c80 = 2:  e847c87dc Add a ChannelTypeFeatures features object for the new channel_type
3:  153c2c7d6 = 3:  b7a8dd4a1 Support de/ser of the new channel_type field in open_channel
4:  33dd6a224 ! 4:  db2e7e75c Support send/recv'ing the new channel_type field in open_channel
    @@ lightning/src/ln/channel.rs: use bitcoin::secp256k1::{Secp256k1,Signature};
     +use ln::features::{ChannelFeatures, ChannelTypeFeatures, InitFeatures};
      use ln::msgs;
      use ln::msgs::{DecodeError, OptionalField, DataLossProtect};
    - use ln::script::ShutdownScript;
    + use ln::script::{self, ShutdownScript};
     @@ lightning/src/ln/channel.rs: pub(super) struct Channel<Signer: Sign> {
        // is fine, but as a sanity check in our failure to generate the second claim, we check here
        // that the original was a claim, and that we aren't now trying to fulfill a failed HTLC.
    @@ lightning/src/ln/channel.rs: impl<Signer: Sign> Channel<Signer> {
                }
        }
      
    -@@ lightning/src/ln/channel.rs: impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
    +@@ lightning/src/ln/channel.rs: impl<Signer: Sign> Writeable for Channel<Signer> {
    +                   (7, self.shutdown_scriptpubkey, option),
    +                   (9, self.target_closing_feerate_sats_per_kw, option),
    +                   (11, self.monitor_pending_finalized_fulfills, vec_type),
    ++                  (13, self.channel_type, required),
    +           });
      
    +           Ok(())
    +@@ lightning/src/ln/channel.rs: impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
                let mut announcement_sigs = None;
                let mut target_closing_feerate_sats_per_kw = None;
    +           let mut monitor_pending_finalized_fulfills = Some(Vec::new());
     +          // Prior to supporting channel type negotiation, all of our channels were static_remotekey
     +          // only, so we default to that if none was written.
     +          let mut channel_type = Some(ChannelTypeFeatures::only_static_remote_key());
    @@ lightning/src/ln/channel.rs: impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K
                        (0, announcement_sigs, option),
                        (1, minimum_depth, option),
     @@ lightning/src/ln/channel.rs: impl<'a, Signer: Sign, K: Deref> ReadableArgs<&'a K> for Channel<Signer>
    -                   (5, config, option), // Note that if none is provided we will *not* overwrite the existing one.
                        (7, shutdown_scriptpubkey, option),
                        (9, target_closing_feerate_sats_per_kw, option),
    -+                  (11, channel_type, option),
    +                   (11, monitor_pending_finalized_fulfills, vec_type),
    ++                  (13, channel_type, option),
                });
      
     +          let chan_features = channel_type.as_ref().unwrap();
$

@TheBlueMatt
Copy link
Collaborator Author

Will land post-0.0.103 given there's no reason to land this for 0.0.103 and we're really close to release-cut time so good to hold off on extra changes.

@TheBlueMatt TheBlueMatt merged commit c0bbd4d into lightningdevkit:main Nov 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants