fix(transactions): widen amount + fee cols to numeric(38,0) so post-fork OS values cannot overflow#863
Conversation
…38,0) so post-fork OS values cannot overflow at INSERT
Concretely: a 10 % transfer out of a 10^18 DEM genesis-funded account
is 10^17 DEM × 10^9 OS/DEM = 10^26 OS — three orders of magnitude past
PG `bigint` (int8, max ≈ 9.22 × 10^18).
Pre-this-fix `validateTransaction` accepted the tx
(`getAccountBalance` already reads `gcr_main.balance` as
`numeric(38, 0)`) but `insertBlock` crashed the consensus loop:
QueryFailedError: value "100000000000000000000000000" is out of
range for type bigint
The four money-shaped columns on `transactions` (`amount`,
`networkFee`, `rpcFee`, `additionalFee`) are widened to
`numeric(38, 0)` to match `gcr_main.balance`. They share its
`bigintNumericTransformer` so the application-level type stays
`bigint` end-to-end — readers / writers don't change shape.
Why not just sub-DEM amounts (which fit)? OS-magnitude wire values
already land in `amount` post-fork (see DemosTransactions.pay where
`wireAmount = amountOs.toString()` post-fork), and any genesis-funded
account big enough to be useful (founder + incentives wallets) will
trip this on first transfer.
Migration: 1779834000000-WidenTransactionsMoneyColsToNumeric. Idempotent
ALTER COLUMN … TYPE numeric(38, 0) USING (implicit cast); defaults
restored explicitly after the TYPE change (Postgres drops them).
`down()` reverses to `bigint` — narrowing is lossy on any row with a
post-fork-magnitude value, so the migration log clarifies operators
must wipe before reverting.
Manual repro on a fresh devnet booted with the funded-genesis fixture:
- pre-fix: confirm() returns valid=true, broadcast lands tx in
reference_block=2, then node-1 crashes at insertBlock with
"value '1e26' is out of range for type bigint", chain stops at
block 2, no receiver-side balance update visible.
- post-fix: chain advances to block 9+, receiver balance moves from
10^27 OS (genesis) to 1.1×10^27 OS (genesis + 10^26 OS transfer).
GCREdit hash compare still passes (PR #861 is the predecessor here).
Why 9 decimals and not 18 (Ethereum-style): Demos uses 1 DEM = 10^9 OS.
The overflow comes from supply magnitude (founder accounts pre-funded
at 10^18 DEM), not decimal count.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
WalkthroughThe PR widens four transaction money columns ( ChangesTransaction Money Column Type Widening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryWidens the four money-shaped columns on
Confidence Score: 3/5The INSERT crash is resolved, but four unguarded DB read functions in chainTransactions.ts will throw on the first post-fork OS-magnitude transaction lookup, leaving peer-sync and RPC tx queries broken for the very rows this migration enables. The migration and entity changes are correct and the write path is fully fixed. However, src/libs/blockchain/transaction.ts ( Important Files Changed
Sequence DiagramsequenceDiagram
participant Client
participant Node as Node (consensus)
participant ORM as TypeORM Entity
participant PG as Postgres
Client->>Node: "broadcast tx (amount = 1e26 OS)"
Node->>Node: validateTransaction (reads gcr_main.balance as numeric ✓)
Node->>ORM: toTransactionsEntity(rawTx) → bigint via toEntityBigint
ORM->>ORM: bigintNumericTransformer.to(1e26n) → "100000000000000000000000000"
ORM->>PG: INSERT INTO transactions (amount) VALUES ('100000000000000000000000000')
Note over PG: numeric(38,0) accepts value ✓ (pre-fix: bigint overflow ✗)
Note over Node,ORM: Read path — UNGUARDED after this fix
Node->>ORM: getTxByHash / getBlockTransactions
ORM->>PG: SELECT amount FROM transactions
PG->>ORM: "100000000000000000000000000" (string)
ORM->>ORM: bigintNumericTransformer.from → 1e26n
ORM->>Node: fromRawTransaction(entity)
Node->>Node: fromEntityToWireNumber(1e26n)
Note over Node: throws Error: exceeds MAX_SAFE_INTEGER ✗
Reviews (1): Last reviewed commit: "fix(transactions): widen amount + fee co..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await queryRunner.query( | ||
| `ALTER TABLE "transactions" ALTER COLUMN "networkFee" SET DEFAULT 0`, | ||
| ) | ||
| await queryRunner.query( | ||
| `ALTER TABLE "transactions" ALTER COLUMN "rpcFee" DROP DEFAULT`, |
There was a problem hiding this comment.
amount column type change lacks USING clause consistency with fee columns
The up() migration changes amount with a bare ALTER COLUMN … TYPE numeric(38, 0) and relies on Postgres's implicit bigint→numeric cast. This is safe in practice (every bigint is representable as numeric(38,0)) but is inconsistent with the fee columns, which explicitly DROP DEFAULT before the TYPE change to avoid a Postgres implicit-rewrite edge case. Since amount has no default (so no drop-restore sequence is needed), this difference is only a minor style inconsistency — but worth noting for symmetry with the rest of the migration.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
…ansactions + batch aggregator (#873) Companion to #863 (top-level `transactions` widening) and #872 (`fromEntityToWireNumber` post-fork string fallback). Audit of every remaining `Number.MAX_SAFE_INTEGER` and `Number(bigint)` site that touches a money field found two more places that silently corrupt or crash on post-fork OS values: 1) `L2PSTransactions.amount` was `@Column("bigint")`, same `int8` cap as the top-level `transactions.amount` widened in #863. L2PS replays per-tx amounts inside aggregated L1 batches, so a 10 % move out of a 10^18 DEM wallet (10^26 OS) overflows the same way the top-level table did before #863. Widened to `numeric(38, 0)` with the shared `bigintNumericTransformer`. New migration `1779834500000-WidenL2PSTransactionsAmountToNumeric` matches the shape of `1779834000000-WidenTransactionsMoneyColsToNumeric`. 2) `L2PSBatchAggregator.zkTransactions` was doing `BigInt(Math.floor(Number(rawAmount)))` for every tx amount. The wire shape is `string | number | bigint`; routing the post-fork decimal-string path through `Number()` first silently truncates to the nearest double-precision value (10^26 OS becomes 10^26 minus ~70 low bits of junk) before `BigInt()` is even reached. Rewrote the coercion to handle each input type directly: `bigint` and `number` pass through, decimal-integer strings go straight to `BigInt()`, fractional strings are truncated at the decimal point. No lossy `Number()` round-trip on the wide path. Both are pre-existing post-fork overflow bombs that would have fired the first time an L2PS batch carried a real founder-wallet transfer. Audit of remaining `MAX_SAFE_INTEGER` sites confirms the others are either dead code (`subOperations.transferNative` — no live caller), seed values for `reduce()` (`l2ps_hashes.ts`), block-number caps (`omniprotocol/sync.ts`), or legacy-GCR migration internals that no longer ship as a live runtime path. Co-authored-by: tcsenpai <tcsenpai@discus.sh>
1 participant
Summary
A 10% transfer out of a 10^18 DEM genesis-funded account is
10^17 DEM × 10^9 OS/DEM = 10^26 OS— three orders of magnitude past PGbigint(int8, max ≈ 9.22 × 10^18).Pre-fix,
validateTransactionaccepted the tx (getAccountBalancereadsgcr_main.balanceasnumeric(38, 0)) butinsertBlockcrashed the consensus loop:Fix
The four money-shaped columns on
transactions(amount,networkFee,rpcFee,additionalFee) are widened tonumeric(38, 0)to matchgcr_main.balance. They share itsbigintNumericTransformerso the application-level type staysbigintend-to-end — readers and writers don't change shape.Why this matters
OS-magnitude wire values already land in
amountpost-fork (seeDemosTransactions.paywherewireAmount = amountOs.toString()), and any genesis-funded account big enough to be useful (founder + incentives wallets) trips this on first transfer. Without the fix, validators crash on the first non-trivial transfer.Why 9 decimals (not 18)
Demos: 1 DEM = 10^9 OS. The overflow comes from supply magnitude (founder accounts pre-funded at 10^18 DEM), not decimal count. Same structural issue Ethereum-style chains hit with
bigint— Geth uses byte arrays for the same reason.Migration
1779834000000-WidenTransactionsMoneyColsToNumeric:ALTER COLUMN … TYPE numeric(38, 0)— implicit cast frombigintis lossless (everybigintfits anumeric(38, 0))TYPEchange (Postgres drops them)down()reverses tobigint, but narrowing is lossy on any row with a post-fork-magnitude value — operator guidance in the migration log clarifies they must wipe before revertingTest plan — devnet repro
Booted local 2-node devnet with funded-genesis fixture (5 identities, 1e18 DEM each, osDenomination active from block 0).
Pre-fix:
confirm()returnsvalid: true, broadcast lands tx inreference_block=2, node-1 crashes atinsertBlock:Chain stops at block 2, no receiver-side balance update visible.
Post-fix: chain advances to block 9+, receiver balance moves from
1e27OS (genesis) →1.1e27OS (genesis + 1e26 OS transfer). GCREdit hash compare still passes (PR fix(forks,validation): post-fork by default on fresh chains + GCREdit hash mismatch #861 prerequisite).Type-check
bun run type-check-ts # new code clean; pre-existing duplicate-import errors in chainBlocks.ts + worker-threads-test unchangedSummary by CodeRabbit