Aa755/rm rb assert

[Yb2411]

No description provided.

[Yb2411]

@claude

[github-actions]

Claude finished @Yb2411's task in 1m 8s —— View job

---

PR Review: Aa755/rm rb assert

• Gather context and read changed files
• Review code changes
• Post review feedback

---

This PR encompasses several distinct changes across 15 commits. Here's a breakdown:

Summary of Changes

1. Remove MONAD_ASSERT_THROW in reserve balance (core change) — Replaces throwing assertions with explicit handling when gas fees exceed the reserve for non-dipping transactions.
2. Refactor combine_senders_and_authorities — Moves a duplicated helper from monad_executor.cpp (anonymous namespace) into monad_chain.cpp/.hpp as a shared utility, and updates all call sites.
3. Improve fast list compaction offset calculation — Adds hysteresis, caps max range to 32MB, and improves the algorithm to prevent over-compaction when history length shrinks.
4. Add static to constexpr in tests — Changes constexpr to static constexpr for local constants in test code.
5. New tests — Adds sender_gas_fee_above_reserve_stays_failed_after_large_credit test and extensive precompile wellformedness tests.
6. Workflow cleanup — Removes old CI workflows and adds claude.yml.

Review Feedback

Core Logic Change (reserve_balance.cpp)

The replacement of MONAD_ASSERT_THROW with explicit handling in update_violation_status (reserve_balance.cpp:238-255) is well-designed:

• When sender_gas_fees_ > reserve, the code now resets the threshold and marks the sender as failed, instead of throwing. This means the sender stays permanently failed for this transaction — even if they later receive a large credit.
• The comment at lines 242-252 clearly explains the reasoning and the RPC vs. speculative execution contexts.
• The corresponding change in dipped_into_reserve (removing MONAD_ASSERT_THROW at the old line 122-127) is consistent — it simply returns true (revert) without asserting.

The test update in monad_executor_test.cpp:3799-3801 correctly changes the expected status from EVMC_INTERNAL_ERROR with a message string to EVMC_MONAD_RESERVE_BALANCE_VIOLATION with nullptr message, matching the new non-throwing behavior.

New Test Coverage

The sender_gas_fee_above_reserve_stays_failed_after_large_credit test (test_monad_chain.cpp:167-242) is a good addition — it verifies that even after crediting the sender with the maximum possible balance (uint256_t::max() - current), both revert_transaction and revert_transaction_cached still return true. This directly validates the sticky failure semantics of the new code path.

The precompile wellformedness tests are thorough, covering all specified validation ordering.

Refactoring

The combine_senders_and_authorities extraction is clean. The new shared function uses std::span parameters instead of const& to std::vector, which is a good interface improvement. All callers (execute_block_test.cpp, monad_executor.cpp) are updated.

Minor Observations

1. constexpr → static constexpr (test_monad_chain.cpp:158-159, 253-256, 266-268): This is a valid fix — constexpr variables at block scope in templates can cause ODR issues; static constexpr is the correct pattern.

2. Compaction algorithm (update_aux.cpp): The new algorithm is substantially more complex but addresses real issues (over-compaction during history shrink). The min_versions_of_growth_before_compact_fast_list = 5000 constant and min_compaction_progress_buffer = 8 are hard-coded — might be worth a comment on how these were calibrated if there's empirical data behind them.

Overall this looks solid. The core change correctly replaces a recoverable assertion (that could crash the RPC process) with proper violation semantics, and the test coverage validates the new behavior well.

---