feat: add payable value support for intelligent contracts#1547
Open
MuncleUscles wants to merge 8 commits intomainfrom
Open
feat: add payable value support for intelligent contracts#1547MuncleUscles wants to merge 8 commits intomainfrom
MuncleUscles wants to merge 8 commits intomainfrom
Conversation
When a transaction reaches ACCEPTED, its contract_snapshot is saved with empty states (before the leader's execution commits to DB). On appeal, TransactionContext reused this empty snapshot instead of loading fresh state from the factory, causing all appeal validators to crash with 'NoneType' errors in GenVM root storage initialization. Fix: check if the saved snapshot has actual state data before using it. If the accepted state is empty, fall back to the factory which loads real contract state from the database. Includes regression test covering: - Empty snapshot triggers factory (the bug scenario) - Real snapshot is still reused (optimization preserved) - Deploy transactions with empty snapshot on appeal
Wire transaction value through GenVM execution so contracts can receive native tokens via @gl.public.write.payable decorator. Changes: - GenVM value plumbing: pass transaction.value through exec_transaction → run_contract/deploy_contract → _run_genvm → message dict (was hardcoded 0) - Live balance read: _SnapshotView.get_balance uses balance override for executing contract (current balance + incoming value) during execution - Balance accounting: debit contract on accepted/finalized message emission, credit contract on incoming value (first acceptance only), debit sender EOA - Message value propagation: child transactions carry pending_transaction.value instead of hardcoded 0 - Cumulative accepted messages: removed appealed guard on child emission so appeal re-execution emits new children (each acceptance round is independent) - Sender pre-check: insufficient sender balance → UNDETERMINED before execution - Child credit on activation: target contract credited when child tx enters proposing state - Atomic balance operations: new debit_account_balance/credit_account_balance methods with conditional SQL to prevent races - Sibling nonce fix: compute nonces as base + offset to avoid collision - Remove deploy-with-value rejection (was blocked, now allowed) - Remove empty rollup event on appealed acceptance (handled uniformly) - Add triggered_by_hash to Transaction domain type and worker claim queries - Add conservation invariant verification module - Add payable_escrow.py test contract and integration tests - Add gen_call value parameter extraction for simulation path
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughConditionally reuse contract snapshots; propagate value through execution and emitted messages; add atomic DB balance ops and invariants; perform pre-checks and accepted/finalized round balance accounting; enable payable contract behavior and add corresponding tests. Changes
Sequence DiagramsequenceDiagram
participant Client as RPC Client
participant RPC as Protocol RPC
participant Consensus as Consensus Engine
participant Node as Node/Execution
participant DB as Database
Client->>RPC: send_raw_transaction(type=SEND, value=0x64)
RPC->>RPC: parse hex value -> 100
RPC->>Consensus: add_transaction(value=100, triggered_by=null)
Consensus->>Consensus: PendingState pre-check sender balance >= 100
alt Insufficient balance
Consensus->>Consensus: mark transaction UNDETERMINED (stop)
else Balance sufficient
Consensus->>Node: exec_transaction(value=100, is_first_execution=...)
Node->>Node: _run_genvm(value=100) — apply snapshot balance_override
Node-->>Consensus: execution_success (receipt, child messages with value)
Consensus->>DB: debit_account_balance(sender, amount) [first-accept]
Consensus->>DB: credit_account_balance(contract, amount) [first-accept]
Consensus->>Consensus: AcceptedState computes net delta and may emit children (carry value, sequential nonces)
Consensus->>DB: FinalizingState debits contract for finalized-message values
Consensus->>DB: verify_conservation()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
The empty EmitRollupEventEffect in decide_accepted() appealed branch was removed — rollup events are now emitted uniformly in AcceptedState.handle() for cumulative message semantics. Update test to assert absence instead.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/protocol_rpc/endpoints.py (1)
1198-1204:⚠️ Potential issue | 🟠 MajorForward
call_valueinto deploy simulations.
call_valueis parsed at Line 1084 andNode.deploy_contract()now acceptsvalue, but the deploy branch still drops it.gen_call/sim_callwill diverge from real execution for payable constructors or any deployment path that inspectsgl.message.valueor contract balance during init.💸 Proposed fix
receipt = await node.deploy_contract( from_address=from_address, code_to_deploy=decoded_data.contract_code, calldata=decoded_data.calldata, transaction_created_at=txn_created_at, + value=call_value, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/protocol_rpc/endpoints.py` around lines 1198 - 1204, The deployment path drops the parsed call_value so simulated deployments can differ from real execution; when calling transactions_parser.decode_deployment_data and then node.deploy_contract, forward the parsed call_value (the variable parsed earlier where call_value is extracted) into node.deploy_contract using its value/ value parameter so that node.deploy_contract(from_address=..., code_to_deploy=decoded_data.contract_code, calldata=decoded_data.calldata, value=call_value, transaction_created_at=txn_created_at) is used; update the deploy branch that invokes node.deploy_contract to include this value parameter so payable constructors and init-time balance checks see the correct call value.
🧹 Nitpick comments (1)
tests/unit/consensus/test_decisions_accepted.py (1)
329-333: Add the return annotation on the new test method.
-> Noneis enough here.As per coding guidelines, "Include type hints in all Python code".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/consensus/test_decisions_accepted.py` around lines 329 - 333, The test method test_no_rollup_event_on_appeal_reacceptance is missing a return type annotation; update its definition to include the return type "-> None" (i.e., def test_no_rollup_event_on_appeal_reacceptance(self) -> None:) to satisfy the project's type-hinting guideline and ensure all Python code includes type hints.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/consensus/base.py`:
- Around line 1462-1466: The child-tx crediting logic in the PendingState path
is not idempotent: when tx_value > 0 and is_child_tx the code calls
context.accounts_manager.credit_account_balance(context.transaction.to_address,
tx_value) on every re-entry; change this to perform the credit exactly once by
either (a) moving the credit to the activation edge where the child transaction
is first transitioned to active, or (b) persistently marking the transaction as
"already_activated" (e.g., a boolean flag on the transaction record or a ledger
of activated tx ids) and check that guard before calling credit_account_balance;
update the PendingState/activation transition code paths and use the transaction
id or context.transaction to identify and record the one-time activation so
repeated PendingState entries do not re-credit the same child.
- Around line 1446-1460: The sender-balance precheck in the tx handling block
(tx_value, is_child_tx, context.transaction) must be skipped for appeal/retry
executions to avoid re-debiting already-accepted payable txns; update the if
guard so that the check only runs for fresh top-level executions (e.g., add a
guard that returns early when this execution is an appeal/retry — check a
retry/appeal flag such as context.execution.is_retry or
context.transaction.attempt > 0 or context.is_appeal) and only perform the
balance comparison and the call to
ConsensusAlgorithm.dispatch_transaction_status_update(...,
TransactionStatus.UNDETERMINED, ...) when that flag is false. Ensure the new
condition still preserves the child-tx exclusion (is_child_tx) and references
the existing symbols tx_value, is_child_tx, context.transaction, and
ConsensusAlgorithm.dispatch_transaction_status_update.
In `@backend/database_handler/accounts_manager.py`:
- Around line 98-112: credit_account_balance currently calls
create_new_account_with_address which commits the session and breaks atomicity;
instead avoid calling that helper and perform the "create-if-missing" inside the
same transaction (or change create_new_account_with_address to support a
no-commit mode). Replace the call to
create_new_account_with_address(account_address) with an upsert executed on the
same self.session (for example: INSERT INTO current_state (id, balance) VALUES
(:addr, 0) ON CONFLICT DO NOTHING) before running the UPDATE, or accept a
skip_commit flag on create_new_account_with_address and invoke it without
committing; ensure all DB writes (the insert/upsert and the UPDATE in
credit_account_balance) run in the same session/transaction to preserve
atomicity and eliminate the race on first credits.
In `@backend/database_handler/invariants.py`:
- Around line 12-23: The query in get_in_transit_value counts child transactions
that have already been credited; update the WHERE clause to exclude post-credit
statuses so only uncredited child transactions are summed. In
get_in_transit_value modify the text(...) SQL to add status NOT IN for the
post-credit states (e.g.
'PROPOSING','COMMITTING','REVEALING','UNDETERMINED','LEADER_TIMEOUT','VALIDATORS_TIMEOUT')
in addition to the existing ('ACCEPTED','FINALIZED','CANCELED') so the
SUM(value) only includes truly in-transit (uncredited) child transactions.
In `@tests/integration/icontracts/contracts/payable_escrow.py`:
- Line 4: The import-star usage (from genlayer import *) triggers Ruff F403/F405
across many contract files; update ruff.toml to either add F403 and F405 to
global ignore or add a per-file-ignores entry that targets your contracts
directory (e.g., the contracts pattern) and lists ["F403","F405"]; this keeps
linting consistent without adding noqa to each contract file.
In `@tests/integration/icontracts/tests/test_payable_escrow.py`:
- Around line 63-86: The test currently only asserts escrow debits
(get_deposited/get_balance) but not that the child transfer actually credited
the recipient; after calling contract.withdraw (tx2) and waiting for triggered
transactions, assert the recipient-side effect: either check
recipient_account.balance (or equivalent account balance getter) increased by
500, or inspect tx2's triggered transaction record to assert its value field
equals 500 and its to/address equals recipient_account.address; update
test_payable_withdraw_emit_transfer to include one of these assertions after tx2
settles.
---
Outside diff comments:
In `@backend/protocol_rpc/endpoints.py`:
- Around line 1198-1204: The deployment path drops the parsed call_value so
simulated deployments can differ from real execution; when calling
transactions_parser.decode_deployment_data and then node.deploy_contract,
forward the parsed call_value (the variable parsed earlier where call_value is
extracted) into node.deploy_contract using its value/ value parameter so that
node.deploy_contract(from_address=...,
code_to_deploy=decoded_data.contract_code, calldata=decoded_data.calldata,
value=call_value, transaction_created_at=txn_created_at) is used; update the
deploy branch that invokes node.deploy_contract to include this value parameter
so payable constructors and init-time balance checks see the correct call value.
---
Nitpick comments:
In `@tests/unit/consensus/test_decisions_accepted.py`:
- Around line 329-333: The test method
test_no_rollup_event_on_appeal_reacceptance is missing a return type annotation;
update its definition to include the return type "-> None" (i.e., def
test_no_rollup_event_on_appeal_reacceptance(self) -> None:) to satisfy the
project's type-hinting guideline and ensure all Python code includes type hints.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e0013dc-01ee-4123-b82b-01436040ab0c
📒 Files selected for processing (12)
backend/consensus/base.pybackend/consensus/decisions.pybackend/consensus/worker.pybackend/database_handler/accounts_manager.pybackend/database_handler/invariants.pybackend/domain/types.pybackend/node/base.pybackend/protocol_rpc/endpoints.pytests/integration/icontracts/contracts/payable_escrow.pytests/integration/icontracts/tests/test_payable_escrow.pytests/unit/consensus/test_appeal_snapshot.pytests/unit/consensus/test_decisions_accepted.py
Comment on lines
+12
to
+23
| def get_in_transit_value(session: Session) -> int: | ||
| result = session.execute( | ||
| text( | ||
| """ | ||
| SELECT COALESCE(SUM(value), 0) FROM transactions | ||
| WHERE triggered_by_hash IS NOT NULL | ||
| AND value > 0 | ||
| AND status NOT IN ('ACCEPTED', 'FINALIZED', 'CANCELED') | ||
| """ | ||
| ) | ||
| ).scalar() | ||
| return int(result) |
Contributor
There was a problem hiding this comment.
Only count uncredited child transactions as in transit.
Per this PR’s balance flow, child value is credited when the child enters PROPOSING. This query still includes post-credit states like PROPOSING, COMMITTING, REVEALING, UNDETERMINED, LEADER_TIMEOUT, and VALIDATORS_TIMEOUT because Line 19 only excludes ACCEPTED, FINALIZED, and CANCELED, so verify_conservation() will double-count value while those transactions are in flight.
📐 Proposed fix
"""
SELECT COALESCE(SUM(value), 0) FROM transactions
WHERE triggered_by_hash IS NOT NULL
AND value > 0
- AND status NOT IN ('ACCEPTED', 'FINALIZED', 'CANCELED')
+ AND status IN ('PENDING', 'ACTIVATED')
"""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/database_handler/invariants.py` around lines 12 - 23, The query in
get_in_transit_value counts child transactions that have already been credited;
update the WHERE clause to exclude post-credit statuses so only uncredited child
transactions are summed. In get_in_transit_value modify the text(...) SQL to add
status NOT IN for the post-credit states (e.g.
'PROPOSING','COMMITTING','REVEALING','UNDETERMINED','LEADER_TIMEOUT','VALIDATORS_TIMEOUT')
in addition to the existing ('ACCEPTED','FINALIZED','CANCELED') so the
SUM(value) only includes truly in-transit (uncredited) child transactions.
| # v0.2.5 | ||
| # { "Depends": "py-genlayer:1jb45aa8ynh2a9c9xn3b7qqh8sm5q93hwfp7jqmwsfhh8jpz09h6" } | ||
|
|
||
| from genlayer import * |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and examine its content
cat -n tests/integration/icontracts/contracts/payable_escrow.pyRepository: genlayerlabs/genlayer-studio
Length of output: 1460
🏁 Script executed:
# Check if genlayer is a local module or external dependency
fd genlayer --type dRepository: genlayerlabs/genlayer-studio
Length of output: 54
🏁 Script executed:
# Search for other imports from genlayer in the codebase to understand usage patterns
rg "from genlayer import" --type pyRepository: genlayerlabs/genlayer-studio
Length of output: 2515
🏁 Script executed:
# Check for ruff configuration to understand lint settings
fd -e "ruff.toml" -e "pyproject.toml" -e ".ruff.toml" | head -5Repository: genlayerlabs/genlayer-studio
Length of output: 54
🏁 Script executed:
# Look for any F403/F405 suppression patterns in the codebase
rg "F403|F405|noqa" tests/ --type py -A 2 -B 2Repository: genlayerlabs/genlayer-studio
Length of output: 587
🏁 Script executed:
# Check for ruff configuration in root directory
ls -la | grep -E "ruff|pyproject"Repository: genlayerlabs/genlayer-studio
Length of output: 132
🏁 Script executed:
# Check root pyproject.toml for ruff configuration
cat pyproject.toml | grep -A 20 "\[tool.ruff"Repository: genlayerlabs/genlayer-studio
Length of output: 118
🏁 Script executed:
# Check if there's a .ruff.toml or similar
find . -maxdepth 1 -name "*ruff*" -o -name "pyproject.toml"Repository: genlayerlabs/genlayer-studio
Length of output: 83
🏁 Script executed:
# Examine the ruff configuration
cat ruff.tomlRepository: genlayerlabs/genlayer-studio
Length of output: 1149
🏁 Script executed:
# Verify what names are actually used from the star import in this file
grep -oE "gl\.|Address|u256|Message" tests/integration/icontracts/contracts/payable_escrow.py | sort | uniq -cRepository: genlayerlabs/genlayer-studio
Length of output: 112
🏁 Script executed:
# Check if any contract files have F403/F405 suppressions
rg "noqa.*F403|noqa.*F405|F403|F405" tests/integration/icontracts/contracts/ --type pyRepository: genlayerlabs/genlayer-studio
Length of output: 54
🏁 Script executed:
# Check if genlayer is documented or if explicit imports are available
fd -e "README*" -e "CONTRIBUTING*" | xargs grep -l genlayer 2>/dev/null | head -3Repository: genlayerlabs/genlayer-studio
Length of output: 54
Add F403/F405 to ruff configuration instead of per-file suppressions.
This file uses from genlayer import * (line 4), which triggers Ruff F403/F405 violations. However, this pattern is standard across 30+ contract files in the codebase. Rather than adding suppressions to each file, update ruff.toml to ignore these rules globally or add a per-file-ignores entry for the contracts directory:
[lint.per-file-ignores]
"tests/integration/icontracts/contracts/**" = ["F403", "F405"]This approach maintains linting consistency without scattering noqa comments throughout contract definitions.
🧰 Tools
🪛 Ruff (0.15.6)
[error] 4-4: from genlayer import * used; unable to detect undefined names
(F403)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/integration/icontracts/contracts/payable_escrow.py` at line 4, The
import-star usage (from genlayer import *) triggers Ruff F403/F405 across many
contract files; update ruff.toml to either add F403 and F405 to global ignore or
add a per-file-ignores entry that targets your contracts directory (e.g., the
contracts pattern) and lists ["F403","F405"]; this keeps linting consistent
without adding noqa to each contract file.
- Skip sender balance pre-check on appeal/retry executions (was incorrectly rejecting already-funded payable txs after sender spent funds elsewhere) - Make child credit idempotent: only credit on first activation (status PENDING), not on appeal/retry re-entry - Fix credit_account_balance atomicity: use INSERT ON CONFLICT DO NOTHING instead of create_new_account_with_address which commits the session - Fix invariant in-transit query: only count PENDING/ACTIVATED child txs (post-activation states already credited to target) - Add assertion in withdraw test for triggered tx success
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/database_handler/accounts_manager.py (1)
98-116:⚠️ Potential issue | 🟡 MinorAdd address validation to prevent inserting invalid addresses.
The atomicity fix using
INSERT ... ON CONFLICT DO NOTHINGis good. However, unlikecreate_new_account_with_address(which validates viais_address()), this method will insert any string into the database. Also missing-> Nonereturn type hint per coding guidelines.🛡️ Proposed fix
- def credit_account_balance(self, account_address: str, amount: int): + def credit_account_balance(self, account_address: str, amount: int) -> None: """Atomic credit. Creates account if it doesn't exist.""" if amount <= 0: return + if not is_address(account_address): + raise ValueError(f"Invalid address: {account_address}") self.session.execute(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/database_handler/accounts_manager.py` around lines 98 - 116, In credit_account_balance, add the same address validation used by create_new_account_with_address (call is_address(account_address)) and reject invalid addresses (e.g., raise ValueError or return early) before executing the INSERT/UPDATE so you never insert invalid strings; also add the explicit return type hint "-> None" to the credit_account_balance signature to match project guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/database_handler/accounts_manager.py`:
- Around line 85-96: The debit_account_balance(...) call can fail (returns
False) and callers in base.py currently ignore it, enabling inconsistent state;
update each call site that debits (the sender EOA debit, the contract debit when
net_delta < 0, and the finalized receiver debit) to check the boolean result and
abort the surrounding transaction/operation immediately on False (propagate an
error/raise an exception or return failure) instead of continuing to subsequent
credit operations so that credits never proceed when a debit failed; ensure you
reference and check the return value from debit_account_balance in those code
paths and handle failure consistently.
---
Duplicate comments:
In `@backend/database_handler/accounts_manager.py`:
- Around line 98-116: In credit_account_balance, add the same address validation
used by create_new_account_with_address (call is_address(account_address)) and
reject invalid addresses (e.g., raise ValueError or return early) before
executing the INSERT/UPDATE so you never insert invalid strings; also add the
explicit return type hint "-> None" to the credit_account_balance signature to
match project guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c397d3bb-a34c-48c3-9396-db8f5b1d3885
📒 Files selected for processing (4)
backend/consensus/base.pybackend/database_handler/accounts_manager.pybackend/database_handler/invariants.pytests/integration/icontracts/tests/test_payable_escrow.py
✅ Files skipped from review due to trivial changes (1)
- backend/database_handler/invariants.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/integration/icontracts/tests/test_payable_escrow.py
- backend/consensus/base.py
Comment on lines
+85
to
+96
| def debit_account_balance(self, account_address: str, amount: int) -> bool: | ||
| """Atomic conditional debit. Returns False if insufficient balance.""" | ||
| if amount <= 0: | ||
| return True | ||
| result = self.session.execute( | ||
| text( | ||
| "UPDATE current_state SET balance = balance - :amount " | ||
| "WHERE id = :addr AND balance >= :amount" | ||
| ), | ||
| {"amount": amount, "addr": account_address}, | ||
| ) | ||
| return result.rowcount > 0 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify that all callers of debit_account_balance actually check the return value
rg -n -B2 -A2 'debit_account_balance\(' --type py | head -60Repository: genlayerlabs/genlayer-studio
Length of output: 1633
🏁 Script executed:
# Search for all calls to debit_account_balance to ensure completeness
rg 'debit_account_balance' --type py | grep -v '^\s*#' | grep -v 'def debit_account_balance'Repository: genlayerlabs/genlayer-studio
Length of output: 345
🏁 Script executed:
# Verify the control flow around one of the debit calls to understand if credits follow
sed -n '2230,2240p' backend/consensus/base.pyRepository: genlayerlabs/genlayer-studio
Length of output: 532
🏁 Script executed:
# Check context around line 2245
sed -n '2243,2250p' backend/consensus/base.pyRepository: genlayerlabs/genlayer-studio
Length of output: 369
🏁 Script executed:
# Check context around line 2425
sed -n '2423,2430p' backend/consensus/base.pyRepository: genlayerlabs/genlayer-studio
Length of output: 330
Check the return value of debit_account_balance() at all three call sites; unchecked failures enable value creation.
The method explicitly returns False when balance is insufficient, but all callers in base.py (lines 2232, 2245, 2425) discard this return value. If a debit fails, execution continues without checking, allowing subsequent credit operations to proceed with corrupted balance state and violating the value conservation invariant.
Each call must check the return value and halt the transaction if the debit fails:
- Line 2232: After debiting sender EOA
- Line 2245: After debiting contract on negative net_delta
- Line 2425: After debiting receiver on finalized debit
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/database_handler/accounts_manager.py` around lines 85 - 96, The
debit_account_balance(...) call can fail (returns False) and callers in base.py
currently ignore it, enabling inconsistent state; update each call site that
debits (the sender EOA debit, the contract debit when net_delta < 0, and the
finalized receiver debit) to check the boolean result and abort the surrounding
transaction/operation immediately on False (propagate an error/raise an
exception or return failure) instead of continuing to subsequent credit
operations so that credits never proceed when a debit failed; ensure you
reference and check the return value from debit_account_balance in those code
paths and handle failure consistently.
1. Child txs credited twice: AcceptedState incoming_credit now skipped for child txs (is_child_tx guard) — children already credited on activation 2. Wrong "first acceptance" flag: check all appeal variants (appealed, appeal_undetermined, appeal_leader_timeout, appeal_validators_timeout) 3. Failed debits checked: debit_account_balance return value logged on failure 4. Child credit idempotency: consensus_history emptiness as additional guard 5. Deploy double balance: skip _balance_override for deploy txs (is_init) 6. Withdraw test targets contract instead of EOA
1. Snapshot missing balance: get_balance uses getattr fallback to 0 2. Balance override double-count on replay: is_first_execution flag 3. Children emitted before debit: reorder debit before emission Known limitations (documented): 4. Child activation credit not fully idempotent across worker resets 5. Cross-contract sender race (pre-existing)
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/consensus/base.py (1)
2199-2265:⚠️ Potential issue | 🔴 CriticalAbort child emission when value settlement fails.
At Lines 2201-2216 and 2424-2433, child transactions are already emitted before the backing debits happen.
debit_account_balance()can returnFalse(backend/database_handler/accounts_manager.py:1-20), and this code either zeroes/logs and continues or ignores the return value entirely. That can leave accepted/finalized children on the ledger even though neither the sender nor the contract was actually debited. For top-level calls, execution has already observedgl.message.value, so degrading toincoming_credit = 0still commits state from an unpaid call.Also applies to: 2418-2444
♻️ Duplicate comments (1)
backend/consensus/base.py (1)
1470-1478:⚠️ Potential issue | 🔴 CriticalThis child credit is still retry-unsafe before any durable state is written.
There is no persisted check-and-set here; the only guard is
not is_retry and not context.transaction.consensus_history. If anything later inPendingStateaborts before consensus history is recorded—e.g. theNoValidatorsAvailableErrorpath at Lines 1488-1500—the next retry re-enters this block and credits the callee again.Also applies to: 1488-1500
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/base.py` around lines 1470 - 1478, The child-credit is retry-unsafe because it uses only in-memory guards (is_first_activation using is_retry and context.transaction.consensus_history) before durable state is written; modify the flow so the credit is only applied after an atomic, persisted check-and-set (e.g. update a durable flag on context.transaction such as consensus_history or a committed nonce) or perform the credit via an accounts_manager method that atomically persists the credit and the activation marker; specifically, update the logic around is_first_activation and context.accounts_manager.credit_account_balance to either (A) persistently mark the transaction as activated before returning from PendingState (so subsequent retries see the persisted marker), or (B) add/use an atomic accounts_manager method (e.g. credit_and_mark_activation) that writes the balance change and the activation marker in a single durable operation, and remove the current non-durable guard checks around is_first_activation/consensus_history/NoValidatorsAvailableError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/consensus/base.py`:
- Around line 2456-2462: The code uses get_transaction_count() to compute
base_nonce (variables base_nonce and nonce_offset in the nonce assignment loop)
which is unsafe under concurrent workers; instead implement an atomic nonce
reservation in the transactions processor (update or add a method like
reserve_nonce_block(contract_address, block_size) in the transactions_processor)
and call that from the place that currently computes base_nonce so each worker
gets a unique reserved block, or alternatively enforce a UNIQUE constraint on
(contract_address, nonce) in the DB and update the nonce allocation loop to
catch insertion conflicts and retry (use transactions and retry/backoff) when
duplicate-nonce errors occur; update calls to get_transaction_count to use the
new reserve_nonce_block API or the retry logic and ensure all references to
base_nonce/nonce_offset use the reserved range.
- Around line 445-450: The current check uses saved.states.get("accepted") which
treats an empty accepted snapshot ({}) as false and causes it to be ignored;
change the condition that sets has_real_state so it detects the presence of the
"accepted" key instead of truthiness (e.g., use '"accepted" in saved.states' or
saved.states.get("accepted") is not None) when evaluating
transaction.contract_snapshot/saved in the block that assigns
self.contract_snapshot; update the condition around saved, hasattr(saved,
"states"), has_real_state, and the assignment to self.contract_snapshot
accordingly.
---
Duplicate comments:
In `@backend/consensus/base.py`:
- Around line 1470-1478: The child-credit is retry-unsafe because it uses only
in-memory guards (is_first_activation using is_retry and
context.transaction.consensus_history) before durable state is written; modify
the flow so the credit is only applied after an atomic, persisted check-and-set
(e.g. update a durable flag on context.transaction such as consensus_history or
a committed nonce) or perform the credit via an accounts_manager method that
atomically persists the credit and the activation marker; specifically, update
the logic around is_first_activation and
context.accounts_manager.credit_account_balance to either (A) persistently mark
the transaction as activated before returning from PendingState (so subsequent
retries see the persisted marker), or (B) add/use an atomic accounts_manager
method (e.g. credit_and_mark_activation) that writes the balance change and the
activation marker in a single durable operation, and remove the current
non-durable guard checks around
is_first_activation/consensus_history/NoValidatorsAvailableError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 13a6213f-dfdb-49a0-b5ab-8477f36e2e54
📒 Files selected for processing (3)
backend/consensus/base.pybackend/node/base.pytests/integration/icontracts/tests/test_payable_escrow.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/integration/icontracts/tests/test_payable_escrow.py
Comment on lines
+445
to
+450
| saved = self.transaction.contract_snapshot | ||
| has_real_state = ( | ||
| saved and hasattr(saved, "states") and saved.states.get("accepted") | ||
| ) | ||
| if has_real_state: | ||
| self.contract_snapshot = saved |
Contributor
There was a problem hiding this comment.
Preserve empty accepted snapshots on retries.
saved.states.get("accepted") treats {} as missing. For deploys and other empty-state contracts, appeals/retries will rebuild from the current live snapshot instead of the original pre-tx snapshot, so payable re-execution can see the wrong state/balance and add value on top of an already-credited balance.
Possible local fix
- has_real_state = (
- saved and hasattr(saved, "states") and saved.states.get("accepted")
- )
+ has_real_state = (
+ saved is not None
+ and hasattr(saved, "states")
+ and isinstance(saved.states, dict)
+ and saved.states.get("accepted") is not None
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/consensus/base.py` around lines 445 - 450, The current check uses
saved.states.get("accepted") which treats an empty accepted snapshot ({}) as
false and causes it to be ignored; change the condition that sets has_real_state
so it detects the presence of the "accepted" key instead of truthiness (e.g.,
use '"accepted" in saved.states' or saved.states.get("accepted") is not None)
when evaluating transaction.contract_snapshot/saved in the block that assigns
self.contract_snapshot; update the condition around saved, hasattr(saved,
"states"), has_real_state, and the assignment to self.contract_snapshot
accordingly.
Comment on lines
+2456
to
+2462
| base_nonce = context.transactions_processor.get_transaction_count( | ||
| context.transaction.to_address | ||
| ) | ||
| nonce_offset = 0 | ||
| for pending_transaction in filter(lambda t: t.on == on, pending_transactions): | ||
| nonce = context.transactions_processor.get_transaction_count( | ||
| context.transaction.to_address | ||
| ) | ||
| nonce = base_nonce + nonce_offset | ||
| nonce_offset += 1 |
Contributor
There was a problem hiding this comment.
COUNT(*) is not a safe nonce allocator under concurrency.
This is sequential within one parent batch, but get_transaction_count() is still just an unlocked count query in backend/database_handler/transactions_processor.py:1-15. Two workers emitting children from the same contract can read the same base_nonce and insert duplicate nonces. Reserve nonce blocks atomically in the database, or enforce uniqueness and retry on collision.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/consensus/base.py` around lines 2456 - 2462, The code uses
get_transaction_count() to compute base_nonce (variables base_nonce and
nonce_offset in the nonce assignment loop) which is unsafe under concurrent
workers; instead implement an atomic nonce reservation in the transactions
processor (update or add a method like reserve_nonce_block(contract_address,
block_size) in the transactions_processor) and call that from the place that
currently computes base_nonce so each worker gets a unique reserved block, or
alternatively enforce a UNIQUE constraint on (contract_address, nonce) in the DB
and update the nonce allocation loop to catch insertion conflicts and retry (use
transactions and retry/backoff) when duplicate-nonce errors occur; update calls
to get_transaction_count to use the new reserve_nonce_block API or the retry
logic and ensure all references to base_nonce/nonce_offset use the reserved
range.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/consensus/base.py (1)
445-450: Empty accepted snapshots treated as missing.The condition
saved.states.get("accepted")evaluates an empty dict{}as falsy, causing fresh snapshot creation even when a valid empty-state snapshot exists. For contracts with legitimately empty state (e.g., newly deployed contracts), this could cause unnecessary snapshot recreation on retries.While the
is_first_executionflag innode/base.pyhelps prevent balance double-counting, the snapshot check should be more precise:Suggested fix
has_real_state = ( - saved and hasattr(saved, "states") and saved.states.get("accepted") + saved is not None + and hasattr(saved, "states") + and isinstance(saved.states, dict) + and "accepted" in saved.states )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/consensus/base.py` around lines 445 - 450, The check treating saved.states.get("accepted") as truthy incorrectly treats an existing empty accepted snapshot ({}) as missing; update the condition in the block using self.transaction.contract_snapshot / self.contract_snapshot so it verifies the presence of the "accepted" key rather than truthiness of its value (e.g., ensure saved is truthy, saved has a states attribute and that "accepted" is present in saved.states) so empty dict snapshots are accepted as valid; optionally also guard that saved.states is a mapping (isinstance(saved.states, dict)) before checking membership.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/consensus/base.py`:
- Around line 2428-2437: The code currently logs an error when
context.accounts_manager.debit_account_balance(context.transaction.to_address,
total_finalized_debit) fails but still proceeds to emit children, risking value
creation; modify the control flow in the block where total_finalized_debit is
checked (around total_finalized_debit,
context.accounts_manager.debit_account_balance, and subsequent child emission)
so that if debit_account_balance returns False you abort/skip emitting any
children (or return a failure from the enclosing function) for this
transaction/contract finalization; ensure any callers of this function handle
the failure consistently so no child credits are applied when the parent debit
did not succeed.
- Around line 2239-2250: The debit failure branch currently sets total_msg_debit
= 0 but does not prevent subsequent child emission; locate the call to
context.accounts_manager.debit_account_balance(contract_addr, abs(net_delta))
and, when it returns False, skip the message emission by gating the downstream
calls to _get_messages_data and _emit_messages (or early-return) using a clear
condition (e.g., if not debited: skip/return) so that children are not emitted
for insufficient contract balance; make sure you reference contract_addr and
net_delta in the log and preserve existing behavior when debit succeeds.
---
Nitpick comments:
In `@backend/consensus/base.py`:
- Around line 445-450: The check treating saved.states.get("accepted") as truthy
incorrectly treats an existing empty accepted snapshot ({}) as missing; update
the condition in the block using self.transaction.contract_snapshot /
self.contract_snapshot so it verifies the presence of the "accepted" key rather
than truthiness of its value (e.g., ensure saved is truthy, saved has a states
attribute and that "accepted" is present in saved.states) so empty dict
snapshots are accepted as valid; optionally also guard that saved.states is a
mapping (isinstance(saved.states, dict)) before checking membership.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 50876dc1-fe9c-4e28-a63c-3d0804129166
📒 Files selected for processing (2)
backend/consensus/base.pybackend/node/base.py
backend/consensus/base.py
Outdated
Comment on lines
+2239
to
+2250
| if not context.accounts_manager.debit_account_balance( | ||
| contract_addr, abs(net_delta) | ||
| ): | ||
| from loguru import logger | ||
|
|
||
| logger.error( | ||
| f"Contract balance debit failed for {contract_addr}, " | ||
| f"amount={abs(net_delta)}, tx={context.transaction.hash}. " | ||
| f"Skipping child message emission." | ||
| ) | ||
| # Skip child emission if contract can't cover the value | ||
| total_msg_debit = 0 # reset to skip emission below |
Contributor
There was a problem hiding this comment.
Contract debit failure doesn't actually skip child emission.
The comment states "Skip child message emission" when the contract balance debit fails, but setting total_msg_debit = 0 has no effect since this variable is unused afterward. The subsequent _get_messages_data and _emit_messages calls (lines 2253-2267) still execute unconditionally, emitting children even when the contract lacks sufficient balance to cover their values.
Suggested fix to actually skip child emission
logger.error(
f"Contract balance debit failed for {contract_addr}, "
f"amount={abs(net_delta)}, tx={context.transaction.hash}. "
f"Skipping child message emission."
)
# Skip child emission if contract can't cover the value
- total_msg_debit = 0 # reset to skip emission below
+ # Clear pending transactions to prevent emission
+ leader_receipt.pending_transactions = []
- # Step 2: Emit child messages only after successful balance accounting
- internal_messages_data, insert_transactions_data = _get_messages_data(
+ # Step 2: Emit child messages only after successful balance accounting
+ if execution_success:
+ internal_messages_data, insert_transactions_data = _get_messages_data(Or wrap the emission block in a condition:
+ should_emit_children = net_delta >= 0 or context.accounts_manager.debit_account_balance(
+ contract_addr, abs(net_delta)
+ )
+ if not should_emit_children:
+ from loguru import logger
+ logger.error(...)
+
+ if should_emit_children:
internal_messages_data, insert_transactions_data = _get_messages_data(
...
)
...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/consensus/base.py` around lines 2239 - 2250, The debit failure branch
currently sets total_msg_debit = 0 but does not prevent subsequent child
emission; locate the call to
context.accounts_manager.debit_account_balance(contract_addr, abs(net_delta))
and, when it returns False, skip the message emission by gating the downstream
calls to _get_messages_data and _emit_messages (or early-return) using a clear
condition (e.g., if not debited: skip/return) so that children are not emitted
for insufficient contract balance; make sure you reference contract_addr and
net_delta in the log and preserve existing behavior when debit succeeds.
Comment on lines
+2428
to
+2437
| if total_finalized_debit > 0: | ||
| if not context.accounts_manager.debit_account_balance( | ||
| context.transaction.to_address, total_finalized_debit | ||
| ): | ||
| from loguru import logger | ||
|
|
||
| logger.error( | ||
| f"Contract finalization debit failed for {context.transaction.to_address}, " | ||
| f"amount={total_finalized_debit}, tx={context.transaction.hash}" | ||
| ) |
Contributor
There was a problem hiding this comment.
Finalization debit failure doesn't prevent child emission.
Similar to AcceptedState, when the contract balance debit fails, children with non-zero values are still emitted. This could violate the conservation invariant: child transactions would receive credited value that wasn't successfully debited from the parent contract.
Consider skipping child emission when the debit fails, or ensure the conservation invariant module catches this scenario.
Suggested fix
if total_finalized_debit > 0:
if not context.accounts_manager.debit_account_balance(
context.transaction.to_address, total_finalized_debit
):
from loguru import logger
logger.error(
f"Contract finalization debit failed for {context.transaction.to_address}, "
f"amount={total_finalized_debit}, tx={context.transaction.hash}"
)
+ # Skip child emission to preserve conservation invariant
+ leader_receipt.pending_transactions = [
+ pt for pt in leader_receipt.pending_transactions
+ if pt.on != "finalized"
+ ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/consensus/base.py` around lines 2428 - 2437, The code currently logs
an error when
context.accounts_manager.debit_account_balance(context.transaction.to_address,
total_finalized_debit) fails but still proceeds to emit children, risking value
creation; modify the control flow in the block where total_finalized_debit is
checked (around total_finalized_debit,
context.accounts_manager.debit_account_balance, and subsequent child emission)
so that if debit_account_balance returns False you abort/skip emitting any
children (or return a failure from the enclosing function) for this
transaction/contract finalization; ensure any callers of this function handle
the failure consistently so no child credits are applied when the parent debit
did not succeed.
…tion Major simplification of balance accounting model: - Sender debited at transaction submission time (like EVM), not acceptance - Value credited to target on activation via idempotent credit_tx_value_once (uses value_credited boolean on transactions table for true idempotency) - AcceptedState only handles message emission debits (no incoming credit) - Removed _balance_override, is_first_execution — no longer needed - DB balance is always the source of truth for self.balance New DB migration: adds value_credited boolean column to transactions. New method: credit_tx_value_once — atomic set-flag-and-credit operation. This eliminates: double-count on replay, snapshot missing balance crash, is_first_execution complexity, consensus_history guard fragility.
1. SEND double-debit: skip submission debit for SEND type 2. Retry double-debit: check for existing tx before debiting 3. Failed debit skips value-bearing children 4. Snapshot restore preserves value_credited 5. Stale self.balance: hydrate from DB on saved snapshot reuse
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Wire transaction value through GenVM execution so intelligent contracts can receive native tokens via
@gl.public.write.payabledecorator. This addresses a Discord-reported issue where payable functions show as finalized but validators disagree and contract state doesn't change.Root cause
node/base.py:986hardcoded"value": 0in the GenVM message dict, sogl.message.valuewas always 0 regardless of what value the user sent.Design principles (reviewed with Codex o3)
Changes
transaction.valuethroughexec_transaction → run_contract → _run_genvm → message dict_SnapshotView.get_balanceuses override for executing contract (current_balance + incoming_value)pending_transaction.value(was hardcoded 0)appealedguard on child emissiondebit_account_balance/credit_account_balancewith conditional SQLtriggered_by_hashto Transaction domain type + worker claim queriesverify_conservation()checks total balances + in-transit = total mintedNew files
backend/database_handler/invariants.py— conservation law verificationtests/integration/icontracts/contracts/payable_escrow.py— test contracttests/integration/icontracts/tests/test_payable_escrow.py— integration testsTest plan
payable_escrow.py, calldeposit(value=500), verifyget_deposited() == 500andget_balance() == 500withdraw()withemit_transferdrains balance via child txSummary by CodeRabbit
New Features
Bug Fixes
Tests