Skip to content

Conversation

@arshbot
Copy link
Contributor

@arshbot arshbot commented Mar 24, 2022

In this PR we introduce V3 loop out and loop in submarine swaps via taproot. The general approach has been to implement v3 htlcs in htlc.go and patch the plumbing from loop -> loopserver to accommodate the extra values. In some cases we default to creating taproot htlcs instead of traditional p2sh, which may be something a keen reviewer might want to look into, should we decide not to go all in on taproot for the next release.

TODOs

  • loopd: btcec version bump
  • loopserver: btcec version bump
  • loopd: loopin
  • loopserver: loopin
  • loopd: loopout
  • loopserver: loopout
  • tidying up: docs

Known Issues:

  • validation on sweep fails
  • de/serialization to etcd fails due to different key size

Copy link
Contributor

@carlaKC carlaKC left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tapscript changes looking good! However the structure of this PR makes it quite difficult to review in depth, needs some cleanup/ refactors before next review please.

}
}

func (h *HtlcScriptV3) IsSuccessWitness(witness wire.TxWitness) bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs values?

<reciever_key> OP_CHECKSIGVERIFY OP_SIZE 20 OP_EQUALVERIFY OP_RIPEMD160 <hash> OP_EQUALVERIFY 1 OP_CHECKSEQUENCEVERIFY
*/
func createClaimPathLeaf(t *testing.T, recieverHtlcKey [32]byte, swapHash lntypes.Hash) (txscript.TapLeaf, []byte) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than storing claimScript/timeoutScript in HtlcScriptV3, move these functions into htlc.go and then we can just recreate them using our receiver/sender key and cltv when we need these scripts?

return nil, err
}

randomPub, _ := hex.DecodeString(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're going to deploy these chagnes incrementally (ie, do tapscript then musig keyspend) this is going to need to be deterministic / communicated between client/server so that each party knows the other doesn't know the privkey (and can just claim the htlc).

Probably not worth the effort, so would just add a clarifying comment that this is a dummy value that should not be used in prod.

swap/htlc.go Outdated

// TODO: Unsilence errors
controlBlockBytes, _ := h.genControlBlock(h.claimScript)
controlBlockBytes, _ := h.genControlBlock(h.timeoutScript)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this correct?

swap/htlc.go Outdated

// TODO: Unsilence errors
controlBlockBytes, _ := h.genControlBlock(h.timeoutScript)
controlBlockBytes, _ := h.genControlBlock(h.claimScript)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise, seems like these have switched?

// SenderKey is the key of the sender that will be used in the on-chain
// HTLC.
SenderKey [33]byte
SenderKey []byte
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move this commit to before we add HtlcScriptV3 + don't intermingle refactors with logical changes (eg SuccessSequence returns 10 now?)


// A human-readable message received from the loop server.
string server_message = 6;
string server_message = 7;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changing the proto number of server_message will break the API for old clients.

HtlcV2

HtlcV3
HtlcV3 = 10
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not use the existing iota?

@lightninglabs-deploy
Copy link

@bhandras: review reminder
@arshbot, remember to re-request review from reviewers when ready

@arshbot
Copy link
Contributor Author

arshbot commented Apr 13, 2022

Closing as #477 has taken over

@arshbot arshbot closed this Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants