fix(bridge): fix bridge invocation hook#393
Merged
Merged
Conversation
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 44 minutes and 56 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThe PR refactors bridge hook invocation patterns by moving the pre-invocation hook from Changes
Sequence Diagram(s)sequenceDiagram
participant FrameInit as frame_init()
participant PreHook as apply_bridge_pre_invocation_hook
participant FrameRun as frame_run()
participant PostHook as apply_bridge_post_invocation_hook
participant Execution as Interpreter Execution
rect rgba(100, 150, 200, 0.5)
Note over FrameInit,PostHook: Old Flow (Pre-Hook in frame_run)
FrameInit->>FrameRun: initialize frame
FrameRun->>PreHook: call with ctx
PreHook->>FrameRun: return
FrameRun->>Execution: execute
Execution->>PostHook: call with ctx
end
rect rgba(150, 200, 100, 0.5)
Note over FrameInit,PostHook: New Flow (Pre-Hook in frame_init)
FrameInit->>FrameInit: match FrameInput::Call(inputs)
FrameInit->>PreHook: call with inputs
PreHook->>FrameInit: return
FrameInit->>FrameRun: initialize frame
FrameRun->>Execution: execute
Execution->>PostHook: call with frame
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/revm/src/bridge.rs (1)
266-277: Consider returning meaningful data fromtry_decode_send_message_value.The function currently returns
Option<()>which just indicates presence. While functional, consider returningOption<sendMessageCall>to match the pattern oftry_decode_receive_message_valuewhich returns the decoded value. This would provide consistency and allow future use of the decoded data without re-decoding.♻️ Suggested improvement
-fn try_decode_send_message_value<T: AsRef<[u8]>>(input: T) -> Option<()> { +fn try_decode_send_message_value<T: AsRef<[u8]>>(input: T) -> Option<sendMessageCall> { let input = input.as_ref(); if input.starts_with(&sendMessageCall::SELECTOR) { - // Decode an ABI message (it should be correct) - if let Ok(_message) = sendMessageCall::abi_decode(input) { - return Some(()); - }; + if let Ok(message) = sendMessageCall::abi_decode(input) { + return Some(message); + } } None }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/revm/src/bridge.rs` around lines 266 - 277, Change try_decode_send_message_value to return Option<sendMessageCall> instead of Option<()>: in function try_decode_send_message_value<T: AsRef<[u8]>>(input: T) -> Option<sendMessageCall>, call sendMessageCall::abi_decode(input.as_ref()), bind the result to a variable (e.g., message) and return Some(message) on Ok, otherwise return None; update the internal branch that currently uses if let Ok(_message) to capture and return the decoded value so it matches the pattern of try_decode_receive_message_value and avoids re-decoding at call sites.evm-e2e/src/fixtures.rs (1)
19-21: Test names don't match fixture file block numbers.The test function names all use
24293147but the fixture paths reference different block numbers (24293144,24293146,24293147). This creates confusion when debugging test failures. All fixture files exist and are correctly referenced, but the naming inconsistency should be resolved.Consider aligning the test names with their fixture file names for clarity:
♻️ Suggested fix
- fn testnet_24293147_bridge_malformed_params_1("fixtures/testnet_24293144_bridge_malformed_params_1.json"); - fn testnet_24293147_bridge_malformed_params_2("fixtures/testnet_24293146_bridge_malformed_params_2.json"); - fn testnet_24293147_bridge_malformed_params_3("fixtures/testnet_24293147_bridge_malformed_params_3.json"); + fn testnet_24293144_bridge_malformed_params_1("fixtures/testnet_24293144_bridge_malformed_params_1.json"); + fn testnet_24293146_bridge_malformed_params_2("fixtures/testnet_24293146_bridge_malformed_params_2.json"); + fn testnet_24293147_bridge_malformed_params_3("fixtures/testnet_24293147_bridge_malformed_params_3.json");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@evm-e2e/src/fixtures.rs` around lines 19 - 21, Rename the test functions so their names match the fixture file block numbers to avoid confusion: change testnet_24293147_bridge_malformed_params_1 to testnet_24293144_bridge_malformed_params_1 (for "fixtures/testnet_24293144_bridge_malformed_params_1.json"), change testnet_24293147_bridge_malformed_params_2 to testnet_24293146_bridge_malformed_params_2 (for "fixtures/testnet_24293146_bridge_malformed_params_2.json"), and ensure testnet_24293147_bridge_malformed_params_3 remains or is explicitly named testnet_24293147_bridge_malformed_params_3 to match "fixtures/testnet_24293147_bridge_malformed_params_3.json"; update any references/imports to these function names accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/revm/src/bridge.rs`:
- Around line 84-96: The special-case branch that checks ctx.tx().chain_id() ==
Some(0x5202), ctx.tx().caller() ==
address!("0x1C92DffBCe76670F69007F22A54e31ff3Ab45d5E"),
[537u64,538u64,539u64].contains(&ctx.tx().nonce()), and ctx.journal().depth() ==
3 before setting *next_action = malformed_interpreter_action() is fragile and
silent; confirm the journal depth is correct for all three nonces (adjust or
broaden the depth check on ctx.journal().depth() if necessary) and add a warning
log (e.g., warn! or trace!) inside this branch that includes chain id, caller,
nonce and journal depth so you can observe when the workaround triggers. Ensure
the log call is placed immediately before assigning
malformed_interpreter_action() to next_action.
---
Nitpick comments:
In `@crates/revm/src/bridge.rs`:
- Around line 266-277: Change try_decode_send_message_value to return
Option<sendMessageCall> instead of Option<()>: in function
try_decode_send_message_value<T: AsRef<[u8]>>(input: T) ->
Option<sendMessageCall>, call sendMessageCall::abi_decode(input.as_ref()), bind
the result to a variable (e.g., message) and return Some(message) on Ok,
otherwise return None; update the internal branch that currently uses if let
Ok(_message) to capture and return the decoded value so it matches the pattern
of try_decode_receive_message_value and avoids re-decoding at call sites.
In `@evm-e2e/src/fixtures.rs`:
- Around line 19-21: Rename the test functions so their names match the fixture
file block numbers to avoid confusion: change
testnet_24293147_bridge_malformed_params_1 to
testnet_24293144_bridge_malformed_params_1 (for
"fixtures/testnet_24293144_bridge_malformed_params_1.json"), change
testnet_24293147_bridge_malformed_params_2 to
testnet_24293146_bridge_malformed_params_2 (for
"fixtures/testnet_24293146_bridge_malformed_params_2.json"), and ensure
testnet_24293147_bridge_malformed_params_3 remains or is explicitly named
testnet_24293147_bridge_malformed_params_3 to match
"fixtures/testnet_24293147_bridge_malformed_params_3.json"; update any
references/imports to these function names accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3c4e1728-3ea1-4444-b1cf-c7dca6f4af24
📒 Files selected for processing (7)
crates/revm/src/bridge.rscrates/revm/src/evm.rse2e/src/bridge.rsevm-e2e/fixtures/testnet_24293144_bridge_malformed_params_1.jsonevm-e2e/fixtures/testnet_24293146_bridge_malformed_params_2.jsonevm-e2e/fixtures/testnet_24293147_bridge_malformed_params_3.jsonevm-e2e/src/fixtures.rs
Comment on lines
+84
to
+96
| // A special case for corrupted bridge transaction on testnet, where we issued 3 | ||
| // transactions from relayer that caused execution failure. We keep it here only to | ||
| // make blockchain syncable for testnet. | ||
| // | ||
| // Note: it can be removed once we have new snapshot for testnet | ||
| if ctx.tx().chain_id() == Some(0x5202) | ||
| && ctx.tx().caller() == address!("0x1C92DffBCe76670F69007F22A54e31ff3Ab45d5E") | ||
| && [537u64, 538u64, 539u64].contains(&ctx.tx().nonce()) | ||
| && ctx.journal().depth() == 3 | ||
| { | ||
| *next_action = malformed_interpreter_action(); | ||
| return Ok(()); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if there are any related tests or documentation for the malformed transactions
rg -n "537|538|539" --type rust -C3 | grep -i "nonce\|bridge\|malformed" | head -40Repository: fluentlabs-xyz/fluentbase
Length of output: 633
🏁 Script executed:
# Search for malformed_interpreter_action function and its usage
rg -n "malformed_interpreter_action" --type rust -B2 -A2Repository: fluentlabs-xyz/fluentbase
Length of output: 2440
🏁 Script executed:
# Look for tests related to bridge or malformed transactions
fd -name "*.rs" -type f | xargs grep -l "malformed\|corrupted" | head -10Repository: fluentlabs-xyz/fluentbase
Length of output: 299
🏁 Script executed:
# Check the context around the bridge.rs file for related functions or test modules
cat -n crates/revm/src/bridge.rs | head -100Repository: fluentlabs-xyz/fluentbase
Length of output: 3965
🏁 Script executed:
# Search for journal.depth() usage patterns in the codebase
rg -n "journal.*depth\(\)" --type rust -B2 -A2 | head -50Repository: fluentlabs-xyz/fluentbase
Length of output: 463
Testnet special case for corrupted bridge transactions.
The hardcoded conditions (chain ID 0x5202, specific caller, nonces 537-539, journal depth 3) create a narrow workaround for known corrupted transactions. The comment correctly notes this is temporary until a new testnet snapshot is available.
Two points worth addressing:
- The journal depth check (
depth() == 3) is uniquely specific to this workaround and appears nowhere else in the codebase. If these transactions ever create nested frames differently, the condition will silently fail to trigger. Verify this depth is correct for all three corrupted nonces. - No logging occurs when this branch executes, making it difficult to debug if the workaround is triggered unexpectedly or not at all. Consider adding a
warn!ortrace!log statement when this condition matches.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/revm/src/bridge.rs` around lines 84 - 96, The special-case branch that
checks ctx.tx().chain_id() == Some(0x5202), ctx.tx().caller() ==
address!("0x1C92DffBCe76670F69007F22A54e31ff3Ab45d5E"),
[537u64,538u64,539u64].contains(&ctx.tx().nonce()), and ctx.journal().depth() ==
3 before setting *next_action = malformed_interpreter_action() is fragile and
silent; confirm the journal depth is correct for all three nonces (adjust or
broaden the depth check on ctx.journal().depth() if necessary) and add a warning
log (e.g., warn! or trace!) inside this branch that includes chain id, caller,
nonce and journal depth so you can observe when the workaround triggers. Ensure
the log call is placed immediately before assigning
malformed_interpreter_action() to next_action.
d1r1
approved these changes
Apr 16, 2026
Criterion results (vs baseline)Heads-up: runner perf is noisy; treat deltas as a smoke check. |
Signed-off-by: Dmitry Savonin <github@dmitry123.xyz>
This was referenced Apr 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests