Skip to content
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

tapchannel: improve aux signer signal handling #1118

Merged
merged 2 commits into from
Sep 12, 2024
Merged
Changes from 1 commit
Commits
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
52 changes: 38 additions & 14 deletions tapchannel/aux_leaf_signer.go
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ import (
"github.com/lightningnetwork/lnd/tlv"
)

// shutdownErr is used in multiple spots when exiting the sig batch processor.
var shutdownErr = fmt.Errorf("tapd is shutting down")

// VirtualPacketSigner is an interface that can be used to sign virtual packets.
type VirtualPacketSigner interface {
// SignVirtualPacket signs the virtual transaction of the given packet
Expand Down Expand Up @@ -241,43 +244,49 @@ func (s *AuxLeafSigner) processAuxSigBatch(chanState *channeldb.OpenChannel,
defer s.Wg.Done()

log.Tracef("Processing %d aux sig jobs", len(sigJobs))

for idx := range sigJobs {
sigJob := sigJobs[idx]
cancelAndErr := func(err error) {
respondErr := func(err error) {
log.Errorf("Error processing aux sig job: %v", err)

close(sigJob.Cancel)
sigJob.Resp <- lnwallet.AuxSigJobResp{
Copy link
Member

Choose a reason for hiding this comment

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

Wherever we send to a channel, we should also read on the Quit channel to make sure we never block on a send (even if it's buffered).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, so if we receive Quit we abandon sending the err? that may then block lnd then IIUC.

Also, if we select on Quit, and we entered this function from a Quit case statement, we would never send the error, since we're reading an already closed channel.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, yeah, you're right. We have to assume the Resp channel is buffered here though (which it is), then this should never block.

Copy link
Member

Choose a reason for hiding this comment

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

We have to assume the Resp channel is buffered here though (which it is), then this should never block.

Yeah this is what was puzzling w.r.t the original stack trace that led us down this line of inquiry: unless some weird mutation happened, why would tapd block on the channel send?

Err: err,
}
}

// If we're shutting down, we cancel the job and return.
// Check for cancel or quit signals before beginning the job.
select {
case <-sigJob.Cancel:
continue
case <-s.Quit:
cancelAndErr(fmt.Errorf("tapd is shutting down"))
respondErr(shutdownErr)
return

default:
}

// If there is no commit blob, this isn't a custom channel. We
// still need to signal the job as done though, even if we don't
// have a signature to return.
if sigJob.CommitBlob.IsNone() {
sigJob.Resp <- lnwallet.AuxSigJobResp{
jharveyb marked this conversation as resolved.
Show resolved Hide resolved
select {
case sigJob.Resp <- lnwallet.AuxSigJobResp{
HtlcIndex: sigJob.HTLC.HtlcIndex,
}:
continue
case <-sigJob.Cancel:
continue
case <-s.Quit:
respondErr(shutdownErr)
return
}
continue
}

com, err := cmsg.DecodeCommitment(
sigJob.CommitBlob.UnsafeFromSome(),
)
if err != nil {
cancelAndErr(fmt.Errorf("error decoding commitment: "+
"%w", err))
respondErr(fmt.Errorf("error decoding commitment: %w",
err))
return
}

Expand All @@ -299,26 +308,41 @@ func (s *AuxLeafSigner) processAuxSigBatch(chanState *channeldb.OpenChannel,
// If the HTLC doesn't have any asset outputs, it's not an
// asset HTLC, so we can skip it.
if len(htlcOutputs) == 0 {
sigJob.Resp <- lnwallet.AuxSigJobResp{
select {
case sigJob.Resp <- lnwallet.AuxSigJobResp{
HtlcIndex: sigJob.HTLC.HtlcIndex,
}:
continue
case <-sigJob.Cancel:
continue
case <-s.Quit:
respondErr(shutdownErr)
return
}
continue
}

resp, err := s.generateHtlcSignature(
chanState, commitTx, htlcOutputs, sigJob.SignDesc,
sigJob.BaseAuxJob,
)
if err != nil {
cancelAndErr(fmt.Errorf("error generating HTLC "+
respondErr(fmt.Errorf("error generating HTLC "+
"signature: %w", err))
return
}

// Success!
log.Tracef("Generated HTLC signature for HTLC with index %d",
sigJob.HTLC.HtlcIndex)
sigJob.Resp <- resp

select {
case sigJob.Resp <- resp:
case <-sigJob.Cancel:
continue
case <-s.Quit:
respondErr(shutdownErr)
return
}
}
}

Expand Down
Loading