feat(execution): expose GasUsed via X-Arb-Gas-Used HTTP header#671
feat(execution): expose GasUsed via X-Arb-Gas-Used HTTP header#671AnkushinDaniil wants to merge 1 commit intomainfrom
Conversation
AnkushinDaniil
requested review from
damian-orzechowski,
hudem1,
svlachakis and
wurdum
as code owners
There was a problem hiding this comment.
Pull request overview
Adds a config-gated GasUsed metric to Arbitrum MessageResult responses to support benchmarking/comparison of execution clients by exposing per-block gas consumption.
Changes:
- Introduces
ExposeGasUsedInMessageResultconfig flag (defaultfalse) and wires it into message result construction. - Extends
MessageResult/MessageResultForRpcwith optionalGasUsedand updates equality logic. - Adds RPC module tests and Makefile targets to run Nethermind with the benchmark flag enabled.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Nethermind.Arbitrum/Execution/ArbitrumExecutionEngine.cs | Conditionally populates MessageResult.GasUsed from block/header gas used. |
| src/Nethermind.Arbitrum/Data/MessageResultForRpc.cs | Adds gasUsed field for RPC (external block fetch DTO) and maps into MessageResult. |
| src/Nethermind.Arbitrum/Data/MessageResult.cs | Adds nullable GasUsed to MessageResult and includes it in Equals. |
| src/Nethermind.Arbitrum/Config/IArbitrumConfig.cs | Adds ExposeGasUsedInMessageResult config item. |
| src/Nethermind.Arbitrum/Config/ArbitrumConfig.cs | Implements the new config property with default false. |
| src/Nethermind.Arbitrum.Test/Rpc/ArbitrumRpcModuleTests.DigestMessage.cs | Adds tests verifying GasUsed is null/filled depending on config. |
| Makefile | Adds benchmark run targets passing --Arbitrum.ExposeGasUsedInMessageResult=true. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| SendRoot.Equals(other.SendRoot) && | ||
| GasUsed == other.GasUsed; |
There was a problem hiding this comment.
MessageResult.Equals now includes GasUsed, but MessageResult is used for external block comparison in ArbitrumExecutionEngineWithComparison (it calls digestResultData.Equals(rpcResultData)). When ExposeGasUsedInMessageResult is disabled (default), internal results will have GasUsed == null while eth_getBlockByNumber responses typically always include gasUsed, causing false mismatches and triggering shutdown in comparison mode. Consider keeping Equals focused on identity fields (BlockHash/SendRoot) and adding a separate comparison method for GasUsed (or otherwise ensuring comparison mode does not factor GasUsed in unless both sides explicitly opt-in).
| SendRoot.Equals(other.SendRoot) && | |
| GasUsed == other.GasUsed; | |
| SendRoot.Equals(other.SendRoot); | |
| /// <summary> | |
| /// Compares all fields, including <see cref="GasUsed"/>. | |
| /// Intended for benchmarking / testing scenarios where gas usage is relevant. | |
| /// </summary> | |
| public bool EqualsIncludingGasUsed(MessageResult other) => | |
| Equals(other) && GasUsed == other.GasUsed; |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #671 +/- ##
==========================================
- Coverage 76.75% 76.73% -0.03%
==========================================
Files 178 178
Lines 11719 11729 +10
Branches 1555 1557 +2
==========================================
+ Hits 8995 9000 +5
- Misses 2160 2165 +5
Partials 564 564
🚀 New features to boost your workflow:
|
wurdum
left a comment
wurdum
left a comment
There was a problem hiding this comment.
It smells really bad. Can we use metrics instead? Can the caller side query this information through JSON RPC?
Add infrastructure to expose block metadata via HTTP response headers for comparison benchmarking between Nitro EL and Nethermind EL. - Add SetMetadataHeaders(BlockHeader) overload for header-only paths - Call SetMetadataHeaders in ResultAtMessageIndexAsync - Gate header injection behind Arbitrum.ExposeMetadataHeaders config - Add benchmark Makefile targets with header exposure enabled
AnkushinDaniil
force-pushed
the
daniil/feature/expose-gas-used-in-message-result
branch
from
e1509dd to
0719b8e
Compare
AnkushinDaniil
changed the title
GasUsed field to MessageResult for benchmarking
Add infrastructure to expose block metadata via HTTP response headers for benchmarking between Nitro EL and Nethermind EL.
Related PR:
Related branches and commits: