Skip to content

Conversation

georgehao
Copy link
Member

@georgehao georgehao commented Aug 29, 2025

@georgehao georgehao changed the title add gaslimit check for execute_sequencer_transactions feat : add gaslimit check for execute_sequencer_transactions Aug 29, 2025
@georgehao georgehao changed the title feat : add gaslimit check for execute_sequencer_transactions feat: add gaslimit check for execute_sequencer_transactions Aug 29, 2025
Copy link

codspeed-hq bot commented Aug 29, 2025

CodSpeed Performance Report

Merging #333 will not alter performance

Comparing add_gas_limit_check_to_sequencer_tx (3258c8a) with scroll (d485f47)

Summary

✅ 77 untouched benchmarks

@georgehao georgehao requested a review from greged93 August 29, 2025 10:29
},
));
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

this won't work for L1 message, as currently we only add gas_used to info.cumulative_gas_used, we need to have the same logic as l2geth:

  • For normal transactions, subtract tx gas used from the available gas.
  • For L1 messages, subtract tx gas limit from the available gas.

Copy link
Member Author

Choose a reason for hiding this comment

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

true

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed bfd3574

Copy link
Member Author

@georgehao georgehao Aug 29, 2025

Choose a reason for hiding this comment

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

and btw, do i need to add this check

            let gas_used = sequencer_tx.is_l1_message() {
                ... 
            } else {
                ...
            }

as we discussed, I think l1 consolidation and l1 message can combine together

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you need to yes, for L1 messages you use gas limit but for normal you use gas used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

so I think we don't need to distinguish the tx type

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.

2 participants