Unlock utxos when TxPublish fails #1827
Conversation
Isolate the tx publishing logic inside a dedicated actor. One actor will be created for each tx that should be published.
Use distinct behaviors for each phase of tx publishing. This will let us handle errors more easily and apply custom clean-up depending on what phase we were in (e.g. unlock utxos after funding).
If our commit tx or the remote commit tx has been confirmed, there's no need to claim our anchor output.
When we fail to publish a transaction to which we added wallet inputs, we must unlock these utxos. This can happen for various reasons, the most frequent one is a node restart (all txs are sent to the TxPublisher again, but they may already be in the mempool).
|
Regarding the first commit (9ed346e):
|
Yes, all txs will be handled by the
That's true. But I really think moving to dedicated actors is important before adding more features... |
| // We retry when the next block has been found, we may have more funds available. | ||
| val nextBlockCount = nodeParams.currentBlockHeight + 1 | ||
| val cltvDelayedTxs1 = cltvDelayedTxs + (nextBlockCount -> (cltvDelayedTxs.getOrElse(nextBlockCount, Seq.empty) :+ result.cmd)) |
There was a problem hiding this comment.
That's a bit hacky, the transaction isn't really cltv-delayed here. Also, there is a potential risk of creating a herd effect if our wallet balance is low. That would be a nice stress test of our ability to concurrently fund many transactions and manages locks, but I'm not sure it's a good thing.
There was a problem hiding this comment.
That will be improved later, when we'll introduce deadlines (block heights at which we should republish/bump some txs), but that requires more work. In the meantime I'd rather err on the safe side and republish aggressively to ensure we don't forget to publish an important transaction because of an unknown error.
IIRC, the most probable scenario would be local-commit and htlc-success |
|
The only scenario where it's an issue is the local commit tx, it must be published before its children, otherwise the rest is CSV-delayed so it's not impacted. |
We ensure that the behavior we rely on works as expected.
We treat each transaction separately, which works fine in most cases and improves parallelism. But there are cases where we must ensure some sequentiality: - for standard commitments: commit tx must be published before htlc txs - for anchor output commitments: commit tx must be published before its anchor We previously relied on a custom execution context, but that doesn't work now that we create one actor per transaction.
This is fixed in 81a1b51 |
|
Closed in favor of #1844 |
This PR refactors the
TxPublisherand adds some new features:TxPublishResult, that will in the future allow the parentTxPublisherto manage the list of pending txs and RBF when neededIt can be useful to review commit by commit to see each change individually.