-
Notifications
You must be signed in to change notification settings - Fork 2.2k
fix scanning issue #10387
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
fix scanning issue #10387
Conversation
f14660a to
7f6b609
Compare
7f6b609 to
a2a2b53
Compare
|
lnd/contractcourt/channel_arbitrator.go Lines 2581 to 2593 in 8c8662c
That height is passed in from the lnd/contractcourt/channel_arbitrator.go Lines 1332 to 1343 in 8c8662c
The lnd/contractcourt/channel_arbitrator.go Lines 1666 to 1673 in 8c8662c
On a remote/local force close, the height used to advance is the
So renaming from |
|
Looks like a test needs to be updated: Details |
a2a2b53 to
9453346
Compare
The broadcastHeight was misleading because the commit resolver is only created when the commitment transaction is confirmed.
|
Linter is failing: |
Roasbeef
left a comment
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.
LGTM 🎎
9453346 to
db3add1
Compare
yyforyongyu
left a comment
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.
LGTM 🚢
In addition to the above analysis I've checked a few things,
- implications on the
to_localoutput on the remote commitment tx: in theory we can just sweep them as long as the force close hits the mempool. However atmRegisterSpendNtfnonly notifies when the tx is confirmed, so this is not a concern. advanceStatewill only start the commit resolver insideprepContractResolutionsuntil the stateStateContractClosedis reached, this guarantees that the commit resolver won't be launched until the force close tx is confirmed.- we also call
c.advanceStateinside<-c.resolutionSignal, given that anchor resolvers may be launched for CPFP, this means the commit resolver may receive a trigger height when the anchor resolver is resolved, which should be the confirm height of the closing tx, due to the blockbeat forces the blocks to be handled sequentially in these systems. - there's also a trigger height used in
<-c.forceCloseReqs, but it should be irrelevant.
| // confirmHeight is the block height that the commitment transaction was | ||
| // confirmed at. We'll use this value to bound any historical queries to | ||
| // the chain for spends/confirmations. | ||
| confirmHeight uint32 |
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.
nit: commitConfirmHeight or forceCloseTxConfirmHeight? at a glance i thought confirmHeight means the confirmed height of this contract
Backport contracourt fix #10387
Summary
This PR renames broadcastHeight to confirmHeight in commitSweepResolver to accurately reflect what the value represents.
The Problem
The field was named broadcastHeight with a comment stating it's "the height that the original contract was broadcast to the main-chain at." However, this is incorrect - the value is
actually the confirmation height of the commitment transaction (the block height where it was included), not the broadcast height (when it was submitted to the mempool).
Verification
The height passed to newCommitSweepResolver is the confirmation height in both code paths:
Normal operation (commitment tx just confirmed):
RegisterSpendNtfn fires when funding outpoint is spent (confirmed)
→ SpendDetail.SpendingHeight = confirmation height
→ dispatchLocalForceClose(commitSpend, ...)
→ advanceState(uint32(closeInfo.SpendingHeight), ...)
→ prepContractResolutions(..., height, ...)
→ newCommitSweepResolver(..., confirmHeight, ...)
Restart (channel already pending close):
ClosingHeight = CloseHeight from ChannelCloseSummary
→ CloseHeight is documented as "the height at which the funding transaction was spent"
→ triggerHeight = c.cfg.ClosingHeight
→ prepContractResolutions(..., triggerHeight, ...)
→ newCommitSweepResolver(..., confirmHeight, ...)
Changes
Backward Compatibility
No backward compatibility issues - the serialization format is a raw uint32 binary value, so only the Go variable name changes, not the wire format.
Future Work
The same misnaming exists in other resolvers that should be addressed in follow-up PRs: