refactor: Conceptual Minimality (File reduction, Deterministic execut…#76
refactor: Conceptual Minimality (File reduction, Deterministic execut…#76KDiamantidis wants to merge 3 commits intoStabilityNexus:mainfrom
Conversation
…ion, and Dead Code elimination)
WalkthroughMoved Changes
Sequence Diagram(s)(Skipped — changes do not introduce a new multi-component sequential flow requiring visualization.) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@main.py`:
- Line 330: The CLI `--host` value is never propagated: change the run_node
signature to accept a host parameter (e.g., add host: str to run_node), update
all callers (notably main()) to pass the parsed CLI host, and use that host when
calling network.start instead of the hardcoded "0.0.0.0"; ensure the
network.start(port=port, host=host) call uses the passed host so the
user-provided --host value is respected.
In `@minichain/chain.py`:
- Line 39: Replace the eager f-string in the logger call with parameterized
logging to avoid unnecessary interpolation: update the logger.warning call that
currently uses f"Block {block.index} rejected: {exc}" (the logger.warning usage
handling rejected blocks) to use a format string with placeholders and pass
block.index and exc as separate arguments (e.g., "Block %s rejected: %s",
block.index, exc) so interpolation is deferred until the message is emitted.
- Around line 43-45: The loop over block.transactions silently returns False
when temp_state.validate_and_apply(tx) fails; update this to log the failure
before returning so observers know which transaction caused rejection. Locate
the transaction validation loop (the for tx in block.transactions block and the
call temp_state.validate_and_apply(tx)) and add a log call using the same logger
used for block validation (the logger used at the earlier block validation
check) that includes identifying context such as block identifier (e.g.,
block.hash or block.header) and the transaction identifier or index (e.g., tx.id
or str(tx) plus its index), then return False as before.
In `@minichain/contract.py`:
- Around line 42-55: The worker process currently only uses a parent-side
timeout in the multiprocessing.Process created in the caller (the p =
multiprocessing.Process targeting _safe_exec_worker), which doesn't prevent
untrusted code from consuming unbounded CPU or memory; modify the child setup
inside _safe_exec_worker (or invoke resource.setrlimit in the child immediately
on entry) to call resource.setrlimit(resource.RLIMIT_AS, (max_bytes, max_bytes))
and resource.setrlimit(resource.RLIMIT_CPU, (max_seconds, max_seconds)) with
appropriate sane caps, ensuring these calls run before executing the untrusted
code so they enforce per-process virtual memory and CPU limits; keep the
existing p.join()/p.kill() timeout as a fallback.
- Around line 71-77: Split each one-line conditional that returns early into a
multi-line if block to satisfy Ruff E701: replace occurrences like `if
isinstance(node, (ast.Import, ast.ImportFrom)): return False` and the other
checks for `ast.Attribute` (node.attr.startswith("__")), `ast.Name`
(node.id.startswith("__")), `ast.Call` (getattr(node.func, "id", "") in
{"type","getattr","setattr","delattr"}), `ast.Constant` (isinstance(node.value,
str) and "__" in node.value) and `ast.JoinedStr` with the equivalent multi-line
form (if <condition>:\n return False) so that each early-return is on its own
line; locate these in the AST-walking block using the ast.walk(ast.parse(code))
loop.
- Around line 30-34: The comment noting "No floats" is inaccurate: floats can
still appear as literals or via arithmetic; update the validator and
serialization to enforce integer-only state by (1) enhancing _validate_code_ast
to walk the AST and reject ast.Constant nodes whose value is a float (including
extreme float literals like 1e309) as well as any ast.UnaryOp/ast.BinOp patterns
that produce float constants if you detect float operands, and (2) change the
JSON serialization of result["storage"] to use json.dumps(..., allow_nan=False)
so NaN/Infinity produce an error instead of being serialized; reference
safe_builtins, _validate_code_ast and the json.dumps(result["storage"]) call
when making these changes.
In `@minichain/p2p.py`:
- Around line 49-51: The setter set_on_peer_connected currently assigns callback
to _on_peer_connected without validation; mirror register_handler's behavior by
checking the provided callback is callable and raising a TypeError with a clear
message if not. Locate set_on_peer_connected and add the same callable check
used for register_handler (validate handler_callback) so _on_peer_connected
always receives a callable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 443405e7-2e6a-4569-97b1-a8004e42c7bc
📒 Files selected for processing (5)
main.pyminichain/chain.pyminichain/contract.pyminichain/p2p.pyminichain/validators.py
💤 Files with no reviewable changes (1)
- minichain/validators.py
| validate_block_link_and_hash(self.last_block, block) | ||
| except ValueError as exc: | ||
| logger.warning("Block %s rejected: %s", block.index, exc) | ||
| logger.warning(f"Block {block.index} rejected: {exc}") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer parameterized logging over f-strings.
F-strings are evaluated eagerly even when the log level would suppress the message, incurring unnecessary overhead. Use %-style formatting which defers string interpolation until the message is actually emitted.
♻️ Proposed fix
- logger.warning(f"Block {block.index} rejected: {exc}")
+ logger.warning("Block %s rejected: %s", block.index, exc)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| logger.warning(f"Block {block.index} rejected: {exc}") | |
| logger.warning("Block %s rejected: %s", block.index, exc) |
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 39-39: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/chain.py` at line 39, Replace the eager f-string in the logger call
with parameterized logging to avoid unnecessary interpolation: update the
logger.warning call that currently uses f"Block {block.index} rejected: {exc}"
(the logger.warning usage handling rejected blocks) to use a format string with
placeholders and pass block.index and exc as separate arguments (e.g., "Block %s
rejected: %s", block.index, exc) so interpolation is deferred until the message
is emitted.
| for tx in block.transactions: | ||
| result = temp_state.validate_and_apply(tx) | ||
|
|
||
| # Reject block if any transaction fails | ||
| if not result: | ||
| logger.warning("Block %s rejected: Transaction failed validation", block.index) | ||
| if not temp_state.validate_and_apply(tx): | ||
| return False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider logging transaction validation failures for observability.
Block validation failures are logged (line 39), but transaction validation failures return False silently. This inconsistency can make debugging harder when blocks are rejected due to invalid transactions.
♻️ Proposed fix
for tx in block.transactions:
if not temp_state.validate_and_apply(tx):
+ logger.warning("Block %s rejected: transaction validation failed", block.index)
return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for tx in block.transactions: | |
| result = temp_state.validate_and_apply(tx) | |
| # Reject block if any transaction fails | |
| if not result: | |
| logger.warning("Block %s rejected: Transaction failed validation", block.index) | |
| if not temp_state.validate_and_apply(tx): | |
| return False | |
| for tx in block.transactions: | |
| if not temp_state.validate_and_apply(tx): | |
| logger.warning("Block %s rejected: transaction validation failed", block.index) | |
| return False |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/chain.py` around lines 43 - 45, The loop over block.transactions
silently returns False when temp_state.validate_and_apply(tx) fails; update this
to log the failure before returning so observers know which transaction caused
rejection. Locate the transaction validation loop (the for tx in
block.transactions block and the call temp_state.validate_and_apply(tx)) and add
a log call using the same logger used for block validation (the logger used at
the earlier block validation check) that includes identifying context such as
block identifier (e.g., block.hash or block.header) and the transaction
identifier or index (e.g., tx.id or str(tx) plus its index), then return False
as before.
| try: | ||
| # Execute in a subprocess with timeout | ||
| queue = multiprocessing.Queue() | ||
| p = multiprocessing.Process( | ||
| target=_safe_exec_worker, | ||
| args=(code, globals_for_exec, context, queue) | ||
| args=(code, {"__builtins__": safe_builtins}, context, queue) | ||
| ) | ||
| p.start() | ||
| p.join(timeout=2) # 2 second timeout | ||
| p.join(timeout=2) | ||
|
|
||
| if p.is_alive(): | ||
| p.kill() | ||
| p.join() | ||
| logger.error("Contract execution timed out") | ||
| logger.error("Contract timeout") | ||
| return False |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Python's multiprocessing API, does Process.join(timeout=...) limit a child process's memory/CPU usage or only how long the parent waits? How is resource.setrlimit(RLIMIT_AS/RLIMIT_CPU) used to constrain spawned child processes on POSIX?
💡 Result:
Process.join(timeout=...) only limits how long the parent process waits for the child to terminate; it does not constrain the child process's memory or CPU usage. The child continues running unaffected until it finishes or is explicitly terminated (e.g., via p.terminate after timeout). If the timeout expires, join returns and the parent can check p.is_alive and act accordingly. On POSIX systems, resource.setrlimit(RLIMIT_AS, limits) or resource.setrlimit(RLIMIT_CPU, limits) called in the parent process sets limits that are inherited by child processes created via fork (including multiprocessing's 'fork' start method), as fork copies the parent's limits and they persist across exec. However, with 'spawn' or 'forkserver' start methods (now defaults on many platforms), the child starts a fresh Python interpreter, inheriting limits from the parent but potentially modified by multiprocessing setup. Limits apply per-process (not tree-wide), so each child has its own independent limits for AS (virtual memory) and CPU time (SIGXCPU on exceed). To set limits specifically for multiprocessing children, call setrlimit in the target function (runs in child) or use 'fork' start method after setting in parent. Note: RLIMIT_AS may not be fully supported on all POSIX (e.g., BSD/macOS lacks it), and requires appropriate privileges to raise hard limits.
Citations:
- 1: https://docs.python.org/3/library/multiprocessing.html
- 2: https://docs.python.org/3.15/library/multiprocessing.html
- 3: https://docs.python.org/3/library/resource.html
- 4: https://stackoverflow.com/questions/1689505/python-ulimit-and-nice-for-subprocess-call-subprocess-popen
- 5: https://luminousmen.com/post/python-resource-limitation
- 6: https://manpages.ubuntu.com/manpages/jammy/en/man2/setrlimit.2.html
- 7: https://stackoverflow.com/questions/64095876/multiprocessing-fork-vs-spawn
Restore hard resource caps via resource.setrlimit() to sandbox untrusted contract execution.
The current timeout mechanism only limits how long the parent waits for the child process; it does not constrain the child's memory or CPU usage. An untrusted contract can still allocate unbounded memory and OOM the host before the 2-second timeout expires. Add resource.setrlimit(RLIMIT_AS, ...) and/or resource.setrlimit(RLIMIT_CPU, ...) in the child process (via _safe_exec_worker target function) or in the parent before spawning to enforce hard per-process limits on virtual memory and CPU time. The timeout should remain as a fallback, but resource limits are the primary sandbox boundary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/contract.py` around lines 42 - 55, The worker process currently
only uses a parent-side timeout in the multiprocessing.Process created in the
caller (the p = multiprocessing.Process targeting _safe_exec_worker), which
doesn't prevent untrusted code from consuming unbounded CPU or memory; modify
the child setup inside _safe_exec_worker (or invoke resource.setrlimit in the
child immediately on entry) to call resource.setrlimit(resource.RLIMIT_AS,
(max_bytes, max_bytes)) and resource.setrlimit(resource.RLIMIT_CPU,
(max_seconds, max_seconds)) with appropriate sane caps, ensuring these calls run
before executing the untrusted code so they enforce per-process virtual memory
and CPU limits; keep the existing p.join()/p.kill() timeout as a fallback.
| for node in ast.walk(ast.parse(code)): | ||
| if isinstance(node, (ast.Import, ast.ImportFrom)): return False | ||
| if isinstance(node, ast.Attribute) and node.attr.startswith("__"): return False | ||
| if isinstance(node, ast.Name) and node.id.startswith("__"): return False | ||
| if isinstance(node, ast.Call) and getattr(node.func, "id", "") in {"type", "getattr", "setattr", "delattr"}: return False | ||
| if isinstance(node, ast.Constant) and isinstance(node.value, str) and "__" in node.value: return False | ||
| if isinstance(node, ast.JoinedStr): return False |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the contract.py file and verify its location
fd -t f "contract.py" | head -20Repository: StabilityNexus/MiniChain
Length of output: 112
🏁 Script executed:
# Read the specific lines mentioned in the review (71-77)
cat -n minichain/contract.py | sed -n '65,80p'Repository: StabilityNexus/MiniChain
Length of output: 1068
🏁 Script executed:
# Check if Ruff is configured and what rules are enabled
find . -maxdepth 3 -type f \( -name "ruff.toml" -o -name "pyproject.toml" -o -name ".ruff.toml" \) -exec cat {} \;Repository: StabilityNexus/MiniChain
Length of output: 2494
Split these one-line if statements to multi-line blocks; E701 is active in Ruff.
Lines 72–77 each have the pattern if ...: return False, which violates Ruff's E701 rule. The Ruff configuration explicitly selects the "E7" rule set, which includes E701 and is not suppressed. This will cause lint failures in CI.
Example fix pattern
- if isinstance(node, (ast.Import, ast.ImportFrom)): return False
+ if isinstance(node, (ast.Import, ast.ImportFrom)):
+ return False📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for node in ast.walk(ast.parse(code)): | |
| if isinstance(node, (ast.Import, ast.ImportFrom)): return False | |
| if isinstance(node, ast.Attribute) and node.attr.startswith("__"): return False | |
| if isinstance(node, ast.Name) and node.id.startswith("__"): return False | |
| if isinstance(node, ast.Call) and getattr(node.func, "id", "") in {"type", "getattr", "setattr", "delattr"}: return False | |
| if isinstance(node, ast.Constant) and isinstance(node.value, str) and "__" in node.value: return False | |
| if isinstance(node, ast.JoinedStr): return False | |
| for node in ast.walk(ast.parse(code)): | |
| if isinstance(node, (ast.Import, ast.ImportFrom)): | |
| return False | |
| if isinstance(node, ast.Attribute) and node.attr.startswith("__"): | |
| return False | |
| if isinstance(node, ast.Name) and node.id.startswith("__"): | |
| return False | |
| if isinstance(node, ast.Call) and getattr(node.func, "id", "") in {"type", "getattr", "setattr", "delattr"}: | |
| return False | |
| if isinstance(node, ast.Constant) and isinstance(node.value, str) and "__" in node.value: | |
| return False | |
| if isinstance(node, ast.JoinedStr): | |
| return False |
🧰 Tools
🪛 Ruff (0.15.7)
[error] 72-72: Multiple statements on one line (colon)
(E701)
[error] 73-73: Multiple statements on one line (colon)
(E701)
[error] 74-74: Multiple statements on one line (colon)
(E701)
[error] 75-75: Multiple statements on one line (colon)
(E701)
[error] 76-76: Multiple statements on one line (colon)
(E701)
[error] 77-77: Multiple statements on one line (colon)
(E701)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/contract.py` around lines 71 - 77, Split each one-line conditional
that returns early into a multi-line if block to satisfy Ruff E701: replace
occurrences like `if isinstance(node, (ast.Import, ast.ImportFrom)): return
False` and the other checks for `ast.Attribute` (node.attr.startswith("__")),
`ast.Name` (node.id.startswith("__")), `ast.Call` (getattr(node.func, "id", "")
in {"type","getattr","setattr","delattr"}), `ast.Constant`
(isinstance(node.value, str) and "__" in node.value) and `ast.JoinedStr` with
the equivalent multi-line form (if <condition>:\n return False) so that each
early-return is on its own line; locate these in the AST-walking block using the
ast.walk(ast.parse(code)) loop.
|
Please resolve the merge conflicts before review. Your PR will only be reviewed by a maintainer after all conflicts have been resolved. 📺 Watch this video to understand why conflicts occur and how to resolve them: |
|
|
||
| class ContractMachine: | ||
| """ | ||
| A minimal execution environment for Python-based smart contracts. |
There was a problem hiding this comment.
Please keep this warning.
There was a problem hiding this comment.
Good catch! I was aggressively cleaning up dead code and accidentally removed the docstring warning along with it. I will restore it right away in the next commit.
main.py
Outdated
| network.set_on_peer_connected(on_peer_connected) | ||
|
|
||
| await network.start(port=port, host=host) | ||
| await network.start(port=port, host="0.0.0.0") |
There was a problem hiding this comment.
Strange change here.
There was a problem hiding this comment.
Let me clarify! If you run the current main branch, the node crashes on startup with an UnboundLocalError: cannot access local variable 'host' because host is never actually defined before network.start() is called. I temporarily hardcoded "0.0.0.0" just to get the CLI to boot. I will fix it properly by defining the fallback variable instead!
| try: | ||
| import resource | ||
| # Limit CPU time (seconds) and memory (bytes) - example values | ||
| resource.setrlimit(resource.RLIMIT_CPU, (2, 2)) # Align with p.join timeout (2 seconds) |
There was a problem hiding this comment.
Could you explain why you removed the setting of resource limits, @KDiamantidis ?
There was a problem hiding this comment.
Absolutely! The resource module is Unix-specific. Including it completely breaks cross-platform execution (e.g., if someone tries to run the node on Windows, it throws an ImportError and crashes instantly). Since we already enforce an absolute time boundary via p.join(timeout=2) and immediately terminate the process, I thought removing it helps conceptual minimality and portability.
If you'd prefer to keep it, I can wrap it in a sys.platform check or a try/except ImportError block. Let me know what you prefer!
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/contract.py`:
- Around line 33-38: Restore the original longer docstring warning above the
safe_builtins definition explaining the sandbox limitations and that floats are
not allowed/deterministic behavior, since the prior discussion agreed it was
accidentally removed; update the comment surrounding safe_builtins (the "No
floats" line) to the original explanatory warning text and reference the
existing AST float rejection (validate_ast/AST visitor) and JSON serialization
setting (allow_nan=False) so the warning accurately reflects that floats are
rejected at parse time and JSON enforces non-NaN serialization.
- Around line 68-70: Replace the idiomatic logging call in the except block that
currently uses logger.error("Contract Execution Failed", exc_info=True) with
logger.exception("Contract Execution Failed") to log the stack trace more
cleanly; keep the surrounding exception handler and the return False intact so
behavior doesn't change (look for the except block that logs "Contract Execution
Failed" in minichain/contract.py).
In `@minichain/p2p.py`:
- Around line 17-18: The is_valid_receiver function should defensively handle
non-string inputs to avoid re.fullmatch raising TypeError; update
is_valid_receiver to first check that receiver is an instance of str (or not
None) and immediately return False for other types, then perform the existing
re.fullmatch check (reference function is_valid_receiver).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 72a22c75-5093-42b4-bf29-37af079a5c6c
📒 Files selected for processing (3)
main.pyminichain/contract.pyminichain/p2p.py
| # Minimal & Deterministic builtins (No floats) | ||
| safe_builtins = { | ||
| "True": True, | ||
| "False": False, | ||
| "None": None, | ||
| "range": range, | ||
| "len": len, | ||
| "min": min, | ||
| "max": max, | ||
| "abs": abs, | ||
| "str": str, # Keeping str for basic functionality, relying on AST checks for safety | ||
| "bool": bool, | ||
| "float": float, | ||
| "list": list, | ||
| "dict": dict, | ||
| "tuple": tuple, | ||
| "sum": sum, | ||
| "Exception": Exception, # Added to allow contracts to raise exceptions | ||
| "True": True, "False": False, "None": None, | ||
| "range": range, "len": len, "str": str, "int": int, | ||
| "bool": bool, "list": list, "dict": dict, "Exception": Exception | ||
| } |
There was a problem hiding this comment.
The docstring warning should be restored per the previous discussion.
The author acknowledged in the past review that the docstring warning was accidentally removed and agreed to restore it. The current comment "No floats" is brief but the original warning provided important context about the sandbox limitations.
Additionally, float literals are now properly rejected at the AST level (line 80) and JSON serialization enforces allow_nan=False (line 64), which addresses the earlier determinism concerns.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/contract.py` around lines 33 - 38, Restore the original longer
docstring warning above the safe_builtins definition explaining the sandbox
limitations and that floats are not allowed/deterministic behavior, since the
prior discussion agreed it was accidentally removed; update the comment
surrounding safe_builtins (the "No floats" line) to the original explanatory
warning text and reference the existing AST float rejection (validate_ast/AST
visitor) and JSON serialization setting (allow_nan=False) so the warning
accurately reflects that floats are rejected at parse time and JSON enforces
non-NaN serialization.
| except Exception: | ||
| logger.error("Contract Execution Failed", exc_info=True) | ||
| return False |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using logger.exception() for cleaner logging.
The static analysis suggests using logger.exception() instead of logger.error(..., exc_info=True). They are functionally equivalent, but exception() is more idiomatic.
♻️ Proposed fix
except Exception:
- logger.error("Contract Execution Failed", exc_info=True)
+ logger.exception("Contract Execution Failed")
return False🧰 Tools
🪛 Ruff (0.15.7)
[warning] 69-69: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/contract.py` around lines 68 - 70, Replace the idiomatic logging
call in the except block that currently uses logger.error("Contract Execution
Failed", exc_info=True) with logger.exception("Contract Execution Failed") to
log the stack trace more cleanly; keep the surrounding exception handler and the
return False intact so behavior doesn't change (look for the except block that
logs "Contract Execution Failed" in minichain/contract.py).
| def is_valid_receiver(receiver): | ||
| return bool(re.fullmatch(r"[0-9a-fA-F]{40}|[0-9a-fA-F]{64}", receiver)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider adding defensive type checking for robustness.
re.fullmatch() raises TypeError if receiver is None or not a string. While current callers guard against this (line 151 checks for None, main.py always passes strings from CLI parsing), the function could be more defensive for future use cases.
♻️ Optional defensive improvement
def is_valid_receiver(receiver):
+ if not isinstance(receiver, str):
+ return False
return bool(re.fullmatch(r"[0-9a-fA-F]{40}|[0-9a-fA-F]{64}", receiver))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def is_valid_receiver(receiver): | |
| return bool(re.fullmatch(r"[0-9a-fA-F]{40}|[0-9a-fA-F]{64}", receiver)) | |
| def is_valid_receiver(receiver): | |
| if not isinstance(receiver, str): | |
| return False | |
| return bool(re.fullmatch(r"[0-9a-fA-F]{40}|[0-9a-fA-F]{64}", receiver)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/p2p.py` around lines 17 - 18, The is_valid_receiver function should
defensively handle non-string inputs to avoid re.fullmatch raising TypeError;
update is_valid_receiver to first check that receiver is an instance of str (or
not None) and immediately return False for other types, then perform the
existing re.fullmatch check (reference function is_valid_receiver).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
minichain/contract.py (3)
76-82:⚠️ Potential issue | 🟡 MinorSplit the one-line guards; Ruff E701 is still active here.
These
if ...: return Falsestatements are still reported as errors, so lint/CI will stay red until they are expanded into normal multi-line blocks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/contract.py` around lines 76 - 82, The one-line guard statements in minichain/contract.py (the series of if ...: return False checks that inspect ast nodes such as ast.Import, ast.Attribute, ast.Name, ast.Call, ast.Constant, ast.JoinedStr) must be expanded into multi-line if blocks to satisfy Ruff E701; replace each single-line form like "if isinstance(node, (ast.Import, ast.ImportFrom)): return False" with a standard block: "if ...:\n return False" and do the same for the checks referencing node.attr.startswith("__"), node.id.startswith("__"), the call whitelist using getattr(node.func, 'id', ''), the string/number Constant checks, and JoinedStr so the function (where these guards live) no longer contains one-line statements.
47-58:⚠️ Potential issue | 🔴 CriticalReintroduce hard resource caps in the child process.
p.join(timeout=2)only bounds how long the parent waits. A hostile contract can still burn CPU or allocate enough memory to OOM the node before Lines 55-59 kill it. If Windows portability is the concern, guard theresourceimport inside_safe_exec_workerinstead of dropping the limit entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/contract.py` around lines 47 - 58, The child process currently lacks hard resource caps because the resource module was removed from the parent; fix this by adding guarded resource limits inside the child entrypoint _safe_exec_worker: inside _safe_exec_worker, import resource in a try/except and, when available, call resource.setrlimit for RLIMIT_CPU (small timeout slightly less than parent join), RLIMIT_AS (memory cap) and optionally RLIMIT_FSIZE/RLIMIT_NPROC to bound file size and processes before executing the untrusted code, and only skip these calls on platforms where resource is unavailable (Windows). This ensures the hostile contract cannot exhaust CPU or memory even if the parent is slow to kill the process.
34-39:⚠️ Potential issue | 🟠 MajorThe “No floats” guarantee is still incomplete.
The AST now rejects float literals, but
1 / 2and2 ** -1still produce finite floats;allow_nan=Falsewill not stop0.5, so those values can still be written at Line 66. Either ban float-producing operators in_validate_code_ast()or recursively rejectfloat/complexvalues before commit.Also applies to: 75-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@minichain/contract.py` around lines 34 - 39, The AST rejection of float literals is incomplete because operations like 1/2 or 2**-1 still produce float/complex values; fix this by (1) extending _validate_code_ast() to explicitly reject float-producing AST nodes (e.g., ast.BinOp with ast.Div, ast.Pow — and flag Pow when exponent can be negative or non-integer) and (2) adding a runtime defensive check function (e.g., _assert_no_float_or_complex(obj)) that recursively traverses return values/committed objects and raises if any float or complex is present; call that check just before commit/serialization and after execution to ensure no float/complex sneaks through, and keep safe_builtins unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@minichain/contract.py`:
- Around line 65-66: The code currently validates JSON serializability but
doesn't enforce that result["storage"] is a dict; before calling
self.state.update_contract_storage(contract_address, result["storage"]) (in the
contract execution path that produces result), add a guard that checks
isinstance(result.get("storage"), dict) and reject non-dict values (raise a
TypeError/ValueError or return an error) so only mappings are persisted; keep
the existing json.dumps(...) check but perform the dict-type validation first to
preserve the storage invariant used by dict(account.get("storage", {})) and
reference result["storage"] and self.state.update_contract_storage for the
change location.
---
Duplicate comments:
In `@minichain/contract.py`:
- Around line 76-82: The one-line guard statements in minichain/contract.py (the
series of if ...: return False checks that inspect ast nodes such as ast.Import,
ast.Attribute, ast.Name, ast.Call, ast.Constant, ast.JoinedStr) must be expanded
into multi-line if blocks to satisfy Ruff E701; replace each single-line form
like "if isinstance(node, (ast.Import, ast.ImportFrom)): return False" with a
standard block: "if ...:\n return False" and do the same for the checks
referencing node.attr.startswith("__"), node.id.startswith("__"), the call
whitelist using getattr(node.func, 'id', ''), the string/number Constant checks,
and JoinedStr so the function (where these guards live) no longer contains
one-line statements.
- Around line 47-58: The child process currently lacks hard resource caps
because the resource module was removed from the parent; fix this by adding
guarded resource limits inside the child entrypoint _safe_exec_worker: inside
_safe_exec_worker, import resource in a try/except and, when available, call
resource.setrlimit for RLIMIT_CPU (small timeout slightly less than parent
join), RLIMIT_AS (memory cap) and optionally RLIMIT_FSIZE/RLIMIT_NPROC to bound
file size and processes before executing the untrusted code, and only skip these
calls on platforms where resource is unavailable (Windows). This ensures the
hostile contract cannot exhaust CPU or memory even if the parent is slow to kill
the process.
- Around line 34-39: The AST rejection of float literals is incomplete because
operations like 1/2 or 2**-1 still produce float/complex values; fix this by (1)
extending _validate_code_ast() to explicitly reject float-producing AST nodes
(e.g., ast.BinOp with ast.Div, ast.Pow — and flag Pow when exponent can be
negative or non-integer) and (2) adding a runtime defensive check function
(e.g., _assert_no_float_or_complex(obj)) that recursively traverses return
values/committed objects and raises if any float or complex is present; call
that check just before commit/serialization and after execution to ensure no
float/complex sneaks through, and keep safe_builtins unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c06f1652-1a7b-4da4-a30a-88fabc494665
📒 Files selected for processing (1)
minichain/contract.py
| json.dumps(result["storage"], allow_nan=False) # Validate JSON serializability | ||
| self.state.update_contract_storage(contract_address, result["storage"]) |
There was a problem hiding this comment.
Reject non-dict storage before persisting it.
Serializability alone does not protect the storage invariant. A contract can rebind storage = 1; that passes this check, minichain/state.py:146-150 will persist it, and the next call blows up at Line 29 when dict(account.get("storage", {})) runs. Require result["storage"] to stay a dict.
Minimal guard
- json.dumps(result["storage"], allow_nan=False) # Validate JSON serializability
- self.state.update_contract_storage(contract_address, result["storage"])
+ if not isinstance(result["storage"], dict):
+ logger.error("Contract returned invalid storage type")
+ return False
+ json.dumps(result["storage"], allow_nan=False) # Validate JSON serializability
+ self.state.update_contract_storage(contract_address, result["storage"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@minichain/contract.py` around lines 65 - 66, The code currently validates
JSON serializability but doesn't enforce that result["storage"] is a dict;
before calling self.state.update_contract_storage(contract_address,
result["storage"]) (in the contract execution path that produces result), add a
guard that checks isinstance(result.get("storage"), dict) and reject non-dict
values (raise a TypeError/ValueError or return an error) so only mappings are
persisted; keep the existing json.dumps(...) check but perform the dict-type
validation first to preserve the storage invariant used by
dict(account.get("storage", {})) and reference result["storage"] and
self.state.update_contract_storage for the change location.
2 participants
Summary
Refactored the core execution and networking layers to achieve "Conceptual Minimality" as per the project roadmap. This PR reduces the codebase by ~170 lines, eliminates redundant files, and fixes a critical bug that prevented the CLI node from starting on the current
mainbranch.Addressed Issues:
Addresses Roadmap task: "- [ ] Find out how the current prototype can be made more minimal/clean"
Screenshots/Recordings:
Node successfully booting and executing commands after refactor:
Additional Notes:
mainbranch was crashing on startup due tomain.pycalling non-existent methods (set_on_peer_connected) and missing variables (host). I implemented the necessary setter inP2PNetworkand fixed the bindings inmain.py.floatfrom contractsafe_builtinsto ensure cross-platform execution consistency (addressing concerns raised in PR PID difficulty adjuster with pure integer arithmetic #63).validators.pyand moved its single regex function intop2p.pyto keep the package structure concise.AI Usage Disclosure:
I have used the following AI models and tools:
I used Gemini 3.1 Pro as a pair-programmer to assist in identifying redundant code patterns, refactoring the AST validation logic, and troubleshooting the P2P connection tracebacks. Every change was manually verified through unit tests and local node execution.
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor