Skip to content

contractcourt: only halt resolution for TapscriptRoot channels #9208

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.

Sign up for GitHub

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

Closed
carlaKC wants to merge 1 commit into from
+48 −4
Closed

contractcourt: only halt resolution for TapscriptRoot channels #9208

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion contractcourt/htlc_incoming_contest_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,10 @@ func (h *htlcIncomingContestResolver) Resolve(
//
// TODO(roasbeef): Implement resolving HTLCs with custom records
// (follow-up PR).
if len(h.htlc.CustomRecords) != 0 {
if len(h.htlc.CustomRecords) != 0 && h.isTapscriptRoot {
log.Warnf("Not resolving HTLC with: %v custom records",
len(h.htlc.CustomRecords))

select { //nolint:gosimple
case <-h.quit:
return nil, errResolverShuttingDown
Expand Down
5 changes: 4 additions & 1 deletion contractcourt/htlc_outgoing_contest_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,10 @@ func (h *htlcOutgoingContestResolver) Resolve(
//
// TODO(roasbeef): Implement resolving HTLCs with custom records
// (follow-up PR).
if len(h.htlc.CustomRecords) != 0 {
if len(h.htlc.CustomRecords) != 0 && h.isTapscriptRoot {
Copy link
Copy Markdown
Collaborator Author

@carlaKC carlaKC Oct 31, 2024

Choose a reason for hiding this comment

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

I believe we could just check h.isTapscriptRoot here as our check?

Happy to remove the CustomRecords check if it's useful, but since this is a to-be-removed workaround I didn't think it matters that much (and more specific = more good?).

log.Warnf("Not resolving HTLC with: %v custom records",
len(h.htlc.CustomRecords))

select { //nolint:gosimple
case <-h.quit:
return nil, errResolverShuttingDown
Expand Down
22 changes: 21 additions & 1 deletion contractcourt/htlc_success_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ type htlcSuccessResolver struct {
// htlc contains information on the htlc that we are resolving on-chain.
htlc channeldb.HTLC

// isTapscriptRoot indicates whether the htlc is on a TapscriptRoot
// channel type.
// TODO: remove this when incoming HTLCs with custom records can be
// resolved for this channel type
isTapscriptRoot bool
Copy link
Copy Markdown
Member

@yyforyongyu yyforyongyu Oct 24, 2024

Choose a reason for hiding this comment

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

think we can already get the info using the scripts, copied from other PRs,

// isTaproot returns true if the htlc output is a taproot output.
func (h *htlcTimeoutResolver) isTaproot() bool {
	return txscript.IsPayToTaproot(
		h.htlcResolution.SweepSignDesc.Output.PkScript,
	)
}

Copy link
Copy Markdown
Collaborator Author

@carlaKC carlaKC Oct 24, 2024

Choose a reason for hiding this comment

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

IIUC IsPayToTaproot isn't a sufficient check here? Based on the original comment, I believe that we're specifically looking for TapscriptRoot channel types - this check would be true for both TapcriptRoot and SimpleTaproot channels because it's just checking that we have a P2TR witness?

Copy link
Copy Markdown
Member

@yyforyongyu yyforyongyu Oct 30, 2024

Choose a reason for hiding this comment

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

yeah you are right these are two different things. In that case do you think it's possible to pass the boolean when creating the resolvers so we don't need the SupplementState hack? I remembered we sort have to use this hack, just wanna double check (sth sth reloading from the disk I think).

Copy link
Copy Markdown
Collaborator Author

@carlaKC carlaKC Oct 31, 2024

Choose a reason for hiding this comment

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

Yeah this is how we currently go about relaunching after startup. We don't have state with our ChanType on hand when we read from disk which means we'll always need to supplementary step on restarts.

I think we should just always do it that way for consistency?

Copy link
Copy Markdown
Member

@yyforyongyu yyforyongyu Nov 8, 2024

Choose a reason for hiding this comment

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

yeah that's a design i'm trying to fix, pre-exiting so no worries.


// currentReport stores the current state of the resolver for reporting
// over the rpc interface. This should only be reported in case we have
// a non-nil SignDetails on the htlcResolution, otherwise the nursery
Expand Down Expand Up @@ -127,7 +133,10 @@ func (h *htlcSuccessResolver) Resolve(
//
// TODO(roasbeef): Implement resolving HTLCs with custom records
// (follow-up PR).
if len(h.htlc.CustomRecords) != 0 {
if len(h.htlc.CustomRecords) != 0 && h.isTapscriptRoot {
log.Warnf("Not resolving incoming htlc with %v custom records",
len(h.htlc.CustomRecords))

select { //nolint:gosimple
case <-h.quit:
return nil, errResolverShuttingDown
Expand Down Expand Up @@ -739,6 +748,17 @@ func (h *htlcSuccessResolver) HtlcPoint() wire.OutPoint {
func (h *htlcSuccessResolver) SupplementDeadline(_ fn.Option[int32]) {
}

// SupplementState allows the user of a ContractResolver to supplement it with
// state required for the proper resolution of a contract.
//
// NOTE: Part of the ContractResolver interface.
func (h *htlcIncomingContestResolver) SupplementState(
state *channeldb.OpenChannel) {

h.isTapscriptRoot = state.ChanType.HasTapscriptRoot()
h.htlcLeaseResolver.SupplementState(state)
}

// A compile time assertion to ensure htlcSuccessResolver meets the
// ContractResolver interface.
var _ htlcContractResolver = (*htlcSuccessResolver)(nil)
20 changes: 19 additions & 1 deletion contractcourt/htlc_timeout_resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,12 @@ type htlcTimeoutResolver struct {
// htlc contains information on the htlc that we are resolving on-chain.
htlc channeldb.HTLC

// isTapscriptRoot indicates whether the htlc is on a TapscriptRoot
// channel type.
// TODO: remove this when incoming HTLCs with custom records can be
// resolved for this channel type
isTapscriptRoot bool

// currentReport stores the current state of the resolver for reporting
// over the rpc interface. This should only be reported in case we have
// a non-nil SignDetails on the htlcResolution, otherwise the nursery
Expand Down Expand Up @@ -430,7 +436,10 @@ func (h *htlcTimeoutResolver) Resolve(
//
// TODO(roasbeef): Implement resolving HTLCs with custom records
// (follow-up PR).
if len(h.htlc.CustomRecords) != 0 {
if len(h.htlc.CustomRecords) != 0 && h.isTapscriptRoot {
log.Warnf("Not resolving incoming htlc with %v custom records",
len(h.htlc.CustomRecords))

select { //nolint:gosimple
case <-h.quit:
return nil, errResolverShuttingDown
Expand Down Expand Up @@ -1095,6 +1104,15 @@ func (h *htlcTimeoutResolver) SupplementDeadline(d fn.Option[int32]) {
h.incomingHTLCExpiryHeight = d
}

// SupplementState allows the user of a ContractResolver to supplement it with
// state required for the proper resolution of a contract.
//
// NOTE: Part of the ContractResolver interface.
func (h *htlcTimeoutResolver) SupplementState(state *channeldb.OpenChannel) {
h.isTapscriptRoot = state.ChanType.HasTapscriptRoot()
h.htlcLeaseResolver.SupplementState(state)
}

// A compile time assertion to ensure htlcTimeoutResolver meets the
// ContractResolver interface.
var _ htlcContractResolver = (*htlcTimeoutResolver)(nil)
Expand Down