soroban-rpc: Remove panics from internal codebase (#1167)

* Remove panic - 1

* Remove panic - 2

* Remove panic - 3

* Remove panic - 4

* Small changes - 1

* undo changes in Get() func

* undo changes - 2

* undo changes - 3

* add test for append error

(cherry picked from commit b6671e2)

cmd/soroban-rpc/internal/config/config_option.go

@@ -88,8 +88,6 @@ func (o *ConfigOption) setValue(i interface{}) (err error) {
 	if o.CustomSetValue != nil {
 		return o.CustomSetValue(o, i)
 	}
-	// it's unfortunate that Set below panics when it cannot set the value..
-	// we'll want to catch this so that we can alert the user nicely.
 	defer func() {
 		if recoverRes := recover(); recoverRes != nil {
 			var ok bool
@@ -101,7 +99,7 @@ func (o *ConfigOption) setValue(i interface{}) (err error) {
 		}
 	}()
 	parser := func(option *ConfigOption, i interface{}) error {
-		panic(fmt.Sprintf("no parser for flag %s", o.Name))
+		return errors.Errorf("no parser for flag %s", o.Name)
 	}
 	switch o.ConfigKey.(type) {
 	case *bool:

cmd/soroban-rpc/internal/config/config_option_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"math"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -115,6 +116,16 @@ func TestUnassignableField(t *testing.T) {
 	require.Contains(t, err.Error(), co.Name)
 }
 
+func TestNoParserForFlag(t *testing.T) {
+	var co ConfigOption
+	var invalidKey []time.Duration
+	co.Name = "mykey"
+	co.ConfigKey = &invalidKey
+	err := co.setValue("abc")
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "no parser for flag mykey")
+}
+
 func TestSetValue(t *testing.T) {
 	var b bool
 	var i int

cmd/soroban-rpc/internal/config/log_format.go

@@ -1,6 +1,8 @@
 package config
 
-import "fmt"
+import (
+	"fmt"
+)
 
 type LogFormat int
 
@@ -47,13 +49,13 @@ func (f *LogFormat) UnmarshalTOML(i interface{}) error {
 	}
 }
 
-func (f LogFormat) String() string {
+func (f LogFormat) String() (string, error) {
 	switch f {
 	case LogFormatText:
-		return "text"
+		return "text", nil
 	case LogFormatJSON:
-		return "json"
+		return "json", nil
 	default:
-		panic(fmt.Sprintf("unknown log format: %d", f))
+		return "", fmt.Errorf("unknown log format: %d", f)
 	}
 }

cmd/soroban-rpc/internal/config/options.go

@@ -130,7 +130,7 @@ func (cfg *Config) options() ConfigOptions {
 				return nil
 			},
 			MarshalTOML: func(option *ConfigOption) (interface{}, error) {
-				return cfg.LogFormat.String(), nil
+				return cfg.LogFormat.String()
 			},
 		},
 		{

cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow.go

@@ -35,12 +35,12 @@ func NewLedgerBucketWindow[T any](retentionWindow uint32) *LedgerBucketWindow[T]
 }
 
 // Append adds a new bucket to the window. If the window is full a bucket will be evicted and returned.
-func (w *LedgerBucketWindow[T]) Append(bucket LedgerBucket[T]) *LedgerBucket[T] {
+func (w *LedgerBucketWindow[T]) Append(bucket LedgerBucket[T]) (*LedgerBucket[T], error) {
 	length := w.Len()
 	if length > 0 {
 		expectedLedgerSequence := w.buckets[w.start].LedgerSeq + length
 		if expectedLedgerSequence != bucket.LedgerSeq {
-			panic(fmt.Errorf("ledgers not contiguous: expected ledger sequence %v but received %v", expectedLedgerSequence, bucket.LedgerSeq))
+			return &LedgerBucket[T]{}, fmt.Errorf("error appending ledgers: ledgers not contiguous: expected ledger sequence %v but received %v", expectedLedgerSequence, bucket.LedgerSeq)
 		}
 	}
 
@@ -57,7 +57,7 @@ func (w *LedgerBucketWindow[T]) Append(bucket LedgerBucket[T]) *LedgerBucket[T]
 		w.start = (w.start + 1) % length
 	}
 
-	return evicted
+	return evicted, nil
 }
 
 // Len returns the length (number of buckets in the window)

cmd/soroban-rpc/internal/ledgerbucketwindow/ledgerbucketwindow_test.go

@@ -18,118 +18,87 @@ func TestAppend(t *testing.T) {
 	m := NewLedgerBucketWindow[uint32](3)
 	require.Equal(t, uint32(0), m.Len())
 
-	// test appending first bucket of events
-	evicted := m.Append(bucket(5))
+	// Test appending first bucket of events
+	evicted, err := m.Append(bucket(5))
+	require.NoError(t, err)
 	require.Nil(t, evicted)
 	require.Equal(t, uint32(1), m.Len())
 	require.Equal(t, bucket(5), *m.Get(0))
 
-	// the next bucket must follow the previous bucket (ledger 5)
-
-	require.PanicsWithError(
-		t, "ledgers not contiguous: expected ledger sequence 6 but received 10",
-		func() {
-			m.Append(LedgerBucket[uint32]{
-				LedgerSeq:            10,
-				LedgerCloseTimestamp: 100,
-				BucketContent:        10,
-			})
-		},
-	)
-	require.PanicsWithError(
-		t, "ledgers not contiguous: expected ledger sequence 6 but received 4",
-		func() {
-			m.Append(LedgerBucket[uint32]{
-				LedgerSeq:            4,
-				LedgerCloseTimestamp: 100,
-				BucketContent:        4,
-			})
-		},
-	)
-	require.PanicsWithError(
-		t, "ledgers not contiguous: expected ledger sequence 6 but received 5",
-		func() {
-			m.Append(LedgerBucket[uint32]{
-				LedgerSeq:            5,
-				LedgerCloseTimestamp: 100,
-				BucketContent:        5,
-			})
-		},
-	)
+	// The next bucket must follow the previous bucket (ledger 5)
+	_, err = m.Append(LedgerBucket[uint32]{
+		LedgerSeq:            10,
+		LedgerCloseTimestamp: 100,
+		BucketContent:        10,
+	})
+	require.Errorf(t, err, "ledgers not contiguous: expected ledger sequence 6 but received 10")
+
+	_, err = m.Append(LedgerBucket[uint32]{
+		LedgerSeq:            4,
+		LedgerCloseTimestamp: 100,
+		BucketContent:        4,
+	})
+	require.Errorf(t, err, "ledgers not contiguous: expected ledger sequence 6 but received 4")
+
 	// check that none of the calls above modified our buckets
 	require.Equal(t, uint32(1), m.Len())
 	require.Equal(t, bucket(5), *m.Get(0))
 
-	// append ledger 6 bucket, now we have two buckets filled
-	evicted = m.Append(bucket(6))
+	// Append ledger 6 bucket, now we have two buckets filled
+	evicted, err = m.Append(bucket(6))
+	require.NoError(t, err)
 	require.Nil(t, evicted)
 	require.Equal(t, uint32(2), m.Len())
 	require.Equal(t, bucket(5), *m.Get(0))
 	require.Equal(t, bucket(6), *m.Get(1))
 
-	// the next bucket of events must follow the previous bucket (ledger 6)
-	require.PanicsWithError(
-		t, "ledgers not contiguous: expected ledger sequence 7 but received 10",
-		func() {
-			m.Append(LedgerBucket[uint32]{
-				LedgerSeq:            10,
-				LedgerCloseTimestamp: 100,
-				BucketContent:        10,
-			})
-		},
-	)
-	require.PanicsWithError(
-		t, "ledgers not contiguous: expected ledger sequence 7 but received 4",
-		func() {
-			m.Append(LedgerBucket[uint32]{
-				LedgerSeq:            4,
-				LedgerCloseTimestamp: 100,
-				BucketContent:        4,
-			})
-		},
-	)
-	require.PanicsWithError(
-		t, "ledgers not contiguous: expected ledger sequence 7 but received 5",
-		func() {
-			m.Append(LedgerBucket[uint32]{
-				LedgerSeq:            5,
-				LedgerCloseTimestamp: 100,
-				BucketContent:        5,
-			})
-		},
-	)
-
-	// append ledger 7, now we have all three buckets filled
-	evicted = m.Append(bucket(7))
-	require.Nil(t, evicted)
+	// Append ledger 7, now we have all three buckets filled
+	evicted, err = m.Append(bucket(7))
+	require.NoError(t, err)
 	require.Nil(t, evicted)
 	require.Equal(t, uint32(3), m.Len())
 	require.Equal(t, bucket(5), *m.Get(0))
 	require.Equal(t, bucket(6), *m.Get(1))
 	require.Equal(t, bucket(7), *m.Get(2))
 
-	// append ledger 8, but all buckets are full, so we need to evict ledger 5
-	evicted = m.Append(bucket(8))
+	// Append ledger 8, but all buckets are full, so we need to evict ledger 5
+	evicted, err = m.Append(bucket(8))
+	require.NoError(t, err)
 	require.Equal(t, bucket(5), *evicted)
 	require.Equal(t, uint32(3), m.Len())
 	require.Equal(t, bucket(6), *m.Get(0))
 	require.Equal(t, bucket(7), *m.Get(1))
 	require.Equal(t, bucket(8), *m.Get(2))
 
-	// append ledger 9 events, but all buckets are full, so we need to evict ledger 6
-	evicted = m.Append(bucket(9))
+	// Append ledger 9 events, but all buckets are full, so we need to evict ledger 6
+	evicted, err = m.Append(bucket(9))
+	require.NoError(t, err)
 	require.Equal(t, bucket(6), *evicted)
 	require.Equal(t, uint32(3), m.Len())
 	require.Equal(t, bucket(7), *m.Get(0))
 	require.Equal(t, bucket(8), *m.Get(1))
 	require.Equal(t, bucket(9), *m.Get(2))
 
-	// append ledger 10, but all buckets are full, so we need to evict ledger 7.
+	// Append ledger 10, but all buckets are full, so we need to evict ledger 7.
 	// The start index must have wrapped around
-	evicted = m.Append(bucket(10))
+	evicted, err = m.Append(bucket(10))
+	require.NoError(t, err)
 	require.Equal(t, bucket(7), *evicted)
 	require.Equal(t, uint32(3), m.Len())
 	require.Equal(t, bucket(8), *m.Get(0))
 	require.Equal(t, bucket(9), *m.Get(1))
 	require.Equal(t, bucket(10), *m.Get(2))
 }
+
+func TestAppendError(t *testing.T) {
+	m := NewLedgerBucketWindow[uint32](3)
+	require.Equal(t, uint32(0), m.Len())
+
+	evicted, err := m.Append(bucket(5))
+	require.NoError(t, err)
+	require.Nil(t, evicted)
+
+	evicted, err = m.Append(bucket(1))
+	require.Error(t, err)
+	require.Contains(t, err.Error(), "error appending ledgers: ledgers not contiguous: expected ledger sequence 6 but received 1")
+}

cmd/soroban-rpc/internal/transactions/transactions.go

@@ -122,7 +122,10 @@ func (m *MemoryStore) IngestTransactions(ledgerCloseMeta xdr.LedgerCloseMeta) er
 
 	m.lock.Lock()
 	defer m.lock.Unlock()
-	evicted := m.transactionsByLedger.Append(bucket)
+	evicted, err := m.transactionsByLedger.Append(bucket)
+	if err != nil {
+		return err
+	}
 	if evicted != nil {
 		// garbage-collect evicted entries
 		for _, evictedTxHash := range evicted.BucketContent {