refactor(block): extract shared transaction selection logic into tx_selection module

[davidtaikocha]

Summary

• Extract overlapping transaction selection logic from payload builder and RPC endpoint into a shared tx_selection module in crates/block
• Add unified TxSelectionConfig supporting gas limits, DA size limits, multi-list generation, min tip filtering, and local account filtering
• Reduce code duplication by ~130 lines while ensuring consistent behavior

Changes

File	Change
crates/block/src/tx_selection.rs	New module with select_and_execute_pool_transactions
crates/payload/src/builder.rs	Refactored to use shared function
crates/rpc/src/eth/auth.rs	Refactored to use shared function

Motivation

execute_anchor_and_pool_transactions (payload builder) and tx_pool_content_with_min_tip (RPC) shared nearly identical logic for:

• Fetching best transactions from pool
• Gas limit and DA size checking
• Transaction execution and error classification
• Invalid transaction marking

Unifying this logic ensures:

1. Single source of truth for transaction selection
2. Consistent behavior between payload building and RPC pre-building
3. Easier maintenance - bugs fixed in one place

[claude]

Claude finished @davidtaikocha's task —— View job

---

Code Review Complete ✓

Todo List

• Read new tx_selection.rs module
• Review refactored payload/builder.rs
• Review refactored rpc/eth/auth.rs
• Analyze code quality and best practices
• Check for potential bugs or issues
• Evaluate performance considerations
• Assess security concerns
• Check test coverage
• Post comprehensive review feedback

---

Review Feedback

✅ Overall Assessment

Solid refactoring that successfully eliminates ~130 lines of duplication. The extracted logic is well-structured and the unified API improves maintainability. A few issues need attention before merging.

---

🐛 Bugs & Issues

1. CRITICAL: Incorrect error mapping for DA limit violations

Location: crates/block/src/tx_selection.rs:153

When a transaction exceeds DA limits, it's marked as Underpriced, but this is semantically wrong and could cause pool corruption.

// Current (line 153):
} else {
    best_txs.mark_invalid(&pool_tx, &InvalidPoolTransactionError::Underpriced);
}

Problem: DA limit violations aren't pricing issues. Marking them as underpriced will affect tip-based pool ordering and could remove valid high-tip transactions.

Fix: Use a more appropriate error like ExceedsGasLimit or Other. Consider:

best_txs.mark_invalid(&pool_tx, &InvalidPoolTransactionError::Other(
    Box::new("Transaction exceeds DA size limit")
));

2. Nonce-too-low handling differs from original

Location: crates/block/src/tx_selection.rs:167-170

Original code in builder.rs didn't have special nonce-too-low handling—it treated all validation errors uniformly by marking them invalid. New code skips nonce-too-low without marking invalid.

Impact: This changes behavior. Not marking invalid means dependent transactions will still be tried (could be intentional, but should be documented in PR description if so).

Recommendation: Either revert to original behavior or add a comment explaining why nonce-too-low deserves special treatment.

3. Unreachable panic in RPC code

Location: crates/rpc/src/eth/auth.rs:321

Ok(SelectionOutcome::Cancelled) => {
    unreachable!("tx selection should never be cancelled")
}

If the is_cancelled closure ever has a bug or future refactor changes behavior, this becomes a DoS vector via panic. Use proper error handling:

Ok(SelectionOutcome::Cancelled) => {
    Err(EthApiError::Internal("unexpected cancellation".into()).into())
}

---

⚡ Performance Considerations

4. Redundant DA size calculation on list overflow

Location: crates/block/src/tx_selection.rs:134

DA size is calculated before checking if it fits in any list. For transactions that don't fit in the current list when max_lists is reached, the calculation is wasted.

Optimization: Move line 134 inside the if exceeds_gas || exceeds_da block, or defer until after gas check passes.

5. Vec allocation pattern

Lists are allocated with vec![ExecutedTxList::default()] and grow via push. Consider pre-allocating with capacity:

let mut lists = Vec::with_capacity(config.max_lists);
lists.push(ExecutedTxList::default());

Minor improvement but reduces allocations for multi-list scenarios.

---

🔒 Security Concerns

6. Local account filtering bypasses tip requirement

Location: crates/block/src/tx_selection.rs:118-122

Non-local transactions are marked Underpriced even if they have sufficient tip but just aren't from a local account. This could be exploited:

• Attacker spams pool with high-tip txs from non-local accounts
• They get marked underpriced, causing pool churn
• Legitimate local txs might benefit but this wastes resources

Recommendation: Use a different error type or add a comment explaining this is intentional censorship of non-locals.

7. No validation of config parameters

TxSelectionConfig has no validation. Callers could pass:

• max_lists = 0 (causes empty vec issue)
• gas_limit_per_list = 0 (everything exceeds)
• max_da_bytes_per_list = 0 (same)

Add validation in tx_pool_content_with_min_tip (already has max_lists check at line 238) or make TxSelectionConfig construction fallible.

---

🧪 Test Coverage

8. Zero test coverage for new module

No tests exist for tx_selection.rs. Critical paths that need coverage:

• Multi-list generation
• DA limit boundary conditions
• Local account filtering
• Cancellation handling
• Error classification (nonce, gas, DA, etc.)

Action Required: Add unit tests, especially for edge cases like:

• Transaction exactly at DA limit
• max_lists = 1 vs > 1
• Mixed locals/non-locals
• Empty pool

---

📝 Code Quality & Best Practices

9. Missing documentation

• TxSelectionConfig fields lack doc comments (what units is base_fee? gwei? wei?)
• select_and_execute_pool_transactions return types could explain when Cancelled vs Completed occurs
• locals behavior (whitelist vs blacklist) isn't obvious from the field name

10. Typo in branch name

Branch is simplify-slelection (should be simplify-selection). Not a code issue but worth fixing for consistency.

11. Inconsistent error messages

• Line 74: "tx selection invariant violated: lists is empty"
• Other places use different styles (e.g., "failed to execute anchor transaction")

Consider standardizing error message format across the module.

---

✨ Strengths

• Clean separation of concerns
• Reusable API with flexible config
• Reduces duplication significantly
• Well-structured types (ExecutedTx, ExecutedTxList, SelectionOutcome)
• Proper use of trace! logging

---

📋 Summary

Must Fix Before Merge:

• docs(repo): update some docs after repository transfer #1: DA limit error mapping (correctness issue)
• feat(consensus): introduce Shasta hardfork #3: Unreachable panic (safety issue)
• feat(rpc): add a missing validate_payload_attributes_against_header method #8: Add basic test coverage

Should Fix:

• chore(main): release 0.3.0 #2: Document nonce-too-low behavior change
• feat(reth): bump reth to 1.7.0 and simplify type signature #7: Config validation

Nice to Have:

• chore(main): release 0.4.0 #4-6, chore(chainspec): update devnet genesis JSON #9-11: Performance, docs, consistency improvements

---

| Branch: simplify-slelection