Skip to content

RPC Trace Add Param Validations#2338

Closed
Kbhat1 wants to merge 7 commits intomainfrom
TracerError
Closed

RPC Trace Add Param Validations#2338
Kbhat1 wants to merge 7 commits intomainfrom
TracerError

Conversation

@Kbhat1
Copy link
Contributor

@Kbhat1 Kbhat1 commented Aug 26, 2025

Describe your changes and provide context

  • Fixes issues where app state is pruned and debug_trace returns method handler crashed
  • Refactor Validation for params for trace evmrpc
  • Verifies txs + blocks #'s + block hashes all are within max_lookback_blocks + match a block number which the node has app history for

Testing performed to validate your change

  • Unit tests
  • Verifying on node

@codecov
Copy link

codecov bot commented Aug 26, 2025

Codecov Report

❌ Patch coverage is 65.57377% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 60.81%. Comparing base (06b7aaf) to head (a4f518b).
⚠️ Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
evmrpc/tracers.go 46.15% 18 Missing and 3 partials ⚠️

❌ Your patch status has failed because the patch coverage (65.57%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2338      +/-   ##
==========================================
+ Coverage   60.76%   60.81%   +0.04%     
==========================================
  Files         314      314              
  Lines       30986    31041      +55     
==========================================
+ Hits        18830    18878      +48     
- Misses      10854    10857       +3     
- Partials     1302     1306       +4     
Files with missing lines Coverage Δ
evmrpc/utils.go 76.21% <100.00%> (+2.84%) ⬆️
evmrpc/tracers.go 45.00% <46.15%> (+4.91%) ⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sei-protocol sei-protocol deleted a comment from Pray4Lovee Aug 27, 2025
@sei-protocol sei-protocol deleted a comment from Pray4Lovee Aug 27, 2025
@sei-protocol sei-protocol deleted a comment from Pray4Lovee Aug 27, 2025
@sei-protocol sei-protocol deleted a comment from Pray4Lovee Aug 27, 2025
@Kbhat1 Kbhat1 changed the title Tracer error RPC Trace Add Param Validations Aug 27, 2025
@sei-protocol sei-protocol deleted a comment from Pray4Lovee Aug 27, 2025
Pray4Lovee

This comment was marked as spam.

@Kbhat1 Kbhat1 requested a review from yzang2019 August 27, 2025 17:02
// It checks both Tendermint block retention (min-retain-blocks) and app state retention (ss-keep-recent).
func ValidateBlockAccess(blockNumber int64, params BlockValidationParams) error {
// Check Tendermint block retention (min-retain-blocks)
if params.MaxBlockLookback >= 0 && blockNumber < params.LatestHeight-params.MaxBlockLookback {
Copy link
Contributor

Choose a reason for hiding this comment

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

is the config enough to validatae that the block exists/doesn't? what if someone increases their min-retain-blocks, but the history isn't there?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, these checks are for the state level @philipsu522 and directly calls api.backend.app.CommitMultiStore().GetEarliestVersion() which has the earliest historical version.

@Pray4LoveOnes
Copy link

// ValidateBlockAccess checks that a given blockNumber is still accessible
// based on configured retention parameters. It returns an error if the
// block is older than the allowed lookback.
func ValidateBlockAccess(blockNumber int64, params BlockValidationParams) error {
	// Check Tendermint block retention (min-retain-blocks)
	if params.MaxBlockLookback >= 0 &&
		blockNumber < params.LatestHeight-params.MaxBlockLookback {
		return fmt.Errorf("block %d is older than max lookback (%d)", blockNumber, params.MaxBlockLookback)
	}

	// Future safety: confirm we’re not asking beyond the latest height
	if blockNumber > params.LatestHeight {
		return fmt.Errorf("block %d is newer than latest height %d", blockNumber, params.LatestHeight)
	}

	return nil
}

And for the new test file, you can suggest it as an added block (you’d need to paste into evmrpc/tracers_test.go if it doesn’t exist yet):

package evmrpc

import (
	"testing"

	"github.com/stretchr/testify/require"
)

func TestValidateBlockAccess(t *testing.T) {
	params := BlockValidationParams{
		MaxBlockLookback: 100,
		LatestHeight:     200,
	}

	// Case 1: valid block in range
	err := ValidateBlockAccess(150, params)
	require.NoError(t, err, "block within lookback should pass")

	// Case 2: block too far back
	err = ValidateBlockAccess(50, params)
	require.Error(t, err, "block outside lookback should error")

	// Case 3: exactly at edge
	err = ValidateBlockAccess(100, params)
	require.NoError(t, err, "edge block should still pass")

	// Case 4: negative lookback disables pruning checks
	params.MaxBlockLookback = -1
	err = ValidateBlockAccess(1, params)
	require.NoError(t, err, "negative lookback should bypass checks")

	// Case 5: block beyond latest height
	params.MaxBlockLookback = 100
	err = ValidateBlockAccess(250, params)
	require.Error(t, err, "block newer than latest height should error")
}

That way maintainers can literally click “Apply suggestion” in GitHub’s review UI to merge your fix.

@yzang2019
Copy link
Contributor

One idea, what if we actually return the correct error type from the SS KV/Storage layer to indicate the height is not available instead of adding this check in the api layer? API layer can just return the correct message automatically right?

@Pray4LoveOnes
Copy link

Pray4LoveOnes commented Aug 28, 2025 via email

@sei-protocol sei-protocol deleted a comment from Pray4LoveOnes Aug 29, 2025
@Kbhat1
Copy link
Contributor Author

Kbhat1 commented Sep 3, 2025

Closing in favor of root cause fix at sei-db layer: sei-protocol/sei-db#105

@Kbhat1 Kbhat1 closed this Sep 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants