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.

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

htlcswitch+routing: handle nil pointer dereference properly #9303

Merged
merged 3 commits into from
Dec 17, 2024
Merged

htlcswitch+routing: handle nil pointer dereference properly #9303

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
944f162
htlcswitch: handle nil circuit properly when settling
yyforyongyu Nov 10, 2024
39584be
routing: fix nil pointer dereference in `exitWithErr`
yyforyongyu Nov 7, 2024
7882e1c
lnwallet: add debug logs
yyforyongyu Nov 5, 2024
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
11 changes: 11 additions & 0 deletions htlcswitch/packet.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package htlcswitch

import (
"fmt"

"github.com/lightningnetwork/lnd/channeldb"
"github.com/lightningnetwork/lnd/graph/db/models"
"github.com/lightningnetwork/lnd/htlcswitch/hop"
Expand Down Expand Up @@ -136,3 +138,12 @@ func (p *htlcPacket) keystone() Keystone {
OutKey: p.outKey(),
}
}

// String returns a human-readable description of the packet.
func (p *htlcPacket) String() string {
return fmt.Sprintf("keystone=%v, sourceRef=%v, destRef=%v, "+
"incomingAmount=%v, amount=%v, localFailure=%v, hasSource=%v "+
"isResolution=%v", p.keystone(), p.sourceRef, p.destRef,
p.incomingAmount, p.amount, p.localFailure, p.hasSource,
p.isResolution)
}
11 changes: 10 additions & 1 deletion htlcswitch/switch.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2994,6 +2994,15 @@ func (s *Switch) handlePacketSettle(packet *htlcPacket) error {
// If the source of this packet has not been set, use the circuit map
// to lookup the origin.
circuit, err := s.closeCircuit(packet)

// If the circuit is in the process of closing, we will return a nil as
// there's another packet handling undergoing.
if errors.Is(err, ErrCircuitClosing) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm I am not sure if we should return nil here, because we shouldn't call the settle logic twice if we are already closing the circuit and if we do so shouldn't we return an error signaling that the circuit is closing rather than allowing it to be called multiple times and neglecting the error ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I also think we should return an error here and let it be handled - unfortunately we cannot easily do it atm. I view this as a followup of 8c0c53e - in addition of catching the closed case, we should also catch the closing case. Tho it does feel like the logic should be put inside closeCircuit?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ahh ok given this fact I think it is ok to place the logic here. But I would recommend to add a comment here why we can end up calling this logic here twice the current comment I would not be able to explain what this really means in detail or how this might happen:

there's another packet handling undergoing.

i this really only the case if resolution and the normal htlcPlex loggic trigger shortly after another ?

log.Debugf("Circuit is closing for packet=%v", packet)
return nil
}

// Exit early if there's another error.
if err != nil {
return err
}
Expand All @@ -3005,7 +3014,7 @@ func (s *Switch) handlePacketSettle(packet *htlcPacket) error {
// and when `UpdateFulfillHTLC` is received. After which `RevokeAndAck`
// is received, which invokes `processRemoteSettleFails` in its link.
if circuit == nil {
log.Debugf("Found nil circuit: packet=%v", spew.Sdump(packet))
log.Debugf("Circuit already closed for packet=%v", packet)
return nil
}

Expand Down
5 changes: 4 additions & 1 deletion lnwallet/channel.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1710,7 +1710,7 @@ func (lc *LightningChannel) restorePendingRemoteUpdates(
localCommitmentHeight uint64,
pendingRemoteCommit *commitment) error {

lc.log.Debugf("Restoring %v dangling remote updates",
lc.log.Debugf("Restoring %v dangling remote updates pending our sig",
len(unsignedAckedUpdates))

for _, logUpdate := range unsignedAckedUpdates {
Expand Down Expand Up @@ -1829,6 +1829,9 @@ func (lc *LightningChannel) restorePendingLocalUpdates(
pendingCommit := pendingRemoteCommitDiff.Commitment
pendingHeight := pendingCommit.CommitHeight

lc.log.Debugf("Restoring pending remote commitment %v at commit "+
"height %v", pendingCommit.CommitTx.TxHash(), pendingHeight)

auxResult, err := fn.MapOptionZ(
lc.leafStore,
func(s AuxLeafStore) fn.Result[CommitDiffAuxResult] {
Expand Down
21 changes: 16 additions & 5 deletions routing/payment_lifecycle.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,10 +189,21 @@ func (p *paymentLifecycle) resumePayment(ctx context.Context) ([32]byte,
p.resultCollector(&a)
}

// Get the payment status.
status := payment.GetStatus()

// exitWithErr is a helper closure that logs and returns an error.
exitWithErr := func(err error) ([32]byte, *route.Route, error) {
log.Errorf("Payment %v with status=%v failed: %v",
p.identifier, payment.GetStatus(), err)
// Log an error with the latest payment status.
//
// NOTE: this `status` variable is reassigned in the loop
// below. We could also call `payment.GetStatus` here, but in a
// rare case when the critical log is triggered when using
// postgres as db backend, the `payment` could be nil, causing
// the payment fetching to return an error.
log.Errorf("Payment %v with status=%v failed: %v", p.identifier,
status, err)

return [32]byte{}, nil, err
}

Expand All @@ -213,10 +224,10 @@ lifecycle:
ps := payment.GetState()
remainingFees := p.calcFeeBudget(ps.FeesPaid)

status = payment.GetStatus()
log.Debugf("Payment %v: status=%v, active_shards=%v, "+
"rem_value=%v, fee_limit=%v", p.identifier,
payment.GetStatus(), ps.NumAttemptsInFlight,
ps.RemainingAmt, remainingFees)
"rem_value=%v, fee_limit=%v", p.identifier, status,
ps.NumAttemptsInFlight, ps.RemainingAmt, remainingFees)

// We now proceed our lifecycle with the following tasks in
// order,
Expand Down
Loading