multi: replace DefaultDustLimit with accurate dust limit #5781
Conversation
Crypt-iQ
added
rpc
Crypt-iQ
force-pushed
the
accurate_dust0922
branch
2 times, most recently
from
1b1e6f1 to
fa6d1f1
Compare
| @@ -121,25 +117,15 @@ type txInputSet struct { | |||
| wallet Wallet | |||
| } | |||
|
|
|||
| func dustLimit(relayFee chainfee.SatPerKWeight) btcutil.Amount { | |||
There was a problem hiding this comment.
Main question is if this affects users running with different min relay fee settings. If the dust limit scales based on this, then it may not be possible for them to open channels as implementations may reject what they deem the dust limit.
There was a problem hiding this comment.
As discussed offline, I think it's somewhat safe to assume that users aren't changing this config value, as otherwise, they would find that their transactions wouldn't always propagate due to to standardness issues.
|
Can update the go.mod to point to the latest tagged versions for the |
Crypt-iQ
force-pushed
the
accurate_dust0922
branch
from
fa6d1f1 to
b3e539f
Compare
| pkscript, _ = input.GenerateUnknownWitness() | ||
|
|
||
| default: | ||
| panic("invalid script size") |
There was a problem hiding this comment.
Why not just return an error here?
There was a problem hiding this comment.
o/w the call-sites have to deal with an error, which can't be done with DefaultBtcChannelConstraints for example
| @@ -121,25 +117,15 @@ type txInputSet struct { | |||
| wallet Wallet | |||
| } | |||
|
|
|||
| func dustLimit(relayFee chainfee.SatPerKWeight) btcutil.Amount { | |||
There was a problem hiding this comment.
As discussed offline, I think it's somewhat safe to assume that users aren't changing this config value, as otherwise, they would find that their transactions wouldn't always propagate due to to standardness issues.
|
Just needs a release notes entry, can you also make a new file in that folder for 0.13.3? Thanks |
What are those different fees? I'd say if it makes any kind of difference, we can get away with having a floor of the least minimum relay fee between the top 3 or so implementations |
| // | ||
| // NOTE: Even with this bounding, the ChannelAcceptor may return an | ||
| // BOLT#02-invalid ChannelReserve. | ||
| maxDustLimit := reservation.OurContribution().DustLimit |
There was a problem hiding this comment.
does this override manually set dust limits?
There was a problem hiding this comment.
Not sure what you mean by manually set dust limits? The channel acceptor's dust limit isn't currently used in the code and a user can't manually set the dust limit in lnd
See comment by @Roasbeef above #5781 (comment) |
This commit updates call-sites to use the proper dust limits for various script types. This also updates the default dust limit used in the funding flow to be 354 satoshis instead of 573 satoshis.
This is necessary and is implied by BOLT#02. Both ChannelReserve parameters should be above both DustLimit parameters. Otherwise, it is possible for one side to have nothing at stake.
Crypt-iQ
force-pushed
the
accurate_dust0922
branch
from
b3e539f to
b8bfaf0
Compare
|
Currently the witness script sizes are different than non-witness. If that changes, then |
Commit overview
multi: replace DefaultDustLimit with script-specific DustLimitForScriptupdates all call-sites toDustLimitForScriptwhich takes a pkscript size and calculates the dust-limit. This also fixes thesendcoinsCLI command to be able to send 294 satoshis to a p2wpkh output for example. With this commit, the previous 573 limit should no longer be in thelndcodebase.funding+lnwallet: validate ChannelReserve is above DustLimitvalidates that both sides'ChannelReserveare above both sides'DustLimit. This is implied by BOLT#02 but should be made explicit. I will follow up with a patch to the spec to do this. We also implement Allowing dust limit below 546 sats lightning/bolts#905 along the way by ensuring we send a dust limit that other implementations accept. This dust limit is 354 satoshis by default (the dust limit for a maximally sized witness output with a 40-byte data push).Q: Should
DustLimitForScripttake into account different back-end minimum relay fees? The underlyingbtcd.GetDustThresholddoes not.Fixes #3946