Skip to content

fix(sequencer_rpc): replace unwrap with proper error handling, surface TX validation errors#378

Open
jimmy-claw wants to merge 1 commit intologos-blockchain:mainfrom
jimmy-claw:jimmy/fix-tx-error-handling
Open

fix(sequencer_rpc): replace unwrap with proper error handling, surface TX validation errors#378
jimmy-claw wants to merge 1 commit intologos-blockchain:mainfrom
jimmy-claw:jimmy/fix-tx-error-handling

Conversation

@jimmy-claw
Copy link
Copy Markdown

@jimmy-claw jimmy-claw commented Mar 3, 2026

🎯 Purpose

Replaces panicking unwrap()/expect() calls in the sequencer RPC handler with proper error handling. A single malformed request could previously crash the entire sequencer node (CVE-adjacent). Also surfaces TX validation errors to the caller instead of swallowing them.

⚙️ Approach

  • borsh::from_slice().unwrap()map_err returning RpcError::invalid_params
  • mempool.push().expect()map_err returning structured RpcError
  • TX validation failures now returned as JSON-RPC errors instead of being silently dropped

🧪 How to Test

# Send a malformed borsh payload — should return RPC error instead of crashing
curl -X POST http://localhost:3040 -H "Content-Type: application/json"   -d "{\"jsonrpc\":\"2.0\",\"method\":\"submit_transaction\",\"params\":\"DEADBEEF\",\"id\":1}"

# Valid TX that fails validation — should now return descriptive error

🔗 Dependencies

None.

🔜 Future Work

  • Structured error codes for different failure modes (malformed TX vs validation failure vs mempool full)

📋 PR Completion Checklist

  • Complete PR description
  • Implement the core functionality
  • Add/update tests
  • Add/update documentation and inline comments

…e TX validation errors

Replace `borsh::from_slice().unwrap()` with `map_err` that returns
`RpcError::invalid_params`, preventing a panic that crashes the entire
sequencer on malformed borsh input.

Replace `mempool.push().expect()` with `map_err` that returns
`RpcError::new_internal_error`, so a closed mempool returns a proper
JSON-RPC error instead of panicking.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines +128 to +133
.map_err(|_| {
RpcErr(RpcError::new_internal_error(
None,
"Mempool is closed, cannot accept transactions",
))
})?;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This is redundant, it's an internal error this should never happen, panicking is okay here

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.

3 participants