[Do not merge] Simple Giga RPC check - #4197
philipsu522 wants to merge 1 commit into
Conversation
PR SummaryLow Risk Overview The script exercises EIP-1559 and legacy transfers, contract creation from hand-written bytecode (counter with Reviewed by Cursor Bugbot for commit 41c91db. Bugbot is set up for automated code reviews on this repo. Configure here. |
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
Adds a standalone Python end-to-end RPC checker for giga-1; the transaction sequencing, receipt assertions, hand-written counter bytecode and LOG0 bloom reconstruction all check out, but the private key is printed verbatim when a cast invocation times out, defeating the redaction the script otherwise applies.
Findings: 1 blocking | 3 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] The script lands at the repo root, while every other Python tool in the tree lives under a package or
scripts/directory (scripts/seid_problems_detector.py,loadtest/scripts/,giga/tests/). Consider moving it next to the giga tests or underscripts/, and adding a short usage note (even a docstring block) listing the required environment variables —EVM_KEY,EVM_FROM,EVM_CHAIN_ID, plus optionalEVM_RPC/EVM_CHECK_TIMEOUT— and thertk/foundry prerequisites. Right now nothing in the repo tells an operator how to run it, and the script hard-fails with a bareKeyErrorif one variable is missing. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
|
|
||
| def cast(*args): | ||
| result = subprocess.run(["rtk", "proxy", "cast", *map(str, args)], | ||
| capture_output=True, text=True, timeout=TIMEOUT) |
There was a problem hiding this comment.
[blocker] subprocess.run(..., timeout=TIMEOUT) raises subprocess.TimeoutExpired on a stalled cast, and that exception's __str__ is "Command '%s' timed out after %s seconds" % (self.cmd, self.timeout) — self.cmd is the full argv, which includes --private-key <EVM_KEY>. Since nothing catches it, the traceback prints the raw private key to stdout/stderr, which is the one thing line 54 and line 98 go out of their way to redact. A node that stops responding is exactly the failure this script exists to surface, so this is a likely path, not a corner case.
Wrap the call and sanitize before re-raising, e.g.:
try:
result = subprocess.run(["rtk", "proxy", "cast", *map(str, args)],
capture_output=True, text=True, timeout=TIMEOUT)
except subprocess.TimeoutExpired as expired:
raise AssertionError(f"cast timed out after {TIMEOUT}s: {str(expired).replace(KEY, '[REDACTED]')}") from NoneWhile here: the failure message on line 62 interpolates raw cast stdout without the replace(KEY, "[REDACTED]") that the evidence file on line 98 applies — worth routing both through one redaction helper so a new call site can't miss it.
|
|
||
|
|
||
| def cast(*args): | ||
| result = subprocess.run(["rtk", "proxy", "cast", *map(str, args)], |
There was a problem hiding this comment.
[suggestion] The rtk proxy prefix is hardcoded, and rtk appears nowhere else in this repo. An operator with foundry installed still gets an immediate FileNotFoundError without it, and there is no way to point the script at a plain cast. Reading the command from the environment keeps both setups working:
CAST = os.environ.get("EVM_CAST", "rtk proxy cast").split()and then subprocess.run([*CAST, *map(str, args)], ...).
| CHAIN = int(os.environ["EVM_CHAIN_ID"], 0) | ||
| TIMEOUT = int(os.environ.get("EVM_CHECK_TIMEOUT", "90")) | ||
| OUT = Path("giga-check-" + time.strftime("%Y%m%d-%H%M%S") + "-" + secrets.token_hex(2)) | ||
| OUT.mkdir() |
There was a problem hiding this comment.
[suggestion] OUT.mkdir() runs at import, before any of the preflight checks on lines 149-151, so every misconfigured or aborted run leaves an empty giga-check-<timestamp>-<rand>/ directory behind in the working directory. giga-check-* is not in .gitignore either, so running this from the repo root (the natural place, given the file lives there) pollutes git status with untracked directories. Creating the directory lazily after the preflight checks pass, and adding a .gitignore entry for the evidence directories, would avoid both.
Describe your changes and provide context
Testing performed to validate your change