[bug]: high values for time_lock_delta can result in unexpected setting of 0

[nomnombtc]

Hello,

updatechanpolicy's time_lock_delta can roll over when very high values are used, which might result in a setting of 0.
I noticed this in a happy accident, where I used the channel_id instead of the channel_point and also forgot to actually
set time_lock_delta. So my channel_id got parsed as --time_lock_delta.

lncli --network testnet updatechanpolicy --fee_rate 0 --base_fee_msat 0 844393044316848128

resulted in time_lock_delta getting set to 0 for all my channels...

This shouldn't even be possible, because normally it complains that the minimum value is 18.

Your environment

tlv/v1.1.0-130-g143eba82e (0.15.99-beta current git master)
Linux lnd-test2 5.10.0-18-amd64 #1 SMP Debian 5.10.140-1 (2022-09-02) x86_64 GNU/Linux
bitcoin-core 23.0

Steps to reproduce

lncli --network testnet updatechanpolicy --fee_rate 0 --base_fee_msat 0 844393044316848128

Expected behaviour

Failure or time_lock_delta set to a high value

Actual behaviour

time_lock_delta got set to 0 for all my channels

$ lncli --network testnet getchaninfo 2610605642201169921
{
[...]
"node1_policy": {
"time_lock_delta": 0,
"min_htlc": "1000",
"fee_base_msat": "0",
"fee_rate_milli_msat": "0",
"disabled": false,
"max_htlc_msat": "1458270000",
"last_update": 1673657439,
"custom_records": {
}
},
[...]
$ lncli --network testnet getchaninfo 2610596846108147712
{
[...]
"node1_policy": {
"time_lock_delta": 0,
"min_htlc": "1000",
"fee_base_msat": "0",
"fee_rate_milli_msat": "0",
"disabled": false,
"max_htlc_msat": "990000000",
"last_update": 1673657439,
"custom_records": {
}
},
[...]

Additional Info

This might be caused by a 32bit roll over, as this still works as expected:
lncli --network testnet updatechanpolicy --fee_rate 0 --base_fee_msat 0 --time_lock_delta 2147483647
results in time_lock_delta 65535
while this results in setting it to zero again:
lncli --network testnet updatechanpolicy --fee_rate 0 --base_fee_msat 0 --time_lock_delta 2147483648

[Roasbeef]

Nice catch! We should make sure we check for overflow directly.

[eval-exec]

@Roasbeef Hi Roasbeef, I found TimeLockDelta's type is inconsistent, it's uint16 in somewhere like:

lnd/lnwire/channel_update.go

Line 100 in 2cf6969

TimeLockDelta uint16

lnd/channeldb/graph.go

Line 3418 in 1aa9626

TimeLockDelta uint16

and it's uint32 in:

lnd/lnrpc/lightning.proto

Line 4048 in 3dd9e04

uint32 time_lock_delta = 5;

I think we should unify TimeLockDelta to uint16 and check for overflow up to max(uint16). What do you think?

[Roasbeef]

I think we should unify TimeLockDelta to uint16 and check for overflow up to max(uint16). What do you think?

Agreed we should make the value consistent. For particular purposes, the uint16 should be enough. IIIRC, for the protos there isn't a uint16 type, so we need to use a uint32 there. Similarly, the database serialization in the other link assumes a uint16, so we can't change that as easily.