Remove watch-confirmed with depth 0 in zero-conf #2310
Conversation
t-bast
force-pushed
the
zeroconf-scid-alias-bast
branch
from
dbfbd72 to
869175b
Compare
pm47
force-pushed
the
zeroconf-scid-alias
branch
from
da46456 to
c20f672
Compare
t-bast
force-pushed
the
zeroconf-scid-alias-bast
branch
2 times, most recently
from
a33b5a8 to
de53949
Compare
Instead of using a watch with minDepth=0, we can directly skip the wait_for_funding_confirmed state when using 0-conf, which is less hacky.
* Small refactorings and typos. * Remove a few redundant changes. * Ensure that the balance is correctly initialized for new channels, otherwise they would be ignored during path-finding
t-bast
force-pushed
the
zeroconf-scid-alias-bast
branch
from
de53949 to
d9ec2b1
Compare
eclair-core/src/main/scala/fr/acinq/eclair/channel/Helpers.scala
Outdated
Show resolved
Hide resolved
eclair-core/src/main/scala/fr/acinq/eclair/channel/fsm/ChannelOpenSingleFunder.scala
Outdated
Show resolved
Hide resolved
| log.info(s"committing txid=${fundingTx.txid}") | ||
| goto(WAIT_FOR_FUNDING_CONFIRMED) using DATA_WAIT_FOR_FUNDING_CONFIRMED(commitments, Some(fundingTx), blockHeight, None, Left(fundingCreated)) storing() calling publishFundingTx() | ||
| case None => | ||
| blockchain ! WatchFundingLost(self, commitments.commitInput.outPoint.txid, nodeParams.channelConf.minDepthBlocks) |
There was a problem hiding this comment.
This makes me think that not implementing WatchFundingLost is more questionable with zero-conf.
There was a problem hiding this comment.
True! It would be useful to implement it in a follow-up PR.
| val channelKeyPath = keyManager.keyPath(commitments.localParams, commitments.channelConfig) | ||
| val nextPerCommitmentPoint = keyManager.commitmentPoint(channelKeyPath, 1) | ||
| val shortIds = ShortIds(RealScidStatus.Unknown, ShortChannelId.generateLocalAlias(), None) |
There was a problem hiding this comment.
We could add those to DATA_WAIT_FOR_FUNDING_CONFIRMED and reduce the diff between the two code paths.
There was a problem hiding this comment.
This would create an issue when migrating old versions of DATA_WAIT_FOR_FUNDING_CONFIRMED, because we don't have an scid there to use in the alias field and we're not sure the alias generation function will be suitable to call during DB migration.
I instead refactored that code to a single function that is called in all three places where we do 0-conf in fcb0fe4
There was a problem hiding this comment.
Went a bit further in branch zeroconf-scid-alias-bast-factor and factored in the 3rd transition. The semantics of skipFundingConfirmation changes to acceptFundingTx.
In a second commit I removed the "optimization" that makes us eagerly process remote's alias to emit less events. I think it's not worth it considering how it complicates the method signature.
And address PR comments.
This is a PR on #2224, best reviewed commit-by-commit.
The first commit contains only some clean-up and nits.
The second commit removes the use a depth 0 watch-confirmed (which is quite hacky) and instead skips the
WAIT_FOR_FUNDING_CONFIRMEDstate entirely, which fits better with our model, at the cost of duplication a few lines of code (which we could refactor to a dedicated function, but I'd rather have them explicitly in each event handlers as they vary slightly depending on the case).The third commit contains a few small changes to
Validation.scala. The most important change is that we lost a call toupdateBalancesin #2224 by moving the initialization of private channels in the handler ofShortChannelIdAssigned, which we restore inhandleLocalChannelUpdate, otherwise path-finding would ignore all newly created channels.