Avoid dust outputs in commitment transactions #115
Conversation
cjamthagen
force-pushed
the
no_dust_outputs
branch
from
e3a4696 to
2eefc2c
Compare
|
Updated to have ForceClose and the utxonursery handle to-self dust. |
|
@cjamthagen Sorry for not being too much responsive on your |
Roasbeef
force-pushed
the
master
branch
2 times, most recently
from
d86107c to
a34df2f
Compare
lnwallet/interface_test.go
Outdated
There was a problem hiding this comment.
There's a function to estimate the current dust limit within the wallet. It can be used here in place of the hard-coded amount.
There was a problem hiding this comment.
Found several more of these occurrences. All should be fixed now.
lnwallet/script_utils_test.go
Outdated
There was a problem hiding this comment.
Same thing here about removing the magic number.
lnwallet/channel.go
Outdated
There was a problem hiding this comment.
With this change, the utxoNursery will also need to be modified to ignore outputs that have a nil SignDescriptor. Atm, if a sub-dust output were some how still created, a panic would occur.
lnwallet/channel.go
Outdated
There was a problem hiding this comment.
Nice! This also fixes an existing bug atm which can arise if a channel is forced closed with uncleared HTLC's
lnwallet/channel_test.go
Outdated
There was a problem hiding this comment.
When easy, lines in files should be folded to 80-characters. Thanks!
cjamthagen
force-pushed
the
no_dust_outputs
branch
from
2eefc2c to
a8e528e
Compare
utxonursery.go
Outdated
There was a problem hiding this comment.
This line is triggering a panic in the integration tests. The incReq struct needs to be properly initialized:
var inqReq incubationRequest
Then:
u.requests <- &incReq
c4f0bb0 to
0f54d49
Compare
Currently non-HTLC outputs will be accepted in the commitment transaction as long as it is non-zero. We change this by not allowing outputs with a value lower than the dust limit. The value of such an output will go towards transaction fees.
If the value of the to-local output is below the dust limit, the ForceCloseSummary should not include a sign descriptor for this output. We also find the proper to-self output by looking for the expected public key script and not assume that no HTLC outputs exist.
If a forceClose happens and the to-self output is below the dust value, then that output will be non-existant in the commitment transaction. UtxoNursery now handles that case by checking to see if the accompanying selfOutPutSignDescriptor is nil. If it is nil, then it will ignore it.
cjamthagen
force-pushed
the
no_dust_outputs
branch
from
0f54d49 to
0e29395
Compare
Roasbeef
left a comment
Roasbeef
left a comment
There was a problem hiding this comment.
Tested everything locally, and all the functionality appears to be in order. Thanks for this PR! This puts us a bit closer towards full compliance w.r.t to the current draft specification.
LGTM 👾
|
@ABISprotocol , I will try to explain it from my point of understanding: |
|
Thanks for this clarification @AndrewSamokhvalov , I am also currently observing a pull request (in some ways related) here: bitcoin/bitcoin#7601 |
4 participants
Currently non-HTLC outputs will be accepted in the commitment
transaction as long as it is non-zero. We change this by not allowing
outputs with a value lower than the dust limit. The value of such
an output will go towards transaction fees.