diff --git a/pkg/settlement/swap/chequebook/cashout.go b/pkg/settlement/swap/chequebook/cashout.go index 5f5c4ce8f8f..a0c380fe9f5 100644 --- a/pkg/settlement/swap/chequebook/cashout.go +++ b/pkg/settlement/swap/chequebook/cashout.go @@ -134,12 +134,6 @@ func (s *cashoutService) CashCheque(ctx context.Context, chequebook, recipient c return common.Hash{}, err } - // a corrupt statestore entry unmarshals into a non-nil cheque with a nil - // cumulativePayout; guard against packing it below. - if cheque.CumulativePayout == nil { - return common.Hash{}, fmt.Errorf("nil cumulative payout on cheque loaded from statestore for chequebook %x: %w", chequebook, ErrNoCheque) - } - callData, err := chequebookABI.Pack("cashChequeBeneficiary", recipient, cheque.CumulativePayout, cheque.Signature) if err != nil { return common.Hash{}, err @@ -176,12 +170,6 @@ func (s *cashoutService) CashoutStatus(ctx context.Context, chequebookAddress co return nil, err } - // a corrupt statestore entry unmarshals into a non-nil cheque with a nil - // cumulativePayout; guard against the arithmetic below. - if cheque.CumulativePayout == nil { - return nil, fmt.Errorf("nil cumulative payout on cheque loaded from statestore for chequebook %x: %w", chequebookAddress, ErrNoCheque) - } - var action cashoutAction err = s.store.Get(cashoutActionKey(chequebookAddress), &action) if err != nil { diff --git a/pkg/settlement/swap/chequebook/chequebook.go b/pkg/settlement/swap/chequebook/chequebook.go index d5bebb16f49..9b1d7a823a0 100644 --- a/pkg/settlement/swap/chequebook/chequebook.go +++ b/pkg/settlement/swap/chequebook/chequebook.go @@ -201,10 +201,6 @@ func (s *service) Issue(ctx context.Context, beneficiary common.Address, amount return nil, err } cumulativePayout = big.NewInt(0) - } else if lastCheque.CumulativePayout == nil { - // a corrupt statestore entry unmarshals into a non-nil cheque with a - // nil cumulativePayout; guard against the arithmetic below. - return nil, fmt.Errorf("nil cumulative payout on cheque loaded from statestore for beneficiary %x: %w", beneficiary, ErrNoCheque) } else { cumulativePayout = lastCheque.CumulativePayout } @@ -280,6 +276,10 @@ func (s *service) LastCheque(beneficiary common.Address) (*SignedCheque, error) return nil, fmt.Errorf("nil cheque loaded from statestore for beneficiary %x: %w", beneficiary, ErrNoCheque) } + if lastCheque.CumulativePayout == nil { + return nil, fmt.Errorf("nil cumulative payout on cheque loaded from statestore for beneficiary %x: %w", beneficiary, ErrNilCumulativePayout) + } + return lastCheque, nil } diff --git a/pkg/settlement/swap/chequebook/chequestore.go b/pkg/settlement/swap/chequebook/chequestore.go index ebdc26a5d81..ce7d6b868b4 100644 --- a/pkg/settlement/swap/chequebook/chequestore.go +++ b/pkg/settlement/swap/chequebook/chequestore.go @@ -26,6 +26,9 @@ const ( var ( // ErrNoCheque is the error returned if there is no prior cheque for a chequebook or beneficiary. ErrNoCheque = errors.New("no cheque") + // ErrNilCumulativePayout is returned when a stored cheque has no cumulative payout. + // It does not wrap ErrNoCheque: callers treat that as "no cheque" and may start a new payout at zero. + ErrNilCumulativePayout = errors.New("nil cumulative payout") // ErrChequeNotIncreasing is the error returned if the cheque amount is the same or lower. ErrChequeNotIncreasing = errors.New("cheque cumulativePayout is not increasing") // ErrChequeInvalid is the error returned if the cheque itself is invalid. @@ -103,6 +106,10 @@ func (s *chequeStore) LastCheque(chequebook common.Address) (*SignedCheque, erro return nil, fmt.Errorf("nil cheque loaded from statestore for chequebook %x: %w", chequebook, ErrNoCheque) } + if cheque.CumulativePayout == nil { + return nil, fmt.Errorf("nil cumulative payout on cheque loaded from statestore for chequebook %x: %w", chequebook, ErrNilCumulativePayout) + } + return cheque, nil } diff --git a/pkg/settlement/swap/chequebook/nilcheque_test.go b/pkg/settlement/swap/chequebook/nilcheque_test.go index 62fab6dfb73..52692c02d1a 100644 --- a/pkg/settlement/swap/chequebook/nilcheque_test.go +++ b/pkg/settlement/swap/chequebook/nilcheque_test.go @@ -104,6 +104,58 @@ func TestServiceLastChequeNilEntry(t *testing.T) { } } +// TestServiceLastChequeNilCumulativePayout covers a last-issued-cheque entry of +// `{}`: it unmarshals into a non-nil cheque whose CumulativePayout is nil. +// LastCheque must reject it with its own error, not ErrNoCheque, so Issue does +// not treat the row as a first cheque and start the payout at zero. +func TestServiceLastChequeNilCumulativePayout(t *testing.T) { + t.Parallel() + + store := newRawStore() + beneficiary := common.HexToAddress("0xbe") + store.setRaw(chequebook.LastIssuedChequeKey(beneficiary), []byte(`{}`)) + + svc, err := chequebook.New(nil, common.Address{}, common.Address{}, store, nil, nil) + if err != nil { + t.Fatal(err) + } + + cheque, err := svc.LastCheque(beneficiary) + if !errors.Is(err, chequebook.ErrNilCumulativePayout) { + t.Fatalf("expected %v, got %v", chequebook.ErrNilCumulativePayout, err) + } + if errors.Is(err, chequebook.ErrNoCheque) { + t.Fatal("corrupt payout must not be reported as no cheque") + } + if cheque != nil { + t.Fatalf("expected nil cheque, got %v", cheque) + } +} + +// TestChequeStoreLastChequeNilCumulativePayout covers a last-received-cheque +// entry of `{}`. Readers such as settlements add CumulativePayout, so LastCheque +// must reject the blank amount before it is returned. +func TestChequeStoreLastChequeNilCumulativePayout(t *testing.T) { + t.Parallel() + + store := newRawStore() + chequebookAddr := common.HexToAddress("0xcb") + store.setRaw(chequebook.LastReceivedChequeKey(chequebookAddr), []byte(`{}`)) + + cs := chequebook.NewChequeStore(store, nil, 1, common.HexToAddress("0xbe"), nil, nil) + + cheque, err := cs.LastCheque(chequebookAddr) + if !errors.Is(err, chequebook.ErrNilCumulativePayout) { + t.Fatalf("expected %v, got %v", chequebook.ErrNilCumulativePayout, err) + } + if errors.Is(err, chequebook.ErrNoCheque) { + t.Fatal("corrupt payout must not be reported as no cheque") + } + if cheque != nil { + t.Fatalf("expected nil cheque, got %v", cheque) + } +} + // TestChequeStoreReceiveChequeNilCumulativePayout is a regression test for a // peer-supplied cheque that simply omits cumulativePayout: it unmarshals into a // non-nil cheque whose CumulativePayout is a nil *big.Int, which big.Int.Sub @@ -148,8 +200,14 @@ func TestChequeStoreEmptyObjectLastReceived(t *testing.T) { cs := chequebook.NewChequeStore(store, nil, 1, beneficiary, nil, nil) lastCheque, err := cs.LastCheque(chequebookAddr) - if err == nil && lastCheque == nil { - t.Fatal("LastCheque returned a nil cheque with a nil error") + if !errors.Is(err, chequebook.ErrNilCumulativePayout) { + t.Fatalf("expected %v, got %v", chequebook.ErrNilCumulativePayout, err) + } + if errors.Is(err, chequebook.ErrNoCheque) { + t.Fatal("corrupt payout must not be reported as no cheque") + } + if lastCheque != nil { + t.Fatalf("expected nil cheque, got %v", lastCheque) } cheque := &chequebook.SignedCheque{ diff --git a/pkg/settlement/swap/chequebook/nilcumulativepayout_test.go b/pkg/settlement/swap/chequebook/nilcumulativepayout_test.go index e86da18b546..12a1ec8ec0c 100644 --- a/pkg/settlement/swap/chequebook/nilcumulativepayout_test.go +++ b/pkg/settlement/swap/chequebook/nilcumulativepayout_test.go @@ -45,12 +45,9 @@ func pendingBackend() transaction.Backend { ) } -// TestCashoutStatusNilCumulativePayoutLastCheque covers the guard in -// CashoutStatus that follows LastCheque. A last-received-cheque entry of `{}` -// unmarshals into a non-nil *SignedCheque whose CumulativePayout is a nil -// *big.Int; every CashoutStatus branch does arithmetic on it, so without the -// guard the arithmetic against the recorded cashout action panics. Reachable -// from GET /chequebook/cashout/{peer}. +// TestCashoutStatusNilCumulativePayoutLastCheque covers a last-received-cheque +// entry of `{}`. LastCheque rejects the nil payout before CashoutStatus does +// arithmetic on it. Reachable from GET /chequebook/cashout/{peer}. func TestCashoutStatusNilCumulativePayoutLastCheque(t *testing.T) { t.Parallel() @@ -87,8 +84,11 @@ func TestCashoutStatusNilCumulativePayoutLastCheque(t *testing.T) { ) _, err = cashoutService.CashoutStatus(context.Background(), chequebookAddress) - if !errors.Is(err, chequebook.ErrNoCheque) { - t.Fatalf("expected error wrapping %v, got %v", chequebook.ErrNoCheque, err) + if !errors.Is(err, chequebook.ErrNilCumulativePayout) { + t.Fatalf("expected error wrapping %v, got %v", chequebook.ErrNilCumulativePayout, err) + } + if errors.Is(err, chequebook.ErrNoCheque) { + t.Fatal("corrupt payout must not be reported as no cheque") } } @@ -133,8 +133,8 @@ func TestCashoutStatusNilCumulativePayoutAction(t *testing.T) { } } -// TestCashChequeNilCumulativePayout covers the guard in CashCheque: the nil -// *big.Int would otherwise be dereferenced by the ABI packer. +// TestCashChequeNilCumulativePayout covers a last-received cheque with a nil +// payout. LastCheque rejects it before CashCheque packs the amount for the chain. func TestCashChequeNilCumulativePayout(t *testing.T) { t.Parallel() @@ -153,14 +153,17 @@ func TestCashChequeNilCumulativePayout(t *testing.T) { ) _, err := cashoutService.CashCheque(context.Background(), chequebookAddress, recipientAddress) - if !errors.Is(err, chequebook.ErrNoCheque) { - t.Fatalf("expected error wrapping %v, got %v", chequebook.ErrNoCheque, err) + if !errors.Is(err, chequebook.ErrNilCumulativePayout) { + t.Fatalf("expected error wrapping %v, got %v", chequebook.ErrNilCumulativePayout, err) + } + if errors.Is(err, chequebook.ErrNoCheque) { + t.Fatal("corrupt payout must not be reported as no cheque") } } -// TestChequebookIssueNilCumulativePayout covers the sending-side mirror in -// Issue: a corrupt last-issued-cheque entry yields a non-nil cheque with a nil -// CumulativePayout, which Issue would otherwise add the amount to. +// TestChequebookIssueNilCumulativePayout covers a corrupt last-issued cheque. +// LastCheque rejects the nil payout with its own error, so Issue must not send +// a new cheque and must not treat the row as a first cheque starting at zero. func TestChequebookIssueNilCumulativePayout(t *testing.T) { t.Parallel() @@ -192,7 +195,10 @@ func TestChequebookIssueNilCumulativePayout(t *testing.T) { t.Fatal("cheque must not be sent") return nil }) - if !errors.Is(err, chequebook.ErrNoCheque) { - t.Fatalf("expected error wrapping %v, got %v", chequebook.ErrNoCheque, err) + if !errors.Is(err, chequebook.ErrNilCumulativePayout) { + t.Fatalf("expected error wrapping %v, got %v", chequebook.ErrNilCumulativePayout, err) + } + if errors.Is(err, chequebook.ErrNoCheque) { + t.Fatal("corrupt payout must not be reported as no cheque") } } diff --git a/pkg/settlement/swap/chequebook/statestore_fuzz_test.go b/pkg/settlement/swap/chequebook/statestore_fuzz_test.go index 667ce9e9e62..2e2e1e3517d 100644 --- a/pkg/settlement/swap/chequebook/statestore_fuzz_test.go +++ b/pkg/settlement/swap/chequebook/statestore_fuzz_test.go @@ -85,10 +85,10 @@ func FuzzChequeStoreStatestoreCorruption(f *testing.F) { func(*chequebook.SignedCheque, int64) (common.Address, error) { return common.Address{}, nil }, ) - // 1. LastCheque must never panic + // 1. LastCheque must never panic, and a success must carry a payout. lastCheque, err := cs.LastCheque(chequebookAddr) - if err == nil && lastCheque == nil { - t.Fatal("NIL-02: LastCheque returned (nil, nil)") + if err == nil && (lastCheque == nil || lastCheque.CumulativePayout == nil) { + t.Fatal("LastCheque returned a cheque with no cumulative payout") } // 2. ReceiveCheque must never panic when reading corrupted last-received cheque @@ -106,8 +106,8 @@ func FuzzChequeStoreStatestoreCorruption(f *testing.F) { svc, err := chequebook.New(nil, common.Address{}, common.Address{}, store, nil, nil) if err == nil { issuedCheque, err := svc.LastCheque(beneficiary) - if err == nil && issuedCheque == nil { - t.Fatal("NIL-04: svc.LastCheque returned (nil, nil)") + if err == nil && (issuedCheque == nil || issuedCheque.CumulativePayout == nil) { + t.Fatal("svc.LastCheque returned a cheque with no cumulative payout") } } })