Update funding timeout fundee #1692
Conversation
Our previous timeout was based on timestamps, mostly because blockCount could be 0 on mobile using Electrum until a new block was received. Now that we're diverging from the mobile wallet codebase, we can use block heights instead which is more accurate. See lightning/bolts#839
t-bast
force-pushed
the
funding-expiry
branch
from
edc801d to
381dd22
Compare
eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/Channel.scala
Outdated
Show resolved
Hide resolved
| goto(OFFLINE) using data | ||
| if (funding.waitingSinceBlock > 1500000) { | ||
| // we were using timestamps instead of block heights when the channel was created: we reset it to use block heights | ||
| goto(OFFLINE) using funding.copy(waitingSinceBlock = nodeParams.currentBlockHeight) |
There was a problem hiding this comment.
If we don't update the db we'll push back forever:
| goto(OFFLINE) using funding.copy(waitingSinceBlock = nodeParams.currentBlockHeight) | |
| goto(OFFLINE) using funding.copy(waitingSinceBlock = nodeParams.currentBlockHeight) storing() |
|
Related: #723 |
| @@ -335,7 +335,7 @@ class ChannelCodecsSpec extends AnyFunSuite { | |||
| // let's decode the old data (this will use the old codec that provides default values for new fields) | |||
| val data_new = stateDataCodec.decode(bin_old.toBitVector).require.value | |||
| assert(data_new.asInstanceOf[DATA_WAIT_FOR_FUNDING_CONFIRMED].fundingTx === None) | |||
| assert(System.currentTimeMillis.milliseconds.toSeconds - data_new.asInstanceOf[DATA_WAIT_FOR_FUNDING_CONFIRMED].waitingSince < 3600) // we just set this timestamp to current time | |||
| assert(System.currentTimeMillis.milliseconds.toSeconds - data_new.asInstanceOf[DATA_WAIT_FOR_FUNDING_CONFIRMED].waitingSinceBlock < 3600) // we just set this timestamp to current time | |||
There was a problem hiding this comment.
Is this comparison still valid ?
There was a problem hiding this comment.
We will still do that "trick" for upgrades from old eclair nodes, but it will then be changed again in Channel.scala to use a block height. I thought about directly using currentBlockHeight in the legacy codec, but it requires changing a val to a def to pipe the blockCount which was messy...
|
How about, the following backward compatibility code? I think we can leave it like that (since this happens before the channel restore, where you handle the timestamp->blockheight migration). |
I agree, this is related to #1692 (comment) and I think we can leave it like that. |
Our previous timeout was based on timestamps, mostly because
blockCountcould be0on mobile using Electrum until a new block was received. Now that we're diverging from the mobile wallet codebase, we can use block heights instead which is more accurate.See lightning/bolts#839
I've done some E2E tests on regtest for the "migration" code. There is one edge case where a channel in
WAIT_FOR_FUNDING_CONFIRMEDwill not be automatically cleaned up: if the peer never connects back to you, the channel will stay inOFFLINEand won't check for funding timeout. I think this edge case is quite harmless and can be ignored (I don't think it's worth adding more code to fix it).