Remove deprecated random nonce selection logic

Author: Jeff Yanta

pkg/code/data/internal.go

@@ -135,7 +135,6 @@ type DatabaseData interface {
 	GetNonceCountByState(ctx context.Context, env nonce.Environment, instance string, state nonce.State) (uint64, error)
 	GetNonceCountByStateAndPurpose(ctx context.Context, env nonce.Environment, instance string, state nonce.State, purpose nonce.Purpose) (uint64, error)
 	GetAllNonceByState(ctx context.Context, env nonce.Environment, instance string, state nonce.State, opts ...query.Option) ([]*nonce.Record, error)
-	GetRandomAvailableNonceByPurpose(ctx context.Context, env nonce.Environment, instance string, purpose nonce.Purpose) (*nonce.Record, error)
 	BatchClaimAvailableNoncesByPurpose(ctx context.Context, env nonce.Environment, instance string, purpose nonce.Purpose, limit int, nodeID string, minExpireAt, maxExpireAt time.Time) ([]*nonce.Record, error)
 	SaveNonce(ctx context.Context, record *nonce.Record) error
 
@@ -530,9 +529,6 @@ func (dp *DatabaseProvider) GetAllNonceByState(ctx context.Context, env nonce.En
 
 	return dp.nonces.GetAllByState(ctx, env, instance, state, req.Cursor, req.Limit, req.SortBy)
 }
-func (dp *DatabaseProvider) GetRandomAvailableNonceByPurpose(ctx context.Context, env nonce.Environment, instance string, purpose nonce.Purpose) (*nonce.Record, error) {
-	return dp.nonces.GetRandomAvailableByPurpose(ctx, env, instance, purpose)
-}
 func (dp *DatabaseProvider) BatchClaimAvailableNoncesByPurpose(ctx context.Context, env nonce.Environment, instance string, purpose nonce.Purpose, limit int, nodeID string, minExpireAt, maxExpireAt time.Time) ([]*nonce.Record, error) {
 	return dp.nonces.BatchClaimAvailableByPurpose(ctx, env, instance, purpose, limit, nodeID, minExpireAt, maxExpireAt)
 }

pkg/code/data/nonce/memory/store.go

@@ -232,22 +232,6 @@ func (s *store) GetAllByState(ctx context.Context, env nonce.Environment, instan
 	return nil, nonce.ErrNonceNotFound
 }
 
-func (s *store) GetRandomAvailableByPurpose(ctx context.Context, env nonce.Environment, instance string, purpose nonce.Purpose) (*nonce.Record, error) {
-	s.mu.Lock()
-	defer s.mu.Unlock()
-
-	items := s.findByStateAndPurpose(env, instance, nonce.StateAvailable, purpose)
-	items = append(items, s.findByStateAndPurpose(env, instance, nonce.StateClaimed, purpose)...)
-	items = s.filterAvailableToClaim(items)
-	if len(items) == 0 {
-		return nil, nonce.ErrNonceNotFound
-	}
-
-	index := rand.Intn(len(items))
-	cloned := items[index].Clone()
-	return &cloned, nil
-}
-
 func (s *store) BatchClaimAvailableByPurpose(ctx context.Context, env nonce.Environment, instance string, purpose nonce.Purpose, limit int, nodeID string, minExpireAt, maxExpireAt time.Time) ([]*nonce.Record, error) {
 	s.mu.Lock()
 	defer s.mu.Unlock()

pkg/code/data/nonce/postgres/model.go

@@ -201,53 +201,6 @@ func dbGetAllByState(ctx context.Context, db *sqlx.DB, env nonce.Environment, in
 	return res, nil
 }
 
-// todo: Implementation still isn't perfect, but better than no randomness. It's
-// sufficiently efficient, as long as our nonce pool is larger than the max offset.
-// todo: We may need to tune the offset based on pool size and environment, but it
-// should be sufficiently good enough for now.
-func dbGetRandomAvailableByPurpose(ctx context.Context, db *sqlx.DB, env nonce.Environment, instance string, purpose nonce.Purpose) (*nonceModel, error) {
-	res := &nonceModel{}
-
-	// Signature null check is required because some legacy records didn't have this
-	// set and causes this call to fail. This is a result of the field not being
-	// defined at the time of record creation.
-	//
-	// todo: Fix said nonce records
-	query := `SELECT
-		id, address, authority, blockhash, environment, environment_instance, purpose, state, signature, claim_node_id, claim_expires_at, version
-		FROM ` + nonceTableName + `
-		WHERE environment = $1 AND environment_instance = $2 AND ((state = $3) OR (state = $4 AND claim_expires_at < $5)) AND purpose = $6 AND signature IS NOT NULL
-		OFFSET FLOOR(RANDOM() * 100)
-		LIMIT 1
-	`
-	fallbackQuery := `SELECT
-		id, address, authority, blockhash, environment, environment_instance, purpose, state, signature, claim_node_id, claim_expires_at, version
-		FROM ` + nonceTableName + `
-		WHERE environment = $1 AND environment_instance = $2 AND ((state = $3) OR (state = $4 AND claim_expires_at < $5)) AND purpose = $6 AND signature IS NOT NULL
-		LIMIT 1
-	`
-
-	nowMs := time.Now().UnixMilli()
-	err := db.GetContext(ctx, res, query, env, instance, nonce.StateAvailable, nonce.StateClaimed, nowMs, purpose)
-	if err != nil {
-		err = pgutil.CheckNoRows(err, nonce.ErrNonceNotFound)
-
-		// No nonces found. Because our query isn't perfect, fall back to a
-		// strategy that will guarantee to select something if an available
-		// nonce exists.
-		if err == nonce.ErrNonceNotFound {
-			err := db.GetContext(ctx, res, fallbackQuery, env, instance, nonce.StateAvailable, nonce.StateClaimed, nowMs, purpose)
-			if err != nil {
-				return nil, pgutil.CheckNoRows(err, nonce.ErrNonceNotFound)
-			}
-			return res, nil
-		}
-
-		return nil, err
-	}
-	return res, nil
-}
-
 func dbBatchClaimAvailableByPurpose(
 	ctx context.Context,
 	db *sqlx.DB,

pkg/code/data/nonce/postgres/store.go

@@ -72,14 +72,6 @@ func (s *store) GetAllByState(ctx context.Context, env nonce.Environment, instan
 	return nonces, nil
 }
 
-func (s *store) GetRandomAvailableByPurpose(ctx context.Context, env nonce.Environment, instance string, purpose nonce.Purpose) (*nonce.Record, error) {
-	model, err := dbGetRandomAvailableByPurpose(ctx, s.db, env, instance, purpose)
-	if err != nil {
-		return nil, err
-	}
-	return fromNonceModel(model), nil
-}
-
 func (s *store) BatchClaimAvailableByPurpose(ctx context.Context, env nonce.Environment, instance string, purpose nonce.Purpose, limit int, nodeID string, minExpireAt, maxExpireAt time.Time) ([]*nonce.Record, error) {
 	models, err := dbBatchClaimAvailableByPurpose(ctx, s.db, env, instance, purpose, limit, nodeID, minExpireAt, maxExpireAt)
 	if err != nil {

pkg/code/data/nonce/store.go

@@ -33,14 +33,6 @@ type Store interface {
 	// Returns ErrNotFound if no records are found.
 	GetAllByState(ctx context.Context, env Environment, instance string, state State, cursor query.Cursor, limit uint64, direction query.Ordering) ([]*Record, error)
 
-	// GetRandomAvailableByPurpose gets a random available nonce for a purpose within
-	// an environment instance.
-	//
-	// Returns ErrNotFound if no records are found.
-	//
-	// Deprecated in favour of BatchClaimAvailableByPurpose
-	GetRandomAvailableByPurpose(ctx context.Context, env Environment, instance string, purpose Purpose) (*Record, error)
-
 	// BatchClaimAvailableByPurpose batch claims up to the specified limit.
 	//
 	// The returned nonces will be marked as claimed by the current node, with

pkg/code/data/nonce/tests/tests.go

@@ -23,7 +23,6 @@ func RunTests(t *testing.T, s nonce.Store, teardown func()) {
 		testUpdate,
 		testGetAllByState,
 		testGetCount,
-		testGetRandomAvailableByPurpose,
 		testBatchClaimAvailableByPurpose,
 		testBatchClaimAvailableByPurposeExpirationRandomness,
 	} {
@@ -285,105 +284,6 @@ func testGetCount(t *testing.T, s nonce.Store) {
 	})
 }
 
-func testGetRandomAvailableByPurpose(t *testing.T, s nonce.Store) {
-	t.Run("testGetRandomAvailableByPurpose", func(t *testing.T) {
-		ctx := context.Background()
-
-		_, err := s.GetRandomAvailableByPurpose(ctx, nonce.EnvironmentSolana, nonce.EnvironmentInstanceSolanaMainnet, nonce.PurposeClientTransaction)
-		assert.Equal(t, nonce.ErrNonceNotFound, err)
-
-		for _, purpose := range []nonce.Purpose{
-			nonce.PurposeClientTransaction,
-			nonce.PurposeInternalServerProcess,
-		} {
-			for _, state := range []nonce.State{
-				nonce.StateUnknown,
-				nonce.StateAvailable,
-				nonce.StateReserved,
-				nonce.StateReleased,
-				nonce.StateClaimed,
-			} {
-				for i := 0; i < 50; i++ {
-					record := &nonce.Record{
-						Address:             fmt.Sprintf("nonce_%s_%s_%d", purpose, state, i),
-						Authority:           "authority",
-						Blockhash:           "bh",
-						Environment:         nonce.EnvironmentSolana,
-						EnvironmentInstance: nonce.EnvironmentInstanceSolanaMainnet,
-						Purpose:             purpose,
-						State:               state,
-						Signature:           "",
-					}
-					if state == nonce.StateClaimed {
-						record.ClaimNodeID = pointer.String("node_id")
-
-						if i < 25 {
-							record.ClaimExpiresAt = pointer.Time(time.Now().Add(-time.Hour))
-						} else {
-							record.ClaimExpiresAt = pointer.Time(time.Now().Add(time.Hour))
-						}
-					}
-					require.NoError(t, s.Save(ctx, record))
-				}
-			}
-		}
-
-		for i := 0; i < 100; i++ {
-			record := &nonce.Record{
-				Address:             fmt.Sprintf("nonce_devnet_%d", i),
-				Authority:           "authority",
-				Blockhash:           "bh",
-				Environment:         nonce.EnvironmentSolana,
-				EnvironmentInstance: nonce.EnvironmentInstanceSolanaDevnet,
-				Purpose:             nonce.PurposeInternalServerProcess,
-				State:               nonce.StateAvailable,
-				Signature:           "",
-			}
-			require.NoError(t, s.Save(ctx, record))
-
-			record = &nonce.Record{
-				Address:             fmt.Sprintf("nonce_cvm_%d", i),
-				Authority:           "authority",
-				Blockhash:           "bh",
-				Environment:         nonce.EnvironmentCvm,
-				EnvironmentInstance: "pubkey",
-				Purpose:             nonce.PurposeClientTransaction,
-				State:               nonce.StateClaimed,
-				Signature:           "",
-				ClaimNodeID:         pointer.String("node_id"),
-				ClaimExpiresAt:      pointer.Time(time.Now().Add(-time.Hour)),
-			}
-			require.NoError(t, s.Save(ctx, record))
-		}
-
-		for _, purpose := range []nonce.Purpose{nonce.PurposeClientTransaction, nonce.PurposeInternalServerProcess} {
-			availableState, claimedState := 0, 0
-			selectedByAddress := make(map[string]struct{})
-			for i := 0; i < 100; i++ {
-				actual, err := s.GetRandomAvailableByPurpose(ctx, nonce.EnvironmentSolana, nonce.EnvironmentInstanceSolanaMainnet, purpose)
-				require.NoError(t, err)
-				assert.Equal(t, purpose, actual.Purpose)
-				assert.Equal(t, nonce.EnvironmentSolana, actual.Environment)
-				assert.Equal(t, nonce.EnvironmentInstanceSolanaMainnet, actual.EnvironmentInstance)
-				assert.True(t, actual.IsAvailableToClaim())
-				assert.True(t, actual.CanReserveWithSignature())
-				switch actual.State {
-				case nonce.StateAvailable:
-					availableState++
-				case nonce.StateClaimed:
-					claimedState++
-					assert.True(t, time.Now().After(*actual.ClaimExpiresAt))
-				default:
-				}
-				selectedByAddress[actual.Address] = struct{}{}
-			}
-			assert.True(t, len(selectedByAddress) > 10)
-			assert.NotZero(t, availableState)
-			assert.NotZero(t, claimedState)
-		}
-	})
-}
-
 func testBatchClaimAvailableByPurpose(t *testing.T, s nonce.Store) {
 	t.Run("testBatchClaimAvailableByPurpose", func(t *testing.T) {
 		ctx := context.Background()