Classify outputs and amounts for SCBForceCloseChannel #216
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds output classification functionality to the scbforceclose command, which helps users identify different types of outputs (to_remote, anchors, to_local/htlc) in force-close transactions derived from static channel backups (SCBs).
- Adds logic to classify transaction outputs by matching against known script templates
- Maps SCB backup versions to channel types for proper script derivation
- Includes a test with real-world transaction data to verify the classification logic
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| cmd/chantools/scbforceclose.go | Adds classifyOutputs, classifyAndLogOutputs, and chanTypeFromBackupVersion functions to identify and log different output types in force-close transactions |
| cmd/chantools/scbforceclose_test.go | Adds test case with real-world data to verify to_remote output identification across different channel versions and initiator roles |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| remoteHex := "029e5f4d86d9d6c845fbcf37b09ac7d59c25c19932ab34a2757e8" + | ||
| "ea88437a876c3" |
There was a problem hiding this comment.
[nitpick] The remote public key hex string is split across lines inconsistently (63 characters on line 18 followed by 15 on line 19). Consider using a more consistent split point or keeping it on a single line for better readability.
| remoteHex := "029e5f4d86d9d6c845fbcf37b09ac7d59c25c19932ab34a2757e8" + | |
| "ea88437a876c3" | |
| remoteHex := "029e5f4d86d9d6c845fbcf37b09ac7d59c25c19932ab34a2757e8ea88437a876c3" |
| "638ab341c5ac77fdd022069db71847c48b1f762242b99b2fa254b1bce8f4" + | ||
| "4a160293fe3b36ed2d2e32f650147522103ae9df242881bb10a2400e781" + | ||
| "2fc8cfe437f0f869538584d39d96f52cb2dbaf622103e71742ef40d13688" + | ||
| "4a1f7368fb096cc5897fd697b41a3b481def37b60188c49152aebf573f20" |
There was a problem hiding this comment.
We can put it into a separate file and load it using go:embed.
| func classifyAndLogOutputs(s chanbackup.Single, tx *wire.MsgTx) { | ||
| class := classifyOutputs(s, tx) | ||
| if class.ToRemoteIdx >= 0 { | ||
| log.Infof( |
There was a problem hiding this comment.
I think it should be formatted like:
log.Infof("to_remote: idx=%d amount=%d sat", ...https://github.com/lightningnetwork/lnd/blob/master/docs/development_guidelines.md
| case chanbackup.AnchorsCommitVersion: | ||
| chanType = channeldb.AnchorOutputsBit | ||
| chanType |= channeldb.SingleFunderTweaklessBit | ||
|
|
||
| case chanbackup.AnchorsZeroFeeHtlcTxCommitVersion: | ||
| chanType = channeldb.ZeroHtlcTxFeeBit | ||
| chanType |= channeldb.AnchorOutputsBit | ||
| chanType |= channeldb.SingleFunderTweaklessBit |
There was a problem hiding this comment.
Idea, if you like it:
case chanbackup.AnchorsCommitVersion:
chanType = channeldb.AnchorOutputsBit
chanType |= channeldb.SingleFunderTweaklessBit
fallthrough
case chanbackup.AnchorsZeroFeeHtlcTxCommitVersion:
chanType |= channeldb.ZeroHtlcTxFeeBit
Also SimpleTaprootVersion and SimpleTaprootVersion can be coupled.
| } | ||
| } | ||
|
|
||
| const anchorVSize = int64(330) // anchor output value in sats |
There was a problem hiding this comment.
Maybe anchorSats?
// anchorSats is anchor output value in sats.
const anchorSats = 330| OtherIdxs []int | ||
| } | ||
|
|
||
| func classifyOutputs(s chanbackup.Single, tx *wire.MsgTx) outputClassification { |
| } | ||
| } | ||
|
|
||
| type outputClassification struct { |
There was a problem hiding this comment.
Aff godoc to the struct and its fields.
| t.Logf( | ||
| "to_remote: idx=%d amount=%d sat", lastClass.ToRemoteIdx, | ||
| lastClass.ToRemoteAmt, | ||
| ) |
There was a problem hiding this comment.
Style in this file:
t.Log("to_remote: ...| } | ||
| var tx wire.MsgTx | ||
| if err := tx.Deserialize(bytes.NewReader(txBytes)); err != nil { | ||
| t.Fatalf("deserialize tx: %v", err) |
There was a problem hiding this comment.
Use require.NoError
Here and in other places in this file.
| if len(lastClass.ToRemotePkScript) > 0 { | ||
| t.Logf( | ||
| "to_remote PkScript (hex): %x", | ||
| lastClass.ToRemotePkScript, | ||
| ) | ||
| } | ||
| for _, idx := range lastClass.AnchorIdxs { | ||
| t.Logf( | ||
| "possible anchor: idx=%d amount=%d sat", idx, | ||
| tx.TxOut[idx].Value, | ||
| ) | ||
| } | ||
| for _, idx := range lastClass.OtherIdxs { | ||
| t.Logf( | ||
| "possible to_local/htlc: idx=%d amount=%d sat", idx, | ||
| tx.TxOut[idx].Value, | ||
| ) | ||
| } |
There was a problem hiding this comment.
What is the point of this section? We can just call classifyAndLogOutputs to print, not to make copy-paste.
| // against known script templates (p2wkh, delayed p2wsh, lease), marks 330-sat | ||
| // anchors, and logs remaining outputs as to_local/htlc without deriving | ||
| // per-commitment data. | ||
| func classifyAndLogOutputs(s chanbackup.Single, tx *wire.MsgTx) { |
There was a problem hiding this comment.
I propose a signature change:
func printOutputClassification(class outputClassification)2 participants
We also log the amounts to the user so they know whether it is worth taking the risk publishing a potential outdated transaction.