Switch to per-channel config

[ariard]

Right now ChannelConfig is global, it's should be per-channel as you may want some of your channel being zero-conf and other not. And also enforce weaker/straighter csv delay according to the peer you're connected to, watchtower usage or not, user risk tolerance, channel value, ...

Channel config is split between ChannelHandshakeLimits, ChannelHandshake, ChannelConfig, all of them should be passed by caller to channel constructors instead of relying on ChannelManager default_configuration (check if current config struct are relevant, maybe need more thinking)

[ariard]

You can implement also #216, really related.

[ccdle12]

Hi,

Was just reading through this issue and the code base. I would be happy to pick this up and work on it, just wanted to clarify the goals of the issue.

• Would there still be a need for a global default UserConfig for all channels in the ChannelManager?

• I guess a user could specify a channel configuration as an Option<UserConfig>? - something like this:

  // Have node0 initiate a channel to node1 with aforementioned parameters
    nodes[0]
        .node
        .create_channel(
            nodes[1].node.get_our_node_id(),
            channel_value_sat,
            push_msat,
            42,
            None,
        )
        .unwrap();

pub fn create_channel(...user_config: Option<UserConfig>)
...

let channel = match user_config {
    Some(c) => Channel::new_outbound(
         &*self.fee_estimator,
         &self.keys_manager,
         their_network_key,
         channel_value_satoshis,
         push_msat,
         user_id,
         Arc::clone(&self.logger),
         &c,
     )?,
     None => Channel::new_outbound(
         &*self.fee_estimator,
         &self.keys_manager,
         their_network_key,
         channel_value_satoshis,
         push_msat,
         user_id,
         Arc::clone(&self.logger),
         &self.default_configuration,
      )?,
 };
let res = channel.get_open_channel(self.genesis_hash.clone(), &*self.fee_estimator);

[ariard]

@ccdle12, to both point yes, ideally you may have such config hierarchy an override_config, a default per-peer config, a global default one. So looking at this, kinda the issue in details:

Split channel policy config options from node policy config ones and make them used by needed components.

• Add a new struct for node-level config values like option_dlp & option_upfront_shutdown_script (NodePolicy)
• Add field node_config in PeerManager using this struct at Init message sending
• Add field node_config in Router using this struct at NodeAnnouncement message generation
• Remove ChannelConfig struct by moving announced_channel/fee_proportional_millionths to both ChannelHandshakeConfig/ChannelHandshakeLimits. Rename UserConfig to ChannelConfig.

Allow to apply a per-peer channel config, i.e pre-configured policy different if channel is with Bob or Caroll.
Ideally that allow to implement "whitelist" policies like low csv-delay for more trusted peers.

• Add field per_node_config in ChannelManager, a HashMap<Pubkey, ChannelConfig>
• Add new top-level public method in ChannelManager like add_channel_policy_for_peer(node_id: Pubkey, associated_policy: ChannelConfig) (cc @TheBlueMatt)
• At receiving open_channnel (internal_open_channel), check if we have a channel policy for this peer, else pass the default ChannelConfig one
• Modify create_channel to add a override_config: Option such that selecting hierarchy for outbound channel is override_config > peer_config > default_config
• Bonus point: rename their_to_self_delay/our_to_self_delay local_csv_delay/remote_csv_delay (names are pretty confusing)

[ccdle12]

thanks @ariard this makes sense, I can probably start working on some of things mentioned here

[TheBlueMatt]

I don’t know that we need to bother with an outbound channel default config - as long as it’s passed in as a reference the user can easily just pass a reference to their object. This mostly looks good to me, though a few comments and alternatives inline below:

> On Feb 14, 2020, at 11:26, Antoine Riard ***@***.***> wrote:  @ccdle12, to both point yes, ideally you may have such config hierarchy an override_config, a default per-peer config, a global default one. So looking at this, kinda the issue in details: Split channel policy config options from node policy config ones and make them used by needed components. Add a new struct for node-level config values like option_dlp & option_upfront_shutdown_script (NodePolicy) Add field node_config in PeerManager using this struct at Init message sending

Hmm, this is awkward, as it needs to be the same across ChannelManager and PeerManager. It may be better to just keep it in ChannelManager and add a new method for PeerManager to get a copy of it.

Add field node_config in Router using this struct at NodeAnnouncement message generation

NodeAnnouncement isn’t in Router, in fact it isn’t anywhere right now :/. It will be in ChannelManager, though.

Remove ChannelConfig struct by moving announced_channel/fee_proportional_millionths to both ChannelHandshakeConfig/ChannelHandshakeLimits. Rename UserConfig to ChannelConfig.

I don’t think this makes sense in Limits? It’s not a limit so either it should be a fee struct (also because it can be updates later) or stay.

Allow to apply a per-peer channel config, i.e pre-configured policy different if channel is with Bob or Caroll. Ideally that allow to implement "whitelist" policies like low csv-delay for more trusted peers. Add field per_node_config in ChannelManager, a HashMap<Pubkey, ChannelConfig> Add new top-level public method in ChannelManager like add_channel_policy_for_peer(node_id: Pubkey, associated_policy: ChannelConfig) (cc @TheBlueMatt) At receiving open_channnel (internal_open_channel), check if we have a channel policy for this peer, else pass the default ChannelConfig one Modify create_channel to add a override_config: Option such that selecting hierarchy for outbound channel is override_config > peer_config > default_config Bonus point: rename their_to_self_delay/our_to_self_delay local_csv_delay/remote_csv_delay (names are pretty confusing)

At a high level this makes sense, though you can choose how you want to do it - either track it per-peer or have some kind of api where the user is queried for a config when the channel gets opened. Both with work and both kinda suck.