diff --git a/cmd/flags.go b/cmd/flags.go index e2e82e3d80..fbe6c7f067 100644 --- a/cmd/flags.go +++ b/cmd/flags.go @@ -344,6 +344,12 @@ func initCovenantSignerFlags(cmd *cobra.Command, cfg *config.Config) { false, "Fail startup when enabled covenant routes are missing route-level approval trust roots. Request-time validation still enforces exact reserve/network trust-root matches.", ) + cmd.Flags().BoolVar( + &cfg.CovenantSigner.BridgeCovenantFraudDefenseConfirmed, + "covenantSigner.bridgeCovenantFraudDefenseConfirmed", + false, + "Set only after confirming the tBTC Bridge covenant fraud-defense path is deployed. Until set, the covenant signer fails closed and refuses to sign, because a covenant signature would otherwise expose the wallet to an undefeatable fraud challenge.", + ) } // Initialize flags for Maintainer configuration. @@ -380,8 +386,17 @@ func initMaintainerFlags(command *cobra.Command, cfg *config.Config) { &cfg.Maintainer.Spv.TransactionLimit, "spv.transactionLimit", spv.DefaultTransactionLimit, - "The maximum number of confirmed transactions returned when getting "+ - "transactions for a public key hash.", + "The maximum number of matching (unproven protocol) transactions "+ + "returned when getting transactions for a public key hash.", + ) + + command.Flags().IntVar( + &cfg.Maintainer.Spv.TransactionScanLimit, + "spv.transactionScanLimit", + spv.DefaultTransactionScanLimit, + "The maximum number of not-yet-classified confirmed transactions "+ + "examined, per wallet, each time the maintainer searches for "+ + "matching (unproven protocol) transactions.", ) command.Flags().DurationVar( diff --git a/cmd/flags_test.go b/cmd/flags_test.go index 29ccddd53a..0a248bbe25 100644 --- a/cmd/flags_test.go +++ b/cmd/flags_test.go @@ -226,6 +226,13 @@ var cmdFlagsTests = map[string]struct { expectedValueFromFlag: true, defaultValue: false, }, + "covenantSigner.bridgeCovenantFraudDefenseConfirmed": { + readValueFunc: func(c *config.Config) interface{} { return c.CovenantSigner.BridgeCovenantFraudDefenseConfirmed }, + flagName: "--covenantSigner.bridgeCovenantFraudDefenseConfirmed", + flagValue: "", + expectedValueFromFlag: true, + defaultValue: false, + }, "tbtc.preParamsPoolSize": { readValueFunc: func(c *config.Config) interface{} { return c.Tbtc.PreParamsPoolSize }, flagName: "--tbtc.preParamsPoolSize", diff --git a/cmd/start.go b/cmd/start.go index 71d8d2fbdf..b07860fe3b 100644 --- a/cmd/start.go +++ b/cmd/start.go @@ -100,6 +100,7 @@ type startDeps struct { clientInfoRegistry *clientinfo.Registry, perfMetrics *clientinfo.PerformanceMetrics, minActiveOutpointConfirmations uint, + bridgeCovenantFraudDefenseConfirmed bool, ) (covenantsigner.Engine, error) initializeSigner func( ctx context.Context, @@ -254,6 +255,7 @@ func startWithDeps(cmd *cobra.Command, deps startDeps) error { clientInfoRegistry, perfMetrics, // Pass the existing performance metrics instance to avoid duplicate registrations clientConfig.CovenantSigner.MinActiveOutpointConfirmations, + clientConfig.CovenantSigner.BridgeCovenantFraudDefenseConfirmed, ) if err != nil { return fmt.Errorf("error initializing TBTC: [%v]", err) diff --git a/pkg/chain/ethereum/tbtc.go b/pkg/chain/ethereum/tbtc.go index 4ef78e8600..54e7bd4c3e 100644 --- a/pkg/chain/ethereum/tbtc.go +++ b/pkg/chain/ethereum/tbtc.go @@ -1251,6 +1251,32 @@ func (tc *TbtcChain) PastDepositRevealedEvents( return convertedEvents, err } +// convertPastRedemptionRequestedEvent maps a single on-chain RedemptionRequested +// event to its off-chain tbtc.RedemptionRequestedEvent representation. The event +// carries two distinct fee fields, TreasuryFee and TxMaxFee, that must be mapped +// to their matching destination fields independently; keeping the mapping in a +// standalone helper lets that field-by-field correspondence be unit tested. +func convertPastRedemptionRequestedEvent( + event *tbtcabi.BridgeRedemptionRequested, +) (*tbtc.RedemptionRequestedEvent, error) { + redeemerOutputScript, err := bitcoin.NewScriptFromVarLenData( + event.RedeemerOutputScript, + ) + if err != nil { + return nil, err + } + + return &tbtc.RedemptionRequestedEvent{ + WalletPublicKeyHash: event.WalletPubKeyHash, + RedeemerOutputScript: redeemerOutputScript, + Redeemer: chain.Address(event.Redeemer.Hex()), + RequestedAmount: event.RequestedAmount, + TreasuryFee: event.TreasuryFee, + TxMaxFee: event.TxMaxFee, + BlockNumber: event.Raw.BlockNumber, + }, nil +} + func (tc *TbtcChain) PastRedemptionRequestedEvents( filter *tbtc.RedemptionRequestedEventFilter, ) ([]*tbtc.RedemptionRequestedEvent, error) { @@ -1282,23 +1308,11 @@ func (tc *TbtcChain) PastRedemptionRequestedEvents( convertedEvents := make([]*tbtc.RedemptionRequestedEvent, 0) for _, event := range events { - redeemerOutputScript, err := bitcoin.NewScriptFromVarLenData( - event.RedeemerOutputScript, - ) + convertedEvent, err := convertPastRedemptionRequestedEvent(event) if err != nil { return nil, err } - convertedEvent := &tbtc.RedemptionRequestedEvent{ - WalletPublicKeyHash: event.WalletPubKeyHash, - RedeemerOutputScript: redeemerOutputScript, - Redeemer: chain.Address(event.Redeemer.Hex()), - RequestedAmount: event.RequestedAmount, - TreasuryFee: event.TreasuryFee, - TxMaxFee: event.TreasuryFee, - BlockNumber: event.Raw.BlockNumber, - } - convertedEvents = append(convertedEvents, convertedEvent) } diff --git a/pkg/chain/ethereum/tbtc_test.go b/pkg/chain/ethereum/tbtc_test.go index 7a9312e9c7..8a6bbfa406 100644 --- a/pkg/chain/ethereum/tbtc_test.go +++ b/pkg/chain/ethereum/tbtc_test.go @@ -15,6 +15,7 @@ import ( "github.com/keep-network/keep-core/pkg/chain" "github.com/ethereum/go-ethereum/common" + "github.com/ethereum/go-ethereum/core/types" commonEthereum "github.com/keep-network/keep-common/pkg/chain/ethereum" "github.com/keep-network/keep-core/internal/testutils" @@ -24,6 +25,91 @@ import ( tbtcpkg "github.com/keep-network/keep-core/pkg/tbtc" ) +func TestConvertPastRedemptionRequestedEvent(t *testing.T) { + // RedeemerOutputScript is stored on-chain as variable-length data (a + // CompactSizeUint length prefix followed by the script bytes), so build the + // fixture from a known script to guarantee the helper can parse it back. + redeemerOutputScript := bitcoin.Script([]byte{0x76, 0xa9, 0x14}) + redeemerOutputScriptVarLen, err := redeemerOutputScript.ToVarLenData() + if err != nil { + t.Fatal(err) + } + + walletPublicKeyHash := [20]byte{1, 2, 3} + redeemer := common.HexToAddress("0x1111111111111111111111111111111111111111") + + // The treasury fee and the per-transaction max fee are two independent + // on-chain fields. They are set to distinct values so that a mapping which + // reads the wrong source (e.g. populating TxMaxFee from TreasuryFee) is + // caught rather than masked by equal values. + const ( + requestedAmount = uint64(1_000_000) + treasuryFee = uint64(2_000) + txMaxFee = uint64(7_500) + blockNumber = uint64(123_456) + ) + + event := &tbtcabi.BridgeRedemptionRequested{ + WalletPubKeyHash: walletPublicKeyHash, + RedeemerOutputScript: redeemerOutputScriptVarLen, + Redeemer: redeemer, + RequestedAmount: requestedAmount, + TreasuryFee: treasuryFee, + TxMaxFee: txMaxFee, + Raw: types.Log{BlockNumber: blockNumber}, + } + + converted, err := convertPastRedemptionRequestedEvent(event) + if err != nil { + t.Fatalf("unexpected error: [%v]", err) + } + + testutils.AssertUintsEqual( + t, + "requested amount", + requestedAmount, + converted.RequestedAmount, + ) + + // Both fee fields must be populated from their own distinct source field. + testutils.AssertUintsEqual( + t, + "treasury fee", + treasuryFee, + converted.TreasuryFee, + ) + testutils.AssertUintsEqual( + t, + "tx max fee", + txMaxFee, + converted.TxMaxFee, + ) + + testutils.AssertUintsEqual( + t, + "block number", + blockNumber, + converted.BlockNumber, + ) + + testutils.AssertBytesEqual(t, redeemerOutputScript, converted.RedeemerOutputScript) + + testutils.AssertStringsEqual( + t, + "redeemer", + chain.Address(redeemer.Hex()).String(), + converted.Redeemer.String(), + ) + + if converted.WalletPublicKeyHash != walletPublicKeyHash { + t.Errorf( + "unexpected wallet public key hash\nexpected: [%x]\nactual: [%x]", + walletPublicKeyHash, + converted.WalletPublicKeyHash, + ) + } +} + func TestComputeOperatorsIDsHash(t *testing.T) { operatorIDs := []chain.OperatorID{ 5, 1, 55, 45435534, 33, 345, 23, 235, 3333, 2, diff --git a/pkg/covenantsigner/config.go b/pkg/covenantsigner/config.go index e11f7fec0a..f8d470f94b 100644 --- a/pkg/covenantsigner/config.go +++ b/pkg/covenantsigner/config.go @@ -37,6 +37,14 @@ type Config struct { // covenant signer accepts it. When zero (unset), the system defaults to 6 // to align with the deposit sweep finality threshold. MinActiveOutpointConfirmations uint `mapstructure:"minActiveOutpointConfirmations"` + // BridgeCovenantFraudDefenseConfirmed must be set only when the operator has + // confirmed that the tBTC Bridge recognizes covenant active UTXO spends as + // honest spends in Fraud.defeatFraudChallenge (the covenant fraud-defense + // path is deployed). Until then the covenant signer fails closed and refuses + // to produce signatures, because a covenant signature over a covenant active + // UTXO would otherwise be a valid, undefeatable tBTC fraud proof that could + // slash the signing wallet. + BridgeCovenantFraudDefenseConfirmed bool `mapstructure:"bridgeCovenantFraudDefenseConfirmed"` // DataDir is the base directory path used by the disk persistence handle. // When set, the store acquires an exclusive file lock to prevent concurrent // process corruption. When empty, file locking is skipped. diff --git a/pkg/covenantsigner/covenantsigner_test.go b/pkg/covenantsigner/covenantsigner_test.go index 4fe889567b..7d4576701f 100644 --- a/pkg/covenantsigner/covenantsigner_test.go +++ b/pkg/covenantsigner/covenantsigner_test.go @@ -14,6 +14,7 @@ import ( "encoding/pem" "errors" "fmt" + "math" "math/big" "os" "reflect" @@ -178,6 +179,28 @@ func (se *scriptedEngine) CurrentBlockHeight(context.Context) (uint64, error) { return se.currentBlockHeight, se.currentBlockErr } +// hookedHeightEngine is a minimal Engine used only by the expiry-recheck +// regression tests below. Unlike scriptedEngine's static currentBlockHeight +// field, blockHeight is an injectable function so a test can deterministically +// control exactly what each individual call observes and synchronize with a +// concurrent goroutine. +type hookedHeightEngine struct { + onSubmit func(*Job) (*Transition, error) + blockHeight func(context.Context) (uint64, error) +} + +func (hhe *hookedHeightEngine) OnSubmit(_ context.Context, job *Job) (*Transition, error) { + return hhe.onSubmit(job) +} + +func (hhe *hookedHeightEngine) OnPoll(context.Context, *Job) (*Transition, error) { + return nil, nil +} + +func (hhe *hookedHeightEngine) CurrentBlockHeight(ctx context.Context) (uint64, error) { + return hhe.blockHeight(ctx) +} + func mustJSON(t *testing.T, value any) []byte { t.Helper() data, err := json.Marshal(value) @@ -1073,6 +1096,270 @@ func TestServiceSubmitReturnsExistingJobWhileInitialEngineCallIsInFlight(t *test } } +// TestServiceSubmitDedupRejectsStaleResultWhenCertificateExpiresWhileOriginalSubmitIsInFlight +// proves createOrDedup's dedup hit is freshly rechecked rather than blindly +// echoed back. The first Submit call creates the job and then blocks inside +// OnSubmit; while that call is still in flight (the job is durable but only +// JobStateSubmitted, not yet terminal), the current-block height advances +// past EndBlock and a second, deduplicating Submit call for the same +// routeRequestId must observe that expiry -- proving the dedup return path +// itself now rechecks, not just the fresh-job pipeline the other expiry +// regression tests above exercise. +func TestServiceSubmitDedupRejectsStaleResultWhenCertificateExpiresWhileOriginalSubmitIsInFlight(t *testing.T) { + handle := newMemoryHandle() + engineStarted := make(chan struct{}) + releaseEngine := make(chan struct{}) + callCount := 0 + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + close(engineStarted) + <-releaseEngine + return &Transition{State: JobStatePending, Detail: "queued"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + callCount++ + if callCount <= 3 { + // Valid for the first Submit call's top-of-function validation + // (1), its pre-OnSubmit recheck (2), and the second Submit + // call's own top-of-function validation (3) -- the second call + // must pass ordinary validation and reach createOrDedup on its + // own merits. + return 100, nil + } + // Expired from the second Submit call's dedup fresh-check (4) + // onward, simulating the certificate lapsing while the first call's + // OnSubmit is still in flight. + return 999999999, nil + } + + service, err := NewService( + handle, + engine, + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + input := SignerSubmitInput{ + RouteRequestID: "dedup_expires_in_flight", + Stage: StageSignerCoordination, + Request: request, + } + + firstDone := make(chan struct{}) + var firstErr error + go func() { + defer close(firstDone) + _, firstErr = service.Submit(context.Background(), TemplateSelfV1, input) + }() + + <-engineStarted + + _, secondErr := service.Submit(context.Background(), TemplateSelfV1, input) + if secondErr == nil || !strings.Contains(secondErr.Error(), "signer approval certificate has expired") { + t.Fatalf("expected the deduplicated submit to reject on fresh expiry, got %v", secondErr) + } + + close(releaseEngine) + <-firstDone + if firstErr == nil || !strings.Contains(firstErr.Error(), "signer approval certificate has expired") { + t.Fatalf("expected the original submit to also reject once its own post-signing recheck runs, got %v", firstErr) + } + + job, ok, err := service.store.GetByRouteRequest(TemplateSelfV1, "dedup_expires_in_flight") + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the job to still be present in the store") + } + if job.State != JobStateSubmitted { + t.Fatalf("expected the job to remain in its prior JobStateSubmitted state, got %v", job.State) + } +} + +// TestServiceSubmitDedupRejectsStaleReadyResultAfterCertificateExpires proves +// that a second Submit call for an already-completed (terminal, ready) job +// does not simply echo back the stored ready artifact once the certificate +// has since expired: createOrDedup's fresh recheck must reject it instead of +// returning the stale ready result, and must leave the durable job untouched. +// +// The height hook is deliberately structured so the second Submit call's own +// pre-existing top-of-function validation (on the freshly resubmitted, +// byte-identical certificate) still observes a VALID height and would let the +// request through on its own -- only createOrDedup's fresh recheck of the +// STORED job observes the expired height. This isolates the new dedup-path +// check: without it, this test fails closed for the wrong reason (or not at +// all), since a naive "advance a static height between two sequential Submit +// calls" design would trip the pre-existing top-of-function check instead and +// pass regardless of whether createOrDedup rechecks anything. +func TestServiceSubmitDedupRejectsStaleReadyResultAfterCertificateExpires(t *testing.T) { + handle := newMemoryHandle() + firstJobCreated := false + dedupCallCount := 0 + onSubmitCallCount := 0 + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + onSubmitCallCount++ + return &Transition{State: JobStateArtifactReady, Detail: "ready", PSBTHash: "0xdeadbeef"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + if !firstJobCreated { + // Unconditionally valid while the original job is created and + // completed. This test does not depend on exactly how many + // provider calls that pipeline makes internally. + return 100, nil + } + dedupCallCount++ + if dedupCallCount == 1 { + // The second Submit call's own top-of-function validation -- + // still valid. + return 100, nil + } + // createOrDedup's fresh recheck of the stored job -- expired. + return 999999999, nil + } + + service, err := NewService( + handle, + engine, + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + input := SignerSubmitInput{ + RouteRequestID: "dedup_stale_ready", + Stage: StageSignerCoordination, + Request: request, + } + + first, err := service.Submit(context.Background(), TemplateSelfV1, input) + if err != nil { + t.Fatal(err) + } + if first.Status != StepStatusReady { + t.Fatalf("expected the first submit to complete as ready, got %v", first.Status) + } + firstJobCreated = true + + _, err = service.Submit(context.Background(), TemplateSelfV1, input) + if err == nil || !strings.Contains(err.Error(), "signer approval certificate has expired") { + t.Fatalf("expected the deduplicated submit to reject the stale ready result, got %v", err) + } + if onSubmitCallCount != 1 { + t.Fatalf("expected OnSubmit to be called exactly once, for the original job only, got %d calls", onSubmitCallCount) + } + + job, ok, err := service.store.GetByRouteRequest(TemplateSelfV1, "dedup_stale_ready") + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the job to still be present in the store") + } + if job.State != JobStateArtifactReady { + t.Fatalf("expected the durable job to remain ArtifactReady, got %v", job.State) + } + if job.PSBTHash != "0xdeadbeef" { + t.Fatalf("expected the original ready artifact to remain untouched, got %q", job.PSBTHash) + } +} + +// TestServiceSubmitDedupRejectsStaleReadyResultWhenProviderStartsFailing +// proves createOrDedup's fresh recheck also fails closed -- rather than +// returning the stale ready artifact -- when the current block height +// provider itself starts erroring after the original job completed. As +// above, the second Submit call's own top-of-function validation still +// observes a healthy provider; only createOrDedup's fresh recheck of the +// stored job observes the failure, isolating the new dedup-path check. +func TestServiceSubmitDedupRejectsStaleReadyResultWhenProviderStartsFailing(t *testing.T) { + handle := newMemoryHandle() + wantErr := errors.New("blockchain is unavailable") + firstJobCreated := false + dedupCallCount := 0 + onSubmitCallCount := 0 + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + onSubmitCallCount++ + return &Transition{State: JobStateArtifactReady, Detail: "ready", PSBTHash: "0xdeadbeef"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + if !firstJobCreated { + return 100, nil + } + dedupCallCount++ + if dedupCallCount == 1 { + // The second Submit call's own top-of-function validation -- + // the provider is still healthy at this point. + return 100, nil + } + // createOrDedup's fresh recheck of the stored job -- the provider + // has now started failing. + return 0, wantErr + } + + service, err := NewService( + handle, + engine, + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + input := SignerSubmitInput{ + RouteRequestID: "dedup_provider_fails", + Stage: StageSignerCoordination, + Request: request, + } + + first, err := service.Submit(context.Background(), TemplateSelfV1, input) + if err != nil { + t.Fatal(err) + } + if first.Status != StepStatusReady { + t.Fatalf("expected the first submit to complete as ready, got %v", first.Status) + } + firstJobCreated = true + + _, err = service.Submit(context.Background(), TemplateSelfV1, input) + if err == nil || !strings.Contains(err.Error(), wantErr.Error()) { + t.Fatalf("expected the deduplicated submit to propagate the provider error, got %v", err) + } + if onSubmitCallCount != 1 { + t.Fatalf("expected OnSubmit to be called exactly once, for the original job only, got %d calls", onSubmitCallCount) + } + + job, ok, err := service.store.GetByRouteRequest(TemplateSelfV1, "dedup_provider_fails") + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the job to still be present in the store") + } + if job.State != JobStateArtifactReady { + t.Fatalf("expected the durable job to remain ArtifactReady, got %v", job.State) + } +} + func TestServicePollReturnsNewerPersistedStateWhenItsTransitionBecomesStale(t *testing.T) { handle := newMemoryHandle() submitStarted := make(chan struct{}) @@ -3412,7 +3699,9 @@ func TestMigrationTransactionPlanCommitmentHashMatchesCanonicalVectors(t *testin // TestServicePollPropagatesCurrentBlockProviderError verifies P1-A: when the // currentBlockProvider returns an error, Poll returns a wrapped error rather -// than panicking or silently proceeding. +// than panicking or silently proceeding. The provider only starts failing +// after Submit succeeds, since Submit now also fetches the current height for +// a certificate-bearing request; this isolates the failure to Poll. func TestServicePollPropagatesCurrentBlockProviderError(t *testing.T) { handle := newMemoryHandle() wantErr := errors.New("blockchain is unavailable") @@ -3424,9 +3713,8 @@ func TestServicePollPropagatesCurrentBlockProviderError(t *testing.T) { return &Transition{State: JobStatePending, Detail: "polling"}, nil }, currentBlockHeight: 100, - currentBlockErr: wantErr, } - service, err := NewService(handle, engine, WithCurrentBlockProvider(engine), WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(request RouteSubmitRequest) error { + service, err := NewService(handle, engine, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(request RouteSubmitRequest) error { return nil }))) if err != nil { @@ -3445,6 +3733,8 @@ func TestServicePollPropagatesCurrentBlockProviderError(t *testing.T) { t.Fatal(err) } + engine.currentBlockErr = wantErr + _, err = service.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ RouteRequestID: "poll_err", RequestID: submitResult.RequestID, @@ -3461,7 +3751,8 @@ func TestServicePollPropagatesCurrentBlockProviderError(t *testing.T) { // TestServiceLoadPollJobPropagatesCurrentBlockProviderError verifies P1-A: when // loadPollJob calls the currentBlockProvider and it returns an error, the job -// load fails with a wrapped error. +// load fails with a wrapped error. As above, the provider only starts failing +// after Submit succeeds. func TestServiceLoadPollJobPropagatesCurrentBlockProviderError(t *testing.T) { handle := newMemoryHandle() wantErr := errors.New("blockchain is unavailable") @@ -3473,9 +3764,8 @@ func TestServiceLoadPollJobPropagatesCurrentBlockProviderError(t *testing.T) { return &Transition{State: JobStatePending, Detail: "polling"}, nil }, currentBlockHeight: 100, - currentBlockErr: wantErr, } - service, err := NewService(handle, engine, WithCurrentBlockProvider(engine), WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(request RouteSubmitRequest) error { + service, err := NewService(handle, engine, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(request RouteSubmitRequest) error { return nil }))) if err != nil { @@ -3492,6 +3782,8 @@ func TestServiceLoadPollJobPropagatesCurrentBlockProviderError(t *testing.T) { t.Fatal(err) } + engine.currentBlockErr = wantErr + // loadPollJob is called by Poll; the error should propagate through Poll. _, err = service.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ RouteRequestID: "load_err", @@ -3507,9 +3799,11 @@ func TestServiceLoadPollJobPropagatesCurrentBlockProviderError(t *testing.T) { } } -// TestServicePollRejectsExpiredCertificate verifies P2-D: when the current block -// has reached or passed EndBlock, the certificate is considered expired and Poll -// returns an error. +// TestServicePollRejectsExpiredCertificate verifies P2-D: when the current +// block is past EndBlock, the certificate is considered expired and Poll +// returns an error. The height stays below EndBlock through Submit (which now +// also enforces expiry) and only advances past it before Poll, so the +// certificate genuinely expires between submission and polling. func TestServicePollRejectsExpiredCertificate(t *testing.T) { handle := newMemoryHandle() engine := &scriptedEngine{ @@ -3519,10 +3813,9 @@ func TestServicePollRejectsExpiredCertificate(t *testing.T) { poll: func(*Job) (*Transition, error) { return &Transition{State: JobStatePending, Detail: "polling"}, nil }, - currentBlockHeight: 200, // EndBlock is 123456; set >= EndBlock to trigger expiration + currentBlockHeight: 100, // EndBlock is 123456; well below it at submit time } - engine.currentBlockHeight = 123456 // satisfy expiration: currentBlock >= EndBlock - service, err := NewService(handle, engine, WithCurrentBlockProvider(engine), WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(request RouteSubmitRequest) error { + service, err := NewService(handle, engine, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(request RouteSubmitRequest) error { return nil }))) if err != nil { @@ -3530,6 +3823,7 @@ func TestServicePollRejectsExpiredCertificate(t *testing.T) { } request := structuredSignerApprovalRequest(TemplateSelfV1) + endBlock := *request.SignerApproval.EndBlock submitResult, err := service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ RouteRequestID: "expired", Stage: StageSignerCoordination, @@ -3539,6 +3833,9 @@ func TestServicePollRejectsExpiredCertificate(t *testing.T) { t.Fatal(err) } + // Advance the current block height one past EndBlock before polling. + engine.currentBlockHeight = endBlock + 1 + _, err = service.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ RouteRequestID: "expired", RequestID: submitResult.RequestID, @@ -3553,6 +3850,52 @@ func TestServicePollRejectsExpiredCertificate(t *testing.T) { } } +// TestServicePollAcceptsCertificateExactlyAtEndBlock verifies EndBlock is an +// inclusive upper bound: a certificate remains valid, not expired, when the +// current block equals EndBlock exactly. +func TestServicePollAcceptsCertificateExactlyAtEndBlock(t *testing.T) { + handle := newMemoryHandle() + engine := &scriptedEngine{ + submit: func(*Job) (*Transition, error) { + return &Transition{State: JobStatePending, Detail: "queued"}, nil + }, + poll: func(*Job) (*Transition, error) { + return &Transition{State: JobStatePending, Detail: "polling"}, nil + }, + currentBlockHeight: 100, + } + service, err := NewService(handle, engine, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(request RouteSubmitRequest) error { + return nil + }))) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + endBlock := *request.SignerApproval.EndBlock + submitResult, err := service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "at_end_block", + Stage: StageSignerCoordination, + Request: request, + }) + if err != nil { + t.Fatal(err) + } + + // The current block is now exactly EndBlock -- still valid (inclusive). + engine.currentBlockHeight = endBlock + + _, err = service.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ + RouteRequestID: "at_end_block", + RequestID: submitResult.RequestID, + Stage: StageSignerCoordination, + Request: request, + }) + if err != nil { + t.Fatalf("expected no error at exactly EndBlock, got %v", err) + } +} + // TestServicePollAcceptsValidCertificate verifies P2-D: when the current block // is before EndBlock, the certificate is valid and Poll proceeds normally. func TestServicePollAcceptsValidCertificate(t *testing.T) { @@ -3593,18 +3936,50 @@ func TestServicePollAcceptsValidCertificate(t *testing.T) { } } -// TestServicePollSkipsBlockProviderWhenNoExpiration verifies P2-D: when the -// SignerApproval has a nil EndBlock (no expiration), Poll proceeds without -// error even though currentBlockProvider is called unconditionally at line 461. -// The actual expiration gate (loadPollJob line 283) correctly skips the check. -func TestServicePollSkipsBlockProviderWhenNoExpiration(t *testing.T) { +// TestServiceSubmitRejectsSignerApprovalWithMissingEndBlock verifies that a +// signer approval certificate without an EndBlock is rejected at Submit. +// EndBlock is a mandatory v2 field: there is no fail-open "never expires" +// certificate state. +func TestServiceSubmitRejectsSignerApprovalWithMissingEndBlock(t *testing.T) { + handle := newMemoryHandle() + service, err := NewService(handle, &scriptedEngine{ + currentBlockHeight: 100, + }, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + }))) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + request.SignerApproval.EndBlock = nil + + _, err = service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "missing_end_block", + Stage: StageSignerCoordination, + Request: request, + }) + if err == nil || !strings.Contains(err.Error(), "endBlock is required") { + t.Fatalf("expected missing endBlock error, got %v", err) + } +} + +// TestServiceAcceptsSignerApprovalCertificateWithEndBlockAboveUint32Range +// proves EndBlock is not artificially capped to uint32. The certificate v2 +// signing digest binds EndBlock as a full 8-byte big-endian uint64 (see +// signerApprovalCertificateSigningDigest in pkg/tbtc), so request +// normalization -- and the full Submit/Poll round trip -- must accept and +// preserve a value above math.MaxUint32 rather than rejecting it. +func TestServiceAcceptsSignerApprovalCertificateWithEndBlockAboveUint32Range(t *testing.T) { handle := newMemoryHandle() + endBlock := uint64(math.MaxUint32) + 100000 + if endBlock <= math.MaxUint32 { + t.Fatalf("test vector must exceed math.MaxUint32, got %d", endBlock) + } + service, err := NewService(handle, &scriptedEngine{ submit: func(*Job) (*Transition, error) { - return &Transition{State: JobStatePending, Detail: "queued"}, nil - }, - poll: func(*Job) (*Transition, error) { - return &Transition{State: JobStatePending, Detail: "polling"}, nil + return &Transition{State: JobStateArtifactReady, Detail: "ready", PSBTHash: "0xdeadbeef"}, nil }, currentBlockHeight: 100, }, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { @@ -3615,24 +3990,758 @@ func TestServicePollSkipsBlockProviderWhenNoExpiration(t *testing.T) { } request := structuredSignerApprovalRequest(TemplateSelfV1) - request.SignerApproval.EndBlock = nil + request.SignerApproval.EndBlock = &endBlock + + normalized, err := normalizeSignerApprovalCertificate(request) + if err != nil { + t.Fatalf("expected EndBlock above math.MaxUint32 to normalize, got %v", err) + } + if normalized.EndBlock == nil || *normalized.EndBlock != endBlock { + t.Fatalf("expected normalized EndBlock to equal %d, got %v", endBlock, normalized.EndBlock) + } submitResult, err := service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ - RouteRequestID: "no_exp", + RouteRequestID: "uint64_end_block", Stage: StageSignerCoordination, Request: request, }) if err != nil { - t.Fatal(err) + t.Fatalf("expected submit to accept EndBlock above math.MaxUint32, got %v", err) + } + if submitResult.Status != StepStatusReady { + t.Fatalf("expected ready status, got %v", submitResult.Status) } - _, err = service.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ - RouteRequestID: "no_exp", + pollResult, err := service.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ + RouteRequestID: "uint64_end_block", RequestID: submitResult.RequestID, Stage: StageSignerCoordination, Request: request, }) if err != nil { - t.Fatalf("expected no error for nil EndBlock, got %v", err) + t.Fatalf("expected poll to accept EndBlock above math.MaxUint32, got %v", err) + } + if pollResult.Status != StepStatusReady { + t.Fatalf("expected ready status on poll, got %v", pollResult.Status) + } +} + +// TestServiceSubmitRejectsSignerApprovalMissingCurrentBlockProvider verifies +// that a certificate-bearing Submit fails closed -- rather than treating the +// certificate as unexpired -- when no current block height provider is +// configured at all (the engine implements neither SignerApprovalVerifier nor +// CurrentBlockHeightProvider, so nothing auto-detects a provider). +func TestServiceSubmitRejectsSignerApprovalMissingCurrentBlockProvider(t *testing.T) { + handle := newMemoryHandle() + service, err := NewService(handle, &passiveEngineWithoutBlockHeight{}, WithSignerApprovalVerifier( + SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { return nil }), + )) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + + _, err = service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "missing_provider", + Stage: StageSignerCoordination, + Request: request, + }) + if err == nil || !strings.Contains(err.Error(), "no current block height is available") { + t.Fatalf("expected missing provider error, got %v", err) + } +} + +// passiveEngineWithoutBlockHeight implements Engine and SignerApprovalVerifier +// but deliberately not CurrentBlockHeightProvider, to exercise the "verifier +// present, provider absent" fail-closed path at the Service level (Initialize +// rejects this combination at startup; Service itself must also fail closed +// per-request for callers that construct a Service directly). +type passiveEngineWithoutBlockHeight struct{} + +func (passiveEngineWithoutBlockHeight) OnSubmit(context.Context, *Job) (*Transition, error) { + return nil, nil +} + +func (passiveEngineWithoutBlockHeight) OnPoll(context.Context, *Job) (*Transition, error) { + return nil, nil +} + +func (passiveEngineWithoutBlockHeight) VerifySignerApproval(RouteSubmitRequest) error { + return nil +} + +// TestServiceSubmitRejectsExpiryDiscoveredBeforeOnSubmit verifies the +// pre-OnSubmit recheck: if the certificate has expired by the time the +// in-flight slot is acquired (e.g. it expired while waiting on the signer +// approval verifier or for a slot), Submit must reject before ever invoking +// OnSubmit -- synchronous threshold signing must never start on an +// authorization that has already lapsed. +func TestServiceSubmitRejectsExpiryDiscoveredBeforeOnSubmit(t *testing.T) { + handle := newMemoryHandle() + callCount := 0 + onSubmitCalled := false + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + onSubmitCalled = true + return &Transition{State: JobStateArtifactReady, Detail: "ready", PSBTHash: "0xdeadbeef"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + callCount++ + if callCount == 1 { + return 100, nil // valid at the initial top-of-Submit validation + } + return 999999999, nil // expired by the pre-OnSubmit recheck onward + } + + service, err := NewService( + handle, + engine, + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + + _, err = service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "expire_before_submit", + Stage: StageSignerCoordination, + Request: request, + }) + if err == nil || !strings.Contains(err.Error(), "signer approval certificate has expired") { + t.Fatalf("expected an expiry rejection, got %v", err) + } + if onSubmitCalled { + t.Fatal("expected OnSubmit to never be called once expiry was discovered pre-signing") + } +} + +// TestServiceSubmitRejectsExpiryDiscoveredAfterRealInFlightSemaphoreWait +// proves the pre-OnSubmit recheck catches expiry that occurs while Submit is +// genuinely blocked waiting for a WithMaxInFlight slot -- an actual blocking +// send on the production s.inFlightSlots channel, not a mocked or simulated +// wait. The test occupies the sole slot itself (rather than via a second, +// concurrent Submit call), which makes the blocking point unambiguous and +// race-free by construction: Submit cannot possibly acquire the slot, and +// therefore cannot reach the pre-OnSubmit recheck or OnSubmit itself, until +// the test explicitly releases it. +func TestServiceSubmitRejectsExpiryDiscoveredAfterRealInFlightSemaphoreWait(t *testing.T) { + handle := newMemoryHandle() + callCount := 0 + onSubmitCalled := false + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + onSubmitCalled = true + return &Transition{State: JobStateArtifactReady, Detail: "ready", PSBTHash: "0xdeadbeef"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + callCount++ + if callCount == 1 { + return 100, nil // valid at the initial top-of-Submit validation + } + // Expired by the pre-OnSubmit recheck, which is only reachable after + // the real semaphore wait below is released. + return 999999999, nil + } + + service, err := NewService( + handle, + engine, + WithMaxInFlight(1), + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + // Occupy the sole in-flight slot before Submit ever runs, using the real + // production channel directly (this test is in-package). Submit's own + // semaphore acquire is then guaranteed to block for real, rather than + // possibly racing ahead of a second goroutine that would otherwise need + // to be observed blocking. + service.inFlightSlots <- struct{}{} + + request := structuredSignerApprovalRequest(TemplateSelfV1) + + submitDone := make(chan struct{}) + var submitErr error + go func() { + defer close(submitDone) + _, submitErr = service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "real_semaphore_wait", + Stage: StageSignerCoordination, + Request: request, + }) + }() + + select { + case <-submitDone: + t.Fatal("expected Submit to block on the in-flight slot semaphore before completing") + case <-time.After(100 * time.Millisecond): + } + + // Release the slot: Submit's semaphore acquire can now proceed, and the + // pre-OnSubmit recheck that immediately follows observes the expired + // height. + <-service.inFlightSlots + + select { + case <-submitDone: + case <-time.After(2 * time.Second): + t.Fatal("expected Submit to finish after the in-flight slot was released") + } + + if submitErr == nil || !strings.Contains(submitErr.Error(), "signer approval certificate has expired") { + t.Fatalf("expected an expiry rejection after the real semaphore wait, got %v", submitErr) + } + if onSubmitCalled { + t.Fatal("expected OnSubmit to never be called once expiry was discovered after the semaphore wait") + } +} + +// TestServiceSubmitRejectsExpiryDiscoveredAfterOnSubmit verifies the +// post-OnSubmit, pre-lock fast recheck: if synchronous threshold signing +// itself takes long enough for the certificate to expire, Submit must reject +// the now-stale result rather than persisting or returning it. +func TestServiceSubmitRejectsExpiryDiscoveredAfterOnSubmit(t *testing.T) { + handle := newMemoryHandle() + callCount := 0 + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + return &Transition{State: JobStateArtifactReady, Detail: "ready", PSBTHash: "0xdeadbeef"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + callCount++ + if callCount <= 2 { + // Valid for both the top-of-Submit validation and the + // pre-OnSubmit recheck. + return 100, nil + } + // Expired for the post-OnSubmit fast recheck onward, simulating + // signing itself having taken long enough to cross EndBlock. + return 999999999, nil + } + + service, err := NewService( + handle, + engine, + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + + _, err = service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "expire_after_submit", + Stage: StageSignerCoordination, + Request: request, + }) + if err == nil || !strings.Contains(err.Error(), "signer approval certificate has expired") { + t.Fatalf("expected an expiry rejection, got %v", err) + } + + job, ok, err := service.store.GetByRouteRequest(TemplateSelfV1, "expire_after_submit") + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the job to still be present in the store") + } + if job.State != JobStateSubmitted { + t.Fatalf( + "expected the job to remain in its prior JobStateSubmitted state, got %v", + job.State, + ) + } + if job.PSBTHash != "" { + t.Fatal("expected the ready artifact to not be persisted after post-signing expiry") + } +} + +// TestServiceSubmitRejectsExpiryDiscoveredWhileWaitingForMutex is the +// deterministic concurrency regression test for the authoritative, +// mutex-held expiry recheck. The pre-lock fast check observes a still-valid +// certificate; while that check is resolving, another goroutine acquires +// s.mutex, advances the current height past EndBlock, and releases it before +// Submit's own s.mutex.Lock() call. Submit must reject once it acquires the +// mutex, and the ready artifact must never be persisted -- this is the +// TOCTOU window between the fast check and the store write that the +// authoritative check closes. +func TestServiceSubmitRejectsExpiryDiscoveredWhileWaitingForMutex(t *testing.T) { + handle := newMemoryHandle() + + height := uint64(100) + callCount := 0 + resumeRequests := make(chan chan struct{}) + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + return &Transition{State: JobStateArtifactReady, Detail: "ready", PSBTHash: "0xdeadbeef"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + callCount++ + if callCount == 3 { + // This is the post-OnSubmit, pre-lock fast check (step 6 of + // Submit's sequence). Block until another goroutine has acquired + // s.mutex, advanced the height past EndBlock, and released it -- + // simulating a concurrent Submit/Poll racing in right as this + // call is about to return a still-valid answer. + resume := make(chan struct{}) + resumeRequests <- resume + <-resume + return 100, nil // still valid for THIS (pre-lock) check + } + return height, nil + } + + service, err := NewService( + handle, + engine, + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + endBlock := *request.SignerApproval.EndBlock + + submitDone := make(chan struct{}) + var submitErr error + go func() { + defer close(submitDone) + _, submitErr = service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "mutex_race", + Stage: StageSignerCoordination, + Request: request, + }) + }() + + resume := <-resumeRequests + service.mutex.Lock() + height = endBlock + 1 + service.mutex.Unlock() + close(resume) + + <-submitDone + if submitErr == nil || !strings.Contains(submitErr.Error(), "signer approval certificate has expired") { + t.Fatalf("expected an expiry rejection after acquiring the mutex, got %v", submitErr) + } + + job, ok, err := service.store.GetByRouteRequest(TemplateSelfV1, "mutex_race") + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the job to still be present in the store") + } + if job.State != JobStateSubmitted { + t.Fatalf( + "expected the job to remain in its prior JobStateSubmitted state, got %v", + job.State, + ) + } + if job.PSBTHash != "" { + t.Fatal("expected the ready artifact to not be persisted after expiry was discovered under the mutex") + } +} + +// TestServiceSubmitRejectsStaleTerminalResultWhenCertificateExpiresWhileOnSubmitIsInFlight +// proves that Submit's mutex-held early-return path -- the one that hands +// back a concurrently-advanced or terminal currentJob instead of applying +// this call's own transition -- rechecks certificate freshness before +// returning that job, not after (or never, if the early return fires first). +// +// OnSubmit blocks. While it is in flight, another operation (simulating a +// concurrent Poll racing this Submit call) advances the same durable job +// straight to a terminal ArtifactReady state via the store. Submit's +// post-OnSubmit, pre-lock recheck runs against the ORIGINAL pre-OnSubmit job +// snapshot held in a local variable, not the store, so it does not observe +// this mutation and still passes on a valid height -- the race is only +// observable by whatever reloads the job from the store under the mutex. +// +// The height provider is call-counted: calls 1-3 cover Submit's own +// top-of-function validation, pre-OnSubmit recheck, and post-OnSubmit +// pre-lock recheck, and must all observe a valid height for the race to be +// genuine. A 4th call is the mutex-held recheck of the freshly reloaded +// currentJob; it must happen at all (proving the early return did not skip +// it) and it must observe the expired height (proving the check runs before, +// not after, the early return). Without the fix, the early return fires as +// soon as currentJob.State is seen to be terminal, the 4th call never +// happens, and the stale ArtifactReady result leaks out with an expired +// certificate. +func TestServiceSubmitRejectsStaleTerminalResultWhenCertificateExpiresWhileOnSubmitIsInFlight(t *testing.T) { + handle := newMemoryHandle() + onSubmitStarted := make(chan struct{}) + releaseOnSubmit := make(chan struct{}) + callCount := 0 + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + close(onSubmitStarted) + <-releaseOnSubmit + return &Transition{State: JobStatePending, Detail: "queued"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + callCount++ + if callCount <= 3 { + // Valid through Submit's top-of-function validation (1), + // pre-OnSubmit recheck (2), and post-OnSubmit pre-lock recheck + // (3) -- none of these reload the concurrently-mutated store, so + // they must all pass on their own merits for the race below to + // be the only thing standing between the stale result and the + // caller. + return 100, nil + } + // The 4th call, if it happens, is the mutex-held recheck of the + // freshly reloaded currentJob -- it must observe the certificate as + // expired. + return 999999999, nil + } + + service, err := NewService( + handle, + engine, + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + input := SignerSubmitInput{ + RouteRequestID: "submit_terminal_race", + Stage: StageSignerCoordination, + Request: request, + } + + submitDone := make(chan struct{}) + var submitResult StepResult + var submitErr error + go func() { + defer close(submitDone) + submitResult, submitErr = service.Submit(context.Background(), TemplateSelfV1, input) + }() + + <-onSubmitStarted + + // Simulate a concurrent Poll advancing the same durable job straight to a + // terminal ArtifactReady state while this Submit call's OnSubmit is still + // in flight -- exactly the race the "another poll already advanced the + // stored job" early return exists to handle. + storedJob, ok, err := service.store.GetByRouteRequest(TemplateSelfV1, input.RouteRequestID) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the submitted job to be durable while OnSubmit is in flight") + } + storedJob.State = JobStateArtifactReady + storedJob.Detail = "ready" + storedJob.PSBTHash = "0xleaked" + storedJob.CompletedAt = "2024-01-01T00:00:00Z" + storedJob.UpdatedAt = storedJob.CompletedAt + if err := service.store.Put(storedJob); err != nil { + t.Fatal(err) + } + + close(releaseOnSubmit) + <-submitDone + + if submitErr == nil || !strings.Contains(submitErr.Error(), "signer approval certificate has expired") { + t.Fatalf( + "expected Submit to reject the stale terminal result on fresh expiry, got result=%#v err=%v", + submitResult, submitErr, + ) + } + if submitResult.PSBTHash == "0xleaked" { + t.Fatal("expected the concurrently-persisted terminal result to not be returned") + } + if callCount != 4 { + t.Fatalf( + "expected the mutex-held recheck to run as a 4th provider call before the early return, got %d calls", + callCount, + ) + } + + persisted, ok, err := service.store.GetByRequestID(storedJob.RequestID) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the job to still be present in the store") + } + if persisted.State != JobStateArtifactReady || persisted.PSBTHash != "0xleaked" { + t.Fatalf( + "expected the concurrently-persisted terminal job to remain untouched, got %#v", + persisted, + ) + } +} + +// TestServiceSubmitRejectsAlreadyExpiredCertificate verifies that Submit +// fails closed on the very first, top-of-function expiry check when the +// certificate is already expired before any validation or signing work +// begins. +func TestServiceSubmitRejectsAlreadyExpiredCertificate(t *testing.T) { + handle := newMemoryHandle() + service, err := NewService(handle, &scriptedEngine{ + currentBlockHeight: 999999999, + }, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + }))) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + + _, err = service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "already_expired", + Stage: StageSignerCoordination, + Request: request, + }) + if err == nil || !strings.Contains(err.Error(), "signer approval certificate has expired") { + t.Fatalf("expected an expiry rejection, got %v", err) + } +} + +// TestServiceSubmitPropagatesCurrentBlockProviderError verifies that Submit +// (not just Poll) fails closed with a wrapped error when the current block +// height provider itself errors for a certificate-bearing request. +func TestServiceSubmitPropagatesCurrentBlockProviderError(t *testing.T) { + handle := newMemoryHandle() + wantErr := errors.New("blockchain is unavailable") + service, err := NewService(handle, &scriptedEngine{ + currentBlockErr: wantErr, + }, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + }))) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + + _, err = service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "submit_provider_err", + Stage: StageSignerCoordination, + Request: request, + }) + if err == nil || !strings.Contains(err.Error(), wantErr.Error()) { + t.Fatalf("expected error containing %q, got %v", wantErr.Error(), err) + } +} + +// TestServicePollRejectsMissingCurrentBlockProvider verifies Poll fails +// closed -- rather than treating the certificate as unexpired -- when no +// current block height provider is configured, even though the job was +// submitted successfully through a different, provider-backed service +// instance sharing the same underlying storage. This models a deployment +// misconfiguration where the poll path loses access to the provider that the +// submit path had. +func TestServicePollRejectsMissingCurrentBlockProvider(t *testing.T) { + handle := newMemoryHandle() + + submitService, err := NewService(handle, &scriptedEngine{ + submit: func(*Job) (*Transition, error) { return &Transition{State: JobStatePending, Detail: "queued"}, nil }, + currentBlockHeight: 100, + }, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + }))) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + submitResult, err := submitService.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "poll_missing_provider", + Stage: StageSignerCoordination, + Request: request, + }) + if err != nil { + t.Fatal(err) + } + + pollService, err := NewService(handle, &passiveEngineWithoutBlockHeight{}, WithSignerApprovalVerifier( + SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { return nil }), + )) + if err != nil { + t.Fatal(err) + } + + _, err = pollService.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ + RouteRequestID: "poll_missing_provider", + RequestID: submitResult.RequestID, + Stage: StageSignerCoordination, + Request: request, + }) + // The exact rejection point (top-of-Poll validation of the resubmitted + // certificate, or the stored-job recheck) is an implementation detail; + // both independently fail closed on a missing provider. What matters is + // that Poll rejects rather than treating the certificate as unexpired. + if err == nil || !strings.Contains(err.Error(), "no current block height") { + t.Fatalf("expected a missing-provider rejection, got %v", err) + } +} + +// TestServicePollRejectsLegacyStoredJobWithMissingEndBlock models a job +// persisted before v2 EndBlock enforcement rolled out: its SignerApproval has +// no EndBlock at all, because that was legal under v1. Such a job cannot have +// passed through the current (v2-enforcing) Submit path, so it is +// constructed directly against the store, the way an on-disk legacy file +// would load. Polling it must fail closed rather than treat the missing +// EndBlock as "never expires". +// +// The poll is resubmitted with a fresh, structurally valid v2 request (not +// the stored legacy one) so that top-of-Poll input validation passes and the +// rejection is specifically attributable to loadPollJob's stored-job recheck, +// not to the resubmitted certificate itself being malformed. A real client +// replaying its only (legacy) certificate would be rejected even earlier, at +// input validation -- also intentionally failing closed, just at a different +// layer. +func TestServicePollRejectsLegacyStoredJobWithMissingEndBlock(t *testing.T) { + handle := newMemoryHandle() + service, err := NewService(handle, &scriptedEngine{ + poll: func(*Job) (*Transition, error) { return nil, nil }, + currentBlockHeight: 100, + }, WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + }))) + if err != nil { + t.Fatal(err) + } + + legacyRequest := structuredSignerApprovalRequest(TemplateSelfV1) + legacyRequest.SignerApproval.CertificateVersion = 1 + legacyRequest.SignerApproval.EndBlock = nil + + // The stored request digest is irrelevant to this test: loadPollJob's + // ensureStoredCertificateTimely check runs, and must reject, before the + // digest is ever compared. It is also no longer computable through the + // normal requestDigest path, since normalization now rejects a nil + // EndBlock unconditionally -- exactly like a real legacy v1 job file + // loaded from disk, whose digest predates that requirement. + legacyJob := &Job{ + RequestID: "kcs_self_legacy", + RouteRequestID: "legacy_no_end_block", + Route: TemplateSelfV1, + RequestDigest: "0xdeadbeef", + State: JobStatePending, + Detail: "queued", + CreatedAt: "2026-01-01T00:00:00Z", + UpdatedAt: "2026-01-01T00:00:00Z", + Request: legacyRequest, + } + if err := service.store.Put(legacyJob); err != nil { + t.Fatal(err) + } + + freshPollBody := structuredSignerApprovalRequest(TemplateSelfV1) + + _, err = service.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ + RouteRequestID: "legacy_no_end_block", + RequestID: "kcs_self_legacy", + Stage: StageSignerCoordination, + Request: freshPollBody, + }) + if err == nil || !strings.Contains(err.Error(), "signer approval certificate has expired") { + t.Fatalf("expected a legacy job with a missing end block to fail closed, got %v", err) + } +} + +// TestServicePollRejectsExpiryDiscoveredDuringOnPoll verifies Poll's +// equivalent of Submit's post-signing recheck: loadPollJob is called both +// before and after OnPoll, so a certificate that was still valid when Poll +// started but expires while OnPoll is running (e.g. OnPoll itself triggers +// slow work) is still caught by the second, mutex-held loadPollJob call +// before any transition is persisted. +func TestServicePollRejectsExpiryDiscoveredDuringOnPoll(t *testing.T) { + handle := newMemoryHandle() + callCount := 0 + + engine := &hookedHeightEngine{ + onSubmit: func(*Job) (*Transition, error) { + // Left non-terminal so the job is still pollable afterward. + return &Transition{State: JobStatePending, Detail: "queued"}, nil + }, + } + engine.blockHeight = func(context.Context) (uint64, error) { + callCount++ + if callCount <= 6 { + // Valid through Submit's four checks, Poll's top-of-function + // validation, and the first (pre-OnPoll) loadPollJob call. + return 100, nil + } + // Expired for the second (post-OnPoll, mutex-held) loadPollJob call + // onward, simulating OnPoll itself having taken long enough to cross + // EndBlock. + return 999999999, nil + } + + service, err := NewService( + handle, + engine, + WithSignerApprovalVerifier(SignerApprovalVerifierFunc(func(RouteSubmitRequest) error { + return nil + })), + ) + if err != nil { + t.Fatal(err) + } + + request := structuredSignerApprovalRequest(TemplateSelfV1) + submitResult, err := service.Submit(context.Background(), TemplateSelfV1, SignerSubmitInput{ + RouteRequestID: "expire_during_poll", + Stage: StageSignerCoordination, + Request: request, + }) + if err != nil { + t.Fatal(err) + } + + _, err = service.Poll(context.Background(), TemplateSelfV1, SignerPollInput{ + RouteRequestID: "expire_during_poll", + RequestID: submitResult.RequestID, + Stage: StageSignerCoordination, + Request: request, + }) + if err == nil || !strings.Contains(err.Error(), "signer approval certificate has expired") { + t.Fatalf("expected an expiry rejection, got %v", err) + } +} + +// TestNormalizeSignerApprovalCertificateRejectsV1CertificateVersion verifies +// that a v1 signerApproval certificate is rejected outright rather than +// accepted through a compatibility path. +func TestNormalizeSignerApprovalCertificateRejectsV1CertificateVersion(t *testing.T) { + request := structuredSignerApprovalRequest(TemplateSelfV1) + request.SignerApproval.CertificateVersion = 1 + + _, err := normalizeSignerApprovalCertificate(request) + if err == nil || !strings.Contains(err.Error(), "request.signerApproval.certificateVersion must equal 2") { + t.Fatalf("expected v1 certificate version rejection, got %v", err) } } diff --git a/pkg/covenantsigner/engine.go b/pkg/covenantsigner/engine.go index b5f7123a21..898cbaab3d 100644 --- a/pkg/covenantsigner/engine.go +++ b/pkg/covenantsigner/engine.go @@ -33,18 +33,27 @@ type Transition struct { // underlying signing job. The service treats this as a terminal failure and // transitions the job to JobStateFailed. // -// CurrentBlockHeight is optional. When implemented, it is called during Submit -// and Poll to determine whether a signer approval certificate has expired. +// An Engine may also implement SignerApprovalVerifier and +// CurrentBlockHeightProvider; see their doc comments for the constraints +// between them. type Engine interface { OnSubmit(ctx context.Context, job *Job) (*Transition, error) OnPoll(ctx context.Context, job *Job) (*Transition, error) } -// CurrentBlockHeightProvider is an optional interface implemented by Engines -// that can provide the current Bitcoin block height. When implemented, the -// service uses it to check signer approval certificate expiration during -// Submit and Poll. Engines that do not implement this interface are assumed -// to never have signer approvals expire. +// CurrentBlockHeightProvider is implemented by Engines that can provide the +// current block height of the host chain (e.g. Ethereum) that signer approval +// certificate EndBlock values are denominated in -- never the Bitcoin chain. +// The service uses it to check signer approval certificate expiration during +// Submit and Poll. +// +// Any engine that implements SignerApprovalVerifier must also implement this +// interface: Initialize rejects engines that verify signer approvals but +// cannot report a current block height, because such an engine could never +// determine whether a certificate has expired. There is no fail-open "never +// expires" behavior for engines that omit this interface -- Submit and Poll +// reject certificate-bearing requests outright when no provider is +// configured. type CurrentBlockHeightProvider interface { CurrentBlockHeight(ctx context.Context) (uint64, error) } diff --git a/pkg/covenantsigner/server.go b/pkg/covenantsigner/server.go index 6cee0883b5..9ac35c19c4 100644 --- a/pkg/covenantsigner/server.go +++ b/pkg/covenantsigner/server.go @@ -34,7 +34,7 @@ func Initialize( config Config, handle persistence.BasicHandle, engine Engine, -) (*Server, bool, error) { +) (_ *Server, _ bool, err error) { if config.Port == 0 { return nil, false, nil } @@ -47,7 +47,9 @@ func Initialize( listenAddress = DefaultListenAddress } - if !isLoopbackListenAddress(listenAddress) && strings.TrimSpace(config.AuthToken) == "" { + isLoopback := isLoopbackListenAddress(listenAddress) + + if !isLoopback && strings.TrimSpace(config.AuthToken) == "" { return nil, false, fmt.Errorf( "covenant signer authToken is required for non-loopback listenAddress [%s]", listenAddress, @@ -66,7 +68,25 @@ func Initialize( if err != nil { return nil, false, err } - if err := validateRequiredApprovalTrustRoots(config, service); err != nil { + // From this point on, service holds the store's exclusive file lock (when + // DataDir is configured). Every subsequent startup failure below must + // release it, or a later retry over the same data directory would fail + // with a spurious lock-contention error instead of the actual startup + // problem. + defer func() { + if err != nil { + if closeErr := service.Close(); closeErr != nil { + logger.Warnf("failed to close covenant signer service after startup failure: [%v]", closeErr) + } + } + }() + + // A non-loopback (production) listen address is treated as requiring the + // full multi-party approval model: the signer approval verifier and the + // route trust roots must be configured, mirroring the non-loopback authToken + // requirement above. Loopback deployments may still run with warnings unless + // requireApprovalTrustRoots is set explicitly. + if err := validateRequiredApprovalTrustRoots(config, service, !isLoopback); err != nil { return nil, false, err } if service.signerApprovalVerifier == nil { @@ -87,6 +107,17 @@ func Initialize( "requests without signerApproval will be accepted; " + "set covenantSigner.requireApprovalTrustRoots=true to enforce approval verification", ) + } else if service.currentBlockProvider == nil { + // An engine that verifies signer approvals but cannot report a + // current block height could never determine whether a certificate + // has expired: every certificate-bearing request would be rejected + // at runtime. Fail startup instead of that deployment surprise. + return nil, false, fmt.Errorf( + "covenant signer engine implements SignerApprovalVerifier but not " + + "CurrentBlockHeightProvider; signer approval certificate expiry " + + "could never be determined -- implement CurrentBlockHeight on " + + "the engine or use an engine that does not verify approvals", + ) } if config.EnableSelfV1 && !hasDepositorTrustRootForRoute( @@ -181,8 +212,9 @@ func Initialize( func validateRequiredApprovalTrustRoots( config Config, service *Service, + requireForNonLoopbackListenAddress bool, ) error { - if !config.RequireApprovalTrustRoots { + if !config.RequireApprovalTrustRoots && !requireForNonLoopbackListenAddress { return nil } @@ -192,7 +224,7 @@ func validateRequiredApprovalTrustRoots( TemplateSelfV1, ) { return fmt.Errorf( - "covenant signer self_v1 routes require depositorTrustRoots when covenantSigner.requireApprovalTrustRoots=true", + "covenant signer self_v1 routes require depositorTrustRoots when covenantSigner.requireApprovalTrustRoots=true or for a non-loopback listen address", ) } @@ -201,7 +233,7 @@ func validateRequiredApprovalTrustRoots( TemplateQcV1, ) { return fmt.Errorf( - "covenant signer qc_v1 routes require depositorTrustRoots when covenantSigner.requireApprovalTrustRoots=true", + "covenant signer qc_v1 routes require depositorTrustRoots when covenantSigner.requireApprovalTrustRoots=true or for a non-loopback listen address", ) } @@ -210,13 +242,13 @@ func validateRequiredApprovalTrustRoots( TemplateQcV1, ) { return fmt.Errorf( - "covenant signer qc_v1 routes require custodianTrustRoots when covenantSigner.requireApprovalTrustRoots=true", + "covenant signer qc_v1 routes require custodianTrustRoots when covenantSigner.requireApprovalTrustRoots=true or for a non-loopback listen address", ) } if service.signerApprovalVerifier == nil { return fmt.Errorf( - "covenant signer requires a signerApprovalVerifier when covenantSigner.requireApprovalTrustRoots=true", + "covenant signer requires a signerApprovalVerifier when covenantSigner.requireApprovalTrustRoots=true or for a non-loopback listen address", ) } diff --git a/pkg/covenantsigner/server_test.go b/pkg/covenantsigner/server_test.go index a81d4c853d..246f103bed 100644 --- a/pkg/covenantsigner/server_test.go +++ b/pkg/covenantsigner/server_test.go @@ -415,6 +415,156 @@ func TestInitializeRequiresSignerApprovalVerifierWhenConfigured(t *testing.T) { } } +// verifierOnlyEngine implements Engine and SignerApprovalVerifier but +// deliberately not CurrentBlockHeightProvider, so NewService succeeds (there +// is nothing to auto-detect a provider from) while Initialize's post-NewService +// invariant check -- a verifier without a way to determine expiry -- must +// still reject startup. +type verifierOnlyEngine struct{} + +func (verifierOnlyEngine) OnSubmit(context.Context, *Job) (*Transition, error) { + return nil, nil +} + +func (verifierOnlyEngine) OnPoll(context.Context, *Job) (*Transition, error) { + return nil, nil +} + +func (verifierOnlyEngine) VerifySignerApproval(RouteSubmitRequest) error { + return nil +} + +func TestInitializeRejectsVerifierWithoutBlockHeightProvider(t *testing.T) { + handle := newMemoryHandle() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + _, enabled, err := Initialize( + ctx, + Config{Port: availableLoopbackPort(t)}, + handle, + &verifierOnlyEngine{}, + ) + if err == nil || enabled { + t.Fatalf("expected startup to fail for a verifier without a block height provider, got enabled=%v err=%v", enabled, err) + } + if !strings.Contains(err.Error(), "CurrentBlockHeightProvider") { + t.Fatalf("unexpected error: %v", err) + } +} + +// TestInitializeReleasesStoreLockAfterLaterStartupFailure proves that a +// startup failure occurring after NewService has already acquired the +// store's exclusive file lock still releases it: a first Initialize attempt +// fails on the verifier/provider invariant (after successfully constructing +// the service and its store), and a second attempt over the very same data +// directory must be able to acquire the lock and succeed. Without the +// deferred cleanup in Initialize, the second attempt would fail with a +// lock-contention error instead of starting. +func TestInitializeReleasesStoreLockAfterLaterStartupFailure(t *testing.T) { + dataDir := t.TempDir() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + _, enabled, err := Initialize( + ctx, + Config{ + Port: availableLoopbackPort(t), + DataDir: dataDir, + }, + newMemoryHandle(), + &verifierOnlyEngine{}, + ) + if err == nil || enabled { + t.Fatalf("expected the first startup attempt to fail, got enabled=%v err=%v", enabled, err) + } + // Assert this failed for the verifier/provider invariant, not because the + // data directory's lock was already held (which would defeat the point + // of this test: proving the *first* attempt's lock gets released). + if !strings.Contains(err.Error(), "CurrentBlockHeightProvider") { + t.Fatalf("expected a verifier/provider invariant error, got: %v", err) + } + + server, enabled, err := Initialize( + ctx, + Config{ + Port: availableLoopbackPort(t), + DataDir: dataDir, + }, + newMemoryHandle(), + &scriptedVerifierEngine{}, + ) + if err != nil || !enabled || server == nil { + t.Fatalf( + "expected retry over the same data directory to succeed, got enabled=%v server=%v err=%v", + enabled, + server != nil, + err, + ) + } +} + +func TestInitializeRequiresTrustRootsForNonLoopbackListenAddress(t *testing.T) { + handle := newMemoryHandle() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // A non-loopback (production) listen address must fail startup when the + // required approval trust roots are missing, even without + // RequireApprovalTrustRoots being set explicitly. The engine provides a + // verifier, so the failure is attributable to the missing trust roots. + _, enabled, err := Initialize( + ctx, + Config{ + Port: availableLoopbackPort(t), + ListenAddress: "0.0.0.0", + AuthToken: "test-token", + }, + handle, + &scriptedVerifierEngine{}, + ) + if err == nil || enabled { + t.Fatalf("expected non-loopback startup without trust roots to fail, got enabled=%v err=%v", enabled, err) + } + if !strings.Contains(err.Error(), "non-loopback listen address") { + t.Fatalf("unexpected error: %v", err) + } +} + +func TestInitializeRequiresSignerApprovalVerifierForNonLoopbackListenAddress(t *testing.T) { + handle := newMemoryHandle() + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + // A non-loopback (production) listen address with all trust roots present + // but no signer approval verifier must fail startup. + _, enabled, err := Initialize( + ctx, + Config{ + Port: availableLoopbackPort(t), + ListenAddress: "0.0.0.0", + AuthToken: "test-token", + DepositorTrustRoots: []DepositorTrustRoot{ + testDepositorTrustRoot(TemplateQcV1), + }, + CustodianTrustRoots: []CustodianTrustRoot{ + testCustodianTrustRoot(TemplateQcV1), + }, + }, + handle, + &scriptedEngine{}, + ) + if err == nil || enabled { + t.Fatalf("expected non-loopback startup without a verifier to fail, got enabled=%v err=%v", enabled, err) + } + if !strings.Contains(err.Error(), "requires a signerApprovalVerifier") { + t.Fatalf("unexpected error: %v", err) + } + if !strings.Contains(err.Error(), "non-loopback listen address") { + t.Fatalf("unexpected error: %v", err) + } +} + func TestIsLoopbackListenAddressAcceptsBracketedIPv6Loopback(t *testing.T) { if !isLoopbackListenAddress("[::1]") { t.Fatal("expected bracketed IPv6 loopback address to be recognized") diff --git a/pkg/covenantsigner/service.go b/pkg/covenantsigner/service.go index 7789c151ca..80328265a5 100644 --- a/pkg/covenantsigner/service.go +++ b/pkg/covenantsigner/service.go @@ -119,6 +119,18 @@ func NewService( if verifier, ok := engine.(SignerApprovalVerifier); ok { service.signerApprovalVerifier = verifier } + // Auto-detect the current block height provider from the engine, mirroring + // the SignerApprovalVerifier auto-detection above. Correctness must not + // depend on callers remembering to pass WithCurrentBlockProvider: a caller + // that forgets it would otherwise silently get fail-open "never expires" + // certificate handling. WithCurrentBlockProvider (below, via options) can + // still override this when a caller genuinely needs a different provider + // than the engine itself. + if provider, ok := engine.(CurrentBlockHeightProvider); ok { + service.currentBlockProvider = func() (uint64, error) { + return provider.CurrentBlockHeight(context.Background()) + } + } for _, option := range options { option(service) } @@ -251,6 +263,75 @@ func sameJobRevision(current *Job, snapshot *Job) bool { reflect.DeepEqual(current.Handoff, snapshot.Handoff) } +// currentBlockForRequest returns the current block height needed to validate +// request's signer approval certificate expiry, or nil if request carries no +// certificate at all, or the certificate has no EndBlock (expiry does not +// apply either way, and the provider is not queried unnecessarily). Gating on +// EndBlock here -- not just on SignerApproval being present -- keeps the +// required validation order (EndBlock structural presence before any +// provider query) even though this helper runs ahead of full request +// validation: a request with a present-but-nil EndBlock must never trigger a +// provider RPC before normalizeSignerApprovalCertificate gets a chance to +// reject it structurally. When request does carry a certificate with an +// EndBlock but no block height provider is configured, this returns (nil, +// nil) rather than a fabricated zero height: the absence stays +// distinguishable so callers fail closed instead of silently treating the +// certificate as unexpired. Provider errors are propagated so callers fail +// closed on provider failures too. +func (s *Service) currentBlockForRequest(request RouteSubmitRequest) (*uint64, error) { + if request.SignerApproval == nil { + return nil, nil + } + if request.SignerApproval.EndBlock == nil { + return nil, nil + } + if s.currentBlockProvider == nil { + return nil, nil + } + + height, err := s.currentBlockProvider() + if err != nil { + return nil, fmt.Errorf("failed to get current block height: %w", err) + } + + return &height, nil +} + +// ensureStoredCertificateTimely rejects a job whose signer approval +// certificate has expired, whose EndBlock is missing (e.g. a legacy v1 job +// persisted before v2 enforcement rolled out), or whose expiry cannot be +// determined because no current block height provider is configured. Jobs +// without a signer approval certificate are always timely, since expiry only +// applies to certificate-bearing requests. This is shared by every place a +// durable job with a certificate is trusted again after having been +// persisted: loadPollJob's two call sites, and Submit's pre-lock and +// authoritative post-lock rechecks. +func (s *Service) ensureStoredCertificateTimely(job *Job) error { + certificate := job.Request.SignerApproval + if certificate == nil { + return nil + } + if certificate.EndBlock == nil { + return &inputError{"signer approval certificate has expired"} + } + + currentBlock, err := s.currentBlockForRequest(job.Request) + if err != nil { + return err + } + if currentBlock == nil { + return fmt.Errorf( + "cannot determine signer approval certificate expiry: " + + "no current block height provider is configured", + ) + } + if certificateExpired(*currentBlock, *certificate.EndBlock) { + return &inputError{"signer approval certificate has expired"} + } + + return nil +} + func (s *Service) loadPollJob(route TemplateID, input SignerPollInput) (*Job, error) { job, ok, err := s.store.GetByRequestID(input.RequestID) if err != nil { @@ -272,24 +353,11 @@ func (s *Service) loadPollJob(route TemplateID, input SignerPollInput) (*Job, er return nil, errJobNotFound } - // Check if the signer approval certificate has expired since submit. - // If expired, reject the poll to avoid producing a signature with an + // Check if the signer approval certificate has expired since submit. If + // expired, reject the poll to avoid producing a signature with an // authorization that is no longer valid. - // - // NOTE: The >= comparison is intentional. A certificate with - // EndBlock=100 is considered expired when the current block is - // 100 or greater. This is because EndBlock is a closed interval: - // the signature is valid only up to and including EndBlock. - if s.currentBlockProvider != nil && job.Request.SignerApproval != nil && job.Request.SignerApproval.EndBlock != nil { - currentBlock, err := s.currentBlockProvider() - if err != nil { - return nil, fmt.Errorf("failed to get current block height: %w", err) - } - if currentBlock >= *job.Request.SignerApproval.EndBlock { - return nil, &inputError{ - "signer approval certificate has expired", - } - } + if err := s.ensureStoredCertificateTimely(job); err != nil { + return nil, err } digest, err := requestDigest( @@ -311,6 +379,16 @@ func (s *Service) loadPollJob(route TemplateID, input SignerPollInput) (*Job, er // createOrDedup creates a new job under the service mutex, or returns the // existing job result if the route request is already known. Returns // (job, nil, nil) for a new job, or (nil, result, nil) for a dedup hit. +// +// A dedup hit -- including one that resolves to an already-terminal +// ready/failed job -- is only returned after a fresh ensureStoredCertificateTimely +// check, run here while s.mutex is held. Without this, a certificate that was +// valid when the original job was created but has since expired (or whose +// provider now errors) could be echoed back as if it were still valid: the +// dedup path short-circuits Submit before any of its other expiry/provider +// rechecks ever run. On failure this returns the expiry/provider error and +// leaves the durable job untouched, the same fail-closed contract Submit +// itself uses for its own rechecks. func (s *Service) createOrDedup( route TemplateID, input SignerSubmitInput, @@ -328,6 +406,9 @@ func (s *Service) createOrDedup( "routeRequestId already exists with a different request payload", } } + if err := s.ensureStoredCertificateTimely(existing); err != nil { + return nil, nil, err + } result := mapJobResult(existing) return nil, &result, nil } @@ -371,6 +452,11 @@ func (s *Service) createOrDedup( } func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubmitInput) (StepResult, error) { + currentBlock, err := s.currentBlockForRequest(input.Request) + if err != nil { + return StepResult{}, err + } + submitValidationOptions := validationOptions{ migrationPlanQuoteTrustRoots: s.migrationPlanQuoteTrustRoots, depositorTrustRoots: s.depositorTrustRoots, @@ -378,6 +464,7 @@ func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubm requireFreshMigrationPlanQuote: true, migrationPlanQuoteVerificationNow: s.now(), signerApprovalVerifier: s.signerApprovalVerifier, + currentBlock: currentBlock, } if err := validateSubmitInput(route, input, submitValidationOptions); err != nil { return StepResult{}, err @@ -418,6 +505,14 @@ func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubm defer func() { <-s.inFlightSlots }() } + // The certificate may have expired while this submission waited on the + // signer approval verifier or for an in-flight slot. Recheck before + // starting synchronous threshold signing so signing never begins on an + // authorization that has already lapsed. + if err := s.ensureStoredCertificateTimely(job); err != nil { + return StepResult{}, err + } + transition, err := s.engine.OnSubmit(ctx, job) if err != nil { return StepResult{}, err @@ -430,6 +525,16 @@ func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubm } } + // Fast rejection path: synchronous threshold signing itself can take long + // enough for the certificate to expire. Recheck before acquiring the + // service mutex so an already-stale result does not even reach the + // authoritative check below. This check is optimistic, not authoritative + // -- the check after acquiring the mutex is what actually protects the + // persisted state from a concurrent expiry. + if err := s.ensureStoredCertificateTimely(job); err != nil { + return StepResult{}, err + } + s.mutex.Lock() defer s.mutex.Unlock() @@ -441,6 +546,21 @@ func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubm return StepResult{}, errJobNotFound } + // Authoritative recheck: the last point before any stored job state -- + // whether this call's own transition or a newer/terminal state a + // concurrent Submit or Poll already persisted -- is persisted or + // returned, running while s.mutex is held so no concurrent Submit or + // Poll can advance the current height and slip past it. This closes the + // TOCTOU window between the fast check above and the store write/return + // below. It must run here, immediately after currentJob is loaded and + // before the early-return just below: a concurrently-advanced or + // terminal currentJob must never be handed back without this check, the + // same fail-closed contract createOrDedup's dedup hit and loadPollJob's + // every call site already use. + if err := s.ensureStoredCertificateTimely(currentJob); err != nil { + return StepResult{}, err + } + // Another poll already advanced the stored job while submit was waiting on // signer work. Return the newer durable state instead of overwriting it with // a transition computed from an older snapshot. @@ -457,20 +577,16 @@ func (s *Service) Submit(ctx context.Context, route TemplateID, input SignerSubm } func (s *Service) Poll(ctx context.Context, route TemplateID, input SignerPollInput) (StepResult, error) { - var currentBlock uint64 - if s.currentBlockProvider != nil { - blockHeight, err := s.currentBlockProvider() - if err != nil { - return StepResult{}, fmt.Errorf("failed to get current block height: %w", err) - } - currentBlock = blockHeight + currentBlock, err := s.currentBlockForRequest(input.Request) + if err != nil { + return StepResult{}, err } if err := validatePollInput( route, input, validationOptions{ policyIndependentDigest: true, - currentBlock: ¤tBlock, + currentBlock: currentBlock, }, ); err != nil { return StepResult{}, err diff --git a/pkg/covenantsigner/store.go b/pkg/covenantsigner/store.go index 287614ca76..289b1e0c80 100644 --- a/pkg/covenantsigner/store.go +++ b/pkg/covenantsigner/store.go @@ -13,9 +13,16 @@ import ( ) const ( - jobsDirectory = "covenant-signer/jobs" - poisonedDirectory = "covenant-signer/poisoned" - lockFileName = ".lock" + // jobsDirectory is a single-level persistence directory name. It must not + // contain a path separator: the disk persistence handle creates and + // enumerates only one directory level, so a nested name is skipped on + // reload (its descriptor directory is reported as the first-level parent). + // legacyJobsDirectory is the previously used nested name; job files + // persisted under it are migrated to jobsDirectory on startup. + jobsDirectory = "covenant-signer-jobs" + legacyJobsDirectory = "covenant-signer/jobs" + poisonedDirectory = "covenant-signer/poisoned" + lockFileName = ".lock" ) type Store struct { @@ -44,6 +51,17 @@ func NewStore(handle persistence.BasicHandle, dataDir string) (*Store, error) { return nil, err } store.lockFile = lockFile + + if err := migrateLegacyJobsDirectory(dataDir); err != nil { + // Release the lock if migration fails after successful acquisition. + if closeErr := store.Close(); closeErr != nil { + logger.Warnf( + "failed to release store lock after migration failure: [%v]", + closeErr, + ) + } + return nil, err + } } if err := store.load(); err != nil { @@ -106,6 +124,64 @@ func acquireFileLock(dataDir string) (*os.File, error) { return lockFile, nil } +// migrateLegacyJobsDirectory moves persisted job files from the previously used +// nested legacyJobsDirectory into the flat jobsDirectory. The nested directory +// could not be reliably reloaded by the single-level disk persistence handle, +// so any job files an operator managed to persist under it would otherwise be +// silently skipped on startup. Migration is best-effort per file and +// idempotent: files already present in the destination are left untouched, and +// a missing legacy directory is not an error. +func migrateLegacyJobsDirectory(dataDir string) error { + legacyDir := filepath.Join(dataDir, legacyJobsDirectory) + + entries, err := os.ReadDir(legacyDir) + if err != nil { + if os.IsNotExist(err) { + return nil + } + return fmt.Errorf( + "cannot read legacy covenant signer jobs directory [%s]: %w", + legacyDir, + err, + ) + } + + newDir := filepath.Join(dataDir, jobsDirectory) + if err := os.MkdirAll(newDir, 0700); err != nil { + return fmt.Errorf( + "cannot create covenant signer jobs directory [%s]: %w", + newDir, + err, + ) + } + + for _, entry := range entries { + // Only migrate persisted job files; skip subdirectories and the lock + // file left behind under the legacy path. + if entry.IsDir() || filepath.Ext(entry.Name()) != ".json" { + continue + } + + newPath := filepath.Join(newDir, entry.Name()) + // Do not clobber a file already present in the destination. + if _, err := os.Stat(newPath); err == nil { + continue + } + + oldPath := filepath.Join(legacyDir, entry.Name()) + if err := os.Rename(oldPath, newPath); err != nil { + return fmt.Errorf( + "cannot migrate covenant signer job file [%s] to [%s]: %w", + oldPath, + newPath, + err, + ) + } + } + + return nil +} + // Close releases the exclusive file lock and closes the underlying lock file // descriptor. For stores created without a dataDir (in-memory handles), Close // is a safe no-op. Close is idempotent. @@ -184,6 +260,13 @@ func (s *Store) load() error { continue } + // Skip non-job files that share the jobs directory, such as the + // store lock file. Persisted jobs are always saved with a .json + // extension. + if filepath.Ext(descriptor.Name()) != ".json" { + continue + } + content, err := descriptor.Content() if err != nil { return fmt.Errorf( diff --git a/pkg/covenantsigner/store_disk_test.go b/pkg/covenantsigner/store_disk_test.go new file mode 100644 index 0000000000..01a7a474d2 --- /dev/null +++ b/pkg/covenantsigner/store_disk_test.go @@ -0,0 +1,143 @@ +package covenantsigner + +import ( + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/keep-network/keep-common/pkg/persistence" +) + +func newDiskTestJob(suffix string) *Job { + return &Job{ + RequestID: "kcs_self_" + suffix, + RouteRequestID: "ors_" + suffix, + Route: TemplateSelfV1, + IdempotencyKey: "idem_" + suffix, + FacadeRequestID: "rf_" + suffix, + RequestDigest: "0xdeadbeef", + State: JobStatePending, + Detail: "queued", + CreatedAt: "2026-03-09T00:00:00Z", + UpdatedAt: "2026-03-09T00:00:00Z", + Request: baseRequest(TemplateSelfV1), + } +} + +// TestStoreReloadPreservesJobsOnDisk exercises the store against a real +// disk-backed persistence handle (not the in-memory handle used elsewhere). +// The disk handle creates and enumerates only a single directory level, so a +// nested jobs directory name would make persisted jobs unrecoverable after a +// restart. This test persists a job, restarts the store over the same data +// directory, and asserts both the request ID and route request indexes are +// restored. +func TestStoreReloadPreservesJobsOnDisk(t *testing.T) { + dataDir := t.TempDir() + + handle, err := persistence.NewBasicDiskHandle(dataDir) + if err != nil { + t.Fatal(err) + } + store, err := NewStore(handle, dataDir) + if err != nil { + t.Fatal(err) + } + + job := newDiskTestJob("disk") + if err := store.Put(job); err != nil { + t.Fatal(err) + } + if err := store.Close(); err != nil { + t.Fatal(err) + } + + // Restart the store over the same data directory. + reloadHandle, err := persistence.NewBasicDiskHandle(dataDir) + if err != nil { + t.Fatal(err) + } + reloaded, err := NewStore(reloadHandle, dataDir) + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { reloaded.Close() }) + + // The route request index must be restored. + byRoute, ok, err := reloaded.GetByRouteRequest(TemplateSelfV1, job.RouteRequestID) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the route request index to be restored after restart") + } + if byRoute.RequestID != job.RequestID { + t.Fatalf("unexpected request ID from route index: %s", byRoute.RequestID) + } + + // The request ID index must be restored. + byID, ok, err := reloaded.GetByRequestID(job.RequestID) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the request ID index to be restored after restart") + } + if byID.RouteRequestID != job.RouteRequestID { + t.Fatalf("unexpected route request ID from request index: %s", byID.RouteRequestID) + } +} + +// TestStoreMigratesLegacyNestedJobs asserts that a job file left under the +// previously used nested directory is migrated to the flat jobs directory on +// startup and becomes reloadable. +func TestStoreMigratesLegacyNestedJobs(t *testing.T) { + dataDir := t.TempDir() + + // Simulate a job persisted under the legacy nested directory. + legacyDir := filepath.Join(dataDir, legacyJobsDirectory) + if err := os.MkdirAll(legacyDir, 0700); err != nil { + t.Fatal(err) + } + job := newDiskTestJob("legacy") + payload, err := json.Marshal(job) + if err != nil { + t.Fatal(err) + } + legacyFile := filepath.Join(legacyDir, job.RequestID+".json") + if err := os.WriteFile(legacyFile, payload, 0600); err != nil { + t.Fatal(err) + } + + handle, err := persistence.NewBasicDiskHandle(dataDir) + if err != nil { + t.Fatal(err) + } + store, err := NewStore(handle, dataDir) // triggers migration and load + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { store.Close() }) + + // The migrated job must be restored in both indexes. + if _, ok, err := store.GetByRequestID(job.RequestID); err != nil { + t.Fatal(err) + } else if !ok { + t.Fatal("expected the legacy job to be migrated and restored") + } + if _, ok, err := store.GetByRouteRequest(TemplateSelfV1, job.RouteRequestID); err != nil { + t.Fatal(err) + } else if !ok { + t.Fatal("expected the legacy route request index to be restored") + } + + // The file must now live under the flat jobs directory. + newFile := filepath.Join(dataDir, jobsDirectory, job.RequestID+".json") + if _, err := os.Stat(newFile); err != nil { + t.Fatalf("expected migrated file at %s: %v", newFile, err) + } + // The legacy file must no longer be present. + if _, err := os.Stat(legacyFile); !os.IsNotExist(err) { + t.Fatalf("expected legacy file at %s to be moved away", legacyFile) + } +} diff --git a/pkg/covenantsigner/testdata/covenant_recovery_approval_vectors_v1.json b/pkg/covenantsigner/testdata/covenant_recovery_approval_vectors_v1.json index db61ffba15..f637e9d861 100644 --- a/pkg/covenantsigner/testdata/covenant_recovery_approval_vectors_v1.json +++ b/pkg/covenantsigner/testdata/covenant_recovery_approval_vectors_v1.json @@ -63,7 +63,7 @@ ] }, "signerApproval": { - "certificateVersion": 1, + "certificateVersion": 2, "signatureAlgorithm": "tecdsa-secp256k1", "approvalDigest": "0xa6ffb42318a8e8b3b9669324ee5ad393133afcc9cc81044739cbaa77d5fa34c9", "walletPublicKey": "0x04d140d1eedb94f53ce43e0f4d68e8e0de6d6f2a444ef98f2a0e6c0f7fca02ef7dc4cb14e7b0f7c23787c93ca4d978f312c64379f38d9f52f86d1a89f0f8572f9f", @@ -92,7 +92,7 @@ "custodianRequired": true } }, - "expectedRequestDigest": "0x8538a608ffbc3264655f9d87e334bfb7fa0d46e37cb6ffdb7c98b803eec900c8" + "expectedRequestDigest": "0x81e134d0bf0d1a7d41ec2c32d4ea555a77558f06bbe0941d0519e52a1132e28c" }, "self_v1": { "expectedApprovalDigest": "0x4820468d065bc627dabac7860ef473ed28806d6352ba3459ff4edfb81e6bb752", @@ -151,7 +151,7 @@ ] }, "signerApproval": { - "certificateVersion": 1, + "certificateVersion": 2, "signatureAlgorithm": "tecdsa-secp256k1", "approvalDigest": "0x4820468d065bc627dabac7860ef473ed28806d6352ba3459ff4edfb81e6bb752", "walletPublicKey": "0x04d140d1eedb94f53ce43e0f4d68e8e0de6d6f2a444ef98f2a0e6c0f7fca02ef7dc4cb14e7b0f7c23787c93ca4d978f312c64379f38d9f52f86d1a89f0f8572f9f", @@ -177,7 +177,7 @@ "custodianRequired": false } }, - "expectedRequestDigest": "0x2da9d108af3d175865ee0654843a2c61eaf7fcbcf5d48afd807044725f310d17" + "expectedRequestDigest": "0x5eb10d5df3818f646b79f1feee1decd34949a688ca44f47c52d2a7b5d3aaf5d1" }, "self_v1_presign": { "expectedApprovalDigest": "0x4820468d065bc627dabac7860ef473ed28806d6352ba3459ff4edfb81e6bb752", @@ -236,7 +236,7 @@ ] }, "signerApproval": { - "certificateVersion": 1, + "certificateVersion": 2, "signatureAlgorithm": "tecdsa-secp256k1", "approvalDigest": "0x4820468d065bc627dabac7860ef473ed28806d6352ba3459ff4edfb81e6bb752", "walletPublicKey": "0x04d140d1eedb94f53ce43e0f4d68e8e0de6d6f2a444ef98f2a0e6c0f7fca02ef7dc4cb14e7b0f7c23787c93ca4d978f312c64379f38d9f52f86d1a89f0f8572f9f", @@ -262,7 +262,7 @@ "custodianRequired": false } }, - "expectedRequestDigest": "0x4399f8651fea31ab227bffd3db96daa969612e9e5195394df3f349af4713cf03" + "expectedRequestDigest": "0x9c53f8df7ffbeece1ba9652bca402ce51561a94083355b3f1809c087d1598e97" } } } diff --git a/pkg/covenantsigner/types.go b/pkg/covenantsigner/types.go index b991bbb7e9..d847cccdab 100644 --- a/pkg/covenantsigner/types.go +++ b/pkg/covenantsigner/types.go @@ -201,7 +201,7 @@ type SignerApprovalCertificate struct { Signature string `json:"signature"` ActiveMembers []uint32 `json:"activeMembers,omitempty"` InactiveMembers []uint32 `json:"inactiveMembers,omitempty"` - EndBlock *uint64 `json:"endBlock,omitempty"` + EndBlock *uint64 `json:"endBlock"` } type SigningRequirements struct { diff --git a/pkg/covenantsigner/validation.go b/pkg/covenantsigner/validation.go index 4daac0fe7b..47e4943814 100644 --- a/pkg/covenantsigner/validation.go +++ b/pkg/covenantsigner/validation.go @@ -20,7 +20,7 @@ const ( canonicalAnchorValueSats uint64 = 330 migrationTransactionPlanVersion uint32 = 1 artifactApprovalVersion uint32 = 1 - signerApprovalCertificateVersion uint32 = 1 + signerApprovalCertificateVersion uint32 = 2 migrationPlanQuoteVersion uint32 = 1 migrationPlanQuoteSignatureVersion uint32 = 1 ) @@ -264,6 +264,15 @@ func normalizeRouteSubmitRequest( }, nil } +// certificateExpired reports whether a signer approval certificate with the +// given EndBlock is expired at the given current block height. EndBlock is +// an inclusive upper bound: the certificate remains valid through and +// including EndBlock, and is only expired once the current height exceeds +// it. +func certificateExpired(current, end uint64) bool { + return current > end +} + func validateCommonRequest( route TemplateID, request RouteSubmitRequest, @@ -447,32 +456,43 @@ func validateCommonRequest( } if request.SignerApproval != nil { - if options.policyIndependentDigest { - // Check if the signer approval certificate has expired. If - // EndBlock is set and the current block height has reached or - // passed it, the certificate is expired and must be re-verified - // to ensure the signer's authorization is still valid. - // - // NOTE: The >= comparison is intentional. A certificate with - // EndBlock=N is considered expired when the current block is - // N or greater, because EndBlock uses a closed interval: the - // signature is valid only up to and including EndBlock. - if request.SignerApproval.EndBlock != nil && - options.currentBlock != nil && - *options.currentBlock >= *request.SignerApproval.EndBlock { - // Certificate expired. When no verifier is present (Poll flow), - // return the expiration error directly. When a verifier is - // present (Submit flow), fall through to re-verify in case the - // signer's authorization was renewed. - if options.signerApprovalVerifier == nil { - return &inputError{ - "signer approval certificate has expired", - } - } - } else { - return nil + certificate := request.SignerApproval + + // EndBlock is a required v2 field; normalizeSignerApprovalCertificate + // (already run above via validateArtifactApprovals) rejects a nil + // EndBlock before execution ever reaches here. This check is kept as + // a defensive, self-contained precondition for this validation phase + // rather than relying solely on that earlier invariant. + if certificate.EndBlock == nil { + return &inputError{"request.signerApproval.endBlock is required"} + } + if options.currentBlock == nil { + return fmt.Errorf( + "cannot validate signer approval certificate expiry: " + + "no current block height is available", + ) + } + if certificateExpired(*options.currentBlock, *certificate.EndBlock) { + // A certificate is never "renewed" by re-verifying it: renewal + // requires a new certificate with a later signed EndBlock. This + // applies uniformly to both Submit (policyIndependentDigest + // false) and Poll (true) -- there is no fall-through to the + // verifier below once expired. + return &inputError{ + "signer approval certificate has expired", } } + + if options.policyIndependentDigest { + // Poll does not re-verify the certificate's cryptographic + // signature -- Submit already verified it once when the + // certificate was first accepted. Poll only needs to confirm the + // resubmitted certificate has not expired (checked above) and + // still matches the stored request digest, which the caller + // checks separately via requestDigest comparison. + return nil + } + if options.signerApprovalVerifier == nil { return &inputError{ "request.signerApproval cannot be verified by this signer deployment", diff --git a/pkg/covenantsigner/validation_approval.go b/pkg/covenantsigner/validation_approval.go index fd5e78cbc8..e4d19454d4 100644 --- a/pkg/covenantsigner/validation_approval.go +++ b/pkg/covenantsigner/validation_approval.go @@ -171,16 +171,21 @@ func normalizeSignerApprovalCertificate( } } - if signerApproval.EndBlock != nil { - if err := validateUint32Range( - "request.signerApproval.endBlock", - *signerApproval.EndBlock, - ); err != nil { - return nil, err + // EndBlock is a required v2 certificate field: a missing or null EndBlock + // must fail closed rather than being treated as "never expires". Unlike + // request.maturityHeight (a Bitcoin nLockTime-style field genuinely + // bounded to uint32), EndBlock is a host-chain block number bound into + // the certificate v2 signing digest as a full 8-byte big-endian uint64 + // (see signerApprovalCertificateSigningDigest in pkg/tbtc), so it must + // accept the entire uint64 range here too -- rejecting values above + // math.MaxUint32 would contradict that contract. + if signerApproval.EndBlock == nil { + return nil, &inputError{ + "request.signerApproval.endBlock is required", } - endBlock := *signerApproval.EndBlock - normalizedSignerApproval.EndBlock = &endBlock } + endBlock := *signerApproval.EndBlock + normalizedSignerApproval.EndBlock = &endBlock return normalizedSignerApproval, nil } diff --git a/pkg/maintainer/spv/bitcoin_chain_test.go b/pkg/maintainer/spv/bitcoin_chain_test.go index 2f790bf11f..1882d00d65 100644 --- a/pkg/maintainer/spv/bitcoin_chain_test.go +++ b/pkg/maintainer/spv/bitcoin_chain_test.go @@ -3,6 +3,7 @@ package spv import ( "bytes" "fmt" + "math" "sync" "github.com/keep-network/keep-core/pkg/bitcoin" @@ -151,7 +152,26 @@ func (lbc *localBitcoinChain) GetTransactionsForPublicKeyHash( func (lbc *localBitcoinChain) GetTxHashesForPublicKeyHash( publicKeyHash [20]byte, ) ([]bitcoin.Hash, error) { - panic("unsupported") + // Delegate to GetTransactionsForPublicKeyHash with an effectively + // unbounded limit instead of duplicating its P2PKH/P2WPKH matching loop. + // Do not hold lbc.mutex here: the delegated call locks it itself, and + // sync.Mutex is not reentrant. + transactions, err := lbc.GetTransactionsForPublicKeyHash( + publicKeyHash, + math.MaxInt32, + ) + if err != nil { + return nil, err + } + + // Preserves the ascending block-height order GetTransactionsForPublicKeyHash + // returns, which the tests rely on. + hashes := make([]bitcoin.Hash, len(transactions)) + for i, transaction := range transactions { + hashes[i] = transaction.Hash() + } + + return hashes, nil } func (lbc *localBitcoinChain) GetMempoolForPublicKeyHash(publicKeyHash [20]byte) ( diff --git a/pkg/maintainer/spv/config.go b/pkg/maintainer/spv/config.go index 49cdfe40d9..6b0a2806c4 100644 --- a/pkg/maintainer/spv/config.go +++ b/pkg/maintainer/spv/config.go @@ -11,17 +11,23 @@ const ( // Ethereum blocks in a week, assuming one block is 12s. DefaultHistoryDepth = 50400 - // DefaultTransactionLimit is the default value for the limit of - // transactions returned for a given wallet public key hash. The value is - // based on the frequency of how often wallet transactions will happen. - // For example, deposit sweep transactions are assumed to happen every 48h. - // Redemption transactions are assumed to happen every 3h. The wallet should - // refuse any proposals from the coordinator if the previously executed - // Bitcoin transaction was not proved to the Bridge yet so in theory, the - // value of 1 should be enough. We make it a bit higher - better to be - // safe than sorry. + // DefaultTransactionLimit is the default value for the limit of matching + // (unproven protocol) transactions returned for a given wallet public key + // hash. The value is based on the frequency of how often wallet + // transactions will happen. For example, deposit sweep transactions are + // assumed to happen every 48h. Redemption transactions are assumed to + // happen every 3h. The wallet should refuse any proposals from the + // coordinator if the previously executed Bitcoin transaction was not + // proved to the Bridge yet so in theory, the value of 1 should be enough. + // We make it a bit higher - better to be safe than sorry. DefaultTransactionLimit = 20 + // DefaultTransactionScanLimit is the default value for the maximum number + // of not-yet-classified confirmed transactions examined per wallet, per + // scan, when searching for matching (unproven protocol) transactions. It + // bounds the per-tick cost of the scan; see TransactionScanLimit. + DefaultTransactionScanLimit = 100 + // DefaultRestartBackoffTime is the default value for restart back-off time. DefaultRestartBackoffTime = 30 * time.Minute @@ -44,19 +50,31 @@ type Config struct { // not yet proven transactions can be found. HistoryDepth uint64 - // TransactionLimit sets the maximum number of confirmed transactions - // returned when getting transactions for a public key hash. Once the - // maintainer establishes the list of wallets, it needs to check Bitcoin - // transactions executed by each wallet. Then, it tries to find the - // transactions matching the given proposal type. For example, if set - // to `20`, only the latest twenty transactions will be returned. This - // value must not be too high so that the transaction lookup is efficient. - // At the same time, this value can not be too low to make sure the - // performed proposal's transaction can be found in case the wallet decided - // to execute some other Bitcoin transaction after the yet-not-proven - // transaction. + // TransactionLimit sets the maximum number of matching (unproven + // protocol) transactions returned for a given wallet public key hash. + // Once the maintainer establishes the list of wallets, it needs to check + // Bitcoin transactions executed by each wallet. Then, it tries to find + // the transactions matching the given proposal type. This value must not + // be too high so that the transaction lookup is efficient. At the same + // time, this value can not be too low to make sure the performed + // proposal's transaction can be found in case the wallet decided to + // execute some other Bitcoin transaction after the yet-not-proven + // transaction. See TransactionScanLimit for the separate bound on how + // many raw transactions are examined to find these matches. TransactionLimit int + // TransactionScanLimit sets the maximum number of not-yet-classified + // confirmed transactions examined, per wallet, each time the maintainer + // searches for matching (unproven protocol) transactions. Unlike + // TransactionLimit, which bounds the number of matches returned, this + // bounds the number of raw address transactions inspected to find them, + // keeping a single scan's cost bounded even when a wallet address has + // accumulated a large amount of unrelated (e.g. spam) transaction + // history. Search progress is remembered across calls, so the scan + // eventually covers the wallet's whole history over enough calls rather + // than starting over each time. + TransactionScanLimit int + // RestartBackoffTime is a restart backoff which should be applied when the // SPV maintainer is restarted. It helps to avoid being flooded with error // logs in case of a permanent error in the SPV maintainer. diff --git a/pkg/maintainer/spv/deposit_sweep.go b/pkg/maintainer/spv/deposit_sweep.go index 2b0b8a5f77..2e69f0e3c4 100644 --- a/pkg/maintainer/spv/deposit_sweep.go +++ b/pkg/maintainer/spv/deposit_sweep.go @@ -251,6 +251,8 @@ func convertVaultAddress(vault *chain.Address) common.Address { func getUnprovenDepositSweepTransactions( historyDepth uint64, transactionLimit int, + transactionScanLimit int, + scanner *walletTransactionScanner, btcChain bitcoin.Chain, spvChain Chain, ) ( @@ -311,40 +313,34 @@ func getUnprovenDepositSweepTransactions( continue } - walletTransactions, err := btcChain.GetTransactionsForPublicKeyHash( + walletUnprovenTransactions, err := scanner.getUnprovenWalletTransactions( + walletTransactionScanKey{ + actionType: tbtc.ActionDepositSweep, + actorWalletPKH: walletPublicKeyHash, + lookupWalletPKH: walletPublicKeyHash, + }, walletPublicKeyHash, transactionLimit, - ) - if err != nil { - return nil, fmt.Errorf( - "failed to get transactions for wallet: [%v]", - err, - ) - } - - for _, transaction := range walletTransactions { - isUnproven, err := - isUnprovenDepositSweepTransaction( + transactionScanLimit, + "deposit sweep", + btcChain, + func(transaction *bitcoin.Transaction) (bool, error) { + return isUnprovenDepositSweepTransaction( transaction, walletPublicKeyHash, btcChain, spvChain, ) - if err != nil { - return nil, fmt.Errorf( - "failed to check if transaction is an unproven deposit sweep "+ - "transaction: [%v]", - err, - ) - } - - if isUnproven { - unprovenDepositSweepTransactions = append( - unprovenDepositSweepTransactions, - transaction, - ) - } + }, + ) + if err != nil { + return nil, err } + + unprovenDepositSweepTransactions = append( + unprovenDepositSweepTransactions, + walletUnprovenTransactions..., + ) } return unprovenDepositSweepTransactions, nil diff --git a/pkg/maintainer/spv/deposit_sweep_test.go b/pkg/maintainer/spv/deposit_sweep_test.go index dc61256ccf..a575663f5e 100644 --- a/pkg/maintainer/spv/deposit_sweep_test.go +++ b/pkg/maintainer/spv/deposit_sweep_test.go @@ -352,6 +352,8 @@ func TestGetUnprovenDepositSweepTransactions(t *testing.T) { transactions, err := getUnprovenDepositSweepTransactions( historyDepth, transactionLimit, + DefaultTransactionScanLimit, + newWalletTransactionScanner(), btcChain, spvChain, ) diff --git a/pkg/maintainer/spv/moved_funds_sweep.go b/pkg/maintainer/spv/moved_funds_sweep.go index 417a1f5347..dc053ad8fa 100644 --- a/pkg/maintainer/spv/moved_funds_sweep.go +++ b/pkg/maintainer/spv/moved_funds_sweep.go @@ -131,6 +131,8 @@ func parseMovedFundsSweepTransactionInputs( func getUnprovenMovedFundsSweepTransactions( historyDepth uint64, transactionLimit int, + transactionScanLimit int, + scanner *walletTransactionScanner, btcChain bitcoin.Chain, spvChain Chain, ) ( @@ -210,46 +212,43 @@ func getUnprovenMovedFundsSweepTransactions( // When wallet makes a moved funds sweep transaction, it transfers // funds to itself. Therefore we can search all the transactions that - // pay to the wallet's public key hash. - walletTransactions, err := btcChain.GetTransactionsForPublicKeyHash( + // pay to the wallet's public key hash. A wallet can have only one + // unproven moved funds sweep transaction at a time, so the scan is + // bounded to a single match: it stops at the first unproven moved funds + // sweep transaction it finds, scanning past any unrelated (e.g. spam) + // transactions along the way. + // A wallet can have only one unproven moved funds sweep transaction at + // a time, so the result limit is always 1 regardless of the caller's + // transactionLimit - but the raw-examination budget below still uses + // the real transactionScanLimit, not this result limit. + walletUnprovenTransactions, err := scanner.getUnprovenWalletTransactions( + walletTransactionScanKey{ + actionType: tbtc.ActionMovedFundsSweep, + actorWalletPKH: walletPublicKeyHash, + lookupWalletPKH: walletPublicKeyHash, + }, walletPublicKeyHash, - transactionLimit, - ) - if err != nil { - return nil, fmt.Errorf( - "failed to get transactions for wallet: [%v]", - err, - ) - } - - for _, transaction := range walletTransactions { - isUnproven, err := - isUnprovenMovedFundsSweepTransaction( + 1, + transactionScanLimit, + "moved funds sweep", + btcChain, + func(transaction *bitcoin.Transaction) (bool, error) { + return isUnprovenMovedFundsSweepTransaction( transaction, walletPublicKeyHash, btcChain, spvChain, ) - if err != nil { - return nil, fmt.Errorf( - "failed to check if transaction is an unproven moved "+ - "funds sweep transaction: [%v]", - err, - ) - } - - if isUnproven { - unprovenMovedFundsSweepTransactions = append( - unprovenMovedFundsSweepTransactions, - transaction, - ) - - // A wallet can have only one unproven moved funds sweep - // transaction at a time. If we found such transaction, we don't - // have to look at this wallet's transactions anymore. - break - } + }, + ) + if err != nil { + return nil, err } + + unprovenMovedFundsSweepTransactions = append( + unprovenMovedFundsSweepTransactions, + walletUnprovenTransactions..., + ) } return unprovenMovedFundsSweepTransactions, nil diff --git a/pkg/maintainer/spv/moved_funds_sweep_test.go b/pkg/maintainer/spv/moved_funds_sweep_test.go index a830cedd4c..eb574bea98 100644 --- a/pkg/maintainer/spv/moved_funds_sweep_test.go +++ b/pkg/maintainer/spv/moved_funds_sweep_test.go @@ -356,6 +356,8 @@ func TestGetUnprovenMovedFundsSweepTransactions(t *testing.T) { transactions, err := getUnprovenMovedFundsSweepTransactions( historyDepth, transactionLimit, + DefaultTransactionScanLimit, + newWalletTransactionScanner(), btcChain, spvChain, ) diff --git a/pkg/maintainer/spv/moving_funds.go b/pkg/maintainer/spv/moving_funds.go index 81d1e13e51..53ccad918d 100644 --- a/pkg/maintainer/spv/moving_funds.go +++ b/pkg/maintainer/spv/moving_funds.go @@ -127,6 +127,8 @@ func parseMovingFundsTransactionInput( func getUnprovenMovingFundsTransactions( historyDepth uint64, transactionLimit int, + transactionScanLimit int, + scanner *walletTransactionScanner, btcChain bitcoin.Chain, spvChain Chain, ) ( @@ -197,41 +199,41 @@ func getUnprovenMovingFundsTransactions( // source wallet. targetWalletPublicKeyHash := targetWallets[0] - walletTransactions, err := btcChain.GetTransactionsForPublicKeyHash( + // The scan key's context hash is the Bridge's own commitment to this + // wallet's ordered target-wallet list, so a wallet that (in some + // future protocol version) could submit more than one moving funds + // commitment over its lifetime gets a distinct, persisted scan state + // per commitment instead of silently reusing a stale one. + walletUnprovenTransactions, err := scanner.getUnprovenWalletTransactions( + walletTransactionScanKey{ + actionType: tbtc.ActionMovingFunds, + actorWalletPKH: walletPublicKeyHash, + lookupWalletPKH: targetWalletPublicKeyHash, + contextHash: wallet.MovingFundsTargetWalletsCommitmentHash, + }, targetWalletPublicKeyHash, transactionLimit, - ) - if err != nil { - return nil, fmt.Errorf( - "failed to get transactions for wallet: [%v]", - err, - ) - } - - for _, transaction := range walletTransactions { - isUnproven, err := - isUnprovenMovingFundsTransaction( + transactionScanLimit, + "moving funds", + btcChain, + func(transaction *bitcoin.Transaction) (bool, error) { + return isUnprovenMovingFundsTransaction( transaction, walletPublicKeyHash, targetWallets, btcChain, spvChain, ) - if err != nil { - return nil, fmt.Errorf( - "failed to check if transaction is an unproven moving funds "+ - "transaction: [%v]", - err, - ) - } - - if isUnproven { - unprovenMovingFundsTransactions = append( - unprovenMovingFundsTransactions, - transaction, - ) - } + }, + ) + if err != nil { + return nil, err } + + unprovenMovingFundsTransactions = append( + unprovenMovingFundsTransactions, + walletUnprovenTransactions..., + ) } return unprovenMovingFundsTransactions, nil diff --git a/pkg/maintainer/spv/moving_funds_test.go b/pkg/maintainer/spv/moving_funds_test.go index c5f1332b95..22924dced8 100644 --- a/pkg/maintainer/spv/moving_funds_test.go +++ b/pkg/maintainer/spv/moving_funds_test.go @@ -268,6 +268,8 @@ func TestGetUnprovenMovingFundsTransactions(t *testing.T) { transactions, err := getUnprovenMovingFundsTransactions( historyDepth, transactionLimit, + DefaultTransactionScanLimit, + newWalletTransactionScanner(), btcChain, spvChain, ) diff --git a/pkg/maintainer/spv/redemptions.go b/pkg/maintainer/spv/redemptions.go index e504860f81..c652ae14b6 100644 --- a/pkg/maintainer/spv/redemptions.go +++ b/pkg/maintainer/spv/redemptions.go @@ -161,6 +161,8 @@ func parseRedemptionTransactionInput( func getUnprovenRedemptionTransactions( historyDepth uint64, transactionLimit int, + transactionScanLimit int, + scanner *walletTransactionScanner, btcChain bitcoin.Chain, spvChain Chain, ) ( @@ -221,40 +223,34 @@ func getUnprovenRedemptionTransactions( continue } - walletTransactions, err := btcChain.GetTransactionsForPublicKeyHash( + walletUnprovenTransactions, err := scanner.getUnprovenWalletTransactions( + walletTransactionScanKey{ + actionType: tbtc.ActionRedemption, + actorWalletPKH: walletPublicKeyHash, + lookupWalletPKH: walletPublicKeyHash, + }, walletPublicKeyHash, transactionLimit, - ) - if err != nil { - return nil, fmt.Errorf( - "failed to get transactions for wallet: [%v]", - err, - ) - } - - for _, transaction := range walletTransactions { - isUnproven, err := - isUnprovenRedemptionTransaction( + transactionScanLimit, + "redemption", + btcChain, + func(transaction *bitcoin.Transaction) (bool, error) { + return isUnprovenRedemptionTransaction( transaction, walletPublicKeyHash, btcChain, spvChain, ) - if err != nil { - return nil, fmt.Errorf( - "failed to check if transaction is an unproven redemption "+ - "transaction: [%v]", - err, - ) - } - - if isUnproven { - unprovenRedemptionTransactions = append( - unprovenRedemptionTransactions, - transaction, - ) - } + }, + ) + if err != nil { + return nil, err } + + unprovenRedemptionTransactions = append( + unprovenRedemptionTransactions, + walletUnprovenTransactions..., + ) } return unprovenRedemptionTransactions, nil diff --git a/pkg/maintainer/spv/redemptions_test.go b/pkg/maintainer/spv/redemptions_test.go index 4f10a3a208..b2c1ea67fe 100644 --- a/pkg/maintainer/spv/redemptions_test.go +++ b/pkg/maintainer/spv/redemptions_test.go @@ -291,6 +291,8 @@ func TestGetUnprovenRedemptionTransactions(t *testing.T) { transactions, err := getUnprovenRedemptionTransactions( historyDepth, transactionLimit, + DefaultTransactionScanLimit, + newWalletTransactionScanner(), btcChain, spvChain, ) diff --git a/pkg/maintainer/spv/scanner.go b/pkg/maintainer/spv/scanner.go new file mode 100644 index 0000000000..b77cfa28e4 --- /dev/null +++ b/pkg/maintainer/spv/scanner.go @@ -0,0 +1,229 @@ +package spv + +import ( + "fmt" + "sort" + + "github.com/keep-network/keep-core/pkg/bitcoin" + "github.com/keep-network/keep-core/pkg/tbtc" +) + +// scanStateEvictAfterRounds is the number of maintainer rounds a scan key's +// state may sit unused before it is evicted. This bounds scanner memory to +// recently-relevant wallets rather than the lifetime total of every wallet +// ever seen. +const scanStateEvictAfterRounds = 100 + +// walletTransactionScanKey identifies one persistent wallet-transaction scan: +// which proof kind it serves, which wallet the scan is being performed on +// behalf of, which wallet's confirmed transaction history is being examined +// (for moving funds, this differs from the actor wallet - see +// getUnprovenMovingFundsTransactions), and, where the same actor/lookup pair +// could recur under different circumstances, a context hash distinguishing +// them (moving funds' ordered target-wallet commitment). +type walletTransactionScanKey struct { + actionType tbtc.WalletActionType + actorWalletPKH [20]byte + lookupWalletPKH [20]byte + contextHash [32]byte +} + +// walletTransactionScanState is the persisted progress for one scan key: a +// circular cursor into the examined wallet's confirmed transaction history, +// and the set of matching (unproven protocol) transactions found so far. +type walletTransactionScanState struct { + lastExaminedHash *bitcoin.Hash + candidates map[bitcoin.Hash]*bitcoin.Transaction + lastUsedRound uint64 +} + +// walletTransactionScanner bounds the per-tick cost of discovering unproven +// wallet transactions under TOB-TBTCACEXT-22's spam-resistant scan. Instead of +// re-walking a wallet's entire confirmed transaction history on every +// maintainer tick, it remembers a circular cursor per scan key and examines +// at most transactionScanLimit not-yet-classified hashes per call, resuming +// where the previous call on that key left off. The cursor wraps at the +// oldest hash, so given enough ticks every confirmed transaction is +// eventually re-examined - this matters because a transaction's +// unproven-match predicate is not permanently stable (for example, it can +// depend on the wallet's current main UTXO, which advances over time as +// earlier transactions are proven). Nothing is cached as a permanent +// negative; only current matches are cached, and they are re-tested every +// round so a just-proven transaction is dropped promptly. +// +// Not safe for concurrent use. The SPV maintainer drives all proof kinds +// sequentially from a single control-loop goroutine (see +// spvMaintainer.maintainSpv), so no locking is needed here. +type walletTransactionScanner struct { + states map[walletTransactionScanKey]*walletTransactionScanState + currentRound uint64 +} + +func newWalletTransactionScanner() *walletTransactionScanner { + return &walletTransactionScanner{ + states: make(map[walletTransactionScanKey]*walletTransactionScanState), + } +} + +// beginRound advances the scanner's round counter and evicts state for scan +// keys unused for scanStateEvictAfterRounds rounds. Call once per maintainSpv +// iteration, before processing any proof kind. +func (s *walletTransactionScanner) beginRound() { + s.currentRound++ + + for key, state := range s.states { + if s.currentRound-state.lastUsedRound > scanStateEvictAfterRounds { + delete(s.states, key) + } + } +} + +// getUnprovenWalletTransactions scans a wallet's confirmed Bitcoin transaction +// history and returns up to transactionLimit transactions that satisfy the +// given predicate, i.e. unproven protocol transactions of the given +// description (used only to phrase wrapped predicate errors). +// +// The full confirmed history is fetched every call (its ordering is this +// function's source of truth for the returned ascending-block-height order), +// but at most transactionScanLimit not-yet-classified hashes are freshly +// examined (one GetTransaction call plus a predicate check each); previously +// found matches are re-tested directly instead of being re-discovered. This +// keeps per-call cost bounded regardless of how much history - real or spam - +// the wallet address has accumulated, while still guaranteeing that a real +// protocol transaction can never be permanently evicted from discovery: the +// persisted cursor (scoped to key) advances across calls and wraps at the +// oldest hash, so it eventually covers the whole history no matter how much +// unrelated history precedes it. +func (s *walletTransactionScanner) getUnprovenWalletTransactions( + key walletTransactionScanKey, + lookupPublicKeyHash [20]byte, + transactionLimit int, + transactionScanLimit int, + description string, + btcChain bitcoin.Chain, + isUnproven func(*bitcoin.Transaction) (bool, error), +) ([]*bitcoin.Transaction, error) { + txHashes, err := btcChain.GetTxHashesForPublicKeyHash(lookupPublicKeyHash) + if err != nil { + return nil, fmt.Errorf( + "failed to get transaction hashes for wallet: [%v]", + err, + ) + } + + state, exists := s.states[key] + if !exists { + state = &walletTransactionScanState{ + candidates: make(map[bitcoin.Hash]*bitcoin.Transaction), + } + s.states[key] = state + } + state.lastUsedRound = s.currentRound + + // txHashes is documented to be in ascending block-height order; index it + // so the final result can be sorted back into that order regardless of + // the (unordered) map iteration used to collect candidates below, and so + // membership/resume-position checks below are O(1). + hashIndex := make(map[bitcoin.Hash]int, len(txHashes)) + for i, hash := range txHashes { + hashIndex[hash] = i + } + + // Drop candidates the confirmed history no longer contains, e.g. after a + // Bitcoin reorganization. + for hash := range state.candidates { + if _, present := hashIndex[hash]; !present { + delete(state.candidates, hash) + } + } + + // A candidate remains a candidate only while still unproven; re-test + // every round so a just-proven transaction is dropped promptly instead + // of lingering until the cursor happens to pass it again. + for hash, transaction := range state.candidates { + stillUnproven, err := isUnproven(transaction) + if err != nil { + return nil, fmt.Errorf( + "failed to check if transaction is an unproven %s "+ + "transaction: [%v]", + description, + err, + ) + } + if !stillUnproven { + delete(state.candidates, hash) + } + } + + if len(txHashes) > 0 && len(state.candidates) < transactionLimit { + // Resume just before the last examined hash, scanning newest to + // oldest. If there is no cursor yet, or its hash fell out of the + // confirmed history, start from the newest hash. + startIndex := len(txHashes) + if state.lastExaminedHash != nil { + if i, present := hashIndex[*state.lastExaminedHash]; present { + startIndex = i + } + } + + examinedNew := 0 + index := startIndex + // Bounding total iterations to len(txHashes) guarantees termination + // in a single wrap of the history regardless of how transactionLimit + // and transactionScanLimit relate to each other or to history size. + // Checking the scan budget in the loop condition (rather than as a + // post-work break) means a non-positive transactionScanLimit examines + // zero new hashes instead of one. + for steps := 0; steps < len(txHashes) && + examinedNew < transactionScanLimit; steps++ { + index-- + if index < 0 { + index = len(txHashes) - 1 + } + + hash := txHashes[index] + state.lastExaminedHash = &hash + + if _, isCandidate := state.candidates[hash]; isCandidate { + // Already classified as a matching candidate above; does not + // use this round's new-examination budget. + continue + } + + transaction, err := btcChain.GetTransaction(hash) + if err != nil { + return nil, fmt.Errorf("cannot get transaction: [%v]", err) + } + + matches, err := isUnproven(transaction) + if err != nil { + return nil, fmt.Errorf( + "failed to check if transaction is an unproven %s "+ + "transaction: [%v]", + description, + err, + ) + } + + if matches { + state.candidates[hash] = transaction + } + + examinedNew++ + if len(state.candidates) >= transactionLimit { + break + } + } + } + + matches := make([]*bitcoin.Transaction, 0, len(state.candidates)) + for _, transaction := range state.candidates { + matches = append(matches, transaction) + } + + sort.Slice(matches, func(i, j int) bool { + return hashIndex[matches[i].Hash()] < hashIndex[matches[j].Hash()] + }) + + return matches, nil +} diff --git a/pkg/maintainer/spv/scanner_test.go b/pkg/maintainer/spv/scanner_test.go new file mode 100644 index 0000000000..31b899291f --- /dev/null +++ b/pkg/maintainer/spv/scanner_test.go @@ -0,0 +1,335 @@ +package spv + +import ( + "testing" + + "github.com/keep-network/keep-core/internal/testutils" + "github.com/keep-network/keep-core/pkg/bitcoin" + "github.com/keep-network/keep-core/pkg/tbtc" +) + +// buildScannerTestWalletTx builds a distinct confirmed transaction paying to +// the given wallet address. The input outpoint index keeps each transaction's +// hash unique. +func buildScannerTestWalletTx( + walletScript bitcoin.Script, + index uint32, +) *bitcoin.Transaction { + return &bitcoin.Transaction{ + Version: 1, + Inputs: []*bitcoin.TransactionInput{ + { + Outpoint: &bitcoin.TransactionOutpoint{ + TransactionHash: bitcoin.Hash{}, + OutputIndex: index, + }, + Sequence: 0xffffffff, + }, + }, + Outputs: []*bitcoin.TransactionOutput{ + { + Value: 546, + PublicKeyScript: walletScript, + }, + }, + Locktime: 0, + } +} + +// TestWalletTransactionScanner_ReclassifiesOnceMatching verifies that a +// transaction whose predicate result is initially false is still found once +// the predicate later starts returning true for it - matching the real +// isUnproven* predicates, whose answer can depend on mutable wallet state +// (e.g. the current main UTXO) rather than being a permanent property of the +// transaction. The scanner must not cache the earlier negative result. +func TestWalletTransactionScanner_ReclassifiesOnceMatching(t *testing.T) { + btcChain := newLocalBitcoinChain() + walletPublicKeyHash := [20]byte{1, 2, 3} + walletScript, err := bitcoin.PayToWitnessPublicKeyHash(walletPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + transaction := buildScannerTestWalletTx(walletScript, 0) + if err := btcChain.BroadcastTransaction(transaction); err != nil { + t.Fatal(err) + } + transactionHash := transaction.Hash() + + matches := false + predicate := func(candidate *bitcoin.Transaction) (bool, error) { + return candidate.Hash() == transactionHash && matches, nil + } + + scanner := newWalletTransactionScanner() + key := walletTransactionScanKey{lookupWalletPKH: walletPublicKeyHash} + + scanner.beginRound() + found, err := scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(found) != 0 { + t.Fatalf("expected no matches before the predicate flips, got %d", len(found)) + } + + // Simulate the wallet's main UTXO advancing so the transaction now + // matches. + matches = true + + scanner.beginRound() + found, err = scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(found) != 1 || found[0].Hash() != transactionHash { + t.Fatal("expected the transaction to be found once it starts matching") + } +} + +// TestWalletTransactionScanner_DropsCandidateOnceProven verifies that a +// transaction previously reported as unproven stops being reported, without +// needing rediscovery, once its predicate starts returning false (i.e. once +// its SPV proof has been submitted). +func TestWalletTransactionScanner_DropsCandidateOnceProven(t *testing.T) { + btcChain := newLocalBitcoinChain() + walletPublicKeyHash := [20]byte{4, 5, 6} + walletScript, err := bitcoin.PayToWitnessPublicKeyHash(walletPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + transaction := buildScannerTestWalletTx(walletScript, 0) + if err := btcChain.BroadcastTransaction(transaction); err != nil { + t.Fatal(err) + } + transactionHash := transaction.Hash() + + unproven := true + predicate := func(candidate *bitcoin.Transaction) (bool, error) { + return candidate.Hash() == transactionHash && unproven, nil + } + + scanner := newWalletTransactionScanner() + key := walletTransactionScanKey{lookupWalletPKH: walletPublicKeyHash} + + scanner.beginRound() + found, err := scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(found) != 1 { + t.Fatalf("expected the transaction to be found as a candidate, got %d", len(found)) + } + + // Simulate the SPV proof having been submitted for this transaction. + unproven = false + + scanner.beginRound() + found, err = scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(found) != 0 { + t.Fatalf("expected the proven transaction to be dropped, got %d", len(found)) + } +} + +// TestWalletTransactionScanner_DropsCandidateOnReorg verifies that a +// previously-found candidate is dropped, without error, if a later call no +// longer sees its hash in the confirmed transaction history (e.g. after a +// Bitcoin reorganization). +func TestWalletTransactionScanner_DropsCandidateOnReorg(t *testing.T) { + btcChain := newLocalBitcoinChain() + walletPublicKeyHash := [20]byte{7, 8, 9} + walletScript, err := bitcoin.PayToWitnessPublicKeyHash(walletPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + transaction := buildScannerTestWalletTx(walletScript, 0) + if err := btcChain.BroadcastTransaction(transaction); err != nil { + t.Fatal(err) + } + transactionHash := transaction.Hash() + + predicate := func(candidate *bitcoin.Transaction) (bool, error) { + return candidate.Hash() == transactionHash, nil + } + + scanner := newWalletTransactionScanner() + key := walletTransactionScanKey{lookupWalletPKH: walletPublicKeyHash} + + scanner.beginRound() + found, err := scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(found) != 1 { + t.Fatalf("expected the transaction to be found as a candidate, got %d", len(found)) + } + + // Simulate a reorganization dropping the transaction from the confirmed + // history by replacing the chain's transaction list. + btcChain.mutex.Lock() + btcChain.transactions = nil + btcChain.mutex.Unlock() + + scanner.beginRound() + found, err = scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(found) != 0 { + t.Fatalf("expected the reorged-out candidate to be dropped, got %d", len(found)) + } +} + +// TestWalletTransactionScanner_IsolatesDistinctScanKeys verifies that two +// distinct scan keys over the same lookup wallet (e.g. two different action +// types, or - as with moving funds - two different context hashes) do not +// share cached state. +func TestWalletTransactionScanner_IsolatesDistinctScanKeys(t *testing.T) { + btcChain := newLocalBitcoinChain() + walletPublicKeyHash := [20]byte{10, 11, 12} + walletScript, err := bitcoin.PayToWitnessPublicKeyHash(walletPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + transaction := buildScannerTestWalletTx(walletScript, 0) + if err := btcChain.BroadcastTransaction(transaction); err != nil { + t.Fatal(err) + } + transactionHash := transaction.Hash() + + predicate := func(candidate *bitcoin.Transaction) (bool, error) { + return candidate.Hash() == transactionHash, nil + } + + scanner := newWalletTransactionScanner() + keyA := walletTransactionScanKey{ + actionType: tbtc.ActionDepositSweep, + lookupWalletPKH: walletPublicKeyHash, + } + keyB := walletTransactionScanKey{ + actionType: tbtc.ActionRedemption, + lookupWalletPKH: walletPublicKeyHash, + } + + scanner.beginRound() + if _, err := scanner.getUnprovenWalletTransactions( + keyA, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ); err != nil { + t.Fatal(err) + } + + if len(scanner.states) != 1 { + t.Fatalf("expected exactly one scan state after the first key, got %d", len(scanner.states)) + } + + scanner.beginRound() + if _, err := scanner.getUnprovenWalletTransactions( + keyB, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ); err != nil { + t.Fatal(err) + } + + if len(scanner.states) != 2 { + t.Fatalf("expected a second, independent scan state for the second key, got %d", len(scanner.states)) + } +} + +// TestWalletTransactionScanner_EvictsUnusedState verifies that a scan key's +// state is evicted after it goes unused for more than +// scanStateEvictAfterRounds rounds, bounding scanner memory to recently +// relevant wallets. +func TestWalletTransactionScanner_EvictsUnusedState(t *testing.T) { + btcChain := newLocalBitcoinChain() + walletPublicKeyHash := [20]byte{13, 14, 15} + + predicate := func(*bitcoin.Transaction) (bool, error) { + return false, nil + } + + scanner := newWalletTransactionScanner() + key := walletTransactionScanKey{lookupWalletPKH: walletPublicKeyHash} + + scanner.beginRound() + if _, err := scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, 20, 20, "test", btcChain, predicate, + ); err != nil { + t.Fatal(err) + } + + if len(scanner.states) != 1 { + t.Fatalf("expected the scan state to exist, got %d entries", len(scanner.states)) + } + + for i := 0; i < scanStateEvictAfterRounds+1; i++ { + scanner.beginRound() + } + + testutils.AssertIntsEqual( + t, + "scanner states after eviction window elapses", + 0, + len(scanner.states), + ) +} + +// TestWalletTransactionScanner_NonPositiveScanLimitExaminesNothing verifies +// that a zero or negative transactionScanLimit examines zero new hashes +// (rather than one, which a post-work budget check would otherwise allow), +// so a misconfigured limit cannot cause even a single unbounded-cost +// GetTransaction call. +func TestWalletTransactionScanner_NonPositiveScanLimitExaminesNothing(t *testing.T) { + btcChain := newLocalBitcoinChain() + walletPublicKeyHash := [20]byte{16, 17, 18} + walletScript, err := bitcoin.PayToWitnessPublicKeyHash(walletPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + transaction := buildScannerTestWalletTx(walletScript, 0) + if err := btcChain.BroadcastTransaction(transaction); err != nil { + t.Fatal(err) + } + transactionHash := transaction.Hash() + + examined := 0 + predicate := func(candidate *bitcoin.Transaction) (bool, error) { + examined++ + return candidate.Hash() == transactionHash, nil + } + + scanner := newWalletTransactionScanner() + key := walletTransactionScanKey{lookupWalletPKH: walletPublicKeyHash} + + for _, scanLimit := range []int{0, -1} { + scanner.beginRound() + found, err := scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, 20, scanLimit, "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(found) != 0 { + t.Fatalf("scanLimit=%d: expected no matches, got %d", scanLimit, len(found)) + } + } + + testutils.AssertIntsEqual(t, "predicate calls with a non-positive scan limit", 0, examined) +} diff --git a/pkg/maintainer/spv/spv.go b/pkg/maintainer/spv/spv.go index 990d8b0ec0..ed1885fee3 100644 --- a/pkg/maintainer/spv/spv.go +++ b/pkg/maintainer/spv/spv.go @@ -30,10 +30,11 @@ func Initialize( btcChain bitcoin.Chain, ) { spvMaintainer := &spvMaintainer{ - config: config, - spvChain: spvChain, - btcDiffChain: btcDiffChain, - btcChain: btcChain, + config: config, + spvChain: spvChain, + btcDiffChain: btcDiffChain, + btcChain: btcChain, + transactionScanner: newWalletTransactionScanner(), } go spvMaintainer.startControlLoop(ctx) @@ -92,10 +93,11 @@ var proofTypes = map[tbtc.WalletActionType]struct { } type spvMaintainer struct { - config Config - spvChain Chain - btcDiffChain btcdiff.Chain - btcChain bitcoin.Chain + config Config + spvChain Chain + btcDiffChain btcdiff.Chain + btcChain bitcoin.Chain + transactionScanner *walletTransactionScanner } func (sm *spvMaintainer) startControlLoop(ctx context.Context) { @@ -124,6 +126,8 @@ func (sm *spvMaintainer) startControlLoop(ctx context.Context) { func (sm *spvMaintainer) maintainSpv(ctx context.Context) error { for { + sm.transactionScanner.beginRound() + for action, v := range proofTypes { logger.Infof("starting [%s] proof task execution...", action) @@ -159,6 +163,8 @@ func (sm *spvMaintainer) maintainSpv(ctx context.Context) error { type unprovenTransactionsGetter func( historyDepth uint64, transactionLimit int, + transactionScanLimit int, + scanner *walletTransactionScanner, btcChain bitcoin.Chain, spvChain Chain, ) ( @@ -185,6 +191,8 @@ func (sm *spvMaintainer) proveTransactions( transactions, err := unprovenTransactionsGetter( sm.config.HistoryDepth, sm.config.TransactionLimit, + sm.config.TransactionScanLimit, + sm.transactionScanner, sm.btcChain, sm.spvChain, ) diff --git a/pkg/maintainer/spv/spv_test.go b/pkg/maintainer/spv/spv_test.go index 94c2084d11..698874ed58 100644 --- a/pkg/maintainer/spv/spv_test.go +++ b/pkg/maintainer/spv/spv_test.go @@ -324,3 +324,137 @@ func TestIsInputCurrentWalletsMainUTXO(t *testing.T) { }) } } + +// TestGetUnprovenWalletTransactions_FindsProtocolTransactionBehindSpam verifies +// that a real protocol transaction is still discovered even when more than +// transactionScanLimit unrelated (spam) transactions are paid to the same +// wallet address after it - it just takes as many scanner rounds as it takes +// to walk past the spam, rather than being permanently evicted. This is the +// shared discovery path used by all SPV scanners, so a bounded tail lookup +// would let the spam evict the protocol transaction forever. +func TestGetUnprovenWalletTransactions_FindsProtocolTransactionBehindSpam(t *testing.T) { + btcChain := newLocalBitcoinChain() + + walletPublicKeyHash := [20]byte{ + 0x8d, 0xb5, 0x0e, 0xb5, 0x20, 0x63, 0xea, 0x9d, 0x98, 0xb3, + 0xea, 0xc9, 0x14, 0x89, 0xa9, 0x0f, 0x73, 0x89, 0x86, 0xf6, + } + walletScript, err := bitcoin.PayToWitnessPublicKeyHash(walletPublicKeyHash) + if err != nil { + t.Fatal(err) + } + + // buildWalletTx builds a distinct confirmed transaction paying to the wallet + // address. The output count distinguishes the protocol transaction (a single + // output, as the deposit sweep scanner requires) from spam, and the input + // outpoint index keeps each transaction unique. + buildWalletTx := func(index uint32, outputCount int) *bitcoin.Transaction { + outputs := make([]*bitcoin.TransactionOutput, outputCount) + for i := range outputs { + outputs[i] = &bitcoin.TransactionOutput{ + Value: 546, + PublicKeyScript: walletScript, + } + } + return &bitcoin.Transaction{ + Version: 1, + Inputs: []*bitcoin.TransactionInput{ + { + Outpoint: &bitcoin.TransactionOutpoint{ + TransactionHash: bitcoin.Hash{}, + OutputIndex: index, + }, + Sequence: 0xffffffff, + }, + }, + Outputs: outputs, + Locktime: 0, + } + } + + // The protocol transaction is the oldest transaction paying to the wallet. + protocolTransaction := buildWalletTx(0, 1) + if err := btcChain.BroadcastTransaction(protocolTransaction); err != nil { + t.Fatal(err) + } + + // Add many unrelated transactions after the protocol one, well beyond the + // per-round scan limit, each paying to the same wallet address. + transactionLimit := 5 + transactionScanLimit := 4 + spamCount := transactionScanLimit * 6 + for i := 0; i < spamCount; i++ { + spamTransaction := buildWalletTx(uint32(i+1), 2) + if err := btcChain.BroadcastTransaction(spamTransaction); err != nil { + t.Fatal(err) + } + } + + protocolTransactionHash := protocolTransaction.Hash() + + scanner := newWalletTransactionScanner() + key := walletTransactionScanKey{lookupWalletPKH: walletPublicKeyHash} + predicate := func(transaction *bitcoin.Transaction) (bool, error) { + return transaction.Hash() == protocolTransactionHash, nil + } + + // A single round with a scan limit far smaller than the spam count must + // not find the protocol transaction yet - it is still buried beyond this + // round's examination budget. + scanner.beginRound() + found, err := scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, transactionLimit, transactionScanLimit, + "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(found) != 0 { + t.Fatalf( + "expected the protocol transaction to still be undiscovered "+ + "after one bounded round, got %d match(es)", + len(found), + ) + } + + // Repeated rounds (simulating repeated maintainer ticks) must eventually + // walk the cursor past all the spam and discover the protocol + // transaction - it must never be permanently evicted. + maxRounds := (spamCount / transactionScanLimit) + 2 + for round := 0; round < maxRounds && len(found) == 0; round++ { + scanner.beginRound() + found, err = scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, transactionLimit, transactionScanLimit, + "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + } + + if len(found) != 1 { + t.Fatalf( + "expected exactly one unproven transaction within %d rounds, got %d", + maxRounds, len(found), + ) + } + if found[0].Hash() != protocolTransactionHash { + t.Fatal( + "expected the protocol transaction to be discovered behind the spam", + ) + } + + // Once discovered, the transaction must stay reported on subsequent + // rounds instead of needing to be rediscovered. + scanner.beginRound() + foundAgain, err := scanner.getUnprovenWalletTransactions( + key, walletPublicKeyHash, transactionLimit, transactionScanLimit, + "test", btcChain, predicate, + ) + if err != nil { + t.Fatal(err) + } + if len(foundAgain) != 1 || foundAgain[0].Hash() != protocolTransactionHash { + t.Fatal("expected the protocol transaction to remain a candidate") + } +} diff --git a/pkg/net/retransmission/strategy.go b/pkg/net/retransmission/strategy.go index fd50384fb2..585824addf 100644 --- a/pkg/net/retransmission/strategy.go +++ b/pkg/net/retransmission/strategy.go @@ -1,6 +1,10 @@ package retransmission -import "github.com/keep-network/keep-core/pkg/net" +import ( + "sync" + + "github.com/keep-network/keep-core/pkg/net" +) // Strategy represents a specific retransmission strategy. type Strategy interface { @@ -44,6 +48,10 @@ func (ss *StandardStrategy) Tick(retransmitFn RetransmitFn) error { // ticks, between third and fourth is 4 ticks and so on. Graphically, the // schedule looks as follows: R _ R _ _ R _ _ _ _ R _ _ _ _ _ _ _ _ R type BackoffStrategy struct { + // mutex guards the retransmission counters below. ScheduleRetransmissions + // invokes Tick from a new goroutine on every tick, so overlapping ticks can + // call Tick concurrently on the same strategy instance. + mutex sync.Mutex tickCounter uint64 delay uint64 retransmitTick uint64 @@ -61,12 +69,23 @@ func WithBackoffStrategy() *BackoffStrategy { // Tick implements the Strategy.Tick function. func (bos *BackoffStrategy) Tick(retransmitFn RetransmitFn) error { + // Update the retransmission counters under the mutex so that concurrent + // Tick calls do not race on them. The decision is captured in a local and + // retransmitFn is invoked after releasing the lock, preserving the original + // behavior of not holding shared state while the message is retransmitted. + bos.mutex.Lock() + bos.tickCounter++ - if bos.tickCounter == bos.retransmitTick { + shouldRetransmit := bos.tickCounter == bos.retransmitTick + if shouldRetransmit { bos.retransmitTick += bos.delay + 1 bos.delay *= 2 + } + + bos.mutex.Unlock() + if shouldRetransmit { return retransmitFn() } diff --git a/pkg/net/retransmission/strategy_test.go b/pkg/net/retransmission/strategy_test.go index 2c4b261f5f..97f685f905 100644 --- a/pkg/net/retransmission/strategy_test.go +++ b/pkg/net/retransmission/strategy_test.go @@ -2,6 +2,7 @@ package retransmission import ( "reflect" + "sync" "testing" ) @@ -43,6 +44,37 @@ func TestStandardStrategy(t *testing.T) { } } +// TestBackoffStrategy_ConcurrentTick verifies that BackoffStrategy.Tick is safe +// to call concurrently, as ScheduleRetransmissions does by invoking Tick from a +// new goroutine on every tick. It is meant to be run with the race detector +// (go test -race); without the mutex guarding the retransmission counters, the +// concurrent read/modify/write of tickCounter, delay, and retransmitTick is a +// data race. +func TestBackoffStrategy_ConcurrentTick(t *testing.T) { + strategy := WithBackoffStrategy() + + const goroutines = 50 + + // A barrier releases all goroutines at once to maximize the overlap of the + // counter updates inside Tick. + start := make(chan struct{}) + var wg sync.WaitGroup + wg.Add(goroutines) + + for i := 0; i < goroutines; i++ { + go func() { + defer wg.Done() + <-start + _ = strategy.Tick(func() error { + return nil + }) + }() + } + + close(start) + wg.Wait() +} + func TestBackoffStrategy(t *testing.T) { strategy := WithBackoffStrategy() diff --git a/pkg/tbtc/covenant_signer.go b/pkg/tbtc/covenant_signer.go index b4f6454e78..2aa7e1921a 100644 --- a/pkg/tbtc/covenant_signer.go +++ b/pkg/tbtc/covenant_signer.go @@ -23,8 +23,26 @@ import ( type covenantSignerEngine struct { node *node minimumActiveOutpointConfirmations uint + // bridgeFraudDefenseConfirmed records the operator's explicit confirmation + // that the tBTC Bridge recognizes covenant active UTXO spends as honest + // spends, i.e. the covenant fraud-defense path is deployed. Until it is set, + // the engine refuses to produce covenant signatures because a covenant + // SIGHASH_ALL signature over a covenant active UTXO is otherwise a valid, + // undefeatable tBTC fraud proof against the signing wallet. + bridgeFraudDefenseConfirmed bool } +// Compile-time assertions that covenantSignerEngine satisfies the full +// covenant signer contract, including CurrentBlockHeightProvider. Signer +// approval certificate expiry enforcement depends on every verifier-capable +// engine also providing a current block height; losing this interface +// silently would make certificates never expire. +var ( + _ covenantsigner.Engine = (*covenantSignerEngine)(nil) + _ covenantsigner.SignerApprovalVerifier = (*covenantSignerEngine)(nil) + _ covenantsigner.CurrentBlockHeightProvider = (*covenantSignerEngine)(nil) +) + // defaultMinActiveOutpointConfirmations is the confirmation threshold applied // when the operator config does not specify a custom value. It aligns with // DepositSweepRequiredFundingTxConfirmations to ensure consistent reorg safety @@ -55,7 +73,15 @@ type qcV1SignerHandoff struct { // newCovenantSignerEngine creates a covenant signer engine bound to the given // node. When minConfirmations is zero (the Go zero-value produced by an unset // config field), defaultMinActiveOutpointConfirmations is used. -func newCovenantSignerEngine(node *node, minConfirmations uint) covenantsigner.Engine { +// +// bridgeFraudDefenseConfirmed must be set only when the operator has confirmed +// that the tBTC Bridge covenant fraud-defense path is deployed. When false (the +// default), the engine fails closed and refuses to produce covenant signatures. +func newCovenantSignerEngine( + node *node, + minConfirmations uint, + bridgeFraudDefenseConfirmed bool, +) covenantsigner.Engine { if minConfirmations == 0 { minConfirmations = defaultMinActiveOutpointConfirmations } @@ -63,6 +89,7 @@ func newCovenantSignerEngine(node *node, minConfirmations uint) covenantsigner.E return &covenantSignerEngine{ node: node, minimumActiveOutpointConfirmations: minConfirmations, + bridgeFraudDefenseConfirmed: bridgeFraudDefenseConfirmed, } } @@ -145,6 +172,23 @@ func (cse *covenantSignerEngine) VerifySignerApproval( return err } + // Fail closed for wallets that are not in a state eligible for covenant + // signing. The signer set hash embedded in a certificate binds only the + // wallet identity, members hash, and threshold, none of which change when a + // wallet is closed or terminated, so a certificate issued while the wallet + // was live would otherwise keep verifying after closure. Rejecting + // non-eligible states here ensures a wallet the closure path intended to + // deauthorize cannot be made to sign a covenant transaction. + if !isCovenantSigningEligibleState(walletChainData.State) { + return covenantsigner.NewInputError( + fmt.Sprintf( + "request.signerApproval.walletPublicKey resolves to a wallet in "+ + "state [%v] that is not eligible for covenant signing", + walletChainData.State, + ), + ) + } + expectedSignerSetHash, err := computeSignerApprovalCertificateSignerSetHash( signerPublicKey, walletChainData, @@ -175,6 +219,16 @@ func (cse *covenantSignerEngine) VerifySignerApproval( return nil } +// isCovenantSigningEligibleState reports whether a wallet in the given state is +// eligible to receive covenant signatures. Covenant migrations are only +// expected for live wallets, so covenant signing fails closed for every other +// state (including closed and terminated wallets that the closure path intends +// to deauthorize). If covenant signing must be allowed for another state in the +// future, add it here explicitly together with justification and tests. +func isCovenantSigningEligibleState(state WalletState) bool { + return state == StateLive +} + func (cse *covenantSignerEngine) resolveSignerApprovalTemplatePublicKey( request covenantsigner.RouteSubmitRequest, ) (*ecdsa.PublicKey, error) { @@ -200,6 +254,26 @@ func (cse *covenantSignerEngine) OnSubmit( ctx context.Context, job *covenantsigner.Job, ) (*covenantsigner.Transition, error) { + // Fail closed unless the operator has confirmed the tBTC Bridge covenant + // fraud-defense path is deployed. Producing a covenant SIGHASH_ALL + // signature over a covenant active UTXO exposes the signing wallet to a + // tBTC fraud challenge it cannot defeat, because the Bridge does not + // recognize a covenant active UTXO spend as an honest spend in + // Fraud.defeatFraudChallenge; only a swept deposit, a spent main UTXO, or a + // processed moved-funds sweep can defeat the challenge. The complete + // remediation is the bridge-side covenant fraud-defense path. Until it is + // deployed and confirmed here, refuse to sign so a valid migration + // signature cannot become a slashable wallet signature. + if !cse.bridgeFraudDefenseConfirmed { + return failedTransition( + covenantsigner.ReasonPolicyRejected, + "covenant signing is disabled until the tBTC Bridge covenant "+ + "fraud-defense path is confirmed deployed; a covenant signature "+ + "would otherwise expose the wallet to an undefeatable fraud "+ + "challenge", + ), nil + } + switch job.Route { case covenantsigner.TemplateSelfV1: return cse.submitSelfV1(ctx, job), nil @@ -221,6 +295,21 @@ func (cse *covenantSignerEngine) OnPoll( return nil, nil } +// CurrentBlockHeight returns the current height of the host chain (e.g. +// Ethereum), obtained through the same node chain connection the signing +// executors use. It is deliberately the host chain, not cse.node.btcChain: +// signer approval certificate EndBlock values are defined in host-chain block +// units so expiry can be enforced independently of Bitcoin's slower, +// reorg-prone confirmation times. +func (cse *covenantSignerEngine) CurrentBlockHeight(context.Context) (uint64, error) { + blockCounter, err := cse.node.chain.BlockCounter() + if err != nil { + return 0, fmt.Errorf("cannot get host chain block counter: %w", err) + } + + return blockCounter.CurrentBlock() +} + func (cse *covenantSignerEngine) submitSelfV1( ctx context.Context, job *covenantsigner.Job, @@ -548,8 +637,7 @@ func (cse *covenantSignerEngine) resolveActiveUtxo( return nil, fmt.Errorf("active outpoint output index is out of range") } - expectedWitnessScriptHash := bitcoin.WitnessScriptHash(witnessScript) - expectedScriptPubKey, err := bitcoin.PayToWitnessScriptHash(expectedWitnessScriptHash) + expectedScriptPubKey, err := payToWitnessScriptHash(witnessScript) if err != nil { return nil, fmt.Errorf( "cannot build expected %s locking script: %v", @@ -750,10 +838,23 @@ func (cse *covenantSignerEngine) buildCovenantTransactionBuilder( activeUtxo *bitcoin.UnspentTransactionOutput, witnessScript bitcoin.Script, ) (*bitcoin.TransactionBuilder, error) { - destinationScript, err := decodePrefixedHex(request.MigrationDestination.DepositScript) + destinationDepositScript, err := decodePrefixedHex(request.MigrationDestination.DepositScript) if err != nil { return nil, fmt.Errorf("migration destination deposit script is invalid") } + if len(destinationDepositScript) == 0 { + return nil, fmt.Errorf("migration destination deposit script must not be empty") + } + // MigrationDestination.DepositScript is the plain tBTC deposit script, not a + // ready-made output script. The Bitcoin funding output must pay to its P2WSH + // script hash (OP_0 ), which is how the tBTC Bridge + // rebuilds and verifies the funding output in revealDepositWithExtraData. + // Using the plain deposit script directly as the output script would make + // the migration deposit unrevealable to the Bridge. + destinationScriptPubKey, err := payToWitnessScriptHash(destinationDepositScript) + if err != nil { + return nil, fmt.Errorf("cannot build migration destination locking script: %v", err) + } destinationValue, err := toBitcoinOutputValue( request.MigrationTransactionPlan.DestinationValueSats, "migration destination value", @@ -779,7 +880,7 @@ func (cse *covenantSignerEngine) buildCovenantTransactionBuilder( builder.SetLocktime(request.MigrationTransactionPlan.LockTime) builder.AddOutput(&bitcoin.TransactionOutput{ Value: destinationValue, - PublicKeyScript: destinationScript, + PublicKeyScript: destinationScriptPubKey, }) anchorScript, err := canonicalAnchorScriptPubKey() @@ -794,6 +895,15 @@ func (cse *covenantSignerEngine) buildCovenantTransactionBuilder( return builder, nil } +// signCovenantTransactionInput produces the wallet's tECDSA signature over the +// single covenant input of the migration transaction. +// +// This signature is a normal Bitcoin SIGHASH_ALL signature over a covenant +// active UTXO. A covenant active UTXO is neither a swept deposit, a spent main +// UTXO, nor a processed moved-funds sweep, so the tBTC Bridge cannot defeat a +// fraud challenge that replays this signature. Callers must therefore only +// reach this path once the bridge-side covenant fraud-defense has been +// confirmed deployed; OnSubmit enforces that fail-closed gate. func signCovenantTransactionInput( ctx context.Context, signingExecutor *signingExecutor, @@ -884,8 +994,15 @@ func (handoff *qcV1SignerHandoff) toMap() map[string]any { } func canonicalAnchorScriptPubKey() (bitcoin.Script, error) { - witnessScriptHash := bitcoin.WitnessScriptHash(bitcoin.Script{txscript.OP_TRUE}) - return bitcoin.PayToWitnessScriptHash(witnessScriptHash) + return payToWitnessScriptHash(bitcoin.Script{txscript.OP_TRUE}) +} + +// payToWitnessScriptHash derives the P2WSH locking script +// (OP_0 ) that pays to the given witness script. This is how +// the tBTC Bridge matches an output against a script it independently +// recomputes; callers wrap the returned error with their own context. +func payToWitnessScriptHash(script bitcoin.Script) (bitcoin.Script, error) { + return bitcoin.PayToWitnessScriptHash(bitcoin.WitnessScriptHash(script)) } func decodePrefixedHex(value string) ([]byte, error) { diff --git a/pkg/tbtc/covenant_signer_test.go b/pkg/tbtc/covenant_signer_test.go index c7794f98ca..67ef7a6846 100644 --- a/pkg/tbtc/covenant_signer_test.go +++ b/pkg/tbtc/covenant_signer_test.go @@ -83,7 +83,7 @@ func TestCovenantSignerEngine_SubmitSelfV1Ready(t *testing.T) { service, err := covenantsigner.NewService( newCovenantSignerMemoryHandle(), - newCovenantSignerEngine(node, 0), + newCovenantSignerEngine(node, 0, true), ) if err != nil { t.Fatal(err) @@ -250,9 +250,21 @@ func TestCovenantSignerEngine_SubmitSelfV1Ready(t *testing.T) { if transaction.Outputs[0].Value != int64(destinationValueSats) { t.Fatalf("unexpected destination value: %d", transaction.Outputs[0].Value) } - if !bytes.Equal(transaction.Outputs[0].PublicKeyScript, destinationScript) { + // The destination output must pay to the P2WSH script hash of the deposit + // script (OP_0 ), not to the plain deposit script + // itself; otherwise the migration deposit is unrevealable to the tBTC Bridge. + expectedDestinationScript, err := bitcoin.PayToWitnessScriptHash( + bitcoin.WitnessScriptHash(destinationScript), + ) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(transaction.Outputs[0].PublicKeyScript, expectedDestinationScript) { t.Fatal("unexpected destination output script") } + if bytes.Equal(transaction.Outputs[0].PublicKeyScript, destinationScript) { + t.Fatal("destination output must not be the raw deposit script") + } expectedAnchorScript, err := canonicalAnchorScriptPubKey() if err != nil { @@ -318,7 +330,7 @@ func TestCovenantSignerEngine_SubmitQcV1HandoffReady(t *testing.T) { service, err := covenantsigner.NewService( newCovenantSignerMemoryHandle(), - newCovenantSignerEngine(node, 0), + newCovenantSignerEngine(node, 0, true), ) if err != nil { t.Fatal(err) @@ -517,9 +529,21 @@ func TestCovenantSignerEngine_SubmitQcV1HandoffReady(t *testing.T) { if unsignedTransaction.Outputs[0].Value != int64(destinationValueSats) { t.Fatalf("unexpected destination value: %d", unsignedTransaction.Outputs[0].Value) } - if !bytes.Equal(unsignedTransaction.Outputs[0].PublicKeyScript, destinationScript) { + // The destination output must pay to the P2WSH script hash of the deposit + // script (OP_0 ), not to the plain deposit script + // itself; otherwise the migration deposit is unrevealable to the tBTC Bridge. + expectedDestinationScript, err := bitcoin.PayToWitnessScriptHash( + bitcoin.WitnessScriptHash(destinationScript), + ) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(unsignedTransaction.Outputs[0].PublicKeyScript, expectedDestinationScript) { t.Fatal("unexpected destination output script") } + if bytes.Equal(unsignedTransaction.Outputs[0].PublicKeyScript, destinationScript) { + t.Fatal("destination output must not be the raw deposit script") + } expectedAnchorScript, err := canonicalAnchorScriptPubKey() if err != nil { @@ -605,7 +629,7 @@ func TestCovenantSignerEngine_SubmitQcV1RejectsInvalidBeta(t *testing.T) { service, err := covenantsigner.NewService( newCovenantSignerMemoryHandle(), - newCovenantSignerEngine(node, 0), + newCovenantSignerEngine(node, 0, true), ) if err != nil { t.Fatal(err) @@ -721,7 +745,7 @@ func TestCovenantSignerEngine_SubmitQcV1RejectsScriptHashMismatch(t *testing.T) service, err := covenantsigner.NewService( newCovenantSignerMemoryHandle(), - newCovenantSignerEngine(node, 0), + newCovenantSignerEngine(node, 0, true), ) if err != nil { t.Fatal(err) @@ -867,7 +891,7 @@ func TestCovenantSignerEngine_SubmitSelfV1RejectsZeroMaturityHeight(t *testing.T service, err := covenantsigner.NewService( newCovenantSignerMemoryHandle(), - newCovenantSignerEngine(node, 0), + newCovenantSignerEngine(node, 0, true), ) if err != nil { t.Fatal(err) @@ -1009,6 +1033,150 @@ func TestCovenantSignerEngine_EnsureActiveOutpointFinalityRejectsUnconfirmed(t * } } +func TestNode_InvalidateSigningExecutorEvictsCachedExecutorOnArchival(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + + // setupCovenantSignerTestNode already created and cached a signing executor + // for the wallet. Confirm it is served from the cache. + executor, ok, err := node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("expected the node to control the wallet before archival") + } + + // Archive the wallet, removing it from the wallet registry, as the wallet + // closure path does. + walletPublicKeyHash := bitcoin.PublicKeyHash(walletPublicKey) + if err := node.walletRegistry.archiveWallet(walletPublicKeyHash); err != nil { + t.Fatal(err) + } + + // Before invalidation, getSigningExecutor still returns the stale cached + // executor even though the wallet is no longer in the registry -- this is + // the behavior the fix must prevent. + staleExecutor, ok, err := node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if !ok || staleExecutor != executor { + t.Fatal("expected the stale cached executor to still be present before invalidation") + } + + // Invalidating the signing executor (as handleWalletClosure does after + // archival) evicts the cached executor, so the archived wallet is no longer + // signable through it. + if err := node.invalidateSigningExecutor(walletPublicKey); err != nil { + t.Fatal(err) + } + + _, ok, err = node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if ok { + t.Fatal("expected the archived wallet to no longer be signable after executor invalidation") + } +} + +func TestArchiveClosedWallets_SkipsWalletNotYetRegisteredOnChain(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + + localChain, ok := node.chain.(*localChain) + if !ok { + t.Fatal("expected local chain implementation") + } + + // Simulate a freshly generated wallet that is present in the local wallet + // registry but not yet recorded on-chain by the Bridge, i.e. still inside + // the DKG approval window. + walletPublicKeyHash := bitcoin.PublicKeyHash(walletPublicKey) + localChain.walletsMutex.Lock() + delete(localChain.wallets, walletPublicKeyHash) + localChain.walletsMutex.Unlock() + + if err := node.archiveClosedWallets(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // The pending wallet must be left in place, not archived. + if len(node.walletRegistry.getSigners(walletPublicKey)) == 0 { + t.Fatal("expected the not-yet-registered wallet to be left in place, but it was archived") + } +} + +func TestArchiveClosedWallets_ArchivesClosedWallet(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + + localChain, ok := node.chain.(*localChain) + if !ok { + t.Fatal("expected local chain implementation") + } + + // Transition the on-chain wallet to the closed state, keeping its identity. + walletPublicKeyHash := bitcoin.PublicKeyHash(walletPublicKey) + existing, err := localChain.GetWallet(walletPublicKeyHash) + if err != nil { + t.Fatal(err) + } + closed := *existing + closed.State = StateClosed + localChain.setWallet(walletPublicKeyHash, &closed) + + if err := node.archiveClosedWallets(); err != nil { + t.Fatalf("unexpected error: %v", err) + } + + // The closed wallet must be archived. + if len(node.walletRegistry.getSigners(walletPublicKey)) != 0 { + t.Fatal("expected the closed wallet to be archived, but it is still present") + } +} + +// TestCovenantSignerEngine_OnSubmitFailsClosedWithoutBridgeFraudDefense verifies +// that the engine refuses to produce covenant signatures unless the operator has +// confirmed the tBTC Bridge covenant fraud-defense path is deployed. A covenant +// signature would otherwise expose the signing wallet to an undefeatable fraud +// challenge. +func TestCovenantSignerEngine_OnSubmitFailsClosedWithoutBridgeFraudDefense(t *testing.T) { + node, _, _ := setupCovenantSignerTestNode(t) + + // Construct the engine with the default (fail-closed) configuration: the + // bridge covenant fraud-defense path is not confirmed deployed. + engine := newCovenantSignerEngine(node, 0, false) + + for _, route := range []covenantsigner.TemplateID{ + covenantsigner.TemplateSelfV1, + covenantsigner.TemplateQcV1, + } { + transition, err := engine.OnSubmit( + context.Background(), + &covenantsigner.Job{Route: route}, + ) + if err != nil { + t.Fatalf("[%s] unexpected error: %v", route, err) + } + if transition == nil { + t.Fatalf("[%s] expected a transition", route) + } + if transition.State != covenantsigner.JobStateFailed { + t.Fatalf( + "[%s] expected a failed transition, got state [%v]", + route, + transition.State, + ) + } + if transition.Reason != covenantsigner.ReasonPolicyRejected { + t.Fatalf( + "[%s] expected a policy rejection, got reason [%v]", + route, + transition.Reason, + ) + } + } +} + func setupCovenantSignerTestNode( t *testing.T, ) (*node, *localBitcoinChain, *ecdsa.PublicKey) { @@ -1329,10 +1497,17 @@ func applyTestArtifactApprovals( t.Fatal(err) } + // Generously far in the future: these submit-flow tests exercise + // certificate expiry enforcement incidentally (through the real engine's + // CurrentBlockHeight wiring), and only care that the certificate is + // valid, not about a specific expiry block. + requestedEndBlock := startBlock + 100000 + signerApproval, err := executor.issueSignerApprovalCertificate( context.Background(), testArtifactApprovalDigest(t, payload), startBlock, + requestedEndBlock, ) if err != nil { t.Fatal(err) @@ -1380,7 +1555,9 @@ func TestCovenantSignerEngine_OnPollReturnsNoTransition(t *testing.T) { } func TestCovenantSignerEngine_SubmitRejectsUnsupportedRoute(t *testing.T) { - transition, err := (&covenantSignerEngine{}).OnSubmit( + // Confirm the bridge fraud-defense so OnSubmit reaches route validation + // rather than the fail-closed gate. + transition, err := (&covenantSignerEngine{bridgeFraudDefenseConfirmed: true}).OnSubmit( context.Background(), &covenantsigner.Job{ Route: covenantsigner.TemplateID("unsupported_route"), @@ -1403,10 +1580,49 @@ func TestCovenantSignerEngine_SubmitRejectsUnsupportedRoute(t *testing.T) { } } +// TestCovenantSignerEngine_CurrentBlockHeightUsesNodeHostChain verifies that +// the production covenantSignerEngine.CurrentBlockHeight is backed by +// node.chain.BlockCounter() (the host/Ethereum chain), matching whatever the +// same block counter reports directly -- never node.btcChain. +func TestCovenantSignerEngine_CurrentBlockHeightUsesNodeHostChain(t *testing.T) { + node, _, _ := setupCovenantSignerTestNode(t) + + engine := newCovenantSignerEngine(node, 0, true) + cse, ok := engine.(*covenantSignerEngine) + if !ok { + t.Fatal("expected engine to be *covenantSignerEngine") + } + + var provider covenantsigner.CurrentBlockHeightProvider = cse + height, err := provider.CurrentBlockHeight(context.Background()) + if err != nil { + t.Fatal(err) + } + + blockCounter, err := node.chain.BlockCounter() + if err != nil { + t.Fatal(err) + } + expectedHeight, err := blockCounter.CurrentBlock() + if err != nil { + t.Fatal(err) + } + + // The two reads are not atomic with each other, so tolerate the local + // block counter having ticked forward by exactly one block in between. + if height != expectedHeight && height != expectedHeight+1 { + t.Fatalf( + "expected CurrentBlockHeight [%d] to match node.chain.BlockCounter() [%d] (+/- 1 tick)", + height, + expectedHeight, + ) + } +} + func TestNewCovenantSignerEngine_DefaultMinConfirmations(t *testing.T) { node, _, _ := setupCovenantSignerTestNode(t) - engine := newCovenantSignerEngine(node, 0) + engine := newCovenantSignerEngine(node, 0, true) cse, ok := engine.(*covenantSignerEngine) if !ok { @@ -1424,7 +1640,7 @@ func TestNewCovenantSignerEngine_DefaultMinConfirmations(t *testing.T) { func TestNewCovenantSignerEngine_ExplicitMinConfirmations(t *testing.T) { node, _, _ := setupCovenantSignerTestNode(t) - engine := newCovenantSignerEngine(node, 3) + engine := newCovenantSignerEngine(node, 3, true) cse, ok := engine.(*covenantSignerEngine) if !ok { @@ -1598,3 +1814,78 @@ func TestComputeQcV1SignerHandoffPayloadHash_DeterministicKeyOrdering(t *testing ) } } + +// TestCovenantSignerEngine_VerifySignerApprovalRejectsNonLiveWallet verifies +// that a signer approval certificate which is otherwise valid (matching the +// wallet identity and members hash) is rejected once the wallet leaves the live +// state, so a closed or terminated wallet cannot be made to sign a covenant +// transaction. +func TestCovenantSignerEngine_VerifySignerApprovalRejectsNonLiveWallet(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + + localChain, ok := node.chain.(*localChain) + if !ok { + t.Fatal("expected local chain implementation") + } + + depositorPrivateKey, _ := btcec.PrivKeyFromBytes(btcec.S256(), bytes.Repeat([]byte{0x42}, 32)) + depositorPublicKey := depositorPrivateKey.PubKey().SerializeCompressed() + signerPublicKey := (*btcec.PublicKey)(walletPublicKey).SerializeCompressed() + + template := &covenantsigner.SelfV1Template{ + Template: covenantsigner.TemplateSelfV1, + DepositorPublicKey: "0x" + hex.EncodeToString(depositorPublicKey), + SignerPublicKey: "0x" + hex.EncodeToString(signerPublicKey), + Delta2: 4320, + } + templateJSON, err := json.Marshal(template) + if err != nil { + t.Fatal(err) + } + + // Build a request whose signer approval certificate is issued while the + // wallet is live, so it is genuinely valid apart from the wallet state. + request := covenantsigner.RouteSubmitRequest{ + Route: covenantsigner.TemplateSelfV1, + DestinationCommitmentHash: "0x" + strings.Repeat("11", 32), + MigrationTransactionPlan: &covenantsigner.MigrationTransactionPlan{ + InputValueSats: 1_000_000, + DestinationValueSats: 998_000, + AnchorValueSats: 330, + FeeSats: 1_670, + InputSequence: 0xfffffffd, + LockTime: 912345, + }, + ScriptTemplate: templateJSON, + Signing: covenantsigner.SigningRequirements{ + SignerRequired: true, + CustodianRequired: false, + }, + } + applyTestMigrationTransactionPlanCommitment(t, &request) + applyTestArtifactApprovals(t, node, walletPublicKey, &request, depositorPrivateKey, nil) + + cse := &covenantSignerEngine{node: node} + + // While the wallet is live, the otherwise-valid request is accepted. + if err := cse.VerifySignerApproval(request); err != nil { + t.Fatalf("expected a live wallet to be accepted, got: %v", err) + } + + // Transition the wallet to the closed state, keeping otherwise-valid + // registry data (same identity and members hash), matching the reported + // scenario where a certificate for a now-closed wallet is replayed. + walletPublicKeyHash := bitcoin.PublicKeyHash(walletPublicKey) + existing, err := localChain.GetWallet(walletPublicKeyHash) + if err != nil { + t.Fatal(err) + } + closed := *existing + closed.State = StateClosed + localChain.setWallet(walletPublicKeyHash, &closed) + + // The closed wallet must now be rejected before any signing occurs. + if err := cse.VerifySignerApproval(request); err == nil { + t.Fatal("expected VerifySignerApproval to reject a closed wallet") + } +} diff --git a/pkg/tbtc/deduplicator.go b/pkg/tbtc/deduplicator.go index 37d0b1704f..24e5e6368e 100644 --- a/pkg/tbtc/deduplicator.go +++ b/pkg/tbtc/deduplicator.go @@ -4,6 +4,7 @@ import ( "encoding/hex" "math/big" "strconv" + "sync" "time" "github.com/keep-network/keep-common/pkg/cache" @@ -39,6 +40,17 @@ type deduplicator struct { dkgSeedCache *cache.TimeCache dkgResultHashCache *cache.TimeCache walletClosedCache *cache.TimeCache + + // inProgressMutex guards the inProgress set below. + inProgressMutex sync.Mutex + // inProgress tracks event keys that have been claimed for handling but + // whose handling has not yet completed successfully. Keeping claimed but + // unfinished events here (rather than immediately in the completed caches + // above) lets concurrent duplicate deliveries be ignored while still + // allowing a later redelivery to retry the handling if the current attempt + // fails. An event key is moved into its completed cache only once its + // handler finishes successfully. + inProgress map[string]bool } func newDeduplicator() *deduplicator { @@ -46,73 +58,181 @@ func newDeduplicator() *deduplicator { dkgSeedCache: cache.NewTimeCache(DKGSeedCachePeriod), dkgResultHashCache: cache.NewTimeCache(DKGResultHashCachePeriod), walletClosedCache: cache.NewTimeCache(WalletClosedCachePeriod), + inProgress: make(map[string]bool), + } +} + +// claim attempts to reserve the given event, identified by key, for handling. +// It returns true when the caller should proceed with handling the event and +// false when the event has already been handled successfully (it lives in +// completedCache) or is currently being handled by another goroutine (it lives +// in the inProgress set). +// +// A successful claim must be paired with exactly one later call: markCompleted +// once the handler finishes successfully, or release if the handler fails. +// Releasing a failed claim (instead of marking it completed) is what lets a +// later redelivery of the same event retry the handling rather than being +// silently dropped as a duplicate. +func (d *deduplicator) claim(completedCache *cache.TimeCache, key string) bool { + d.inProgressMutex.Lock() + defer d.inProgressMutex.Unlock() + + // Drop expired entries so a long-past event can be handled again. + completedCache.Sweep() + + if completedCache.Has(key) { + return false + } + + if d.inProgress[key] { + return false } + + d.inProgress[key] = true + return true +} + +// markCompleted records the claimed event, identified by key, as successfully +// handled and releases the in-progress claim. Subsequent deliveries of the same +// event are treated as duplicates until the completedCache entry expires. +func (d *deduplicator) markCompleted(completedCache *cache.TimeCache, key string) { + d.inProgressMutex.Lock() + defer d.inProgressMutex.Unlock() + + completedCache.Add(key) + delete(d.inProgress, key) +} + +// release drops the in-progress claim for the given event, identified by key, +// without recording it as completed. It is called when handling fails so a +// later redelivery of the same event can be retried instead of being dropped +// as a duplicate. +func (d *deduplicator) release(key string) { + d.inProgressMutex.Lock() + defer d.inProgressMutex.Unlock() + + delete(d.inProgress, key) +} + +// dkgStartedKey builds the deduplication key identifying a DKG started event. +// The key is the hexadecimal representation of the seed. +func dkgStartedKey(newDKGSeed *big.Int) string { + return newDKGSeed.Text(16) } // notifyDKGStarted notifies the client wants to start the distributed key // generation upon receiving an event. It returns boolean indicating whether the // client should proceed with the execution or ignore the event as a duplicate. +// +// A successful claim must be released with confirmDKGStarted once the event has +// been terminally handled (the local DKG join was started, or the event turned +// out to be unconfirmed) or with abortDKGStarted if handling did not complete, +// so a later redelivery of the same event can retry. func (d *deduplicator) notifyDKGStarted( newDKGSeed *big.Int, ) bool { - d.dkgSeedCache.Sweep() - - // The cache key is the hexadecimal representation of the seed. - cacheKey := newDKGSeed.Text(16) - // If the key is not in the cache, that means the seed was not handled - // yet and the client should proceed with the execution. - if !d.dkgSeedCache.Has(cacheKey) { - d.dkgSeedCache.Add(cacheKey) - return true - } + return d.claim(d.dkgSeedCache, dkgStartedKey(newDKGSeed)) +} + +// confirmDKGStarted marks the given DKG started event as successfully handled +// so subsequent deliveries of the same event are ignored as duplicates. +func (d *deduplicator) confirmDKGStarted(newDKGSeed *big.Int) { + d.markCompleted(d.dkgSeedCache, dkgStartedKey(newDKGSeed)) +} + +// abortDKGStarted releases the in-progress claim for the given DKG started +// event without marking it handled, so a later redelivery of the same event can +// retry the handling. +func (d *deduplicator) abortDKGStarted(newDKGSeed *big.Int) { + d.release(dkgStartedKey(newDKGSeed)) +} - // Otherwise, the DKG seed is a duplicate and the client should not proceed - // with the execution. - return false +// dkgResultSubmittedKey builds the deduplication key identifying a DKG result +// submission event. +func dkgResultSubmittedKey( + newDKGResultSeed *big.Int, + newDKGResultHash DKGChainResultHash, + newDKGResultBlock uint64, +) string { + return newDKGResultSeed.Text(16) + + hex.EncodeToString(newDKGResultHash[:]) + + strconv.Itoa(int(newDKGResultBlock)) } // notifyDKGResultSubmitted notifies the client wants to start some actions // upon the DKG result submission. It returns boolean indicating whether the // client should proceed with the actions or ignore the event as a duplicate. +// +// A successful claim must be released with confirmDKGResultSubmitted once the +// result has been validated (challenged if invalid, or its approval scheduled +// if valid) or with abortDKGResultSubmitted if validation did not reach a +// terminal state, so a later redelivery of the same event can retry. func (d *deduplicator) notifyDKGResultSubmitted( newDKGResultSeed *big.Int, newDKGResultHash DKGChainResultHash, newDKGResultBlock uint64, ) bool { - d.dkgResultHashCache.Sweep() + return d.claim( + d.dkgResultHashCache, + dkgResultSubmittedKey(newDKGResultSeed, newDKGResultHash, newDKGResultBlock), + ) +} - cacheKey := newDKGResultSeed.Text(16) + - hex.EncodeToString(newDKGResultHash[:]) + - strconv.Itoa(int(newDKGResultBlock)) +// confirmDKGResultSubmitted marks the given DKG result submission as +// successfully handled so subsequent deliveries of the same event are ignored +// as duplicates. +func (d *deduplicator) confirmDKGResultSubmitted( + newDKGResultSeed *big.Int, + newDKGResultHash DKGChainResultHash, + newDKGResultBlock uint64, +) { + d.markCompleted( + d.dkgResultHashCache, + dkgResultSubmittedKey(newDKGResultSeed, newDKGResultHash, newDKGResultBlock), + ) +} - // If the key is not in the cache, that means the result was not handled - // yet and the client should proceed with the execution. - if !d.dkgResultHashCache.Has(cacheKey) { - d.dkgResultHashCache.Add(cacheKey) - return true - } +// abortDKGResultSubmitted releases the in-progress claim for the given DKG +// result submission without marking it handled, so a later redelivery of the +// same event can retry the validation. +func (d *deduplicator) abortDKGResultSubmitted( + newDKGResultSeed *big.Int, + newDKGResultHash DKGChainResultHash, + newDKGResultBlock uint64, +) { + d.release( + dkgResultSubmittedKey(newDKGResultSeed, newDKGResultHash, newDKGResultBlock), + ) +} - // Otherwise, the DKG result is a duplicate and the client should not - // proceed with the execution. - return false +// walletClosedKey builds the deduplication key identifying a wallet closure +// event. +func walletClosedKey(walletID [32]byte) string { + return hex.EncodeToString(walletID[:]) } +// notifyWalletClosed notifies the client wants to handle a wallet closure upon +// receiving an event. It returns a boolean indicating whether the client should +// proceed with the handling or ignore the event as a duplicate. +// +// A successful claim must be released with confirmWalletClosed once the wallet +// has actually been archived, or with abortWalletClosed if archival did not +// complete, so a later redelivery of the same event can retry. func (d *deduplicator) notifyWalletClosed( - WalletID [32]byte, + walletID [32]byte, ) bool { - d.walletClosedCache.Sweep() - - // Use wallet ID converted to string as the cache key. - cacheKey := hex.EncodeToString(WalletID[:]) + return d.claim(d.walletClosedCache, walletClosedKey(walletID)) +} - // If the key is not in the cache, that means the wallet closure was not - // handled yet and the client should proceed with the execution. - if !d.walletClosedCache.Has(cacheKey) { - d.walletClosedCache.Add(cacheKey) - return true - } +// confirmWalletClosed marks the given wallet closure as successfully handled so +// subsequent deliveries of the same event are ignored as duplicates. +func (d *deduplicator) confirmWalletClosed(walletID [32]byte) { + d.markCompleted(d.walletClosedCache, walletClosedKey(walletID)) +} - // Otherwise, the wallet closure is a duplicate and the client should not - // proceed with the execution. - return false +// abortWalletClosed releases the in-progress claim for the given wallet closure +// without marking it handled, so a later redelivery of the same event can retry +// the archival. +func (d *deduplicator) abortWalletClosed(walletID [32]byte) { + d.release(walletClosedKey(walletID)) } diff --git a/pkg/tbtc/deduplicator_test.go b/pkg/tbtc/deduplicator_test.go index b75432a8c0..f56afc8b6a 100644 --- a/pkg/tbtc/deduplicator_test.go +++ b/pkg/tbtc/deduplicator_test.go @@ -18,24 +18,28 @@ const ( func TestNotifyDKGStarted(t *testing.T) { deduplicator := deduplicator{ dkgSeedCache: cache.NewTimeCache(testDKGSeedCachePeriod), + inProgress: make(map[string]bool), } seed1 := big.NewInt(100) seed2 := big.NewInt(200) - // Add the first seed. + // Claim and confirm the first seed. canJoinDKG := deduplicator.notifyDKGStarted(seed1) if !canJoinDKG { t.Fatal("should be allowed to join DKG") } + deduplicator.confirmDKGStarted(seed1) - // Add the second seed. + // Claim and confirm the second seed. canJoinDKG = deduplicator.notifyDKGStarted(seed2) if !canJoinDKG { t.Fatal("should be allowed to join DKG") } + deduplicator.confirmDKGStarted(seed2) - // Add the first seed before caching period elapses. + // The first seed is now a confirmed duplicate before the caching period + // elapses. canJoinDKG = deduplicator.notifyDKGStarted(seed1) if canJoinDKG { t.Fatal("should not be allowed to join DKG") @@ -44,16 +48,55 @@ func TestNotifyDKGStarted(t *testing.T) { // Wait until caching period elapses. time.Sleep(testDKGSeedCachePeriod) - // Add the first seed again. + // The first seed can be processed again after expiry. canJoinDKG = deduplicator.notifyDKGStarted(seed1) if !canJoinDKG { t.Fatal("should be allowed to join DKG") } } +// TestNotifyDKGStarted_RetryOpenAfterAbort verifies that a DKG started event +// whose handling did not complete (aborted) can be retried on a later +// redelivery, and that a confirmed one is dropped as a duplicate. +func TestNotifyDKGStarted_RetryOpenAfterAbort(t *testing.T) { + deduplicator := deduplicator{ + dkgSeedCache: cache.NewTimeCache(testDKGSeedCachePeriod), + inProgress: make(map[string]bool), + } + + seed := big.NewInt(100) + + // Claim the event for handling. + if !deduplicator.notifyDKGStarted(seed) { + t.Fatal("first claim should be allowed to process") + } + + // While the claim is in progress, a concurrent duplicate delivery must be + // ignored. + if deduplicator.notifyDKGStarted(seed) { + t.Fatal("in-progress event should not be claimable again") + } + + // Handling failed, so the claim is released. + deduplicator.abortDKGStarted(seed) + + // A later redelivery of the same event must be allowed to retry, rather + // than being dropped as an already-processed duplicate. + if !deduplicator.notifyDKGStarted(seed) { + t.Fatal("event should be claimable again after an aborted attempt") + } + + // Once handling completes successfully, further deliveries are duplicates. + deduplicator.confirmDKGStarted(seed) + if deduplicator.notifyDKGStarted(seed) { + t.Fatal("confirmed event should not be claimable again") + } +} + func TestNotifyDKGResultSubmitted(t *testing.T) { deduplicator := deduplicator{ dkgResultHashCache: cache.NewTimeCache(testDKGResultHashCachePeriod), + inProgress: make(map[string]bool), } hash1Bytes, err := hex.DecodeString("92327ddff69a2b8c7ae787c5d590a2f14586089e6339e942d56e82aa42052cd9") @@ -70,37 +113,33 @@ func TestNotifyDKGResultSubmitted(t *testing.T) { var hash2 [32]byte copy(hash2[:], hash2Bytes) - // Add the original parameters. + // Claim and confirm the original parameters. canProcess := deduplicator.notifyDKGResultSubmitted(big.NewInt(100), hash1, 500) if !canProcess { t.Fatal("should be allowed to process") } + deduplicator.confirmDKGResultSubmitted(big.NewInt(100), hash1, 500) - // Add with different seed. - canProcess = deduplicator.notifyDKGResultSubmitted(big.NewInt(101), hash1, 500) - if !canProcess { - t.Fatal("should be allowed to process") - } - - // Add with different result hash. - canProcess = deduplicator.notifyDKGResultSubmitted(big.NewInt(100), hash2, 500) - if !canProcess { - t.Fatal("should be allowed to process") + // Different seed, different result hash, different result block, and + // all-different parameters must be treated as independent events. + for _, tc := range []struct { + seed *big.Int + hash [32]byte + block uint64 + }{ + {big.NewInt(101), hash1, 500}, + {big.NewInt(100), hash2, 500}, + {big.NewInt(100), hash1, 501}, + {big.NewInt(101), hash2, 501}, + } { + if !deduplicator.notifyDKGResultSubmitted(tc.seed, tc.hash, tc.block) { + t.Fatalf("should be allowed to process seed [%v]", tc.seed) + } + deduplicator.confirmDKGResultSubmitted(tc.seed, tc.hash, tc.block) } - // Add with different result block. - canProcess = deduplicator.notifyDKGResultSubmitted(big.NewInt(100), hash1, 501) - if !canProcess { - t.Fatal("should be allowed to process") - } - - // Add with all different parameters. - canProcess = deduplicator.notifyDKGResultSubmitted(big.NewInt(101), hash2, 501) - if !canProcess { - t.Fatal("should be allowed to process") - } - - // Add the original parameters before caching period elapses. + // The original parameters are now a confirmed duplicate before the caching + // period elapses. canProcess = deduplicator.notifyDKGResultSubmitted(big.NewInt(100), hash1, 500) if canProcess { t.Fatal("should not be allowed to process") @@ -109,34 +148,78 @@ func TestNotifyDKGResultSubmitted(t *testing.T) { // Wait until caching period elapses. time.Sleep(testDKGResultHashCachePeriod) - // Add the original parameters again. + // The original parameters can be processed again after expiry. canProcess = deduplicator.notifyDKGResultSubmitted(big.NewInt(100), hash1, 500) if !canProcess { t.Fatal("should be allowed to process") } } +// TestNotifyDKGResultSubmitted_RetryOpenAfterAbort verifies that a DKG result +// submission whose handling did not complete (aborted) can be retried on a +// later redelivery, and that a confirmed one is dropped as a duplicate. +func TestNotifyDKGResultSubmitted_RetryOpenAfterAbort(t *testing.T) { + deduplicator := deduplicator{ + dkgResultHashCache: cache.NewTimeCache(testDKGResultHashCachePeriod), + inProgress: make(map[string]bool), + } + + var hash [32]byte + seed := big.NewInt(100) + block := uint64(500) + + // Claim the event for handling. + if !deduplicator.notifyDKGResultSubmitted(seed, hash, block) { + t.Fatal("first claim should be allowed to process") + } + + // While the claim is in progress, a concurrent duplicate delivery must be + // ignored. + if deduplicator.notifyDKGResultSubmitted(seed, hash, block) { + t.Fatal("in-progress event should not be claimable again") + } + + // Handling failed, so the claim is released. + deduplicator.abortDKGResultSubmitted(seed, hash, block) + + // A later redelivery of the same event must be allowed to retry, rather + // than being dropped as an already-processed duplicate. + if !deduplicator.notifyDKGResultSubmitted(seed, hash, block) { + t.Fatal("event should be claimable again after an aborted attempt") + } + + // Once handling completes successfully, further deliveries are duplicates. + deduplicator.confirmDKGResultSubmitted(seed, hash, block) + if deduplicator.notifyDKGResultSubmitted(seed, hash, block) { + t.Fatal("confirmed event should not be claimable again") + } +} + func TestNotifyWalletClosed(t *testing.T) { deduplicator := deduplicator{ walletClosedCache: cache.NewTimeCache(testWalletClosedCachePeriod), + inProgress: make(map[string]bool), } wallet1 := [32]byte{1} wallet2 := [32]byte{2} - // Add the first wallet ID. + // Claim and confirm the first wallet ID. canProcess := deduplicator.notifyWalletClosed(wallet1) if !canProcess { t.Fatal("should be allowed to process") } + deduplicator.confirmWalletClosed(wallet1) - // Add the second wallet ID. + // Claim and confirm the second wallet ID. canProcess = deduplicator.notifyWalletClosed(wallet2) if !canProcess { t.Fatal("should be allowed to process") } + deduplicator.confirmWalletClosed(wallet2) - // Add the first wallet ID before caching period elapses. + // The first wallet ID is now a confirmed duplicate before the caching + // period elapses. canProcess = deduplicator.notifyWalletClosed(wallet1) if canProcess { t.Fatal("should not be allowed to process") @@ -145,9 +228,47 @@ func TestNotifyWalletClosed(t *testing.T) { // Wait until caching period elapses. time.Sleep(testWalletClosedCachePeriod) - // Add the first wallet ID again. + // The first wallet ID can be processed again after expiry. canProcess = deduplicator.notifyWalletClosed(wallet1) if !canProcess { t.Fatal("should be allowed to process") } } + +// TestNotifyWalletClosed_RetryOpenAfterAbort verifies that a wallet closure +// whose archival did not complete (aborted) can be retried on a later +// redelivery, and that a confirmed one is dropped as a duplicate. +func TestNotifyWalletClosed_RetryOpenAfterAbort(t *testing.T) { + deduplicator := deduplicator{ + walletClosedCache: cache.NewTimeCache(testWalletClosedCachePeriod), + inProgress: make(map[string]bool), + } + + wallet := [32]byte{1} + + // Claim the event for handling. + if !deduplicator.notifyWalletClosed(wallet) { + t.Fatal("first claim should be allowed to process") + } + + // While the claim is in progress, a concurrent duplicate delivery must be + // ignored. + if deduplicator.notifyWalletClosed(wallet) { + t.Fatal("in-progress event should not be claimable again") + } + + // Archival failed, so the claim is released. + deduplicator.abortWalletClosed(wallet) + + // A later redelivery of the same event must be allowed to retry, rather + // than being dropped as an already-processed duplicate. + if !deduplicator.notifyWalletClosed(wallet) { + t.Fatal("event should be claimable again after an aborted attempt") + } + + // Once archival completes successfully, further deliveries are duplicates. + deduplicator.confirmWalletClosed(wallet) + if deduplicator.notifyWalletClosed(wallet) { + t.Fatal("confirmed event should not be claimable again") + } +} diff --git a/pkg/tbtc/dkg.go b/pkg/tbtc/dkg.go index 177e225a18..cfd4db18df 100644 --- a/pkg/tbtc/dkg.go +++ b/pkg/tbtc/dkg.go @@ -576,12 +576,19 @@ func (de *dkgExecutor) publishDkgResult( // challenge. If the result is valid and the given node was involved in the DKG, // this function schedules an on-chain approve that is submitted once the // challenge period elapses. +// +// It returns a nil error once the result has reached a terminal handled state +// (invalid result challenged, valid result with no local approval duty, or +// valid result with approval scheduled). It returns a non-nil error when a +// transient failure (e.g. an RPC error) prevented validation from reaching a +// terminal state; callers may use that signal to retry on a later redelivery +// of the same event. func (de *dkgExecutor) executeDkgValidation( seed *big.Int, submissionBlock uint64, result *DKGChainResult, resultHash [32]byte, -) { +) error { dkgLogger := logger.With( zap.String("seed", fmt.Sprintf("0x%x", seed)), zap.String("groupPublicKey", fmt.Sprintf("0x%x", result.GroupPublicKey)), @@ -597,7 +604,7 @@ func (de *dkgExecutor) executeDkgValidation( isValid, err := de.chain.IsDKGResultValid(result) if err != nil { dkgLogger.Errorf("cannot validate DKG result: [%v]", err) - return + return fmt.Errorf("cannot validate DKG result: [%w]", err) } if !isValid { @@ -620,7 +627,10 @@ func (de *dkgExecutor) executeDkgValidation( "cannot challenge invalid DKG result: [%v]", err, ) - return + return fmt.Errorf( + "cannot challenge invalid DKG result: [%w]", + err, + ) } if de.metricsRecorder != nil { @@ -642,20 +652,23 @@ func (de *dkgExecutor) executeDkgValidation( "error while waiting for challenge confirmation: [%v]", err, ) - return + return fmt.Errorf( + "error while waiting for challenge confirmation: [%w]", + err, + ) } state, err := de.chain.GetDKGState() if err != nil { dkgLogger.Errorf("cannot check DKG state: [%v]", err) - return + return fmt.Errorf("cannot check DKG state: [%w]", err) } if state != Challenge { dkgLogger.Infof( "invalid DKG result challenged successfully", ) - return + return nil } dkgLogger.Infof( @@ -669,7 +682,7 @@ func (de *dkgExecutor) executeDkgValidation( operatorID, err := de.operatorIDFn() if err != nil { dkgLogger.Errorf("cannot get node's operator ID: [%v]", err) - return + return fmt.Errorf("cannot get node's operator ID: [%w]", err) } // Determine the member indexes controlled by this node's operator. @@ -689,7 +702,7 @@ func (de *dkgExecutor) executeDkgValidation( operatorID, result.Members, ) - return + return nil } dkgLogger.Infof("scheduling DKG result approval") @@ -697,7 +710,7 @@ func (de *dkgExecutor) executeDkgValidation( parameters, err := de.chain.DKGParameters() if err != nil { dkgLogger.Errorf("cannot get current DKG parameters: [%v]", err) - return + return fmt.Errorf("cannot get current DKG parameters: [%w]", err) } // The challenge period starts at the result submission block and lasts @@ -780,6 +793,10 @@ func (de *dkgExecutor) executeDkgValidation( dkgLogger.Infof("[member:%v] approving DKG result", memberIndex) }(currentMemberIndex) } + + // The result was valid and this node's approval duties have been scheduled; + // the event is terminally handled. + return nil } // finalSigningGroup takes three parameters: diff --git a/pkg/tbtc/dkg_result_submitted_handler.go b/pkg/tbtc/dkg_result_submitted_handler.go new file mode 100644 index 0000000000..7d17650c72 --- /dev/null +++ b/pkg/tbtc/dkg_result_submitted_handler.go @@ -0,0 +1,71 @@ +package tbtc + +// handleDKGResultSubmittedEvent runs the deduplication-guarded handling of a +// DKGResultSubmitted event. It claims the event, runs handle, and then either +// confirms the deduplication entry once handling reaches a terminal state (so +// subsequent deliveries of the same event are ignored as duplicates) or releases +// the claim when handle returns an error, so a later redelivery of the same +// event can retry. +// +// handle performs the actual DKG result validation and returns a non-nil error +// when validation did not reach a terminal state (e.g. a transient RPC error +// before the result could be challenged or its approval scheduled). Releasing +// the claim on such a failure is what prevents the node from being silently +// removed from the challenger set for the rest of the challenge window: the +// deduplication entry is recorded as completed only after handling succeeds. +func handleDKGResultSubmittedEvent( + deduplicator *deduplicator, + event *DKGResultSubmittedEvent, + handle func(event *DKGResultSubmittedEvent) error, +) { + if ok := deduplicator.notifyDKGResultSubmitted( + event.Seed, + event.ResultHash, + event.BlockNumber, + ); !ok { + logger.Warnf( + "Result with hash [0x%x] for DKG with seed [0x%x] "+ + "and starting block [%v] has been already processed", + event.ResultHash, + event.Seed, + event.BlockNumber, + ) + return + } + + logger.Infof( + "Result with hash [0x%x] for DKG with seed [0x%x] "+ + "submitted at block [%v]", + event.ResultHash, + event.Seed, + event.BlockNumber, + ) + + if err := handle(event); err != nil { + // Validation did not reach a terminal state (e.g. a transient + // RPC error before the result could be challenged or its + // approval scheduled). Release the deduplication claim so a + // later redelivery of the same event retries validation instead + // of being dropped as an already-processed duplicate. + logger.Warnf( + "DKG result validation for result with hash [0x%x] and "+ + "seed [0x%x] did not complete; allowing retry on event "+ + "redelivery: [%v]", + event.ResultHash, + event.Seed, + err, + ) + deduplicator.abortDKGResultSubmitted( + event.Seed, + event.ResultHash, + event.BlockNumber, + ) + return + } + + deduplicator.confirmDKGResultSubmitted( + event.Seed, + event.ResultHash, + event.BlockNumber, + ) +} diff --git a/pkg/tbtc/dkg_result_submitted_handler_test.go b/pkg/tbtc/dkg_result_submitted_handler_test.go new file mode 100644 index 0000000000..f766d4d42b --- /dev/null +++ b/pkg/tbtc/dkg_result_submitted_handler_test.go @@ -0,0 +1,103 @@ +package tbtc + +import ( + "fmt" + "math/big" + "testing" + + "github.com/keep-network/keep-core/internal/testutils" +) + +// TestHandleDKGResultSubmittedEvent_RetriesAfterTransientFailure verifies the +// TOB-TBTCACEXT-56 fix at the callback level: when the first delivery of a +// DKGResultSubmitted event fails transiently during validation, an identical +// redelivery must reach validation again instead of being dropped as an +// already-processed duplicate. Once validation reaches a terminal state, a +// further redelivery must be ignored as a duplicate. +func TestHandleDKGResultSubmittedEvent_RetriesAfterTransientFailure(t *testing.T) { + deduplicator := newDeduplicator() + + var resultHash DKGChainResultHash + copy(resultHash[:], []byte{0x01, 0x02, 0x03}) + event := &DKGResultSubmittedEvent{ + Seed: big.NewInt(100), + ResultHash: resultHash, + BlockNumber: 500, + } + + var validationCalls int + handle := func(*DKGResultSubmittedEvent) error { + validationCalls++ + if validationCalls == 1 { + // Simulate a transient RPC error before the result could be + // challenged or its approval scheduled. + return fmt.Errorf("transient validation failure") + } + return nil + } + + // First delivery: the event is claimed, validation fails, and the claim is + // released so the event stays retryable. + handleDKGResultSubmittedEvent(deduplicator, event, handle) + + // Redelivery of the identical event must reach validation again rather than + // being dropped as an already-processed duplicate. + handleDKGResultSubmittedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "validation calls after a failed delivery and its retry", + 2, + validationCalls, + ) + + // The retry succeeded and confirmed the event, so a further redelivery must + // be ignored as a duplicate and must not reach validation again. + handleDKGResultSubmittedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "validation calls after the event was confirmed handled", + 2, + validationCalls, + ) +} + +// TestHandleDKGResultSubmittedEvent_IgnoresConcurrentDuplicate verifies that +// while a DKGResultSubmitted event is being handled, a concurrent duplicate +// delivery of the same event is ignored rather than starting a second handling. +func TestHandleDKGResultSubmittedEvent_IgnoresConcurrentDuplicate(t *testing.T) { + deduplicator := newDeduplicator() + + var resultHash DKGChainResultHash + copy(resultHash[:], []byte{0x0a, 0x0b, 0x0c}) + event := &DKGResultSubmittedEvent{ + Seed: big.NewInt(200), + ResultHash: resultHash, + BlockNumber: 700, + } + + var nestedCalls int + handle := func(*DKGResultSubmittedEvent) error { + // While this handling is in progress, a duplicate delivery must be + // dropped and must not enter the handler a second time. + handleDKGResultSubmittedEvent( + deduplicator, + event, + func(*DKGResultSubmittedEvent) error { + nestedCalls++ + return nil + }, + ) + return nil + } + + handleDKGResultSubmittedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "handler calls triggered by an in-progress duplicate", + 0, + nestedCalls, + ) +} diff --git a/pkg/tbtc/dkg_started_handler.go b/pkg/tbtc/dkg_started_handler.go new file mode 100644 index 0000000000..285cdc5a17 --- /dev/null +++ b/pkg/tbtc/dkg_started_handler.go @@ -0,0 +1,53 @@ +package tbtc + +// handleDKGStartedEvent runs the deduplication-guarded handling of a DKGStarted +// event. It claims the event, runs handle, and then either confirms the +// deduplication entry once handling reaches a terminal state (so subsequent +// deliveries of the same event are ignored as duplicates) or releases the claim +// when handle returns an error, so a later redelivery of the same event can +// retry. +// +// handle performs the local DKG-start handling and returns a nil error once the +// event has been terminally handled (the local DKG join was dispatched, or the +// event turned out to be unconfirmed) and a non-nil error when a transient early +// return (e.g. a block-confirmation wait failure, a DKG-state check error, or a +// past-event lookup error) prevented handling from completing. Releasing the +// claim on such a failure is what lets a redelivered event retry so the operator +// still joins the new group: the deduplication entry is recorded as completed +// only after handling succeeds. +func handleDKGStartedEvent( + deduplicator *deduplicator, + event *DKGStartedEvent, + handle func(event *DKGStartedEvent) error, +) { + if ok := deduplicator.notifyDKGStarted( + event.Seed, + ); !ok { + logger.Infof( + "DKG started event with seed [0x%x] has been "+ + "already processed", + event.Seed, + ) + return + } + + if err := handle(event); err != nil { + // Handling did not reach a terminal state (e.g. a block-confirmation + // wait failure, a DKG-state check error, or a past-event lookup error + // before the local DKG join could be dispatched). Release the + // deduplication claim so a later redelivery of the same event retries + // the handling instead of being dropped as an already-processed + // duplicate, which would otherwise leave the operator out of the new + // group. + logger.Warnf( + "handling of DKG started event with seed [0x%x] did "+ + "not complete; allowing retry on event redelivery: [%v]", + event.Seed, + err, + ) + deduplicator.abortDKGStarted(event.Seed) + return + } + + deduplicator.confirmDKGStarted(event.Seed) +} diff --git a/pkg/tbtc/dkg_started_handler_test.go b/pkg/tbtc/dkg_started_handler_test.go new file mode 100644 index 0000000000..3dcccc96e0 --- /dev/null +++ b/pkg/tbtc/dkg_started_handler_test.go @@ -0,0 +1,99 @@ +package tbtc + +import ( + "fmt" + "math/big" + "testing" + + "github.com/keep-network/keep-core/internal/testutils" +) + +// TestHandleDKGStartedEvent_RetriesAfterTransientFailure verifies the +// TOB-TBTCACEXT-72 fix at the callback level: when the first delivery of a +// DKGStarted event fails after the deduplication claim (e.g. a block-confirmation +// wait failure before the local DKG join is dispatched), an identical redelivery +// must reach the handling path again instead of being dropped as an +// already-processed duplicate. Once handling reaches a terminal state, a further +// redelivery must be ignored as a duplicate. +func TestHandleDKGStartedEvent_RetriesAfterTransientFailure(t *testing.T) { + deduplicator := newDeduplicator() + + event := &DKGStartedEvent{ + Seed: big.NewInt(100), + BlockNumber: 500, + } + + var handlingCalls int + handle := func(*DKGStartedEvent) error { + handlingCalls++ + if handlingCalls == 1 { + // Simulate a transient early return (e.g. a failed block-height + // confirmation) before the local DKG join could be dispatched. + return fmt.Errorf("transient DKG started handling failure") + } + return nil + } + + // First delivery: the event is claimed, handling fails, and the claim is + // released so the event stays retryable. + handleDKGStartedEvent(deduplicator, event, handle) + + // Redelivery of the identical event must reach the handling path again + // rather than being dropped as an already-processed duplicate; otherwise the + // operator would never join the new group. + handleDKGStartedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "handling calls after a failed delivery and its retry", + 2, + handlingCalls, + ) + + // The retry succeeded and confirmed the event, so a further redelivery must + // be ignored as a duplicate and must not reach the handling path again. + handleDKGStartedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "handling calls after the event was confirmed handled", + 2, + handlingCalls, + ) +} + +// TestHandleDKGStartedEvent_IgnoresConcurrentDuplicate verifies that while a +// DKGStarted event is being handled, a concurrent duplicate delivery of the same +// event is ignored rather than starting a second handling. +func TestHandleDKGStartedEvent_IgnoresConcurrentDuplicate(t *testing.T) { + deduplicator := newDeduplicator() + + event := &DKGStartedEvent{ + Seed: big.NewInt(200), + BlockNumber: 700, + } + + var nestedCalls int + handle := func(*DKGStartedEvent) error { + // While this handling is in progress, a duplicate delivery must be + // dropped and must not enter the handler a second time. + handleDKGStartedEvent( + deduplicator, + event, + func(*DKGStartedEvent) error { + nestedCalls++ + return nil + }, + ) + return nil + } + + handleDKGStartedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "handler calls triggered by an in-progress duplicate", + 0, + nestedCalls, + ) +} diff --git a/pkg/tbtc/dkg_test.go b/pkg/tbtc/dkg_test.go index 547d1b5079..e3a88bde50 100644 --- a/pkg/tbtc/dkg_test.go +++ b/pkg/tbtc/dkg_test.go @@ -347,7 +347,7 @@ func TestDkgExecutor_ExecuteDkgValidation(t *testing.T) { }, ) - dkgExecutor.executeDkgValidation( + _ = dkgExecutor.executeDkgValidation( dkgResultSubmittedEvent.Seed, dkgResultSubmittedEvent.BlockNumber, dkgResultSubmittedEvent.Result, diff --git a/pkg/tbtc/node.go b/pkg/tbtc/node.go index 9a15e97096..87f92a6a20 100644 --- a/pkg/tbtc/node.go +++ b/pkg/tbtc/node.go @@ -4,6 +4,7 @@ import ( "context" "crypto/ecdsa" "encoding/hex" + "errors" "fmt" "math/big" "sync" @@ -303,13 +304,17 @@ func (n *node) joinDKGIfEligible( // challenge. If the result is valid and the given node was involved in the DKG, // this function schedules an on-chain approve that is submitted once the // challenge period elapses. +// +// It returns a nil error once the result has been terminally handled and a +// non-nil error when a transient failure prevented handling from completing, +// so the caller can retry on a later redelivery of the same event. func (n *node) validateDKG( seed *big.Int, submissionBlock uint64, result *DKGChainResult, resultHash [32]byte, -) { - n.dkgExecutor.executeDkgValidation(seed, submissionBlock, result, resultHash) +) error { + return n.dkgExecutor.executeDkgValidation(seed, submissionBlock, result, resultHash) } // getSigningExecutor gets the signing executor responsible for executing @@ -415,6 +420,28 @@ func (n *node) getSigningExecutor( return executor, true, nil } +// invalidateSigningExecutor removes any cached signing executor for the given +// wallet. It is used when a wallet is archived (closed or terminated) so that a +// stale cached executor cannot keep the node signing for a wallet that has been +// removed from the wallet registry. getSigningExecutor consults the executor +// cache before the wallet registry, so without this eviction an executor +// created while the wallet was live would remain usable after archival. +func (n *node) invalidateSigningExecutor(walletPublicKey *ecdsa.PublicKey) error { + walletPublicKeyBytes, err := marshalPublicKey(walletPublicKey) + if err != nil { + return fmt.Errorf("cannot marshal wallet public key: [%v]", err) + } + + executorKey := hex.EncodeToString(walletPublicKeyBytes) + + n.signingExecutorsMutex.Lock() + defer n.signingExecutorsMutex.Unlock() + + delete(n.signingExecutors, executorKey) + + return nil +} + // getCoordinationExecutor gets the coordination executor responsible for // executing coordination related to a specific wallet whose part is controlled // by this node. The second boolean return value indicates whether the node @@ -1279,29 +1306,31 @@ func (n *node) archiveClosedWallets() error { for _, walletPublicKey := range walletPublicKeys { walletPublicKeyHash := bitcoin.PublicKeyHash(walletPublicKey) - walletID, err := n.chain.CalculateWalletID(walletPublicKey) + walletChainData, err := n.chain.GetWallet(walletPublicKeyHash) if err != nil { - return fmt.Errorf( - "could not calculate wallet ID for wallet with public key "+ - "hash [0x%x]: [%v]", - walletPublicKeyHash, - err, - ) - } + if errors.Is(err, ErrWalletNotFound) { + // The wallet is not recorded on-chain by the Bridge. This is the + // case for a freshly generated wallet whose DKG result has not + // yet been approved on-chain; during that approval window the + // wallet exists only on the node's disk. Such a wallet must not + // be archived because it may be about to become active. Only + // wallets that were registered on-chain and are now closed or + // terminated should be archived, so leave this one in place. + continue + } - isRegistered, err := n.chain.IsWalletRegistered(walletID) - if err != nil { return fmt.Errorf( - "could not check if wallet is registered for wallet with ID "+ - "[0x%x]: [%v]", + "could not get on-chain data for wallet with public key "+ + "hash [0x%x]: [%v]", walletPublicKeyHash, err, ) } - if !isRegistered { - // If the wallet is no longer registered it means the wallet has - // been closed or terminated. + // The wallet is recorded on-chain, so it has been registered. Archive it + // only if it has reached a closed or terminated state. + if walletChainData.State == StateClosed || + walletChainData.State == StateTerminated { err := n.walletRegistry.archiveWallet(walletPublicKeyHash) if err != nil { return fmt.Errorf( @@ -1312,10 +1341,10 @@ func (n *node) archiveClosedWallets() error { } logger.Infof( - "successfully archived wallet with ID [0x%x] and public key "+ - "hash [0x%x]", - walletID, + "successfully archived wallet with public key hash [0x%x] "+ + "in state [%v]", walletPublicKeyHash, + walletChainData.State, ) } } @@ -1385,6 +1414,17 @@ func (n *node) handleWalletClosure(walletID [32]byte) error { return fmt.Errorf("failed to archive the wallet: [%v]", err) } + // Evict any cached signing executor for the archived wallet so that the + // closure actually deauthorizes signing for it. getSigningExecutor consults + // the executor cache before the wallet registry, so a stale cached executor + // would otherwise keep the wallet signable until the process restarts. + if err := n.invalidateSigningExecutor(wallet.publicKey); err != nil { + return fmt.Errorf( + "failed to invalidate signing executor for archived wallet: [%v]", + err, + ) + } + logger.Infof( "successfully archived wallet with wallet ID [0x%x] and public key "+ "hash [0x%x]", diff --git a/pkg/tbtc/signer_approval_certificate.go b/pkg/tbtc/signer_approval_certificate.go index 3ec27a812f..44a0b0aec9 100644 --- a/pkg/tbtc/signer_approval_certificate.go +++ b/pkg/tbtc/signer_approval_certificate.go @@ -4,6 +4,7 @@ import ( "context" "crypto/ecdsa" "crypto/sha256" + "encoding/binary" "encoding/hex" "fmt" "math/big" @@ -31,9 +32,10 @@ var ( ) const ( - signerApprovalCertificateVersion uint32 = 1 + signerApprovalCertificateVersion uint32 = 2 signerApprovalCertificateSignatureAlgorithm = "tecdsa-secp256k1" signerApprovalCertificateSignerSetDomain = "covenant-signer-set-v1:" + signerApprovalCertificateSigningDomain = "covenant-signer-approval-certificate-v2:" ) type signerApprovalCertificateSignerSetPayload struct { @@ -62,10 +64,47 @@ func ensureWalletRegistryDataAvailable( return nil } +// signerApprovalCertificateSigningDigest computes the certificate v2 signing +// digest that the threshold signature is produced over: +// +// signingDigest = SHA256( +// ASCII("covenant-signer-approval-certificate-v2:") || +// rawApprovalDigest[32] || +// uint64_big_endian(endBlock) +// ) +// +// Binding endBlock into the signed digest ensures a certificate's expiry +// cannot be altered after issuance without invalidating the signature: an +// attacker who tampers with EndBlock (e.g. to extend a certificate's +// validity) produces a digest that no longer matches the wallet's signature. +func signerApprovalCertificateSigningDigest( + approvalDigest []byte, + endBlock uint64, +) ([]byte, error) { + if len(approvalDigest) != sha256.Size { + return nil, fmt.Errorf( + "approval digest must be exactly %d bytes", + sha256.Size, + ) + } + + preimage := make([]byte, 0, len(signerApprovalCertificateSigningDomain)+sha256.Size+8) + preimage = append(preimage, []byte(signerApprovalCertificateSigningDomain)...) + preimage = append(preimage, approvalDigest...) + + var endBlockBytes [8]byte + binary.BigEndian.PutUint64(endBlockBytes[:], endBlock) + preimage = append(preimage, endBlockBytes[:]...) + + digest := sha256.Sum256(preimage) + return digest[:], nil +} + func (se *signingExecutor) issueSignerApprovalCertificate( ctx context.Context, approvalDigest []byte, startBlock uint64, + requestedEndBlock uint64, ) (*covenantsigner.SignerApprovalCertificate, error) { if len(approvalDigest) != sha256.Size { return nil, fmt.Errorf( @@ -89,14 +128,35 @@ func (se *signingExecutor) issueSignerApprovalCertificate( return nil, err } - signature, activityReport, endBlock, err := se.sign( + signingDigest, err := signerApprovalCertificateSigningDigest( + approvalDigest, + requestedEndBlock, + ) + if err != nil { + return nil, err + } + + signature, activityReport, completionBlock, err := se.sign( ctx, - new(big.Int).SetBytes(approvalDigest), + new(big.Int).SetBytes(signingDigest), startBlock, ) if err != nil { return nil, err } + // If signing took long enough that the requested expiry has already + // passed by the time it completed, the resulting certificate would be + // expired the moment it is issued. Reject it outright rather than + // returning a certificate that can never be used: the caller must issue a + // new certificate with a later requested end block. + if completionBlock > requestedEndBlock { + return nil, fmt.Errorf( + "signer approval certificate signing completed at block [%d], "+ + "after the requested end block [%d]", + completionBlock, + requestedEndBlock, + ) + } return buildSignerApprovalCertificate( wallet, @@ -105,7 +165,7 @@ func (se *signingExecutor) issueSignerApprovalCertificate( approvalDigest, signature, activityReport, - endBlock, + requestedEndBlock, ) } @@ -233,6 +293,9 @@ func verifySignerApprovalCertificate( if certificate.SignatureAlgorithm != signerApprovalCertificateSignatureAlgorithm { return fmt.Errorf("unsupported signature algorithm: %s", certificate.SignatureAlgorithm) } + if certificate.EndBlock == nil { + return fmt.Errorf("certificate end block is required") + } if strings.TrimSpace(expectedSignerSetHash) == "" { return fmt.Errorf("expected signer set hash must not be empty") } @@ -247,6 +310,13 @@ func verifySignerApprovalCertificate( if err != nil { return fmt.Errorf("invalid approval digest: %w", err) } + signingDigest, err := signerApprovalCertificateSigningDigest( + approvalDigest, + *certificate.EndBlock, + ) + if err != nil { + return fmt.Errorf("cannot compute certificate signing digest: %w", err) + } signatureBytes, err := decodeSignerApprovalCertificateHex( certificate.Signature, 0, @@ -277,7 +347,7 @@ func verifySignerApprovalCertificate( return fmt.Errorf("threshold signature S value is not low-S normalized") } - if !ecdsa.Verify(walletPublicKey, approvalDigest, parsedSignature.R, parsedSignature.S) { + if !ecdsa.Verify(walletPublicKey, signingDigest, parsedSignature.R, parsedSignature.S) { return fmt.Errorf("threshold signature does not verify against wallet public key") } diff --git a/pkg/tbtc/signer_approval_certificate_test.go b/pkg/tbtc/signer_approval_certificate_test.go index 1d06551693..d75f2097b9 100644 --- a/pkg/tbtc/signer_approval_certificate_test.go +++ b/pkg/tbtc/signer_approval_certificate_test.go @@ -6,12 +6,15 @@ import ( "crypto/ecdsa" "crypto/sha256" "encoding/asn1" + "encoding/binary" "encoding/hex" "encoding/json" "errors" + "math" "math/big" "strings" "testing" + "time" "github.com/btcsuite/btcd/btcec" "github.com/keep-network/keep-core/pkg/bitcoin" @@ -123,6 +126,7 @@ func validStructuredSignerApprovalVerificationRequest( context.Background(), testArtifactApprovalDigest(t, request.ArtifactApprovals.Payload), startBlock, + startBlock+100000, ) if err != nil { t.Fatal(err) @@ -151,11 +155,13 @@ func TestSigningExecutorCanIssueSignerApprovalCertificateForArbitraryDigest(t *t approvalDigest := sha256.Sum256( []byte("psbt-covenant-signer-approval-certificate-spike"), ) + requestedEndBlock := startBlock + 100000 certificate, err := executor.issueSignerApprovalCertificate( context.Background(), approvalDigest[:], startBlock, + requestedEndBlock, ) if err != nil { t.Fatal(err) @@ -201,15 +207,83 @@ func TestSigningExecutorCanIssueSignerApprovalCertificateForArbitraryDigest(t *t certificate.ActiveMembers, ) } - if certificate.EndBlock == nil || *certificate.EndBlock < startBlock { + if certificate.EndBlock == nil || *certificate.EndBlock != requestedEndBlock { t.Fatalf( - "expected end block [%v] to be >= start block [%v]", + "expected end block [%v] to equal the requested end block [%v]", certificate.EndBlock, - startBlock, + requestedEndBlock, ) } } +// TestSigningExecutorIssueSignerApprovalCertificateAcceptsEndBlockAboveUint32Range +// proves an EndBlock above math.MaxUint32 round-trips correctly through +// issuance (signing) and verification. The certificate v2 digest binds +// EndBlock as a full 8-byte big-endian uint64 (see +// signerApprovalCertificateSigningDigest), so the full uint64 range must be +// usable end to end, not just the low 32 bits. +func TestSigningExecutorIssueSignerApprovalCertificateAcceptsEndBlockAboveUint32Range(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + + executor, ok, err := node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("node is supposed to control wallet signers") + } + + startBlock, err := executor.getCurrentBlockFn() + if err != nil { + t.Fatal(err) + } + + approvalDigest := sha256.Sum256( + []byte("psbt-covenant-signer-approval-certificate-uint64-end-block"), + ) + requestedEndBlock := uint64(math.MaxUint32) + 100000 + if requestedEndBlock <= math.MaxUint32 { + t.Fatalf("test vector must exceed math.MaxUint32, got %d", requestedEndBlock) + } + + certificate, err := executor.issueSignerApprovalCertificate( + context.Background(), + approvalDigest[:], + startBlock, + requestedEndBlock, + ) + if err != nil { + t.Fatal(err) + } + + if certificate.EndBlock == nil || *certificate.EndBlock != requestedEndBlock { + t.Fatalf( + "expected end block [%v] to equal the requested end block [%v]", + certificate.EndBlock, + requestedEndBlock, + ) + } + + walletChainData, err := executor.chain.GetWallet( + bitcoin.PublicKeyHash(executor.wallet().publicKey), + ) + if err != nil { + t.Fatal(err) + } + + expectedSignerSetHash, err := computeSignerApprovalCertificateSignerSetHash( + executor.wallet().publicKey, + walletChainData, + executor.groupParameters, + ) + if err != nil { + t.Fatal(err) + } + if err := verifySignerApprovalCertificate(certificate, expectedSignerSetHash); err != nil { + t.Fatalf("expected certificate verification to succeed: %v", err) + } +} + func TestSigningExecutorIssueSignerApprovalCertificateFailsWhenWalletRegistryUnavailable(t *testing.T) { node, _, walletPublicKey := setupCovenantSignerTestNode(t) @@ -240,6 +314,7 @@ func TestSigningExecutorIssueSignerApprovalCertificateFailsWhenWalletRegistryUna context.Background(), approvalDigest[:], startBlock, + startBlock+100000, ) if err == nil || !strings.Contains( err.Error(), @@ -270,6 +345,7 @@ func TestSignerApprovalCertificateVerificationRejectsTamperedDigest(t *testing.T context.Background(), approvalDigest[:], startBlock, + startBlock+100000, ) if err != nil { t.Fatal(err) @@ -669,3 +745,289 @@ func TestVerifySignerApprovalCertificateRejectsMalformedWalletPublicKey(t *testi t.Fatalf("expected malformed wallet public key error, got %v", err) } } + +func TestVerifySignerApprovalCertificateRejectsV1CertificateVersion(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + request := validStructuredSignerApprovalVerificationRequest( + t, + node, + walletPublicKey, + covenantsigner.TemplateSelfV1, + ) + + certificate := *request.SignerApproval + certificate.CertificateVersion = 1 + + walletExecutor, ok, err := node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("node is supposed to control wallet signers") + } + walletChainData, err := walletExecutor.chain.GetWallet(bitcoin.PublicKeyHash(walletPublicKey)) + if err != nil { + t.Fatal(err) + } + expectedSignerSetHash, err := computeSignerApprovalCertificateSignerSetHash( + walletPublicKey, + walletChainData, + walletExecutor.groupParameters, + ) + if err != nil { + t.Fatal(err) + } + + err = verifySignerApprovalCertificate(&certificate, expectedSignerSetHash) + if err == nil || !strings.Contains(err.Error(), "unsupported certificate version: 1") { + t.Fatalf("expected v1 certificate version rejection, got %v", err) + } +} + +func TestVerifySignerApprovalCertificateRejectsMissingEndBlock(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + request := validStructuredSignerApprovalVerificationRequest( + t, + node, + walletPublicKey, + covenantsigner.TemplateSelfV1, + ) + + certificate := *request.SignerApproval + certificate.EndBlock = nil + + walletExecutor, ok, err := node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("node is supposed to control wallet signers") + } + walletChainData, err := walletExecutor.chain.GetWallet(bitcoin.PublicKeyHash(walletPublicKey)) + if err != nil { + t.Fatal(err) + } + expectedSignerSetHash, err := computeSignerApprovalCertificateSignerSetHash( + walletPublicKey, + walletChainData, + walletExecutor.groupParameters, + ) + if err != nil { + t.Fatal(err) + } + + err = verifySignerApprovalCertificate(&certificate, expectedSignerSetHash) + if err == nil || !strings.Contains(err.Error(), "certificate end block is required") { + t.Fatalf("expected missing end block rejection, got %v", err) + } +} + +// TestVerifySignerApprovalCertificateRejectsTamperedEndBlock proves EndBlock +// is bound into the signed digest, not just carried alongside it: changing +// EndBlock on an otherwise-untouched, validly-issued certificate must +// invalidate the threshold signature. Without this, an attacker (or a +// compromised relay) could extend a certificate's validity window after +// issuance without the wallet's cooperation. +func TestVerifySignerApprovalCertificateRejectsTamperedEndBlock(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + request := validStructuredSignerApprovalVerificationRequest( + t, + node, + walletPublicKey, + covenantsigner.TemplateSelfV1, + ) + + certificate := *request.SignerApproval + tamperedEndBlock := *certificate.EndBlock + 1_000_000 + certificate.EndBlock = &tamperedEndBlock + + walletExecutor, ok, err := node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("node is supposed to control wallet signers") + } + walletChainData, err := walletExecutor.chain.GetWallet(bitcoin.PublicKeyHash(walletPublicKey)) + if err != nil { + t.Fatal(err) + } + expectedSignerSetHash, err := computeSignerApprovalCertificateSignerSetHash( + walletPublicKey, + walletChainData, + walletExecutor.groupParameters, + ) + if err != nil { + t.Fatal(err) + } + + err = verifySignerApprovalCertificate(&certificate, expectedSignerSetHash) + if err == nil || !strings.Contains(err.Error(), "threshold signature does not verify against wallet public key") { + t.Fatalf("expected tampered end block to invalidate the signature, got %v", err) + } +} + +// TestSigningExecutorIssueSignerApprovalCertificateRejectsCompletionAfterRequestedExpiry +// verifies that issuance is rejected outright -- not merely issued with a +// signing-completion-block EndBlock -- when synchronous threshold signing +// does not finish until after the requested expiry. A short real sleep +// ensures the local block counter has ticked past block zero, so the chosen +// requestedEndBlock (one below the observed start block) is guaranteed to be +// exceeded by the actual signing completion block regardless of how fast +// signing itself completes. +func TestSigningExecutorIssueSignerApprovalCertificateRejectsCompletionAfterRequestedExpiry(t *testing.T) { + node, _, walletPublicKey := setupCovenantSignerTestNode(t) + + executor, ok, err := node.getSigningExecutor(walletPublicKey) + if err != nil { + t.Fatal(err) + } + if !ok { + t.Fatal("node is supposed to control wallet signers") + } + + time.Sleep(600 * time.Millisecond) + + startBlock, err := executor.getCurrentBlockFn() + if err != nil { + t.Fatal(err) + } + if startBlock == 0 { + t.Fatal("expected the local block counter to have advanced past zero") + } + requestedEndBlock := startBlock - 1 + + approvalDigest := sha256.Sum256([]byte("already-expired-by-completion")) + _, err = executor.issueSignerApprovalCertificate( + context.Background(), + approvalDigest[:], + startBlock, + requestedEndBlock, + ) + if err == nil || !strings.Contains(err.Error(), "after the requested end block") { + t.Fatalf("expected completion-after-expiry rejection, got %v", err) + } +} + +// TestSignerApprovalCertificateSigningDigestMatchesCrossLanguageVector pins +// the exact byte layout of the certificate v2 signing digest so independent +// (non-Go) certificate producers can verify their implementation against the +// same inputs and output: +// +// signingDigest = SHA256( +// ASCII("covenant-signer-approval-certificate-v2:") || +// rawApprovalDigest[32] || +// uint64_big_endian(endBlock) +// ) +func TestSignerApprovalCertificateSigningDigestMatchesCrossLanguageVector(t *testing.T) { + approvalDigest, err := hex.DecodeString(strings.Repeat("11", 32)) + if err != nil { + t.Fatal(err) + } + const endBlock = uint64(123456) + + // Independently constructed preimage and digest -- deliberately not + // calling the production helper -- so this test actually catches a + // broken implementation rather than just echoing it back. + preimage := append( + []byte("covenant-signer-approval-certificate-v2:"), + approvalDigest..., + ) + var endBlockBytes [8]byte + binary.BigEndian.PutUint64(endBlockBytes[:], endBlock) + preimage = append(preimage, endBlockBytes[:]...) + expectedDigest := sha256.Sum256(preimage) + expectedDigestHex := "0x" + hex.EncodeToString(expectedDigest[:]) + + // Pinned exact value: SHA256( + // "covenant-signer-approval-certificate-v2:" || + // 0x1111111111111111111111111111111111111111111111111111111111111111 || + // 0x000000000001e240 + // ) + const expectedPinnedHex = "0x636bf1d70b67dd7f54e1d4e090fd258928cce229b66a9a8cae3a5e75e288e19d" + if expectedDigestHex != expectedPinnedHex { + t.Fatalf( + "test vector itself is inconsistent\nindependent: %s\npinned: %s", + expectedDigestHex, + expectedPinnedHex, + ) + } + + actualDigest, err := signerApprovalCertificateSigningDigest(approvalDigest, endBlock) + if err != nil { + t.Fatal(err) + } + actualDigestHex := "0x" + hex.EncodeToString(actualDigest) + + if actualDigestHex != expectedDigestHex { + t.Fatalf( + "unexpected v2 signing digest\nexpected: %s\nactual: %s", + expectedDigestHex, + actualDigestHex, + ) + } +} + +// TestSignerApprovalCertificateSigningDigestMatchesCrossLanguageVectorAboveUint32Range +// pins a second cross-language vector whose EndBlock exceeds math.MaxUint32, +// so the digest's high-order 4 bytes are nonzero. The vector above alone +// cannot catch an implementation that silently truncates EndBlock to 32 bits +// before hashing it, since 123456 fits entirely in the low-order bytes; this +// one specifically exercises the full 8-byte big-endian encoding. +func TestSignerApprovalCertificateSigningDigestMatchesCrossLanguageVectorAboveUint32Range(t *testing.T) { + approvalDigest, err := hex.DecodeString(strings.Repeat("11", 32)) + if err != nil { + t.Fatal(err) + } + // One past math.MaxUint32, plus the same 123456 low-order value used by + // the vector above. + const endBlock = uint64(math.MaxUint32) + 1 + 123456 + if endBlock <= math.MaxUint32 { + t.Fatalf("test vector must exceed math.MaxUint32, got %d", endBlock) + } + + // Independently constructed preimage and digest -- deliberately not + // calling the production helper -- so this test actually catches a + // broken implementation rather than just echoing it back. + preimage := append( + []byte("covenant-signer-approval-certificate-v2:"), + approvalDigest..., + ) + var endBlockBytes [8]byte + binary.BigEndian.PutUint64(endBlockBytes[:], endBlock) + if endBlockBytes != ([8]byte{0x00, 0x00, 0x00, 0x01, 0x00, 0x01, 0xe2, 0x40}) { + t.Fatalf("unexpected big-endian encoding of endBlock: %x", endBlockBytes) + } + preimage = append(preimage, endBlockBytes[:]...) + expectedDigest := sha256.Sum256(preimage) + expectedDigestHex := "0x" + hex.EncodeToString(expectedDigest[:]) + + // Pinned exact value, independently computed (Python hashlib, not this + // Go codebase): SHA256( + // "covenant-signer-approval-certificate-v2:" || + // 0x1111111111111111111111111111111111111111111111111111111111111111 || + // 0x000000010001e240 + // ) + const expectedPinnedHex = "0x67e035629f14cad309d349046c6c6ec0ec2c18a79b0c948796a6668b15d4dd60" + if expectedDigestHex != expectedPinnedHex { + t.Fatalf( + "test vector itself is inconsistent\nindependent: %s\npinned: %s", + expectedDigestHex, + expectedPinnedHex, + ) + } + + actualDigest, err := signerApprovalCertificateSigningDigest(approvalDigest, endBlock) + if err != nil { + t.Fatal(err) + } + actualDigestHex := "0x" + hex.EncodeToString(actualDigest) + + if actualDigestHex != expectedDigestHex { + t.Fatalf( + "unexpected v2 signing digest\nexpected: %s\nactual: %s", + expectedDigestHex, + actualDigestHex, + ) + } +} diff --git a/pkg/tbtc/signing.go b/pkg/tbtc/signing.go index 40bf947d3b..2a735a7401 100644 --- a/pkg/tbtc/signing.go +++ b/pkg/tbtc/signing.go @@ -248,11 +248,19 @@ func (se *signingExecutor) sign( se.membershipValidator, ) - doneCheck := newSigningDoneCheck( - se.groupParameters.GroupSize, - se.broadcastChannel, - se.membershipValidator, - ) + // A fresh signingDoneCheck is constructed for every retry attempt + // (see signingRetryLoop.start) rather than reusing one instance + // across attempts. Reusing a single instance previously let a + // slow-to-exit listener goroutine from a failed attempt race + // with the very next attempt's reset of the same shared struct + // fields. + newDoneCheck := func() signingDoneCheckStrategy { + return newSigningDoneCheck( + se.groupParameters.GroupSize, + se.broadcastChannel, + se.membershipValidator, + ) + } retryLoop := newSigningRetryLoop( signingLogger, @@ -262,7 +270,7 @@ func (se *signingExecutor) sign( wallet.signingGroupOperators, se.groupParameters, announcer, - doneCheck, + newDoneCheck, ) // Set up the loop timeout signal. This context is associated with diff --git a/pkg/tbtc/signing_done.go b/pkg/tbtc/signing_done.go index 58dfeccc83..2ffe8c1283 100644 --- a/pkg/tbtc/signing_done.go +++ b/pkg/tbtc/signing_done.go @@ -96,6 +96,17 @@ func (sdc *signingDoneCheck) listen( }) sdc.expectedSignersCount = len(attemptMembersIndexes) + // Record the members selected for this attempt so that done messages from + // members excluded from the current attempt can be rejected. This lookup is + // kept local to the listener goroutine below (captured by its closure and + // passed to isValidDoneMessage) instead of being stored on the shared + // signingDoneCheck struct. Keeping it out of shared state means concurrent + // listen calls on the same instance cannot race, and in particular cannot + // trigger a "concurrent map writes" fatal error, when populating it. + attemptMembers := make(map[group.MemberIndex]bool) + for _, memberIndex := range attemptMembersIndexes { + attemptMembers[memberIndex] = true + } sdc.doneSigners = make(map[group.MemberIndex]*signingDoneMessage) go func() { @@ -113,6 +124,7 @@ func (sdc *signingDoneCheck) listen( message, attemptNumber, attemptTimeoutBlock, + attemptMembers, ) { continue } @@ -128,6 +140,19 @@ func (sdc *signingDoneCheck) listen( }() } +// stopListening cancels the message receiver started by listen, stopping its +// listener goroutine. It is safe to call even if listen was never called, +// and safe to call more than once (waitUntilAllDone also calls it, via +// cancelReceiveCtx, when the attempt reaches that point normally). Call it +// on any early-exit path taken after listen but before waitUntilAllDone, so +// a failed attempt's listener does not linger until its context's own block +// timeout. +func (sdc *signingDoneCheck) stopListening() { + if sdc.cancelReceiveCtx != nil { + sdc.cancelReceiveCtx() + } +} + // signalDone broadcasts the signing done check along with information necessary // to attribute the result to the given signing attempt. func (sdc *signingDoneCheck) signalDone( @@ -169,30 +194,44 @@ func (sdc *signingDoneCheck) waitUntilAllDone(ctx context.Context) ( return nil, 0, errWaitDoneTimedOut case <-ticker.C: - if sdc.expectedSignersCount == len(sdc.doneSigners) { - var signature *tecdsa.Signature - var latestEndBlock uint64 - - for _, doneMessage := range sdc.doneSigners { - if signature == nil { - signature = doneMessage.signature - } else { - if !signature.Equals(doneMessage.signature) { - return nil, 0, fmt.Errorf( - "not matching signatures detected: [%v] and [%v]", - signature, - doneMessage.signature, - ) - } - } - - if doneMessage.endBlock > latestEndBlock { - latestEndBlock = doneMessage.endBlock - } + // Read doneSigners under the mutex to avoid a data race with the + // listen goroutine that writes to the same map. Results are captured + // into local variables so the lock can be released before returning. + sdc.doneSignersMutex.Lock() + + if sdc.expectedSignersCount != len(sdc.doneSigners) { + sdc.doneSignersMutex.Unlock() + continue + } + + var signature *tecdsa.Signature + var latestEndBlock uint64 + var mismatchedSignature *tecdsa.Signature + + for _, doneMessage := range sdc.doneSigners { + if signature == nil { + signature = doneMessage.signature + } else if !signature.Equals(doneMessage.signature) { + mismatchedSignature = doneMessage.signature + break } - return &signing.Result{Signature: signature}, latestEndBlock, nil + if doneMessage.endBlock > latestEndBlock { + latestEndBlock = doneMessage.endBlock + } + } + + sdc.doneSignersMutex.Unlock() + + if mismatchedSignature != nil { + return nil, 0, fmt.Errorf( + "not matching signatures detected: [%v] and [%v]", + signature, + mismatchedSignature, + ) } + + return &signing.Result{Signature: signature}, latestEndBlock, nil } } } @@ -205,8 +244,11 @@ func (sdc *signingDoneCheck) isValidDoneMessage( message *big.Int, attemptNumber uint64, attemptTimeoutBlock uint64, + attemptMembers map[group.MemberIndex]bool, ) bool { + sdc.doneSignersMutex.Lock() _, signerDone := sdc.doneSigners[doneMessage.senderID] + sdc.doneSignersMutex.Unlock() if signerDone { // only one done message allowed return false @@ -219,6 +261,13 @@ func (sdc *signingDoneCheck) isValidDoneMessage( return false } + // The sender must have been selected for the current signing attempt. A + // valid wallet group member that was excluded from this attempt must not + // count toward its completion threshold. + if !attemptMembers[doneMessage.senderID] { + return false + } + if doneMessage.message.Cmp(message) != 0 { return false } diff --git a/pkg/tbtc/signing_done_test.go b/pkg/tbtc/signing_done_test.go index 792edd6b68..1c2ff05b7a 100644 --- a/pkg/tbtc/signing_done_test.go +++ b/pkg/tbtc/signing_done_test.go @@ -60,6 +60,19 @@ func TestSigningDoneCheck(t *testing.T) { err error } + // listen is called once, matching how a single local node uses + // signingDoneCheck for one attempt; the other simulated members' + // perspectives are represented by their signalDone messages arriving + // over the (shared, in-memory) broadcast channel below, not by each of + // them separately calling listen on this same instance. + doneCheck.listen( + ctx, + message, + attemptNumber, + attemptTimeoutBlock, + attemptMemberIndexes, + ) + wg := sync.WaitGroup{} wg.Add(len(memberIndexes)) outcomesChan := make(chan *outcome, len(memberIndexes)) @@ -68,14 +81,6 @@ func TestSigningDoneCheck(t *testing.T) { go func(memberIndex group.MemberIndex) { defer wg.Done() - doneCheck.listen( - ctx, - message, - attemptNumber, - attemptTimeoutBlock, - attemptMemberIndexes, - ) - if slices.Contains(attemptMemberIndexes, memberIndex) { err := doneCheck.signalDone( ctx, @@ -293,12 +298,279 @@ func TestSigningDoneCheck_AnotherSignature(t *testing.T) { } } +// TestSigningDoneCheck_ConcurrentDoneSignersAccess exercises the concurrent +// access to the doneSigners map: the listen goroutine writes to the map as done +// messages arrive while waitUntilAllDone reads it on every tick. It is meant to +// be run with the race detector (go test -race); without the doneSigners mutex +// guarding every access, the concurrent read/write is a data race. +// +// Done messages are broadcast with the standard, stateless retransmission +// strategy so this test isolates the doneSigners race and does not depend on +// the separate backoff-strategy synchronization. +func TestSigningDoneCheck_ConcurrentDoneSignersAccess(t *testing.T) { + groupParameters := &GroupParameters{ + GroupSize: 5, + GroupQuorum: 4, + HonestThreshold: 3, + } + + doneCheck := setupSigningDoneCheck(t, groupParameters) + + memberIndexes := make([]group.MemberIndex, doneCheck.groupSize) + for i := range memberIndexes { + memberIndexes[i] = group.MemberIndex(i + 1) + } + + ctx, cancelCtx := context.WithTimeout(context.Background(), 10*time.Second) + defer cancelCtx() + + message := big.NewInt(100) + attemptNumber := uint64(1) + attemptTimeoutBlock := uint64(1000) + attemptMemberIndexes := memberIndexes[:groupParameters.HonestThreshold] + result := &signing.Result{ + Signature: &tecdsa.Signature{ + R: big.NewInt(200), + S: big.NewInt(300), + RecoveryID: 2, + }, + } + + // Start the listener goroutine, which writes to doneSigners as messages + // arrive. + doneCheck.listen( + ctx, + message, + attemptNumber, + attemptTimeoutBlock, + attemptMemberIndexes, + ) + + // Concurrently broadcast every attempt member's done message so the listener + // writes to doneSigners while waitUntilAllDone reads it below. + for _, memberIndex := range attemptMemberIndexes { + go func(memberIndex group.MemberIndex) { + _ = doneCheck.broadcastChannel.Send( + ctx, + &signingDoneMessage{ + senderID: memberIndex, + message: message, + attemptNumber: attemptNumber, + signature: result.Signature, + endBlock: 500 + uint64(memberIndex), + }, + net.StandardRetransmissionStrategy, + ) + }(memberIndex) + } + + returnedResult, _, err := doneCheck.waitUntilAllDone(ctx) + if err != nil { + t.Fatalf("unexpected error: [%v]", err) + } + if returnedResult == nil { + t.Fatal("unexpected nil result") + } + if !result.Signature.Equals(returnedResult.Signature) { + t.Errorf( + "unexpected signature\nexpected: [%v]\nactual: [%v]", + result.Signature, + returnedResult.Signature, + ) + } +} + +// TestSigningDoneCheck_RejectsNonAttemptMember covers the scenario where a +// valid wallet group member that was not selected for the current signing +// attempt sends a done message. Such a message must not count toward the +// attempt completion threshold; otherwise the check could complete before all +// selected signers finish. +func TestSigningDoneCheck_RejectsNonAttemptMember(t *testing.T) { + groupParameters := &GroupParameters{ + GroupSize: 5, + GroupQuorum: 4, + HonestThreshold: 3, + } + + doneCheck := setupSigningDoneCheck(t, groupParameters) + + memberIndexes := make([]group.MemberIndex, doneCheck.groupSize) + for i := range memberIndexes { + memberIndexes[i] = group.MemberIndex(i + 1) + } + + // The attempt selects members 1, 2, and 3. Member 5 is a valid wallet group + // member but is excluded from this attempt. + attemptMemberIndexes := memberIndexes[:groupParameters.HonestThreshold] + nonAttemptMember := memberIndexes[groupParameters.GroupSize-1] + + ctx, cancelCtx := context.WithTimeout(context.Background(), 500*time.Millisecond) + defer cancelCtx() + + message := big.NewInt(100) + attemptNumber := uint64(1) + attemptTimeoutBlock := uint64(1000) + result := &signing.Result{ + Signature: &tecdsa.Signature{ + R: big.NewInt(200), + S: big.NewInt(300), + RecoveryID: 2, + }, + } + + doneCheck.listen( + ctx, + message, + attemptNumber, + attemptTimeoutBlock, + attemptMemberIndexes, + ) + + // Two attempt members (1 and 2) plus the excluded member (5) send done + // messages. If the excluded member were counted, waitUntilAllDone would see + // three messages, match the expected count, and complete before the third + // attempt member (3) reports. + for _, memberIndex := range []group.MemberIndex{ + attemptMemberIndexes[0], + attemptMemberIndexes[1], + nonAttemptMember, + } { + err := doneCheck.signalDone( + ctx, + memberIndex, + message, + attemptNumber, + result, + 100, + ) + if err != nil { + t.Fatal(err) + } + } + + // The excluded member must not count toward the completion threshold, so + // waitUntilAllDone must time out rather than returning a result. + returnedResult, endBlock, err := doneCheck.waitUntilAllDone(ctx) + if returnedResult != nil { + t.Errorf("expected nil result, got [%v]", returnedResult) + } + testutils.AssertIntsEqual(t, "end block", 0, int(endBlock)) + testutils.AssertErrorsSame(t, errWaitDoneTimedOut, err) +} + +// TestSigningDoneCheck_StaleAttemptDoesNotAffectNextAttempt verifies the fix +// for the retry-interleaving hazard: production code (signing.go, +// signing_loop.go) constructs a fresh signingDoneCheck for every retry +// attempt instead of reusing one instance. This test reproduces the +// interleaving that motivates that fix - an earlier attempt's listener is +// still running (it failed before reaching waitUntilAllDone, so nothing +// canceled its receive context yet) when the next attempt starts listening, +// and a stale done message from the earlier attempt is delivered after that +// - and asserts the stale message cannot be miscounted into the next +// attempt's result. Run with -race: reusing one instance for both attempts +// here would both data-race on receiveCtx/doneSigners and let the stale +// message land in the next attempt's map, since isValidDoneMessage's +// membership map lookup and both listener goroutines would share it. +func TestSigningDoneCheck_StaleAttemptDoesNotAffectNextAttempt(t *testing.T) { + groupParameters := &GroupParameters{ + GroupSize: 5, + GroupQuorum: 4, + HonestThreshold: 3, + } + + newDoneCheck := setupSigningDoneCheckFactory(t, groupParameters) + + memberIndexes := make([]group.MemberIndex, groupParameters.GroupSize) + for i := range memberIndexes { + memberIndexes[i] = group.MemberIndex(i + 1) + } + attemptMemberIndexes := memberIndexes[:groupParameters.HonestThreshold] + + message := big.NewInt(100) + attemptTimeoutBlock := uint64(1000) + + ctx, cancelCtx := context.WithTimeout(context.Background(), 10*time.Second) + defer cancelCtx() + + // Attempt 1 starts listening but never reaches waitUntilAllDone, as if + // signingAttemptFn or signalDone had failed right after listen - the + // exact interleaving signing_loop.go's early continue paths handle. + attempt1DoneCheck := newDoneCheck() + defer attempt1DoneCheck.stopListening() + attempt1DoneCheck.listen(ctx, message, 1, attemptTimeoutBlock, attemptMemberIndexes) + + // Attempt 2 gets its own, fresh instance sharing the same broadcast + // channel - this is the fix under test. + attempt2DoneCheck := newDoneCheck() + defer attempt2DoneCheck.stopListening() + attempt2DoneCheck.listen(ctx, message, 2, attemptTimeoutBlock, attemptMemberIndexes) + + staleResult := &signing.Result{ + Signature: &tecdsa.Signature{R: big.NewInt(999), S: big.NewInt(999), RecoveryID: 1}, + } + attempt2Result := &signing.Result{ + Signature: &tecdsa.Signature{R: big.NewInt(200), S: big.NewInt(300), RecoveryID: 2}, + } + + // A stale attempt-1 done message (e.g. a retransmission) arrives late, + // after attempt 2 has already started listening. + if err := attempt1DoneCheck.signalDone( + ctx, attemptMemberIndexes[0], message, 1, staleResult, 600, + ); err != nil { + t.Fatal(err) + } + + // The real attempt-2 quorum signals done. + for _, memberIndex := range attemptMemberIndexes { + if err := attempt2DoneCheck.signalDone( + ctx, memberIndex, message, 2, attempt2Result, 500+uint64(memberIndex), + ); err != nil { + t.Fatal(err) + } + } + + returnedResult, _, err := attempt2DoneCheck.waitUntilAllDone(ctx) + if err != nil { + t.Fatalf("unexpected error: [%v]", err) + } + if returnedResult == nil || !returnedResult.Signature.Equals(attempt2Result.Signature) { + t.Fatalf( + "expected attempt 2 to complete with its own signature, got [%v]", + returnedResult, + ) + } + + // The stale attempt-1 message must not have been miscounted into + // attempt 2's map: exactly the attempt-2 quorum, no more. + attempt2DoneCheck.doneSignersMutex.Lock() + doneSignersCount := len(attempt2DoneCheck.doneSigners) + attempt2DoneCheck.doneSignersMutex.Unlock() + testutils.AssertIntsEqual( + t, + "attempt 2 doneSigners count", + len(attemptMemberIndexes), + doneSignersCount, + ) +} + // setupSigningDoneCheck sets up an instance of the signing done check ready // to perform test checks. func setupSigningDoneCheck( t *testing.T, groupParameters *GroupParameters, ) *signingDoneCheck { + return setupSigningDoneCheckFactory(t, groupParameters)() +} + +// setupSigningDoneCheckFactory sets up the shared network plumbing (broadcast +// channel, membership validator) a signing done check needs, and returns a +// factory that constructs a fresh signingDoneCheck sharing that plumbing - +// mirroring how production code (see signing.go) constructs one instance per +// retry attempt rather than reusing a single instance. +func setupSigningDoneCheckFactory( + t *testing.T, + groupParameters *GroupParameters, +) func() *signingDoneCheck { operatorPrivateKey, operatorPublicKey, err := operator.GenerateKeyPair( local_v1.DefaultCurve, ) @@ -337,9 +609,11 @@ func setupSigningDoneCheck( localChain.Signing(), ) - return newSigningDoneCheck( - groupParameters.GroupSize, - broadcastChannel, - membershipValidator, - ) + return func() *signingDoneCheck { + return newSigningDoneCheck( + groupParameters.GroupSize, + broadcastChannel, + membershipValidator, + ) + } } diff --git a/pkg/tbtc/signing_loop.go b/pkg/tbtc/signing_loop.go index 7e787f1975..d0f0ad3ce4 100644 --- a/pkg/tbtc/signing_loop.go +++ b/pkg/tbtc/signing_loop.go @@ -76,6 +76,10 @@ type signingDoneCheckStrategy interface { ) error waitUntilAllDone(ctx context.Context) (*signing.Result, uint64, error) + + // stopListening stops the listener goroutine started by listen. Called + // on any early-exit path taken after listen but before waitUntilAllDone. + stopListening() } // signingRetryLoop is a struct that encapsulates the signing retry logic. @@ -95,7 +99,10 @@ type signingRetryLoop struct { attemptStartBlock uint64 attemptSeed int64 - doneCheck signingDoneCheckStrategy + // newDoneCheck constructs a fresh signingDoneCheckStrategy for each retry + // attempt (see start), instead of one instance being reused across + // attempts. + newDoneCheck func() signingDoneCheckStrategy } func newSigningRetryLoop( @@ -106,7 +113,7 @@ func newSigningRetryLoop( signingGroupOperators chain.Addresses, groupParameters *GroupParameters, announcer signingAnnouncer, - doneCheck signingDoneCheckStrategy, + newDoneCheck func() signingDoneCheckStrategy, ) *signingRetryLoop { // Compute the 8-byte seed needed for the random retry algorithm. We take // the first 8 bytes of the hash of the signed message. This allows us to @@ -125,7 +132,7 @@ func newSigningRetryLoop( attemptCounter: 0, attemptStartBlock: initialStartBlock, attemptSeed: attemptSeed, - doneCheck: doneCheck, + newDoneCheck: newDoneCheck, } } @@ -339,7 +346,17 @@ func (srl *signingRetryLoop) start( // participants have a chance to receive signingDoneMessage. doneCheckTimeoutCtx, _ := withCancelOnBlock(ctx, timeoutBlock, waitForBlockFn) - srl.doneCheck.listen( + // A fresh doneCheck is constructed for this attempt alone, rather + // than reusing one instance across attempts: reusing a single + // instance let a slow-to-exit listener goroutine from a failed + // attempt still be reading/writing the shared struct fields this + // same code resets for the next attempt. Every early-exit path below + // (taken after listen but before waitUntilAllDone) explicitly stops + // this attempt's listener so it does not linger until + // doneCheckTimeoutCtx's own block timeout. + doneCheck := srl.newDoneCheck() + + doneCheck.listen( doneCheckTimeoutCtx, srl.message, uint64(srl.attemptCounter), @@ -368,6 +385,7 @@ func (srl *signingRetryLoop) start( srl.attemptCounter, err, ) + doneCheck.stopListening() continue } @@ -377,7 +395,7 @@ func (srl *signingRetryLoop) start( srl.attemptCounter, ) - err = srl.doneCheck.signalDone( + err = doneCheck.signalDone( doneCheckTimeoutCtx, srl.signingGroupMemberIndex, srl.message, @@ -393,6 +411,7 @@ func (srl *signingRetryLoop) start( srl.attemptCounter, err, ) + doneCheck.stopListening() continue } } else { @@ -404,7 +423,7 @@ func (srl *signingRetryLoop) start( ) } - result, latestEndBlock, err := srl.doneCheck.waitUntilAllDone(doneCheckTimeoutCtx) + result, latestEndBlock, err := doneCheck.waitUntilAllDone(doneCheckTimeoutCtx) if err != nil { srl.logger.Warnf( "[member:%v] cannot wait for signing done "+ diff --git a/pkg/tbtc/signing_loop_test.go b/pkg/tbtc/signing_loop_test.go index 93397a9ef2..52bde5bac1 100644 --- a/pkg/tbtc/signing_loop_test.go +++ b/pkg/tbtc/signing_loop_test.go @@ -600,6 +600,11 @@ func TestSigningRetryLoop(t *testing.T) { incomingAnnouncementsFn: test.incomingAnnouncementsFn, } + // The retry loop under test constructs a fresh doneCheck per + // attempt (see signingRetryLoop.start), but this suite asserts + // on outgoingDoneChecks accumulated across every attempt of a + // single test case, so the factory always returns the same + // mock instance rather than a new one per attempt. doneCheck := &mockSigningDoneCheck{ waitUntilAllDoneOutcomeFn: test.waitUntilAllDoneOutcomeFn, } @@ -612,7 +617,7 @@ func TestSigningRetryLoop(t *testing.T) { signingGroupOperators, groupParameters, announcer, - doneCheck, + func() signingDoneCheckStrategy { return doneCheck }, ) ctx, cancelCtx := test.ctxFn() @@ -760,3 +765,5 @@ func (msdc *mockSigningDoneCheck) signalDone( func (msdc *mockSigningDoneCheck) waitUntilAllDone(ctx context.Context) (*signing.Result, uint64, error) { return msdc.waitUntilAllDoneOutcomeFn(msdc.currentAttemptNumber) } + +func (msdc *mockSigningDoneCheck) stopListening() {} diff --git a/pkg/tbtc/tbtc.go b/pkg/tbtc/tbtc.go index 70a9756e98..e746dc1f90 100644 --- a/pkg/tbtc/tbtc.go +++ b/pkg/tbtc/tbtc.go @@ -85,6 +85,7 @@ func Initialize( clientInfo *clientinfo.Registry, perfMetrics *clientinfo.PerformanceMetrics, minActiveOutpointConfirmations uint, + bridgeCovenantFraudDefenseConfirmed bool, ) (covenantsigner.Engine, error) { groupParameters := &GroupParameters{ GroupSize: 100, @@ -172,161 +173,142 @@ func Initialize( _ = chain.OnDKGStarted(func(event *DKGStartedEvent) { go func() { - if ok := deduplicator.notifyDKGStarted( - event.Seed, - ); !ok { - logger.Infof( - "DKG started event with seed [0x%x] has been "+ - "already processed", - event.Seed, - ) - return - } - - confirmationBlock := event.BlockNumber + dkgStartedConfirmationBlocks - - logger.Infof( - "observed DKG started event with seed [0x%x] and "+ - "starting block [%v]; waiting for block [%v] to confirm", - event.Seed, - event.BlockNumber, - confirmationBlock, + // handleDKGStartedEvent records the deduplication entry as completed + // only once the local DKG join has been dispatched (or the event was + // authoritatively found unconfirmed), so a transient early return in + // the handler below leaves the event retryable on redelivery. + handleDKGStartedEvent( + deduplicator, + event, + func(event *DKGStartedEvent) error { + confirmationBlock := event.BlockNumber + dkgStartedConfirmationBlocks + + logger.Infof( + "observed DKG started event with seed [0x%x] and "+ + "starting block [%v]; waiting for block [%v] to confirm", + event.Seed, + event.BlockNumber, + confirmationBlock, + ) + + if err := node.waitForBlockHeight(ctx, confirmationBlock); err != nil { + return fmt.Errorf( + "failed to confirm DKG started event: [%w]", + err, + ) + } + + dkgState, err := chain.GetDKGState() + if err != nil { + return fmt.Errorf("failed to check DKG state: [%w]", err) + } + + if dkgState != AwaitingResult { + logger.Infof( + "DKG started event with seed [0x%x] and starting "+ + "block [%v] was not confirmed", + event.Seed, + event.BlockNumber, + ) + + // The event was authoritatively determined to be + // unconfirmed; there is nothing to retry, so treat it as + // terminally handled. + return nil + } + + // Fetch all past DKG started events starting from one + // confirmation period before the original event's block. + // If there was a chain reorg, the event we received could be + // moved to a block with a lower number than the one + // we received. + pastEvents, err := chain.PastDKGStartedEvents( + &DKGStartedEventFilter{ + StartBlock: event.BlockNumber - dkgStartedConfirmationBlocks, + }, + ) + if err != nil { + return fmt.Errorf( + "failed to get past DKG started events: [%w]", + err, + ) + } + + // Should not happen but just in case. + if len(pastEvents) == 0 { + return fmt.Errorf("no past DKG started events") + } + + lastEvent := pastEvents[len(pastEvents)-1] + + logger.Infof( + "DKG started with seed [0x%x] at block [%v]", + lastEvent.Seed, + lastEvent.BlockNumber, + ) + + // The off-chain protocol should be started as close as + // possible to the current block or even further. Starting the + // off-chain protocol with a past block will likely cause a + // failure of the first attempt as the start block is used to + // synchronize the announcements and the state machine. Here we + // ensure a proper start point by delaying the execution by the + // confirmation period length. + node.joinDKGIfEligible( + lastEvent.Seed, + lastEvent.BlockNumber, + dkgStartedConfirmationBlocks, + ) + + // The local DKG join has been dispatched; the event is + // terminally handled. + return nil + }, ) - - err := node.waitForBlockHeight(ctx, confirmationBlock) - if err != nil { - logger.Errorf("failed to confirm DKG started event: [%v]", err) - return - } - - dkgState, err := chain.GetDKGState() - if err != nil { - logger.Errorf("failed to check DKG state: [%v]", err) - return - } - - if dkgState == AwaitingResult { - // Fetch all past DKG started events starting from one - // confirmation period before the original event's block. - // If there was a chain reorg, the event we received could be - // moved to a block with a lower number than the one - // we received. - pastEvents, err := chain.PastDKGStartedEvents( - &DKGStartedEventFilter{ - StartBlock: event.BlockNumber - dkgStartedConfirmationBlocks, - }, - ) - if err != nil { - logger.Errorf("failed to get past DKG started events: [%v]", err) - return - } - - // Should not happen but just in case. - if len(pastEvents) == 0 { - logger.Errorf("no past DKG started events") - return - } - - lastEvent := pastEvents[len(pastEvents)-1] - - logger.Infof( - "DKG started with seed [0x%x] at block [%v]", - lastEvent.Seed, - lastEvent.BlockNumber, - ) - - // The off-chain protocol should be started as close as possible - // to the current block or even further. Starting the off-chain - // protocol with a past block will likely cause a failure of the - // first attempt as the start block is used to synchronize - // the announcements and the state machine. Here we ensure - // a proper start point by delaying the execution by the - // confirmation period length. - node.joinDKGIfEligible( - lastEvent.Seed, - lastEvent.BlockNumber, - dkgStartedConfirmationBlocks, - ) - } else { - logger.Infof( - "DKG started event with seed [0x%x] and starting "+ - "block [%v] was not confirmed", - event.Seed, - event.BlockNumber, - ) - } }() }) _ = chain.OnDKGResultSubmitted(func(event *DKGResultSubmittedEvent) { go func() { - if ok := deduplicator.notifyDKGResultSubmitted( - event.Seed, - event.ResultHash, - event.BlockNumber, - ); !ok { - logger.Warnf( - "Result with hash [0x%x] for DKG with seed [0x%x] "+ - "and starting block [%v] has been already processed", - event.ResultHash, - event.Seed, - event.BlockNumber, - ) - return - } - - logger.Infof( - "Result with hash [0x%x] for DKG with seed [0x%x] "+ - "submitted at block [%v]", - event.ResultHash, - event.Seed, - event.BlockNumber, - ) - - node.validateDKG( - event.Seed, - event.BlockNumber, - event.Result, - event.ResultHash, + // handleDKGResultSubmittedEvent records the deduplication entry as + // completed only after validation reaches a terminal state, so a + // transient failure below leaves the event retryable on redelivery. + handleDKGResultSubmittedEvent( + deduplicator, + event, + func(event *DKGResultSubmittedEvent) error { + return node.validateDKG( + event.Seed, + event.BlockNumber, + event.Result, + event.ResultHash, + ) + }, ) }() }) _ = chain.OnWalletClosed(func(event *WalletClosedEvent) { go func() { - if ok := deduplicator.notifyWalletClosed( - event.WalletID, - ); !ok { - logger.Warnf( - "Wallet closure for wallet with ID [0x%x] at block [%v] "+ - "has been already processed", - event.WalletID, - event.BlockNumber, - ) - return - } - - logger.Infof( - "Wallet with ID [0x%x] has been closed at block [%v]; "+ - "proceeding with handling wallet closure", - event.WalletID, - event.BlockNumber, - ) - - err := node.handleWalletClosure( - event.WalletID, + // handleWalletClosedEvent records the deduplication entry as + // completed only after the wallet has actually been archived, so a + // transient archival failure below leaves the event retryable on + // redelivery. + handleWalletClosedEvent( + deduplicator, + event, + func(event *WalletClosedEvent) error { + return node.handleWalletClosure(event.WalletID) + }, ) - if err != nil { - logger.Errorf( - "Failure while handling wallet closure with ID [0x%x]: [%v]", - event.WalletID, - err, - ) - } }() }) - return newCovenantSignerEngine(node, minActiveOutpointConfirmations), nil + return newCovenantSignerEngine( + node, + minActiveOutpointConfirmations, + bridgeCovenantFraudDefenseConfirmed, + ), nil } // enoughPreParamsInPoolPolicy is a policy that enforces the sufficient size diff --git a/pkg/tbtc/wallet_closed_handler.go b/pkg/tbtc/wallet_closed_handler.go new file mode 100644 index 0000000000..591fd75f36 --- /dev/null +++ b/pkg/tbtc/wallet_closed_handler.go @@ -0,0 +1,59 @@ +package tbtc + +// handleWalletClosedEvent runs the deduplication-guarded handling of a +// WalletClosed event. It claims the event, runs handle, and then either confirms +// the deduplication entry once the wallet has actually been archived (so +// subsequent deliveries of the same event are ignored as duplicates) or releases +// the claim when handle returns an error, so a later redelivery of the same +// event can retry the archival. +// +// handle performs the actual wallet-closure archival and returns a non-nil error +// when archival did not complete (e.g. a transient RPC error while waiting for +// closure confirmation, or a failed archive write). Releasing the claim on such +// a failure is what prevents a closed wallet from remaining signable through +// fresh getSigningExecutor lookups until process restart: the deduplication +// entry is recorded as completed only after the wallet is removed from the local +// registry. +func handleWalletClosedEvent( + deduplicator *deduplicator, + event *WalletClosedEvent, + handle func(event *WalletClosedEvent) error, +) { + if ok := deduplicator.notifyWalletClosed( + event.WalletID, + ); !ok { + logger.Warnf( + "Wallet closure for wallet with ID [0x%x] at block [%v] "+ + "has been already processed", + event.WalletID, + event.BlockNumber, + ) + return + } + + logger.Infof( + "Wallet with ID [0x%x] has been closed at block [%v]; "+ + "proceeding with handling wallet closure", + event.WalletID, + event.BlockNumber, + ) + + if err := handle(event); err != nil { + // Archival did not complete (e.g. a transient RPC error while + // waiting for closure confirmation, or a failed archive write). + // Release the deduplication claim so a later redelivery of the + // same event retries the archival instead of being dropped as + // an already-processed duplicate, which would otherwise leave + // the closed wallet signable in the local registry. + logger.Errorf( + "Failure while handling wallet closure with ID [0x%x]; "+ + "allowing retry on event redelivery: [%v]", + event.WalletID, + err, + ) + deduplicator.abortWalletClosed(event.WalletID) + return + } + + deduplicator.confirmWalletClosed(event.WalletID) +} diff --git a/pkg/tbtc/wallet_closed_handler_test.go b/pkg/tbtc/wallet_closed_handler_test.go new file mode 100644 index 0000000000..e5ecd1cdd0 --- /dev/null +++ b/pkg/tbtc/wallet_closed_handler_test.go @@ -0,0 +1,97 @@ +package tbtc + +import ( + "fmt" + "testing" + + "github.com/keep-network/keep-core/internal/testutils" +) + +// TestHandleWalletClosedEvent_RetriesAfterArchivalFailure verifies the +// TOB-TBTCACEXT-57 fix at the callback level: when the first delivery of a +// WalletClosed event fails during archival, an identical redelivery must reach +// the archival handler again instead of being dropped as an already-processed +// duplicate. Once archival succeeds, a further redelivery must be ignored as a +// duplicate. +func TestHandleWalletClosedEvent_RetriesAfterArchivalFailure(t *testing.T) { + deduplicator := newDeduplicator() + + event := &WalletClosedEvent{ + WalletID: [32]byte{0x01, 0x02, 0x03}, + BlockNumber: 1000, + } + + var archivalCalls int + handle := func(*WalletClosedEvent) error { + archivalCalls++ + if archivalCalls == 1 { + // Simulate a transient failure while waiting for closure + // confirmation before the wallet could be archived. + return fmt.Errorf("transient archival failure") + } + return nil + } + + // First delivery: the event is claimed, archival fails, and the claim is + // released so the event stays retryable. + handleWalletClosedEvent(deduplicator, event, handle) + + // Redelivery of the identical event must reach the archival handler again + // rather than being dropped as an already-processed duplicate; otherwise the + // closed wallet would remain signable in the local registry. + handleWalletClosedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "archival calls after a failed delivery and its retry", + 2, + archivalCalls, + ) + + // The retry succeeded and confirmed the event, so a further redelivery must + // be ignored as a duplicate and must not reach the archival handler again. + handleWalletClosedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "archival calls after the event was confirmed handled", + 2, + archivalCalls, + ) +} + +// TestHandleWalletClosedEvent_IgnoresConcurrentDuplicate verifies that while a +// WalletClosed event is being handled, a concurrent duplicate delivery of the +// same event is ignored rather than starting a second archival. +func TestHandleWalletClosedEvent_IgnoresConcurrentDuplicate(t *testing.T) { + deduplicator := newDeduplicator() + + event := &WalletClosedEvent{ + WalletID: [32]byte{0x0a, 0x0b, 0x0c}, + BlockNumber: 2000, + } + + var nestedCalls int + handle := func(*WalletClosedEvent) error { + // While this archival is in progress, a duplicate delivery must be + // dropped and must not enter the handler a second time. + handleWalletClosedEvent( + deduplicator, + event, + func(*WalletClosedEvent) error { + nestedCalls++ + return nil + }, + ) + return nil + } + + handleWalletClosedEvent(deduplicator, event, handle) + + testutils.AssertIntsEqual( + t, + "handler calls triggered by an in-progress duplicate", + 0, + nestedCalls, + ) +} diff --git a/pkg/tecdsa/retry/retry.go b/pkg/tecdsa/retry/retry.go index 246e8b1fae..798d3bed30 100644 --- a/pkg/tecdsa/retry/retry.go +++ b/pkg/tecdsa/retry/retry.go @@ -305,7 +305,7 @@ func excludeOperatorTriplets( for k := j + 1; k < len(operators); k++ { leftOperator := operators[i] middleOperator := operators[j] - rightOperator := operators[j] + rightOperator := operators[k] // Only include the operators triples that have few enough seats such // that if they were excluded we still have at least diff --git a/pkg/tecdsa/retry/retry_test.go b/pkg/tecdsa/retry/retry_test.go index 24775c4c7e..30e15060d9 100644 --- a/pkg/tecdsa/retry/retry_test.go +++ b/pkg/tecdsa/retry/retry_test.go @@ -2,6 +2,7 @@ package retry import ( "fmt" + "math/rand" "reflect" "strings" "testing" @@ -249,3 +250,90 @@ func assertInvariants( affectedBySeed(t, groupMemberRandomizer, groupMembers, seed, retryCount, retryParticipantsCount) affectedByRetryCount(t, groupMemberRandomizer, groupMembers, seed, retryCount, retryParticipantsCount) } + +// TestExcludeOperatorTriplets_EligibilityFilterUsesThirdOperatorSeats verifies +// that the triplet eligibility filter subtracts the seat count of all three +// distinct operators in a triple. The filter must count the third operator's +// (operators[k]) seats; reusing the middle operator's (operators[j]) seats in +// its place double-counts the middle operator and ignores the third, which both +// admits triples whose true post-exclusion seat count is below +// retryParticipantsCount and drops valid triples. +func TestExcludeOperatorTriplets_EligibilityFilterUsesThirdOperatorSeats(t *testing.T) { + // Four operators with deliberately distinguishable seat counts. The last + // operator (D) controls far more seats than the others, so every triple that + // includes D must be rejected once D's seats are correctly subtracted. + operators := []chain.Address{"A", "B", "C", "D"} + operatorToSeatCount := map[chain.Address]uint{ + "A": 1, + "B": 1, + "C": 1, + "D": 10, + } + + // groupMembers is consistent with the seat counts above: 1+1+1+10 = 13 seats. + groupMembers := make([]chain.Address, 0, 13) + for _, operator := range operators { + for i := uint(0); i < operatorToSeatCount[operator]; i++ { + groupMembers = append(groupMembers, operator) + } + } + + // With retryParticipantsCount = 5, only the triple {A, B, C} leaves enough + // seats: 13 - 1 - 1 - 1 = 10 >= 5. Every triple that includes D leaves + // 13 - 1 - 1 - 10 = 1 < 5 and must be filtered out. The buggy arithmetic + // (subtracting the middle operator's seats twice and ignoring D's seats) + // would instead admit all four triples. + retryParticipantsCount := 5 + + rng := rand.New(rand.NewSource(1)) + + // An out-of-range index makes excludeOperatorTriplets report the number of + // eligible triplets without shuffling, which is exactly the value produced + // by the eligibility filter. + _, eligibleTripletCount, ok := excludeOperatorTriplets( + rng, + groupMembers, + 1<<30, + operatorToSeatCount, + operators, + retryParticipantsCount, + ) + if ok { + t.Fatal("expected excludeOperatorTriplets to report an out-of-range index") + } + if eligibleTripletCount != 1 { + t.Fatalf( + "unexpected eligible triplet count\nexpected: [1] (only {A, B, C})\nactual: [%d]", + eligibleTripletCount, + ) + } + + // The single eligible triplet must be {A, B, C}; excluding it leaves only + // D's seats. If the filter had admitted a triple containing D, the resulting + // subset would still contain one of A, B, or C. + subset, _, ok := excludeOperatorTriplets( + rng, + groupMembers, + 0, + operatorToSeatCount, + operators, + retryParticipantsCount, + ) + if !ok { + t.Fatal("expected excludeOperatorTriplets to select the eligible triplet") + } + for _, operator := range subset { + if operator != "D" { + t.Errorf( + "subset should exclude {A, B, C} and contain only D seats, found [%s]", + operator, + ) + } + } + if len(subset) != 10 { + t.Errorf( + "unexpected subset size\nexpected: [10] (all D seats)\nactual: [%d]", + len(subset), + ) + } +} diff --git a/solidity-v1/contracts/TokenStakingEscrow.sol b/solidity-v1/contracts/TokenStakingEscrow.sol index e7a1564d2e..5192742ca7 100644 --- a/solidity-v1/contracts/TokenStakingEscrow.sol +++ b/solidity-v1/contracts/TokenStakingEscrow.sol @@ -442,7 +442,12 @@ contract TokenStakingEscrow is Ownable { address grantManager ) internal { uint256 amount = availableAmount(operator); - deposits[operator].withdrawn = amount; + // Add to the existing withdrawn counter instead of overwriting it, so + // that a prior partial withdrawal is not discarded. Overwriting would + // reset availableAmount and let the revoked deposit be withdrawn + // repeatedly, draining the escrow's shared token balance. This mirrors + // the accounting used by withdraw and migrate. + deposits[operator].withdrawn = deposit.withdrawn.add(amount); keepToken.safeTransfer(grantManager, amount); emit RevokedDepositWithdrawn(operator, grantManager, amount); diff --git a/solidity-v1/test/token_stake/TestTokenStakingEscrow.js b/solidity-v1/test/token_stake/TestTokenStakingEscrow.js index c135e62b4b..735d86fe86 100644 --- a/solidity-v1/test/token_stake/TestTokenStakingEscrow.js +++ b/solidity-v1/test/token_stake/TestTokenStakingEscrow.js @@ -565,6 +565,28 @@ describe("TokenStakingEscrow", () => { ) }) + it("does not allow a second withdrawal after a partial withdraw", async () => { + // Withdraw part of the unlocked deposit first, so the withdrawn counter + // is non-zero before the grant is revoked. + await time.increaseTo(grantStart.add(time.duration.days(15))) + await escrow.withdraw(operator, { from: operator }) // (300k / 30) * 15 = 150k KEEP + await tokenGrant.revoke(grantId, { from: grantManager }) + + // The first revoked withdrawal transfers the remaining 150k. + await escrow.withdrawRevoked(operator, { from: grantManager }) + + // A second revoked withdrawal must transfer nothing. Overwriting the + // withdrawn counter instead of adding to it would discard the prior + // partial withdrawal and let the same tokens be withdrawn again, draining + // the escrow's shared token balance. + const balanceBefore = await token.balanceOf(grantManager) + await escrow.withdrawRevoked(operator, { from: grantManager }) + const balanceAfter = await token.balanceOf(grantManager) + + const diff = balanceAfter.sub(balanceBefore) + expect(diff).to.eq.BN(0) + }) + it("withdraws entire deposited amount if nothing has been withdrawn before", async () => { await time.increaseTo(grantStart.add(time.duration.days(15))) await tokenGrant.revoke(grantId, { from: grantManager })