Skip to content

fix: use checked arithmetic for cumulative gas accounting in payload builder#24

Closed
Himess wants to merge 23 commits intocirclefin:mainfrom
Himess:fix/checked-gas-accumulation
Closed

fix: use checked arithmetic for cumulative gas accounting in payload builder#24
Himess wants to merge 23 commits intocirclefin:mainfrom
Himess:fix/checked-gas-accumulation

Conversation

@Himess
Copy link
Copy Markdown

@Himess Himess commented Apr 11, 2026

Summary

Use checked arithmetic for cumulative gas accounting in the payload builder to prevent potential overflow on u64 addition.

Two instances in arc_ethereum_payload:

  • Line 579: cumulative_gas_used + pool_tx.gas_limit() — replaced with checked_add + is_none_or to reject the transaction if the addition overflows
  • Line 660: cumulative_gas_used += gas_used — replaced with saturating_add to prevent wrapping

This follows the same defensive arithmetic pattern applied to reward_beneficiary in #21, where u128 multiplication was replaced with U256 arithmetic to prevent overflow.

circle-terraform-github Bot and others added 22 commits February 12, 2026 14:32
…ments (circlefin#2)

## Summary

- Add `arcup/` installer scripts for downloading and managing arc-node
binaries from GitHub releases
- Add `docs/installation.md` and `docs/running-an-arc-node.md` public
guides
- Refactor consensus streaming (`malachite-app`) with LRU cache for
improved memory management
- Add Zero5 hardfork and Osaka timestamp activation
- Replace internal snapshot URL with public `snapshots.arc.network`
endpoint
- Add rate limiter to transaction spammer with governor-based throttling
- Refresh README with simplified feature list and links to public docs
- Add Apache-2.0 license headers to `mesh-analysis` crate
- Remove internal-only `CLAUDE.md` and `atlantis.yaml`
- Bump dependencies (Cargo.lock, package-lock.json)
…circlefin#5)

## Summary
  
- Add `rustls-tls-native-roots` to snapshot download reqwest features
(fixes TLS on platforms without bundled certs)
- Document spammer backpressure and fire-and-forget operating modes in
spammer + quake READMEs
- Refresh `docs/ARCHITECTURE.md` with current crate paths, precompile
addresses, remove stale denylist references
  - Add ADR 0002-0004 entries to `docs/adr/README.md`
  - Add testnet/alpha software disclaimer to root `README.md`
  - Add persistence latency tracking module to eth-engine
  - Extend consensus ready handler with validator set management
  - Fix follow endpoint URL handling
  - Add new CLI config options for consensus tuning
  - Update quake localdev scenarios and compose templates
  - Update node docs and dependency versions

---------

Co-authored-by: atiwari-circle <ayush.tiwari@circle.com>
Update REGISTRY_NAMESPACE from circle/arc to circle/arc-network in
build-docker.yaml

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…dValue error reply, quake storage config (circlefin#7)

## Summary

- **arcup**: redirect info/warn/error to stderr, fail on HTTP errors
with `curl -f`, surface `update_arcup` curl errors, respect `ARC_DIR`
  in env file
- **evm**: remove `can_load_account` indirection, propagate
`load_account` errors directly in blocklist check
- **consensus**: send reply before propagating error in
`ProcessSyncedValue` (CCHAIN-1498), add error path tests
  - **quake**: add `legacy_state_root` and `storage.v2` manifest fields
- **docs**: improve installation cargo install instructions (drop
hardcoded `--root /usr/local`)

  ## Test plan

- [ ] `arcup install` and `arcup` self-update fail gracefully on bad
URLs
- [ ] EVM blocklist check propagates load errors instead of silently
skipping
- [ ] ProcessSyncedValue sends reply on validation failure before
returning error
- [ ] Quake manifest generation includes `legacy_state_root` and
`storage.v2` fields

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ndling, multi-subnet ENI support, spammer ERC-20/mix mode (circlefin#8)

## Summary

- Migrate CL config to CLI flags, support remote multi-subnet testnets,
improve mesh analysis
  reporting
- Add snapshot skip-existing logic with .snapshot-url version marker and
--force flag
- Improve arcup installer: surface curl errors, fail on HTTP errors,
respect ARC_DIR in env file
- Add secondary ENI configuration for multi-subnet bridge nodes
(cloud-init systemd service +
  IMDS-based policy routing)
  - Bundle arc-snapshots binary into EL and CL Docker images
- Spammer: add ERC-20 function mix (transfer/approve/transferFrom),
--mix flag for transaction type
  blending, backpressure send mode
  - Add quake sync speed test and sync_speed check module
  - Fix localdev genesis addresses, update package-lock.json
  - Doc updates: Docker coming soon note, installation additions

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- update node sync docs
- persistent metering for sync protocol
- mention pre-built binaries are coming soon
- Add centered Arc logo with light/dark mode support
- Add Apache 2.0 COPYRIGHT file
- Add Acknowledgements section to README
## Summary

- update Public CI to run on `main` instead of `master`
- update the Docker workflow PR trigger to target `main`
- update the `Makefile` fallback branch for `buf-breaking` to `main`

## Verification

- confirmed the repository default branch is `main`
- checked the touched workflow/tooling files for remaining `master`
references
- ran `make -n buf-breaking` and verified it now targets
`.git#branch=main`
- ran `git diff --check`
…irclefin#16)

Fixes circlefin#15

## Problem

When running the node with `docker-compose -f arc_execution.yaml -f
arc_consensus.yaml up -d`, the execution engine crashes on startup:

\`\`\`
Error: failed to listen on ipc endpoint '/sockets/reth.ipc': No such
file or directory
\`\`\`

The command section already references `--ipcpath=/sockets/reth.ipc` and
`--auth-ipc.path=/sockets/auth.ipc`, but there's no corresponding volume
mount for `/sockets` in the compose file so the directory doesn't exist
inside the container.

## Fix

Added `./sockets:/sockets` to the volumes section of the `arc_execution`
service.

## Testing

After adding the mount and creating the local `sockets/` directory, both
`arc_execution` and `arc_consensus` containers boot and sync correctly.
## Summary

Syncs changes

### Changes included:
- **crates/quake/src/infra/remote.rs**: Fix shell quoting in pssh
command (double → single quotes)
- **crates/quake/terraform/cc-data.yaml**: Fix shell variable escaping
in pssh.sh cloud-init script
- **scripts/scenarios/nightly-chaos-testing.sh**: Remove hardcoded
`--seed` from quake commands, add chaos loop replay seed documentation
- **crates/malachite-app/src/handlers/restream_proposal.rs**: Refactor
block lookup to use `get_first(height, hash)` instead of round-based
lookup
- **docs/installation.md**: Add OS-specific build dependency
instructions, remove outdated submodule reference
- **deployments/blockscout.yaml**: Add Blockscout block explorer
docker-compose configuration
- **deployments/monitoring/config-blockscout/backend.env**: Add
Blockscout backend environment configuration
## Summary
- Add frontend environment configuration for Blockscout block explorer
deployment

## Test plan

This fix `make testnet` command in readme
https://github.com/circlefin/arc-node/blob/main/README.md#local-testnet
## Summary
- Sync changes
…builder

Use checked_add and saturating_add for cumulative gas tracking to
prevent potential u64 overflow, consistent with the defensive arithmetic
pattern applied to reward_beneficiary in circlefin#21.
@Himess Himess force-pushed the fix/checked-gas-accumulation branch from 3d93e31 to eae0f2a Compare April 11, 2026 13:14
@atiwari-circle
Copy link
Copy Markdown
Contributor

Overflow here is not practically reachable -- cumulative_gas_used and pool_tx.gas_limit() are both bounded by block_gas_limit (~30M), nowhere near u64::MAX (~18.4 exagas). The checked_add at line 579 is fine if you want to keep it as defensive hardening, but saturating_add at line 660 is the wrong choice -- it would silently clamp at u64::MAX instead of failing the block build. Use checked_add with error propagation there, or leave it as plain addition.

…review

Per reviewer feedback, saturating_add silently clamps at u64::MAX which
would corrupt cumulative_gas_used rather than fail the block build cleanly.
Switch to checked_add with PayloadBuilderError propagation, matching the
defensive pattern used at the capacity check on line 579.
@Himess
Copy link
Copy Markdown
Author

Himess commented Apr 13, 2026

Thanks for the review @atiwari-circle. Agreed, replaced saturating_add with checked_add + PayloadBuilderError propagation in 4755edd, so an overflow (still unreachable in practice) would now fail the build cleanly instead of silently clamping. Kept checked_add at line 579 as defensive hardening.

@ZhiyuCircle
Copy link
Copy Markdown
Contributor

Hi @Himess ,

Apologies for the unexpected close. This PR was auto-closed as a side-effect of an update to the main branch in circlefin/arc-node. GitHub closes PRs when their base commit no longer exists on the target branch, which is why your contribution ended up here.

Your change is still very much welcome. To restore it, please rebase your branch onto the new main and open a fresh PR:

git fetch origin
git rebase origin/main
# resolve any conflicts if they come up
git push --force-with-lease

Then open a new PR against main — feel free to link back to this one for context, and we'll prioritize review. Thanks for contributing to Arc, and sorry for the inconvenience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants