Skip to content

fix(isthmus): Correctly filter refunds for deposit transactions#2330

Merged
rakita merged 1 commit intobluealloy:mainfrom
clabby:cl/isthmus-refunds
Mar 28, 2025
Merged

fix(isthmus): Correctly filter refunds for deposit transactions#2330
rakita merged 1 commit intobluealloy:mainfrom
clabby:cl/isthmus-refunds

Conversation

@clabby
Copy link
Contributor

@clabby clabby commented Mar 28, 2025

Overview

Corrects the current condition for operator fee refunds in trunk. Previously the check was using a source hash heuristic that wasn't correct - what we want is to not refund the operator fee for deposit transactions.

Also adds a test for expected refund behavior for both deposit transactions and normal transactions.

note - this bug isn't present in tag v64 (v19.7.0)

@clabby clabby force-pushed the cl/isthmus-refunds branch from a81cf02 to c36a921 Compare March 28, 2025 00:43
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 28, 2025

CodSpeed Performance Report

Merging #2330 will degrade performances by 7.75%

Comparing clabby:cl/isthmus-refunds (c36a921) with main (d8291d7)

Summary

❌ 1 regressions
✅ 24 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Benchmarks breakdown

Benchmark BASE HEAD Change
evm-build 8.3 µs 9 µs -7.75%

Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

this would otherwise cause panics because the L1BlockInfo isnt loaded for deposit txs

@rakita
Copy link
Member

rakita commented Mar 28, 2025

this would otherwise cause panics because the L1BlockInfo isnt loaded for deposit txs
Probably not a panic, but the wrong values would be used, not that better outcome :)

@rakita rakita merged commit 7a98764 into bluealloy:main Mar 28, 2025
28 of 29 checks passed
@github-actions github-actions bot mentioned this pull request Mar 28, 2025
@mattsse
Copy link
Collaborator

mattsse commented Mar 28, 2025

definitely panics:

[op-el-2-op-reth-op-node-op-kurtosis] thread 'Tree Task' panicked at /home/runner/.cargo/registry/src/index.crates.io-1949cf8c6b5b557f/op-revm-1.0.0-alpha.6/src/l1block.rs:164:14:
[op-el-2-op-reth-op-node-op-kurtosis] Missing operator fee scalar for isthmus L1 Block
[op-el-2-op-reth-op-node-op-kurtosis] stack backtrace:
[op-el-2-op-reth-op-node-op-kurtosis] note: Some details are omitted, run with RUST_BACKTRACE=full for a verbose backtrace.
[op-el-2-op-reth-op-node-op-kurtosis] 2025-03-27T22:10:47.295132Z ERROR Fatal error
[op-el-2-op-reth-op-node-op-kurtosis] 2025-03-27T22:10:47.295159Z ERROR Fatal error in consensus engine
[op-el-2-op-reth-op-node-op-kurtosis] 2025-03-27T22:10:47.295202Z ERROR shutting down due to error
[op-el-2-op-reth-op-node-op-kurtosis] Error: Fatal error in consensus engine
[op-el-2-op-reth-op-node-op-kurtosis]
[op-el-2-op-reth-op-node-op-kurtosis] Location:
[op-el-2-op-reth-op-node-op-kurtosis] /home/runner/work/reth/reth/crates/node/builder/src/launch/engine.rs:341:43

This was referenced Mar 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants