Dynamic and user definable channel params #506
Conversation
| Name: "updatefees", | ||
| Usage: "update the fee policy for all channels, or a single channel", | ||
| ArgsUsage: "base_fee_msat fee_rate [channel_point]", | ||
| var updateChannelPolicyCommand = cli.Command{ |
There was a problem hiding this comment.
Hmm, wondering if I should just keep the name updatefees even though it is used for more than strictly setting fees, as it might be confusing that we change the name suddenly.
There was a problem hiding this comment.
Eh, this is more descriptive as it's no longer just about fees as you note. I think the naming change would be an issue if we were publishing stable releases, but at this point, I don't think it'll critically affect anyone's deployed applications.
Naming suggestion to shorten a bit: updatechanpolicy. Less to type on the command line 😛
routing/router.go
Outdated
|
|
||
| // TimeLockDelta is the required HTLC timelock delta to be used | ||
| // when forwarding payments. | ||
| TimeLockDelta uint32 |
There was a problem hiding this comment.
Well...wouldn't really say that the timelock delta is part of the fee schema. Instead we can embed the FeeSchema in a larger struct hat includes the other updatable routing parameters.
There was a problem hiding this comment.
Embedded within struct ChannelPolicy.
| Name: "updatefees", | ||
| Usage: "update the fee policy for all channels, or a single channel", | ||
| ArgsUsage: "base_fee_msat fee_rate [channel_point]", | ||
| var updateChannelPolicyCommand = cli.Command{ |
There was a problem hiding this comment.
Eh, this is more descriptive as it's no longer just about fees as you note. I think the naming change would be an issue if we were publishing stable releases, but at this point, I don't think it'll critically affect anyone's deployed applications.
Naming suggestion to shorten a bit: updatechanpolicy. Less to type on the command line 😛
| // HTLCs on our channels. | ||
| minTimeLockDelta = 4 | ||
|
|
||
| defaultBitcoinMinHTLCMSat = 1000 |
There was a problem hiding this comment.
There're already the defaultXXXForwardingPolicy structs in chainregistry.go. With these definitions it's all duplicated again. When setting the defaults on the config to be parsed, those structs can be used directly as they're statically initialized.
There was a problem hiding this comment.
I removed the structs from chainregistry. Do you prefer we keep the constants there?
fundingmanager.go
Outdated
| // The actual delay we will require will be somewhere between these | ||
| // values, depending on channel size. | ||
| minRemoteDelay = 4 | ||
| maxRemoteDelay = 144 |
There was a problem hiding this comment.
A max delay of 144 blocks (1 day) is insufficient. Keep in mind that this parameters serves as the minimum time delta a node can go w/o coming online (assuming no watch towers). To lie on the conservative side of things, I'd say the max should be somewhere around 3024 blocks (1.5 retarget periods). In order to ensure a channel of like 10k satoshis doesn't trigger such a high CSV value, we can use a piecewise function to place channel amounts into various buckets.
Similarly, a min delay of 4 is also too low IMO. This means that there's a 40 minute window of vulnerability. This one is a bit more subjective as there may be situations where you're OK with a small CSV value (possibly making a channel with a friend, or frequent business partner). With that said, what do you think of a min value of 72 (12 hours) or 144 (24 hours)?
I haven't finished going over the remainder of the commits, but we should also be using these values as bounds for rejecting when the remote party proposes a CSV value.
There was a problem hiding this comment.
Totally agree that a minimum of 4 is too low. I set it that low because 4 was the previous default.
I think a minimum of 144 is fine, and I don't think it is a problem to wait 3024 if ~$3 is at stake, so 3024 sounds good.
There was a problem hiding this comment.
However, maybe 3 weeks is a bit large if unreliable peers require you to use that. Also autopilot will open max value channels, which can tie up a lot of your funds for weeks. 2016 is maybe a better number.
There was a problem hiding this comment.
Made min 144, max 2016.
fundingmanager.go
Outdated
| // The actual delay we will require will be somewhere between these | ||
| // values, depending on channel size. | ||
| minRemoteDelay = 4 | ||
| maxRemoteDelay = 144 |
There was a problem hiding this comment.
Granted that the current max channel size is ~$3k...the upper limit I suggested may bit a bit much
lnd_test.go
Outdated
| } | ||
|
|
||
| // Sleep to let the update propagate. | ||
| time.Sleep(300 * time.Millisecond) |
There was a problem hiding this comment.
We can use the SubcribeChannelGraph call here instead. Utilizing this, we'll receive a notification once a new channel update is encountered.
lnd_test.go
Outdated
| t.Fatalf("unable to get alice's balance: %v", err) | ||
| } | ||
|
|
||
| time.Sleep(300 * time.Millisecond) |
There was a problem hiding this comment.
Similar comment here w.r.t eliminating the sleep.
| // gets propagated. | ||
| timestamp := time.Now().Unix() | ||
| if timestamp <= edge.LastUpdate.Unix() { | ||
| timestamp = edge.LastUpdate.Unix() + 1 |
halseth
force-pushed
the
dynamic-channel-params
branch
from
65d2d41
to
6120373
Compare
halseth
force-pushed
the
dynamic-channel-params
branch
5 times, most recently
from
ba9265a
to
1e6d64d
Compare
This commit specifies that the MinHTLC value for a link is static over the lifetime of a channel, and don't process the field during a policyUpdate.
This commit embeds the FeeSchema within the new struct ChannelPolicy, which also contains the TimeLockDelta for a channel.
…y update Also rename various instances of "FeeUpdate" to "PolicyUpdate"
This commit renames the UpdateFee RPC call together with associated types to UpdateChannelPolicy. In addition to fees, now also timelock delta can be specified using this call.
This commit renames the `lncli updatefees` command to `lncli updatechanpolicy` and adds `time_lock_delta` as one of the passed parameters.
This commit changes the name of the UpdateFee method to UpdateChannelPolicy, to mimic the recent proto change. It also reads and validates the passed TimeLockDelta, and sends it to the gossiper for announcing it to the network, and to the switch for updating the forwarding policy of the links.
This commit makes the fundingmanager read the minHtlc field of the initFundingMsg, and add it to the reservation as this node's htlc_minimum_msat for the open_channel message. If the field is not specified in the initFundingMsg, the default value found in the DefaultRoutingPolicy will be used.
This commit moves the forwarding policy rules for Bitcoin and Litecoin, previously defined in the chainregistry, to config.go, making them possible to define by the user. We validate that the TimeLockDelta set is at least 4, the other rules we let the user specify arbitrarily, even 0.
…rwardingPolicy.TimeLockDelta
This commit removes the definitions of defaultBitcoinForwardingPolicy and defaultLitecoinForwardingPolicy from the the chainregistry, and instead creates a routingPolicy from the values found in the config.
This commit moves the definition of DefaultNumChanConfs into the chainConfig (such that it is set as e.g. "--bitcoin.defaultchanconfs"), making it possible to set individually for different chains. It also adds the flag DefaultRemoteDelay to the chainConfig, which can be used to set the CSV delay we will require the remote to wait before retrieving its own funds in case of an uncooperative close of the channel. Both these are set 0 by default (if not specified by the user), which in that case we will dynamically set the values, scaling them according to the channel size.
This commit defines minRemoteDelay and maxRemoteDelay, which is the extremes of the CSV delay we will require the remote to use for its commitment transaction. The actual delay we will require will be somewhere between these values, depending on channel size.
This commit makes the value returned fomr NumRequiredConfs and RequiredRemoteDelay used during the funding process scale linearly with the channel size. This is done to ensure that in cases there are more at stake in a channel, we have more time to react to reorgs, or unilateral closes. If the user explicitly specified values for these two at startup, we return those instead, without doing the scaling.
This commit ensures that we always increment the timestamp of ChannelUpdates we send telling the network about changes to our channel policy. We do this because it could happen (especially during tests) that we issued an update, but the ChannelUpdate would have the same timestamp as our last ChannelUpdate, and would be ignored by the network.
This commit extracts the launching of a goroutine subscribing to and forwarding graph topology notifications into its own utility method, such that it can be used in other tests as well.
This commit adds a new integration test, that checks that policy/fee updates get propagated properly in the network, such that the other nodes learn about the changes.
halseth
force-pushed
the
dynamic-channel-params
branch
from
1e6d64d
to
d030773
Compare
| // will send us notifications upon detected changes in the channel graph. | ||
| // subscribeGraphNotifications subscribes to channel graph updates and launches | ||
| // a goroutine that forwards these to the returned channel. | ||
| func subscribeGraphNotifications(t *harnessTest, ctxb context.Context, |
There was a problem hiding this comment.
What do you mean? 😮
…tion Since #506, `defaultchanconfs` is nested under `bitcoin`. Using this parameter at the top level generates a warning message: ``` 2018-03-17 23:07:08.273 [WRN] LTND: /home/bitcoin/.lnd/lnd.conf:7: unknown option: defaultchanconfs ``` and using it nested under `bitcoin` does not.
…tion Since lightningnetwork#506, `defaultchanconfs` is nested under `bitcoin`. Using this parameter at the top level generates a warning message: ``` 2018-03-17 23:07:08.273 [WRN] LTND: /home/bitcoin/.lnd/lnd.conf:7: unknown option: defaultchanconfs ``` and using it nested under `bitcoin` does not.
This PR gives the user more control over several channel parameters, and also scale them accoding to channel size, ensuring the defaults are safe in case the channels get large.
htlc_minimum_msatThe
htlc_minimum_msat/min_htlc_msatvalue is used to require incoming HTLCs on a channel to be of a certain minimum size. This is to ensure we we don't get overflowed by low-value payments. This parameter is only definable at channel open, and cannot be changed later. This PR adds the option to set the default for this value atlndstartup using the flag--bitcoin.minhtlc=, or when opening a channel withlncli openchannel --min_htlc_msat=.fee_base_msat,fee_proportional_millionths,cltv_expiry_deltaThese values, (also called
base_fee_msat,fee_rate,time_lock_delta) are communicated using a node'sChannelUpdate, and can be changed at will. Before this PR the first two were changeable, and this PR makes all three possible to set at runtime. This is done usinglncli updatechannelpolicy(previouslylncli updatefees). Default values can also now be set at startup, using the flagslnd --bitcoin.basefee= --bitcoin.feerate= --bitcoin.timelockdelta=.A test is added to ensure these are correctly propagated in the network.
--bitcoin.defaultchanconfs,--bitcoin.defaultremotedelayThese two flags can now be provided at startup (
--defaultchanconfsexisted from before), letting the user set how many blocks should pass before a channel is considered open, and how long the remote's CSV delay must be. If these are not specified at startup, we will attempt to scale the used value depending on the size of the channel being created. From3to6block for the confirmations, and from4to144for the CSV delay.