lnwallet: zeroconf/just-in-time improvements and tests#10463
lnwallet: zeroconf/just-in-time improvements and tests#10463f321x wants to merge 9 commits intospesmilo:masterfrom
Conversation
f321x
force-pushed
the
jit_2
branch
3 times, most recently
from
c565b34 to
de6ad87
Compare
| if not self.lnworker.wallet.is_up_to_date() or not self.lnworker.network \ | ||
| or self.lnworker.network.blockchain().is_tip_stale(): | ||
| # ensure we are up to date to prevent accidentally dropping a channel that is funded | ||
| return |
There was a problem hiding this comment.
what if the electrum server is lying by omission? maybe we should not delete the channel...
There was a problem hiding this comment.
right, maybe we should only freeze it for receiving and then delete it after has_funding_timed_out becomes true. This would leave much more time to connect to a honest server and find the funding tx if it really has been omitted.
There was a problem hiding this comment.
Ok 1f17574 now freezes the channel after 10 minutes without funding transaction and deletes the channel after has_funding_timed_out (~2 weeks). If the channel gets funded it is unfrozen.
There was a problem hiding this comment.
why wait 10 minutes? should we not freeze it by default, and unfreeze it when we detect the funding tx?
There was a problem hiding this comment.
I agree this would be simpler.
As freezing the channel doesn't prevent the channel peer from sending htlcs through it we could simply not release the preimage for any htlc(set) that arrived on a zeroconf channel without funding tx (except for the first one which pays for the channel).
In practice the funding tx should become visible in the mempool quickly, so we wouldn't have to fail any htlcs that already made it to us during this time period.
If the electrum server omits the transaction, we could either fail after some timeout, or the user restarts the wallet, we connect to a different server that tells us about it and can still settle.
On first thought this seems like an acceptable tradeoff, what do you think?
Check the just-in-time channel opening fee when receiving an incoming channel opening.
Fixes AbstractChannel.update_unfunded_state to stop calling a non-existent method (unwatch_channel). Adds unittest to execute the zeroconf path of update_unfunded_state.
On LSP side we were only checking if ACCEPT_ZEROCONF_CHANNELS is enabled while forwarding a non-trampoline htlc. During trampoline forwarding the config was ignored. The ACCEPT_* prefix implied this was only for accepting inbound zeroconf channels, but it also controls whether we open them when forwarding HTLCs. Renames the config var to OPEN_ZEROCONF_CHANNELS to clarify it enables zeroconf channel opens in both directions, and add the missing check when forwarding trampoline HTLCs.
...so we can have multiple just in time channels with the same lsp. We already save a remote scid alias in `on_channel_ready` which we already have received after the new zeroconf channel is in open state. So setting the alias to the static node id hash is counterproductive because it doesn't allow to differentiate between channels. Also extends the regtest (`just_in_time`) to do a second channel opening, to cover this scenario. This doesn't add much runtime to the test, so the cost seems reasonable.
Adds cleanup logic to `LNWallet.open_channel_just_in_time` so that the channel provider removes unfunded channels again, e.g. if the client didn't release the preimage or the provider failed to broadcast the funding transaction. Also adds more robust transaction broadcast logic so we retry to broadcast if it failed and check against adb to see if any previous broadcast was successful.
Adds unittests for `LNWallet.open_channel_just_in_time()`, `LNWallet._cleanup_failed_jit_channel()`, `LNWallet.can_get_zeroconf_channel()` and `LNWallet.receive_requires_jit_channel()`.
Make the just in time channel fees and channel size configvars, as in practice not every provider would use the same hardcoded fees or channel sizes. Add the mining fees required for the funding transaction on top of the opening fees to prevent opening channels at a loss in a higher fee environment.
Only signal `OPTION_ZEROCONF_OPT` to peers if we either: 1. Have no trusted peer configured (assuming that we are LSP) 2. Have a trusted peer configured, and the peer we are connecting to is this trusted peer. Otherwise peers that are LSPs but are not the clients trusted LSP might try to open a channel to the client but it would get rejected.
The ReceiveTab gets updated regularly (e.g. when syncing headers). Every time it updates we would first show the invoice and then the zeroconf confirmation overlay. This caused the overly to appear flickering when there are updates in higher frequency. Also we need to keep state if the user has already confirmed the zeroconf message for this request, otherwise the question will re-appear each time the user clicked "Accept" and the ReceiveTab updates again.
2 participants
Successor of #9584
Some incremental improvements and fixes of the existing just-in-time channel logic and new unittests to prevent code rot:
AbstractChannel.update_unfunded_state(), unittests itOPTION_ZEROCONF_OPTto untrusted peers, unittest it