Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce new BumpTransactionEvent variant HTLCResolution #1825
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce new BumpTransactionEvent variant HTLCResolution #1825
Changes from all commits
af02d10
d618bf1
baf2dec
d7027c2
6874126
d0847bd
0aaba2c
c17ce2e
8170c84
ec1f334
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here I think we can
debug_assert!(seff.onchain_tx_handler.opt_anchors())
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably replace most
#[cfg(anchors)]
with this assertion so will leave it for a follow-up.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is assumption is broken, and per se our code will still miss the detection and claim of revoked second-stage HTLC transactions.
With
SIGHASH_SINGLE | SIGHASH_ANYONECANPAY
, the second-stage HTLC transaction claims can be aggregated across commitment transactions, as there is no commitment to the spend txid in the shared transaction fields (e.g the parent txid could be in the nLocktime field). Our logic iterate on all the inputs, and at the first one with a confirmed commitment txid incounterparty_commitment_txn_on_chain
, we'll call incheck_spend_counterparty_htlc()
L3028. There, we iterate against all inputs and if the input outpoint txid is different from the passedcommitment_txid
, we skip it further processing. Once we're out ofcheck_spend_counterparty_htlc()
, we end up here and break from thefor
control flow, am I correct ? At the very least I don't think we have a test coverage for this case.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I 100% got your issue here, but I don't think this is broken - we can't have two transactions confirmed spending different commitment transactions, thus, if
commitment_txid
is set based oncounterparty_commitment_txn_on_chain
its definitely the one and only remote commitment transaction that is spendable. Thus, the loop incheck_spend_counterparty_htlc
looks correct to me.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Only one commitment transaction can ultimately confirm per channel though, which is why we break. If a second-stage HTLC transaction claims HTLCs across different channels, then it's up to each
ChannelMonitor
to claim the HTLCs only for their channel.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay after looking more, the code is correct, even if the comment is wrong or confusing on the
(all from the same commitment)
as you can have HTLCs claims in the same transaction spending multiple counterparty commitment transactions.Note, here we're checking the second-stage HTLC transactions, and we're filtering out the input/output pair to be claimed based on the spent commitment transaction id. Post-anchor output, second-stage HTLC transactions are signed with
SIGHASH_SINGLE
enabling aggregation of HTLC claims from the same commitment transaction, but there is no constraint restraining the aggregation of HTLC claims across commitment transactions. Even if for oneChannelMonitor
there is a singlecommitment_txid
that can confirms.This is a correct, the claim of HTLCs is a per-ChannelMonitor responsibility, and I don't think there is a way open by malleability where the aggregated HTLC claims crafted by our counterparty could blind our parsing logic. Note, it makes the logic dependent on the implementation of
Chain::Confirm::transaction_confirmed
. Our default implementation is correct as we're iterating over all the monitor states, though a more naive logic which would dedup transaction confirmation, or if it'll stop transaction processing after first monitor state match would be concerned by the security issue I'm raising.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the
ChannelMonitor
is already on a per-channel basis, the comment really means "all from the same commitment for this channel". I agree it could definitely be re-worded better though.Doesn't seem like there's much we can do other than document it? Even then, I would imagine all LDK users want to use our
ChainMonitor
as is. Rather than re-implementing it for their own needs, they just need to implement the trait dependencies onChainMonitor
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More documentation of our assumptions is always better. I think with the extended flexibility offered by our interfaces we have far less visibility on how it could be re-implemented by users rather than a monolithic Lightning node directly consuming from standard Core interfaces. This is raising the bar for funds safety, as implicit assumptions of our low-level code could be silently broken. Beyond, we might have in the future to re-implement a
ChainMonitor
for "monitor-replica" support, a bit more sophisticated than the standard one.