loopout: allow per-swap confirmation targets for server HTLC #258
Conversation
carlaKC
force-pushed
the
65-confirmationparam
branch
from
f1bbd3f
to
1075769
Compare
interface.go
Outdated
| @@ -64,6 +64,10 @@ type OutRequest struct { | |||
| // client sweep tx. | |||
| SweepConfTarget int32 | |||
|
|
|||
| // HtlcConfTarget specifies the number of confirmations we require for | |||
| // on chain loop out htlcs. | |||
| HtlcConfTarget int32 | |||
There was a problem hiding this comment.
related to the discussion on the CLI flag name: why do we call it conf target here? Make it sound more like a fee estimation param, maybe we should be consistent in using something like HtlcRequiredConfs?
There was a problem hiding this comment.
oh yeah def, I think my mind just saw sweep target and went with it.
There was a problem hiding this comment.
should this one be renamed as well?
carlaKC
force-pushed
the
65-confirmationparam
branch
from
1075769
to
a2584a7
Compare
carlaKC
force-pushed
the
65-confirmationparam
branch
from
a2584a7
to
3cf49ce
Compare
|
@halseth renamed that last var, PTAL :) |
interface.go
Outdated
| @@ -175,7 +175,7 @@ type LoopInRequest struct { | |||
| // call. | |||
| MaxMinerFee btcutil.Amount | |||
|
|
|||
| // HtlcConfTarget specifies the targeted confirmation target for the | |||
| // HtlcConfirmations specifies the targeted confirmation target for the | |||
There was a problem hiding this comment.
field name not actually changed
There was a problem hiding this comment.
also, the comment is a bit weird, maybe use something similar to the one for loop out below?
interface.go
Outdated
| @@ -216,7 +216,7 @@ type LoopInQuoteRequest struct { | |||
| // include the swap and miner fee. | |||
| Amount btcutil.Amount | |||
|
|
|||
| // HtlcConfTarget specifies the targeted confirmation target for the | |||
| // HtlcConfirmations specifies the targeted confirmation target for the | |||
There was a problem hiding this comment.
ah, this is actually find and replace gone wild, shouldn't have been renamed in the first place, removed.
interface.go
Outdated
| @@ -64,6 +64,10 @@ type OutRequest struct { | |||
| // client sweep tx. | |||
| SweepConfTarget int32 | |||
|
|
|||
| // HtlcConfTarget specifies the number of confirmations we require for | |||
| // on chain loop out htlcs. | |||
| HtlcConfTarget int32 | |||
There was a problem hiding this comment.
should this one be renamed as well?
looprpc/client.proto
Outdated
| @@ -182,6 +182,12 @@ message LoopOutRequest { | |||
| */ | |||
| int32 sweep_conf_target = 9; | |||
|
|
|||
| /* | |||
| The number of confirmations that we require for the on chain htlc that will | |||
| be published by the server. | |||
There was a problem hiding this comment.
add "before we reveal the preimage"?
carlaKC
force-pushed
the
65-confirmationparam
branch
from
3cf49ce
to
f56fbc3
Compare
To allow users to specify differing confirmation targets, we store the swap conf target per-swap. This makes us restart safe, so we do not forget confirmation values for swaps that are in flight when we restart.
carlaKC
force-pushed
the
65-confirmationparam
branch
from
f56fbc3
to
13449fb
Compare
This PR allows setting of the number of confirmations we require for the loop out htlc that is published by the server on a per-swap basis. This allows setting of higher confirmation targets for larger value swaps (reducing counterparty risk of being double spent).
On the cli, we fail for 0 values because one would need to specifically set a 0 value (the flag defaults to 1), but rpc side we just bump 0 values up to the default of 1 conf so that older clients don't start failing when the server introduces this new parameter. Conf targets are persisted so that we remember them across restarts.