More custom channel bugfixes #939
Conversation
To avoid calling any channel functionality when tapd is running in standalone mode (not integrated into litd), we add a new flag that can be set by litd.
This commit mainly fixes a bug that when we copy an asset to create a new output, we didn't empty out the split commitment when we spent an output of a previous split in a full-value send output. This lead to a validation error in the VM. We also add some more context to the error that we were previously running into with this. And just because we can, we fix a package shadowing issue.
In order for us to send the proofs to the correct proof courier address, we need to be able to specify different delivery addresses for each allocation.
We want to be able to tell the initiator of the shutdown what proof delivery address we use, so we can fetch the proof of the co-op close correctly (only the initiator of the shutdown will create an asset transfer and hand it to the freighter, so if we're on the other side, we'll need to fetch the proof instead).
If we're not the initiator of the shutdown process, we won't be called back from lnd after the channel closing TX confirmed. So in order for us to pick up on the transfer and download the proof from the courier, we need to add our expected output as a TAP address, so the custodian will detect the confirmed output on chain. This is not optimal, as it will not work if there are multiple assets in that close output. But since we'll need to add a whole new callback in lnd that invokes a method in the aux closer after the channel confirmed, we solve it this way instead for now. Because the whole funding logic isn't ready to add multiple assets to a channel anyway.
| const ( | ||
| // CustomChannelRemoteReserve is the custom channel minimum remote | ||
| // reserve that we'll use for our channels. | ||
| CustomChannelRemoteReserve = 1062 |
There was a problem hiding this comment.
Yeah, not sure where exactly this is coming from. Took over the commit from #934, @GeorgeTsagk did some calculations to arrive at it.
Probably the 1% of our 100k default channel size plus some dust margin?
| @@ -447,6 +453,7 @@ func DistributeCoins(inputs []*proof.Proof, allocations []*Allocation, | |||
| AnchorOutputInternalKey: a.InternalKey, | |||
| AnchorOutputTapscriptSibling: sibling, | |||
| ScriptKey: a.ScriptKey, | |||
| ProofDeliveryAddress: deliveryAddr, | |||
There was a problem hiding this comment.
In order for us to send the proofs to the correct proof courier address,
we need to be able to specify different delivery addresses for each
allocation.
In the future, or today?
There was a problem hiding this comment.
Today. Otherwise if the initiator of the shutdown process uses the non-default proof courier address, the respondent wouldn't ever learn that and wouldn't know where to download the proof from.
| AssetVersion: asset.V0, | ||
| BtcAmount: tapsend.DummyAmtSats, | ||
| SortTaprootKeyBytes: sortKeyBytes, | ||
| ProofDeliveryAddress: proofDeliveryUrl, |
There was a problem hiding this comment.
Ah existing tests assumed that they were all using the same courier.
There was a problem hiding this comment.
Before this commit the respondent to the shutdown process didn't actually know what proof courier the final proof would be uploaded to. Now the desired delivery address is sent along with the internal and script key, so the non-shutdown-initiator will know where to look for the proof.
The allocation code incorrectly assumed that if there was no asset balance on one side, there also wouldn't be a BTC balance. With that incorrect assumptions, no commitment or anchor allocations were added if there for example was a BTC push amount added in the channel opening transaction.
|
Added one more bugfix, kudos to @jamaljsr for the amazing testing. Commit message reads: |
Fixes the following issues found during testing:
FundChannelRPC cannot be called whentapdis running in standalone mode (not integrated intolitd)