Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions zetaclient/chains/sui/signer/signer.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"context"
"crypto/sha256"
"fmt"
"runtime/debug"

"github.com/block-vision/sui-go-sdk/models"
suiptb "github.com/pattonkan/sui-go/sui"
Expand Down Expand Up @@ -92,14 +93,25 @@ func New(baseSigner *base.Signer,

// ProcessCCTX schedules outbound cross-chain transaction.
// Build --> Sign --> Broadcast --(async)--> Wait for execution --> PostOutboundTracker
func (s *Signer) ProcessCCTX(ctx context.Context, cctx *cctypes.CrossChainTx, zetaHeight uint64) error {
func (s *Signer) ProcessCCTX(ctx context.Context, cctx *cctypes.CrossChainTx, zetaHeight uint64) (err error) {
var (
outboundID = base.OutboundIDFromCCTX(cctx)
nonce = cctx.GetCurrentOutboundParam().TssNonce
)

s.MarkOutbound(outboundID, true)
defer func() { s.MarkOutbound(outboundID, false) }()
defer func() {
s.MarkOutbound(outboundID, false)
if r := recover(); r != nil {
s.Logger().Std.Error().
Str(logs.FieldCctxIndex, cctx.Index).
Str(logs.FieldOutboundID, outboundID).
Interface("panic", r).
Str("stack_trace", string(debug.Stack())).

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 debug.Stack() called after recover() — panic site stack is lost

Once recover() is called in a deferred function the Go runtime considers the panic resolved and unwinds the call stack to the deferred function's frame. Any subsequent call to debug.Stack() returns the current (deferred) goroutine stack, not the stack at the panic origin. The logged stack_trace field will point to the defer wrapper rather than the line that panicked, making post-incident analysis significantly harder. The same pattern exists in the EVM signer (evm/signer/signer.go:276), so this is a pre-existing footgun being replicated here. Consider capturing the stack trace before recover() or accepting that the field's value has limited diagnostic use.

Prompt To Fix With AI
This is a comment left during a code review.
Path: zetaclient/chains/sui/signer/signer.go
Line: 110

Comment:
**`debug.Stack()` called after `recover()` — panic site stack is lost**

Once `recover()` is called in a deferred function the Go runtime considers the panic resolved and unwinds the call stack to the deferred function's frame. Any subsequent call to `debug.Stack()` returns the current (deferred) goroutine stack, not the stack at the panic origin. The logged `stack_trace` field will point to the defer wrapper rather than the line that panicked, making post-incident analysis significantly harder. The same pattern exists in the EVM signer (`evm/signer/signer.go:276`), so this is a pre-existing footgun being replicated here. Consider capturing the stack trace before `recover()` or accepting that the field's value has limited diagnostic use.

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Addressed on the current branch: Sui now captures panicStack := debug.Stack() before calling recover(), then logs that captured stack when a panic is recovered. The TON signer added in this PR follows the same pattern.

Msg("caught panic error")
err = errors.Errorf("caught panic during outbound processing: %v", r)
}
}()

// prepare logger
logger := s.Logger().Std.With().Uint64(logs.FieldNonce, nonce).Logger()
Expand Down
35 changes: 35 additions & 0 deletions zetaclient/chains/sui/signer/signer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,6 +358,41 @@ func TestSigner(t *testing.T) {

require.Eventually(t, wait, 5*time.Second, 100*time.Millisecond)
})

t.Run("ProcessCCTX recovers panic", func(t *testing.T) {
// ARRANGE
ts := newTestSuite(t)

const zetaHeight = 1000

nonce := uint64(123)
receiver := "0xdecb47015beebed053c19ef48fe4d722fa3870f567133d235ebe3a70da7b0000"

cctx := sample.CrossChainTxV2(t, "0xABC123")
cctx.InboundParams = nil
cctx.OutboundParams = []*cc.OutboundParams{{
Receiver: receiver,
ReceiverChainId: ts.Chain.ChainId,
CoinType: coin.CoinType_Gas,
Amount: math.NewUint(100_000),
TssNonce: nonce,
GasPrice: "1000",
CallOptions: &cc.CallOptions{
GasLimit: 42,
},
}}
outboundID := base.OutboundIDFromCCTX(cctx)

ts.MockGatewayNonce(nonce)
ts.MockWithdrawCapID("0xWithdrawCapID")

// ACT
err := ts.Signer.ProcessCCTX(ts.Ctx, cctx, zetaHeight)

// ASSERT
require.ErrorContains(t, err, "caught panic during outbound processing")
require.False(t, ts.Signer.IsOutboundActive(outboundID))
})
}

type testSuite struct {
Expand Down
13 changes: 12 additions & 1 deletion zetaclient/chains/ton/signer/signer.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package signer

import (
"context"
"runtime/debug"
"strings"

"github.com/pkg/errors"
Expand Down Expand Up @@ -64,7 +65,17 @@ func (s *Signer) TryProcessOutbound(
) {
outboundID := base.OutboundIDFromCCTX(cctx)
s.MarkOutbound(outboundID, true)
defer s.MarkOutbound(outboundID, false)
defer func() {
s.MarkOutbound(outboundID, false)
if r := recover(); r != nil {
s.Logger().Std.Error().
Str(logs.FieldCctxIndex, cctx.Index).
Str(logs.FieldOutboundID, outboundID).
Interface("panic", r).
Str("stack_trace", string(debug.Stack())).
Msg("caught panic error")
}
}()

outcome, err := s.processOutbound(ctx, cctx, zetaRepo, zetaBlockHeight)

Expand Down
20 changes: 20 additions & 0 deletions zetaclient/chains/ton/signer/signer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,26 @@ func TestDrySigner(t *testing.T) {
require.Len(t, ts.trackerBag, 0)
}

func TestSigner_TryProcessOutboundRecoversPanic(t *testing.T) {
ts := newTestSuite(t)

const nonce uint64 = 2
cctx := sample.CrossChainTx(t, "panic")
cctx.InboundParams = nil
cctx.OutboundParams = []*cc.OutboundParams{{
ReceiverChainId: ts.chain.ChainId,
TssNonce: nonce,
}}
outboundID := base.OutboundIDFromCCTX(cctx)

signer := New(ts.baseSigner, ts.rpc, ts.gw)

require.NotPanics(t, func() {
signer.TryProcessOutbound(ts.ctx, cctx, ts.zetaRepo, 0)
})
require.False(t, signer.IsOutboundActive(outboundID))
}

func testSigner(t *testing.T, ts *testSuite, nonce uint64) (ton.Transaction, ton.Transaction) {
// Given TON signer
signer := New(ts.baseSigner, ts.rpc, ts.gw)
Expand Down
Loading