Watch spent outputs before watching for confirmation #3092
Conversation
We previously immediately watched for confirmation of transactions that our peer couldn't double-spend, which had a few issues: - if we RBF-ed those transactions after a restart and a previous version confirmed, we wouldn't detect it and wouldn't move to the CLOSED state - if the transaction had a long CSV delay, we were wasting performance in the watcher while that CSV isn't complete We change that behavior and instead watch all outputs of the commitment transaction that we may spend. We only watch for confirmations after we detect that the output has been spent (either in the mempool or in a block). This ensures that RBF attempts are correctly handled, and that we don't watch transactions until they've been published (and thus CSV delays are satisfied). We also explicitly split 2nd-stage and 3rd-stage transactions. It is a bit verbose and awkward for now, but it will become cleaner once we stop storing unconfirmed transactions and instead re-compute them when restarting. It also makes testing easier: we took this opportunity to ensure that closing tests cover all scenarios and use better assertions on watches and transactions.
t-bast
force-pushed
the
refactor-watch-spent-before-confirmed
branch
from
0e01118 to
fe6748c
Compare
sstone
left a comment
sstone
left a comment
There was a problem hiding this comment.
We now set WatchOutputSpent on our main and htlc outputs, and will only set WatchConfirm once these outputs have been spent, but it seems we only test this behaviour with HTLC transactions, not with our "claim main ouput" tx ? (maybe I've missed it)
eclair-core/src/test/scala/fr/acinq/eclair/balance/CheckBalanceSpec.scala
Show resolved
Hide resolved
eclair-core/src/test/scala/fr/acinq/eclair/balance/CheckBalanceSpec.scala
Outdated
Show resolved
Hide resolved
This is tested in |
Yes it does. I was looking for explicit tests in |
We previously immediately watched for confirmation of transactions that our peer couldn't double-spend, which had a few issues:
We change that behavior and instead watch all outputs of the commitment transaction that we may spend. We only watch for confirmations after we detect that the output has been spent (either in the mempool or in a block). This ensures that RBF attempts are correctly handled, and that we don't watch transactions until they've been published (and thus CSV delays are satisfied).
We also explicitly split 2nd-stage and 3rd-stage transactions. It is a bit verbose and awkward for now, but it will become cleaner once we stop storing unconfirmed transactions and instead re-compute them when restarting. It also makes testing easier: we took this opportunity to ensure that closing tests cover all scenarios and use better assertions on watches and transactions.
Updating the closing tests was a lot of work, but it was a good opportunity to ensure that they are more complete and easier to read.