| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
📝 Walkthrough
WalkthroughAdds a new scripts/derive_galileo_gas_parameter/ toolset to collect or load cached on-chain data and derive Galileo L1 gas parameters via a multi-stage pipeline, plus utilities, environment examples, packaging, and documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Script as derive_galileo_gas_parameter.py
participant RPC as Blockchain / Beacon RPC
participant FS as File System
User->>Script: run (mode=collect|load, options)
alt collect
Script->>RPC: fetch finalized batch events & block headers
RPC-->>Script: events & block data
Script->>RPC: fetch blob data, transactions, receipts (parallel)
RPC-->>Script: raw txs, blobs, receipts
Script->>Script: parse blobs, compress txs, compute sizes & L1 fees
Script->>Script: build tx_df and batch_df
Script->>FS: save pickle (tx_df, batch_df)
else load
Script->>FS: load pickle (tx_df, batch_df)
FS-->>Script: cached data
end
Script->>Script: calculate penalty_multiplier (P95) or use fixed
Script->>Script: aggregate batch-level costs and effective sizes
Script->>Script: solve for commit_scalar & blob_scalar
Script->>Script: analyze results (RMSE, MAE, recovery, penalty analysis)
Script-->>User: print parameters, metrics, and file locations
User->>Script: optionally run show_batches.py
Script-->>User: human-readable batch/tx summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem🚥 Pre-merge checks | ✅ 3 ✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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. ❤️ ShareComment @coderabbitai help to get the list of available commands and usage tips. |
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)scripts/derive_galileo_gas_parameter/Pipfile (1)📜 Review detailsscripts/derive_galileo_gas_parameter/show_batches.py (2)6-13: Consider pinning dependency versions for reproducibility.
Using wildcard "*" for most dependencies can lead to non-reproducible builds and potential breaking changes when new versions are released. For production tooling, consider pinning to specific versions or at least setting upper bounds.
🔎 Suggested improvement[packages] web3 = "<7,>=6" -pandas = "*" -numpy = "*" -rlp = "*" -zstandard = "*" -requests = "*" -async_timeout = "*" +pandas = ">=2.0,<3" +numpy = ">=1.24,<2" +rlp = ">=3.0,<4" +zstandard = ">=0.21,<1" +requests = ">=2.28,<3" +async_timeout = ">=4.0,<5"Alternatively, run pipenv lock to generate a Pipfile.lock that captures exact versions for reproducibility.
scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py (3)31-31: Remove extraneous f-prefix from string literal.
This f-string has no placeholders.
🔎 Proposed fix- print(f"\nBatch IDs:") + print("\nBatch IDs:")
72-73: Consider logging the exception type for better debugging.
While catching broad Exception is acceptable for a CLI utility, including the exception type would help with debugging.
🔎 Proposed fixexcept Exception as e: - print(f"Error reading file: {e}") + print(f"Error reading file: {type(e).__name__}: {e}")373-374: Global mutable state for caching.
Using a global dictionary for caching works for single-threaded execution but could cause issues if this module is ever used in a multi-threaded context. Consider using a class-based approach or passing the cache explicitly if thread-safety becomes a concern.
426-426: Unused variable version.
This variable is assigned but never used. Consider either removing it or using it for validation (e.g., asserting expected version).
🔎 Proposed fix- version = int(batch_data[0]) + version = int(batch_data[0]) # Currently unused, but parsed from batch header + # TODO: Add version validation if neededOr remove entirely if not needed:
- version = int(batch_data[0]) + # batch_data[0] contains version byte (unused)
1107-1112: DataFrame mutation side effect.
This function modifies tx_df directly by adding new columns. Since tx_df is passed in and also used later in analyze_results (which also modifies it), this side effect is likely intentional. However, consider either:
- Documenting this side effect in the docstring
- Using tx_df = tx_df.copy() if you want to avoid mutation
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 7de388e and 8fc6d65.
⛔ Files ignored due to path filters (1)19-19: pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue
(S301)
31-31: f-string without any placeholders
Remove extraneous f prefix
(F541)
72-72: Do not catch blind exception: Exception
(BLE001)
scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py38-38: Avoid specifying long messages outside the exception class
(TRY003)
40-40: Avoid specifying long messages outside the exception class
(TRY003)
146-146: Probable use of requests call without timeout
(S113)
186-186: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
398-398: Do not catch blind exception: Exception
(BLE001)
426-426: Local variable version is assigned to but never used
Remove assignment to unused variable version
(F841)
476-476: f-string without any placeholders
Remove extraneous f prefix
(F541)
518-518: Avoid specifying long messages outside the exception class
(TRY003)
522-522: f-string without any placeholders
Remove extraneous f prefix
(F541)
566-566: f-string without any placeholders
Remove extraneous f prefix
(F541)
607-607: f-string without any placeholders
Remove extraneous f prefix
(F541)
678-678: f-string without any placeholders
Remove extraneous f prefix
(F541)
744-744: f-string without any placeholders
Remove extraneous f prefix
(F541)
795-795: f-string without any placeholders
Remove extraneous f prefix
(F541)
809-809: f-string without any placeholders
Remove extraneous f prefix
(F541)
846-846: f-string without any placeholders
Remove extraneous f prefix
(F541)
884-884: f-string without any placeholders
Remove extraneous f prefix
(F541)
948-948: f-string without any placeholders
Remove extraneous f prefix
(F541)
959-959: f-string without any placeholders
Remove extraneous f prefix
(F541)
968-968: f-string without any placeholders
Remove extraneous f prefix
(F541)
1022-1022: Avoid specifying long messages outside the exception class
(TRY003)
1025-1025: pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue
(S301)
1067-1067: f-string without any placeholders
Remove extraneous f prefix
(F541)
1096-1096: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF002)
1097-1097: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF002)
1127-1127: f-string without any placeholders
Remove extraneous f prefix
(F541)
1168-1168: f-string without any placeholders
Remove extraneous f prefix
(F541)
1170-1170: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
1173-1173: f-string without any placeholders
Remove extraneous f prefix
(F541)
1175-1175: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
1228-1228: f-string without any placeholders
Remove extraneous f prefix
(F541)
1240-1240: f-string without any placeholders
Remove extraneous f prefix
(F541)
1297-1297: f-string without any placeholders
Remove extraneous f prefix
(F541)
1302-1302: f-string without any placeholders
Remove extraneous f prefix
(F541)
1307-1307: f-string without any placeholders
Remove extraneous f prefix
(F541)
1330-1330: f-string without any placeholders
Remove extraneous f prefix
(F541)
1335-1335: f-string without any placeholders
Remove extraneous f prefix
(F541)
1340-1340: f-string without any placeholders
Remove extraneous f prefix
(F541)
1409-1409: Avoid specifying long messages outside the exception class
(TRY003)
1440-1440: f-string without any placeholders
Remove extraneous f prefix
(F541)
1444-1444: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)scripts/derive_galileo_gas_parameter/claude.md (1)scripts/derive_galileo_gas_parameter/show_batches.py (1)1-70: Documentation looks good overall.
The documentation clearly explains the CLI options, modes, and monitoring approach. A couple of minor suggestions:
- The hardcoded /tmp/ path won't work on Windows - consider noting this is Linux/macOS specific or using a cross-platform alternative.
- Consider renaming to CLAUDE.md (uppercase) or README.md for better visibility in the directory listing.
scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py (3)17-19: Pickle deserialization - acceptable for local tooling.
The static analysis flags pickle.load() as a potential security issue (S301). Since this script only loads locally-generated pickle files from the companion derive_galileo_gas_parameter.py script, this is acceptable. However, adding a brief comment noting this assumption would be helpful for future maintainers.
740-741: Verify block range calculation - potential off-by-one issue.
The last_block is calculated as max(initial_L2_block_number + num_blocks), and then range(first_block, last_block) is used. Since Python's range() excludes the end value, the last block in the range won't be fetched.
If num_blocks represents the count of blocks starting from initial_L2_block_number, the calculation should be:
- Last block index = initial_L2_block_number + num_blocks - 1
- Range should be range(first_block, last_block + 1) to include it
Please verify the intended behavior:
- Does num_blocks include or exclude the initial block?
- Should the last block be fetched/processed?
Also applies to: 775-775, 813-813
198-331: Well-structured transaction serialization with good type coverage.
The get_raw_transaction_from_structured function handles multiple transaction types comprehensively (Legacy, EIP-2930, EIP-1559, L1 message, and EIP-7702). The error handling for unsupported types is appropriate.
1362-1458: Well-structured main function with clear execution flow.
The main function provides a clear step-by-step execution flow with good progress reporting. The argument handling is appropriate, and returning a results dictionary enables programmatic use of the script.
Sorry, something went wrong.
| url = f"{mainnet_beacon_url}/eth/v1/beacon/headers/head" | ||
| headers = {'accept': 'application/json'} | ||
| response = requests.get(url, headers=headers) | ||
| return int(response.json()['data']['header']['message']['slot']) - (latest_block_number - l1_head) |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Add timeout to prevent indefinite hangs.
This requests.get() call lacks a timeout, which could cause the script to hang indefinitely if the beacon API is unresponsive.
🔎 Proposed fix url = f"{mainnet_beacon_url}/eth/v1/beacon/headers/head"
headers = {'accept': 'application/json'}
- response = requests.get(url, headers=headers)
+ response = requests.get(url, headers=headers, timeout=10)
return int(response.json()['data']['header']['message']['slot']) - (latest_block_number - l1_head)146-146: Probable use of requests call without timeout
(S113)
🤖 Prompt for AI AgentsIn scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py around lines 144 to 147, the requests.get call has no timeout and can hang indefinitely; modify the call to include a reasonable timeout (e.g., timeout=10) and handle potential requests.exceptions.Timeout and other RequestException errors by raising or returning a clear error so the script doesn't block forever.
Sorry, something went wrong.
| while l1_base_fee == 0: | ||
| block = scroll_w3.eth.get_block(cur_block_num, full_transactions=True) | ||
| for tx in reversed(block['transactions']): | ||
| if tx.to == '0x5300000000000000000000000000000000000002' and tx.input.hex()[:10] == '0x39455d3a': | ||
| l1_base_fee = int.from_bytes(bytes.fromhex(tx.input.hex()[2:])[-64:-32], 'big') | ||
| l1_blob_base_fee = int.from_bytes(bytes.fromhex(tx.input.hex()[2:])[-32:], 'big') | ||
| cur_block_num -= 1 |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Potential infinite loop or crash if L1 fee oracle transaction not found.
This while loop decrements cur_block_num without a bound check. If the fee oracle transaction is never found in the blocks being searched, this will either loop indefinitely or eventually crash when trying to fetch block 0 or negative block numbers.
🔎 Proposed fix+ max_blocks_to_search = 1000 # Safety limit
+ blocks_searched = 0
while l1_base_fee == 0:
+ if blocks_searched >= max_blocks_to_search:
+ raise RuntimeError(f"Could not find L1 fee oracle transaction in {max_blocks_to_search} blocks starting from {first_block}")
block = scroll_w3.eth.get_block(cur_block_num, full_transactions=True)
for tx in reversed(block['transactions']):
if tx.to == '0x5300000000000000000000000000000000000002' and tx.input.hex()[:10] == '0x39455d3a':
l1_base_fee = int.from_bytes(bytes.fromhex(tx.input.hex()[2:])[-64:-32], 'big')
l1_blob_base_fee = int.from_bytes(bytes.fromhex(tx.input.hex()[2:])[-32:], 'big')
cur_block_num -= 1
+ blocks_searched += 1In scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py around lines 800 to 806, the while loop that decrements cur_block_num can run forever or underflow if the target L1 fee oracle transaction is never found; add a bounded search by introducing a minimum block number or max_lookback counter and check it each iteration, bail out when reached (raise a clear exception or log and exit) and wrap the block fetch in try/except to handle out-of-range requests; optionally add a configurable parameter (e.g., MAX_LOOKBACK or min_block_num) and use it to stop the loop gracefully instead of looping until negative block numbers.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 2
Fix all issues with AI Agents 🤖In @scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py: - Around line 123-141: The latest_finalized_event_block function can loop indefinitely or compute negative block ranges; add safety bounds by enforcing a minimum from_block of 0 and a maximum iteration/count limit (e.g., max_attempts) inside the while loop and exit gracefully if exceeded. Update the loop to decrement from_block but clamp it to >= 0, increment an attempt counter each iteration, and if attempts exceed the limit or from_block reaches 0 with no events, return None or raise a clear exception so callers of latest_finalized_event_block can handle the missing FinalizeBatch event. - Around line 393-416: The loop using found_the_blob and cur_slot can run forever if blob_hash is never found; add a maximum search depth (e.g., max_search_slots or max_iterations) and decrement cur_slot only while count < max; if the limit is reached, stop the loop and raise an explicit error or return a clear sentinel instead of looping forever. Update the loop around found_the_blob/cur_slot (and the return path that uses indexed_blob[blob_hash]) to enforce the bound and handle the "not found within max" case by raising an exception or returning (None, None, None) so callers don't get a KeyError.
scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py (4)📜 Review details429-432: Remove unused version variable.
The version variable is assigned but never used. Either remove it or add a comment explaining why it's extracted if it's for documentation purposes.
🔎 Proposed fix- version = int(batch_data[0]) + _ = int(batch_data[0]) # version byte (unused) payload_N = int.from_bytes(batch_data[1:4], 'big')
1027-1028: Consider pickle security for untrusted data scenarios.
Using pickle.load() is fine for locally-generated cache files, but be aware that pickle can execute arbitrary code during deserialization. If this tool might ever load files from external sources, consider adding integrity checks or using a safer serialization format like JSON for the metadata.
953-959: Consider pre-building block-to-batch index for better performance.
The current implementation iterates over batch_df for every transaction, resulting in O(n×m) complexity. For larger datasets, pre-building a block-to-batch mapping would improve performance.
🔎 Proposed optimization# Pre-build block to batch mapping block_to_batch_map = {} for idx, row in batch_df.iterrows(): start = row['initial_L2_block_number'] end = start + row['num_blocks'] for block_num in range(start, end): block_to_batch_map[block_num] = idx # Use O(1) lookup tx_df['batch_index'] = tx_df['block_number'].map(block_to_batch_map)
758-770: Consider more specific exception handling.
The rate-limit detection relies on string matching ('429' in str(e)), which could be fragile. For more robust handling, consider catching requests.exceptions.HTTPError and checking response.status_code, or using the web3 library's specific exception types.
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between 8fc6d65 and c799808.
📒 Files selected for processing (4)Learnt from: Thegaram Repo: scroll-tech/scroll PR: 1769 File: rollup/internal/config/relayer.go:112-114 Timestamp: 2025-11-27T18:50:44.578Z Learning: In `rollup/internal/config/relayer.go`, the fields `L1BaseFeeLimit` and `L1BlobBaseFeeLimit` in `GasOracleConfig` should never be set to 0. Zero values would break the gas oracle fee enforcement logic in `l1_relayer.go` by capping all fees to 0.
Applied to files:
[warning] 2-2: [UnorderedKey] The MAINNET_URL key should go before the SCROLL_URL key
(UnorderedKey)
[warning] 3-3: [UnorderedKey] The BEACON_URL key should go before the MAINNET_URL key
(UnorderedKey)
🪛 Ruff (0.14.10) scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py39-39: Avoid specifying long messages outside the exception class
(TRY003)
41-41: Avoid specifying long messages outside the exception class
(TRY003)
43-43: Avoid specifying long messages outside the exception class
(TRY003)
149-149: Probable use of requests call without timeout
(S113)
189-189: Avoid specifying long messages outside the exception class
(TRY003)
334-334: Avoid specifying long messages outside the exception class
(TRY003)
401-401: Do not catch blind exception: Exception
(BLE001)
429-429: Local variable version is assigned to but never used
Remove assignment to unused variable version
(F841)
479-479: f-string without any placeholders
Remove extraneous f prefix
(F541)
521-521: Avoid specifying long messages outside the exception class
(TRY003)
525-525: f-string without any placeholders
Remove extraneous f prefix
(F541)
569-569: f-string without any placeholders
Remove extraneous f prefix
(F541)
610-610: f-string without any placeholders
Remove extraneous f prefix
(F541)
681-681: f-string without any placeholders
Remove extraneous f prefix
(F541)
747-747: f-string without any placeholders
Remove extraneous f prefix
(F541)
798-798: f-string without any placeholders
Remove extraneous f prefix
(F541)
812-812: f-string without any placeholders
Remove extraneous f prefix
(F541)
849-849: f-string without any placeholders
Remove extraneous f prefix
(F541)
887-887: f-string without any placeholders
Remove extraneous f prefix
(F541)
951-951: f-string without any placeholders
Remove extraneous f prefix
(F541)
962-962: f-string without any placeholders
Remove extraneous f prefix
(F541)
971-971: f-string without any placeholders
Remove extraneous f prefix
(F541)
1025-1025: Avoid specifying long messages outside the exception class
(TRY003)
1028-1028: pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue
(S301)
1070-1070: f-string without any placeholders
Remove extraneous f prefix
(F541)
1099-1099: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF002)
1100-1100: Docstring contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF002)
1130-1130: f-string without any placeholders
Remove extraneous f prefix
(F541)
1171-1171: f-string without any placeholders
Remove extraneous f prefix
(F541)
1173-1173: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
1176-1176: f-string without any placeholders
Remove extraneous f prefix
(F541)
1178-1178: String contains ambiguous × (MULTIPLICATION SIGN). Did you mean x (LATIN SMALL LETTER X)?
(RUF001)
1231-1231: f-string without any placeholders
Remove extraneous f prefix
(F541)
1243-1243: f-string without any placeholders
Remove extraneous f prefix
(F541)
1300-1300: f-string without any placeholders
Remove extraneous f prefix
(F541)
1305-1305: f-string without any placeholders
Remove extraneous f prefix
(F541)
1310-1310: f-string without any placeholders
Remove extraneous f prefix
(F541)
1333-1333: f-string without any placeholders
Remove extraneous f prefix
(F541)
1338-1338: f-string without any placeholders
Remove extraneous f prefix
(F541)
1343-1343: f-string without any placeholders
Remove extraneous f prefix
(F541)
1412-1412: Avoid specifying long messages outside the exception class
(TRY003)
1443-1443: f-string without any placeholders
Remove extraneous f prefix
(F541)
1447-1447: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)scripts/derive_galileo_gas_parameter/README.md (1)scripts/derive_galileo_gas_parameter/.env.example (1)1-54: LGTM!
Clear and comprehensive documentation covering prerequisites, installation, and usage examples for both collect and load modes. The CLI options are well-documented with sensible defaults.
scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py (2)1-3: LGTM!
Environment variable examples are clear and consistent with the script's requirements. The placeholder YOUR_API_KEY pattern appropriately indicates where users need to substitute their own credentials.
32-43: LGTM!
Good practice validating required environment variables at startup. The validation provides clear error messages for missing configuration.
1365-1461: LGTM!
Well-structured main function with clear workflow orchestration. The argument parsing is comprehensive and the step-by-step execution with progress output provides good user feedback.
Sorry, something went wrong.
| def latest_finalized_event_block(width=5): | ||
| """Find the latest L1 block with a FinalizeBatch event""" | ||
| finalized_l1_head = -1 | ||
|
|
||
| to_block = w3.eth.block_number | ||
| from_block = to_block - width | ||
|
|
||
| while finalized_l1_head == -1: | ||
| event_filter = rollup_contract.events.FinalizeBatch.create_filter(fromBlock=from_block, toBlock=to_block) | ||
| events = event_filter.get_all_entries() | ||
|
|
||
| if len(events) > 0: | ||
| finalized_l1_head = events[-1]['blockNumber'] | ||
| break | ||
|
|
||
| to_block = from_block - 1 | ||
| from_block = from_block - width - 1 | ||
|
|
||
| return finalized_l1_head |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Add safety bounds to prevent indefinite looping.
The while loop at line 130 could loop indefinitely or access invalid block numbers if no FinalizeBatch events are ever found. Consider adding a minimum block number check or maximum iteration count.
🔎 Proposed fix def latest_finalized_event_block(width=5):
"""Find the latest L1 block with a FinalizeBatch event"""
finalized_l1_head = -1
to_block = w3.eth.block_number
from_block = to_block - width
+ max_iterations = 1000 # Safety limit
+ iterations = 0
while finalized_l1_head == -1:
+ if iterations >= max_iterations or from_block < 0:
+ raise RuntimeError(f"Could not find FinalizeBatch event within {iterations} iterations")
event_filter = rollup_contract.events.FinalizeBatch.create_filter(fromBlock=from_block, toBlock=to_block)
events = event_filter.get_all_entries()
if len(events) > 0:
finalized_l1_head = events[-1]['blockNumber']
break
to_block = from_block - 1
from_block = from_block - width - 1
+ iterations += 1
return finalized_l1_headIn @scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py around lines 123-141, The latest_finalized_event_block function can loop indefinitely or compute negative block ranges; add safety bounds by enforcing a minimum from_block of 0 and a maximum iteration/count limit (e.g., max_attempts) inside the while loop and exit gracefully if exceeded. Update the loop to decrement from_block but clamp it to >= 0, increment an attempt counter each iteration, and if attempts exceed the limit or from_block reaches 0 with no events, return None or raise a clear exception so callers of latest_finalized_event_block can handle the missing FinalizeBatch event.
Sorry, something went wrong.
| found_the_blob = False | ||
| while not found_the_blob: | ||
| cur_slot -= 1 | ||
| url = f"{mainnet_beacon_url}/eth/v1/beacon/blob_sidecars/{cur_slot}" | ||
|
|
||
| headers = {'accept': 'application/json'} | ||
| try: | ||
| response = requests.get(url, headers=headers, timeout=5) | ||
| except Exception as e: | ||
| print(f"[warn] request error at slot {cur_slot}: {e}") | ||
| continue | ||
|
|
||
| if response.status_code != 200: | ||
| print(f"[warn] non-200 from beacon at slot {cur_slot}: {response.status_code}") | ||
| continue | ||
|
|
||
| if 'data' in response.json().keys(): | ||
| for blob in response.json()['data']: | ||
| hash = kzg_to_versioned_hash(blob['kzg_commitment']) | ||
| if blob_hash == hash: | ||
| found_the_blob = True | ||
| indexed_blob[hash] = blob['blob'] | ||
|
|
||
| return indexed_blob[blob_hash], cur_slot, cur_block |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Add maximum slot search depth to prevent infinite loop.
The while not found_the_blob loop at line 394 could loop indefinitely if the blob hash is never found. Even though individual requests have timeouts, the outer loop has no termination bound.
🔎 Proposed fix found_the_blob = False
+ max_slots_to_search = 500 # Safety limit
+ slots_searched = 0
while not found_the_blob:
+ if slots_searched >= max_slots_to_search:
+ raise RuntimeError(f"Could not find blob {blob_hash} within {max_slots_to_search} slots")
cur_slot -= 1
+ slots_searched += 1
url = f"{mainnet_beacon_url}/eth/v1/beacon/blob_sidecars/{cur_slot}"401-401: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI AgentsIn @scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py around lines 393-416, The loop using found_the_blob and cur_slot can run forever if blob_hash is never found; add a maximum search depth (e.g., max_search_slots or max_iterations) and decrement cur_slot only while count < max; if the limit is reached, stop the loop and raise an explicit error or return a clear sentinel instead of looping forever. Update the loop around found_the_blob/cur_slot (and the return path that uses indexed_blob[blob_hash]) to enforce the bound and handle the "not found within max" case by raising an exception or returning (None, None, None) so callers don't get a KeyError.
Sorry, something went wrong.
Add scripts and environment setup for deriving Scroll gas fee parameters using data collected from L1 batches. Includes data collection, analysis, and batch visualization utilities.
Add read_current_gas_parameters() to display current commitScalar, blobScalar, penaltyFactor, and fee values from L1GasPriceOracle at script startup for comparison with derived results.
Receipt data (gas_used, L1_fee, gas_price, base_fee_per_gas) was collected but never used in downstream calculations. Removing this saves ~346s (41% of runtime) for 30-batch runs.
Doubles block fetching throughput from ~74 to ~160 blocks/s.
Compare derived gas parameters with current on-chain parameters to evaluate impact on historical transactions before deploying. Includes four analyses: per-tx fee changes, fee change by size group, cost recovery by batch, and penalty proportion breakdown.
- Increase event scan width from 5 to 1000 blocks per RPC call - Split Step 2 into 3 phases: scan events, parallel fetch tx/receipt (20 workers), then sequential blob parsing - Add L1 gas price distribution stats to comparison analysis
Replace L1 event log scanning with Rollup Explorer REST API for batch info, enabling faster and more reliable collection of commit/finalize transaction data. Remove beacon chain dependency and unused rollup contract ABI.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py (1)🧹 Nitpick comments (1)621-627: ⚠️ Potential issue | 🟠 Major
Bound the backward fee-oracle scan to avoid unbounded lookback.
The loop at Line 621 decrements block numbers indefinitely when no matching tx is found, which can hang or underflow block queries.
🛡️ Suggested fix🤖 Prompt for AI Agents- while l1_base_fee == 0: + max_lookback = 5000 + looked_back = 0 + while l1_base_fee == 0: + if cur_block_num < 0 or looked_back >= max_lookback: + raise RuntimeError( + f"Could not find L1 fee oracle update within {max_lookback} blocks before L2 block {first_block}" + ) block = scroll_w3.eth.get_block(cur_block_num, full_transactions=True) for tx in reversed(block['transactions']): if tx.to == '0x5300000000000000000000000000000000000002' and tx.input.hex()[:10] == '0x39455d3a': l1_base_fee = int.from_bytes(bytes.fromhex(tx.input.hex()[2:])[-64:-32], 'big') l1_blob_base_fee = int.from_bytes(bytes.fromhex(tx.input.hex()[2:])[-32:], 'big') cur_block_num -= 1 + looked_back += 1Verify each finding against the current code and only fix it if needed. In `@scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py` around lines 621 - 627, The backward scan loop that decrements cur_block_num while searching for the fee-oracle transaction (using scroll_w3.eth.get_block and checking tx.to and tx.input) is unbounded and can underflow or hang; add a bounded lookback (e.g., max_lookback or stop_at_block = 0) and change the loop to stop when cur_block_num <= stop_at_block or when the number of scanned blocks exceeds max_lookback, and if no matching tx is found after the bound, raise a clear exception or return an error rather than continuing to decrement; update the loop that sets l1_base_fee and l1_blob_base_fee to respect this new stop condition and include a clear failure path.
scripts/derive_galileo_gas_parameter/show_batches.py (1)🤖 Prompt for all review comments with AI agents72-73: Narrow exception handling to expected read/parse failures.
Catching Exception here makes operational failures harder to diagnose and can mask non-I/O bugs.
♻️ Suggested fix🤖 Prompt for AI Agents- except Exception as e: + except (OSError, pickle.UnpicklingError, KeyError, TypeError, ValueError) as e: print(f"Error reading file: {e}")Verify each finding against the current code and only fix it if needed. In `@scripts/derive_galileo_gas_parameter/show_batches.py` around lines 72 - 73, The broad except Exception in the try/except around the file read should be narrowed to only expected read/parse errors: replace "except Exception as e:" (and the print(f"Error reading file: {e}")) with a specific catch such as "except (OSError, IOError, UnicodeDecodeError, json.JSONDecodeError) as e:" to handle I/O and parsing failures in show_batches.py, and let any other exceptions propagate (or re-raise them) so non-I/O bugs are not swallowed; keep the same error message content but consider using logging instead of print if available.
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py`:
- Around line 803-827: The function calculate_penalty_multiplier (and CLI
batch-construction code using n_batches) assumes numeric inputs are positive;
add explicit validation to ensure target_penalty > 0 before computing
penalty_multiplier and that the resulting penalty_multiplier > 0, and validate
n_batches > 0 before building batch ranges; on invalid values raise a clear
ValueError (or exit with a descriptive error) so you never perform divisions by
zero or construct invalid ranges—apply the same checks where similar math occurs
around the other spots referenced (the n_batches/batch range logic).
- Around line 507-510: When commit_tx.blobVersionedHashes is empty, the current
division by n_blobs causes a divide-by-zero; change the logic around computing
commit_cost and blob_cost to guard that case: compute n_blobs =
len(commit_tx.blobVersionedHashes) and if n_blobs == 0 set commit_cost =
commit_receipt['gasUsed'] * commit_receipt['effectiveGasPrice'] and blob_cost =
0, otherwise perform the existing amortization using commit_receipt['gasUsed'],
commit_receipt['effectiveGasPrice'], commit_receipt['blobGasPrice'], and
commit_receipt['blobGasUsed'] divided by n_blobs.
- Around line 789-790: The current pickle.load call (with open(filename, 'rb')
as f: data = pickle.load(f)) deserializes untrusted data because filename is
derived from CLI args; replace it with a safe approach: either (A) switch to a
safe serialization format (e.g., write/read JSON or MessagePack using only
primitive types) and update upstream producers/consumers to use
json.load/json.dump, or (B) if pickle must be retained, verify authenticity
before loading by checking a cryptographic signature/HMAC of the file contents
(compute and verify HMAC using a server-side secret) and only then call
pickle.load. Also enforce that the filename is read-only/non-writable location
(e.g., restrict to an internal directory or reject paths outside a canonical
data directory) and add explicit validation of loaded data types/structure after
deserialization (e.g., ensure expected dict keys/types) to prevent code
execution via malicious payloads.
- Around line 927-928: Protect the scalar calculations by checking denominators
before dividing: in the block computing commit_scalar and blob_scalar, verify
total_sum_l1_base_effective and total_sum_blob_base_effective are non-zero and
handle zero cases instead of dividing; for example, if
total_sum_l1_base_effective == 0 or total_sum_blob_base_effective == 0 raise a
clear ValueError or log and set the corresponding scalar to a safe fallback
(e.g., 0 or None) — reference the variables commit_scalar, blob_scalar,
total_commit_finalize_cost, total_sum_l1_base_effective, total_blob_cost, and
total_sum_blob_base_effective to locate and update the code.
In `@scripts/derive_galileo_gas_parameter/show_batches.py`:
- Around line 18-19: The script currently calls pickle.load(f) on a
user-supplied filename (variable filename sourced from sys.argv[1]) which is
unsafe; replace this with a safe deserialization approach or add strict
validation and explicit user confirmation before unpickling. Specifically,
either: (a) switch to a safe format (e.g., JSON/protobuf) and update the loading
logic to json.load() or equivalent, or (b) restrict allowed input
paths/filenames, validate the file comes from a trusted source, and prompt the
user to confirm before calling pickle.load(filename) (or use a restricted
Unpickler if absolutely necessary). Update the code around the filename variable
and the pickle.load call to implement one of these safer alternatives.
---
Duplicate comments:
In `@scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py`:
- Around line 621-627: The backward scan loop that decrements cur_block_num
while searching for the fee-oracle transaction (using scroll_w3.eth.get_block
and checking tx.to and tx.input) is unbounded and can underflow or hang; add a
bounded lookback (e.g., max_lookback or stop_at_block = 0) and change the loop
to stop when cur_block_num <= stop_at_block or when the number of scanned blocks
exceeds max_lookback, and if no matching tx is found after the bound, raise a
clear exception or return an error rather than continuing to decrement; update
the loop that sets l1_base_fee and l1_blob_base_fee to respect this new stop
condition and include a clear failure path.
---
Nitpick comments:
In `@scripts/derive_galileo_gas_parameter/show_batches.py`:
- Around line 72-73: The broad except Exception in the try/except around the
file read should be narrowed to only expected read/parse errors: replace "except
Exception as e:" (and the print(f"Error reading file: {e}")) with a specific
catch such as "except (OSError, IOError, UnicodeDecodeError,
json.JSONDecodeError) as e:" to handle I/O and parsing failures in
show_batches.py, and let any other exceptions propagate (or re-raise them) so
non-I/O bugs are not swallowed; keep the same error message content but consider
using logging instead of print if available.
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📥 CommitsReviewing files that changed from the base of the PR and between c799808 and c0c7ae8.
⛔ Files ignored due to path filters (1)
Sorry, something went wrong.
| n_blobs = len(commit_tx.blobVersionedHashes) | ||
| commit_cost = (commit_receipt['gasUsed'] * commit_receipt['effectiveGasPrice']) / n_blobs | ||
| blob_cost = (commit_receipt['blobGasPrice'] * commit_receipt['blobGasUsed']) / n_blobs | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Guard commit-cost amortization when blob count is zero.
If blobVersionedHashes is empty, Line 508/509 divides by zero and aborts the run.
🛠️ Suggested fix- n_blobs = len(commit_tx.blobVersionedHashes)
+ blob_hashes = getattr(commit_tx, 'blobVersionedHashes', None) or []
+ n_blobs = len(blob_hashes)
+ if n_blobs == 0:
+ raise RuntimeError(
+ f"Commit tx {info['commit_tx_hash']} has no blobVersionedHashes; cannot amortize commit/blob cost."
+ )
commit_cost = (commit_receipt['gasUsed'] * commit_receipt['effectiveGasPrice']) / n_blobs
blob_cost = (commit_receipt['blobGasPrice'] * commit_receipt['blobGasUsed']) / n_blobsVerify each finding against the current code and only fix it if needed. In `@scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py` around lines 507 - 510, When commit_tx.blobVersionedHashes is empty, the current division by n_blobs causes a divide-by-zero; change the logic around computing commit_cost and blob_cost to guard that case: compute n_blobs = len(commit_tx.blobVersionedHashes) and if n_blobs == 0 set commit_cost = commit_receipt['gasUsed'] * commit_receipt['effectiveGasPrice'] and blob_cost = 0, otherwise perform the existing amortization using commit_receipt['gasUsed'], commit_receipt['effectiveGasPrice'], commit_receipt['blobGasPrice'], and commit_receipt['blobGasUsed'] divided by n_blobs.
Sorry, something went wrong.
| with open(filename, 'rb') as f: | ||
| data = pickle.load(f) |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🏁 Script executed:
# Verify pickle deserialization points and context around lines 789-790
rg -n -C5 "pickle\.load\(" scripts/derive_galileo_gas_parameterRepository: scroll-tech/scroll
Length of output: 2127
🏁 Script executed:
# Get broader context around line 789-790 to understand the function signature and filename source
sed -n '770,810p' scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.pyRepository: scroll-tech/scroll
Length of output: 1293
🏁 Script executed:
# Check if there are any security-related comments or validation before the pickle.load call
sed -n '750,795p' scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py | cat -nRepository: scroll-tech/scroll
Length of output: 1703
🏁 Script executed:
# Find where load_data is called to understand the data flow
rg -n "load_data\(" scripts/derive_galileo_gas_parameter/ -A2 -B2Repository: scroll-tech/scroll
Length of output: 1115
🏁 Script executed:
# Check how start_batch and end_batch are sourced (CLI args, user input, etc.)
rg -n "start_batch|end_batch" scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py | head -30Repository: scroll-tech/scroll
Length of output: 1156
Use safe deserialization or validate pickle file integrity.
Pickle deserialization on line 790 creates a code execution risk since the filename is constructed from CLI arguments (args.start_batch, args.end_batch). An attacker can provide batch parameters to load a predictable file path and replace the .pkl file with a malicious pickle payload.
Replace pickle with a safer format (JSON, MessagePack with restricted types) or implement cryptographic validation (e.g., HMAC signature) before deserialization. At minimum, never load pickle files from user-writable or externally controlled locations.
🧰 Tools 🪛 Ruff (0.15.2)[error] 790-790: pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue
(S301)
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py` around lines 789 - 790, The current pickle.load call (with open(filename, 'rb') as f: data = pickle.load(f)) deserializes untrusted data because filename is derived from CLI args; replace it with a safe approach: either (A) switch to a safe serialization format (e.g., write/read JSON or MessagePack using only primitive types) and update upstream producers/consumers to use json.load/json.dump, or (B) if pickle must be retained, verify authenticity before loading by checking a cryptographic signature/HMAC of the file contents (compute and verify HMAC using a server-side secret) and only then call pickle.load. Also enforce that the filename is read-only/non-writable location (e.g., restrict to an internal directory or reject paths outside a canonical data directory) and add explicit validation of loaded data types/structure after deserialization (e.g., ensure expected dict keys/types) to prevent code execution via malicious payloads.
Sorry, something went wrong.
| def calculate_penalty_multiplier(tx_df, target_penalty=0.1): | ||
| """ | ||
| Calculate penalty_multiplier based on transaction size distribution | ||
|
|
||
| Sets penalty_multiplier such that P95 transactions get target_penalty (default 10%) | ||
| from the quadratic term. | ||
|
|
||
| Args: | ||
| tx_df: DataFrame with 'compressed_tx_size' column | ||
| target_penalty: Target penalty ratio at P95 (default 0.1 = 10%) | ||
|
|
||
| Returns: | ||
| penalty_multiplier | ||
| """ | ||
| sizes = tx_df['compressed_tx_size'].values | ||
|
|
||
| p50 = np.percentile(sizes, 50) | ||
| p95 = np.percentile(sizes, 95) | ||
| p99 = np.percentile(sizes, 99) | ||
|
|
||
| # For P95 transaction: quadratic_term / linear_term = target_penalty | ||
| # (P95^2 / penalty_multiplier) / P95 = target_penalty | ||
| # penalty_multiplier = P95 / target_penalty | ||
| penalty_multiplier = p95 / target_penalty | ||
|
|
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Validate numeric CLI inputs before using them in divisions.
target_penalty <= 0 (Line 826) and penalty_multiplier <= 0 can trigger divide-by-zero or invalid fee math. n_batches <= 0 also breaks batch range construction.
✅ Suggested validation def calculate_penalty_multiplier(tx_df, target_penalty=0.1):
@@
+ if target_penalty <= 0:
+ raise ValueError("target_penalty must be > 0")
@@
def main():
@@
args = parser.parse_args()
+ if args.n_batches <= 0:
+ raise ValueError("--n-batches must be > 0")
+ if args.target_penalty <= 0:
+ raise ValueError("--target-penalty must be > 0")
+ if args.penalty_multiplier is not None and args.penalty_multiplier <= 0:
+ raise ValueError("--penalty-multiplier must be > 0")Also applies to: 1374-1384, 1420-1428
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py` around lines 803 - 827, The function calculate_penalty_multiplier (and CLI batch-construction code using n_batches) assumes numeric inputs are positive; add explicit validation to ensure target_penalty > 0 before computing penalty_multiplier and that the resulting penalty_multiplier > 0, and validate n_batches > 0 before building batch ranges; on invalid values raise a clear ValueError (or exit with a descriptive error) so you never perform divisions by zero or construct invalid ranges—apply the same checks where similar math occurs around the other spots referenced (the n_batches/batch range logic).
Sorry, something went wrong.
| commit_scalar = total_commit_finalize_cost / total_sum_l1_base_effective | ||
| blob_scalar = total_blob_cost / total_sum_blob_base_effective |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
Protect scalar solving from zero-denominator datasets.
If either aggregated denominator is zero, Lines 927-928 crash with division by zero and no actionable error.
🧮 Suggested guard total_blob_cost = batch_df['blob_cost'].sum()
total_sum_blob_base_effective = batch_df['sum_blob_base_effective'].sum()
+ if total_sum_l1_base_effective == 0 or total_sum_blob_base_effective == 0:
+ raise ValueError(
+ "Cannot solve scalars: effective-size denominators are zero."
+ )
+
commit_scalar = total_commit_finalize_cost / total_sum_l1_base_effective
blob_scalar = total_blob_cost / total_sum_blob_base_effectiveVerify each finding against the current code and only fix it if needed. In `@scripts/derive_galileo_gas_parameter/derive_galileo_gas_parameter.py` around lines 927 - 928, Protect the scalar calculations by checking denominators before dividing: in the block computing commit_scalar and blob_scalar, verify total_sum_l1_base_effective and total_sum_blob_base_effective are non-zero and handle zero cases instead of dividing; for example, if total_sum_l1_base_effective == 0 or total_sum_blob_base_effective == 0 raise a clear ValueError or log and set the corresponding scalar to a safe fallback (e.g., 0 or None) — reference the variables commit_scalar, blob_scalar, total_commit_finalize_cost, total_sum_l1_base_effective, total_blob_cost, and total_sum_blob_base_effective to locate and update the code.
Sorry, something went wrong.
| with open(filename, 'rb') as f: | ||
| data = pickle.load(f) |
There was a problem hiding this comment.
⚠️ Potential issue | 🟠 Major
🧩 Analysis chain🏁 Script executed:
#!/bin/bash
# Check if the file exists and examine its content
wc -l scripts/derive_galileo_gas_parameter/show_batches.pyRepository: scroll-tech/scroll
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Show the relevant sections of the file to verify the vulnerability
echo "=== Lines 15-25 (around the pickle.load call) ==="
sed -n '15,25p' scripts/derive_galileo_gas_parameter/show_batches.py
echo ""
echo "=== Lines 75-85 (around the CLI entry point) ==="
sed -n '75,85p' scripts/derive_galileo_gas_parameter/show_batches.py
echo ""
echo "=== Full show_batches function ==="
sed -n '10,30p' scripts/derive_galileo_gas_parameter/show_batches.pyRepository: scroll-tech/scroll
Length of output: 1278
🏁 Script executed:
#!/bin/bash
# Verify import statements and overall structure
head -15 scripts/derive_galileo_gas_parameter/show_batches.pyRepository: scroll-tech/scroll
Length of output: 354
Avoid unsafe pickle deserialization from untrusted CLI input.
Lines 18–19 deserialize file content with pickle.load() using a user-supplied path from sys.argv[1] (line 81). pickle.load() can execute arbitrary code during deserialization, presenting a code execution risk if a malicious pickle file is passed.
Consider using a safer serialization format (e.g., JSON, Protocol Buffers) or restricting this script to only load pickle files from trusted, validated sources with explicit user confirmation.
🧰 Tools 🪛 Ruff (0.15.2)[error] 19-19: pickle and modules that wrap it can be unsafe when used to deserialize untrusted data, possible security issue
(S301)
🤖 Prompt for AI AgentsVerify each finding against the current code and only fix it if needed. In `@scripts/derive_galileo_gas_parameter/show_batches.py` around lines 18 - 19, The script currently calls pickle.load(f) on a user-supplied filename (variable filename sourced from sys.argv[1]) which is unsafe; replace this with a safe deserialization approach or add strict validation and explicit user confirmation before unpickling. Specifically, either: (a) switch to a safe format (e.g., JSON/protobuf) and update the loading logic to json.load() or equivalent, or (b) restrict allowed input paths/filenames, validate the file comes from a trusted source, and prompt the user to confirm before calling pickle.load(filename) (or use a restricted Unpickler if absolutely necessary). Update the code around the filename variable and the pickle.load call to implement one of these safer alternatives.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
Summary
Changes
Summary by CodeRabbit
New Features
Documentation
Chores