fix(gnovm): bound type-expansion fan-out before go/types validType - #5826
ltzmaxwell wants to merge 58 commits into
Conversation
🛠 PR Checks SummaryAll Automated Checks passed. ✅ Manual Checks (for Reviewers):
Read More🤖 This bot helps streamline PR reviews by verifying automated checks and providing guidance for contributors and reviewers. ✅ Automated Checks (for Contributors):🟢 Maintainers must be able to edit this pull request (more info) ☑️ Contributor Actions:
☑️ Reviewer Actions:
📚 Resources:Debug
|
davd-gzl
left a comment
There was a problem hiding this comment.
The fix is correct and the guard is linear, but the validType DoS is still reachable through three type shapes the guard under-counts. Verified on 088ce87: each drop-in txtar deploys past the guard and hangs the node on the unmetered addpkg path, where the direct fan-out is rejected.
| return nil, errs | ||
| } | ||
|
|
||
| // STEP 3.5: Guard against pathological type-expansion fan-out before the |
There was a problem hiding this comment.
It's noted STEP 3, then STEP 3.5, then STEP 3 again
There was a problem hiding this comment.
Fixed in c049157 — the pre-type-check guard comments now reuse STEP 3 plainly, matching the repeated STEP 4 blocks below.
| case *ast.IndexExpr: | ||
| return cost(t.X) // generic instantiation: bound by the base type | ||
| case *ast.IndexListExpr: | ||
| return cost(t.X) |
There was a problem hiding this comment.
For W[A{n-1}] the guard counts the base W and ignores the type argument, where the doubling lives. A generic fan-out reads as a constant and passes into the validType hang. Fix: count the type arguments too, or reject the instantiation.
repro
The direct [0]T%d form at this depth is rejected in milliseconds; the generic form is not:
gh pr checkout 5826 -R gnolang/gno && git checkout 088ce87
cat > gno.land/pkg/integration/testdata/addpkg_typecheck_fanout_generic.txtar <<'EOF'
# Value-containment fan-out routed through a generic: each A_n embeds W[A_{n-1}]
# by value, W holds its type parameter twice, so validType still doubles. The
# guard drops the IndexExpr type argument, so it must ALSO reject this, not hang.
# start a new node
gnoland start
# adding the generic fan-out package must fail with a denial-of-service rejection
! gnokey maketx addpkg -pkgdir $WORK/fanout -pkgpath gno.land/r/foobar/fanout -gas-fee 350001ugnot -gas-wanted 20_000_000 -chainid=tendermint_test $test1_user_addr
stderr 'denial-of-service'
-- fanout/gnomod.toml --
module = "gno.land/r/foobar/fanout"
gno = "0.9"
-- fanout/fanout.gno --
package fanout
type W[P any] struct{ a, b [0]P }
type A0 struct{ v int }
type A1 struct{ x W[A0] }
type A2 struct{ x W[A1] }
type A3 struct{ x W[A2] }
type A4 struct{ x W[A3] }
type A5 struct{ x W[A4] }
type A6 struct{ x W[A5] }
type A7 struct{ x W[A6] }
type A8 struct{ x W[A7] }
type A9 struct{ x W[A8] }
type A10 struct{ x W[A9] }
type A11 struct{ x W[A10] }
type A12 struct{ x W[A11] }
type A13 struct{ x W[A12] }
type A14 struct{ x W[A13] }
type A15 struct{ x W[A14] }
type A16 struct{ x W[A15] }
type A17 struct{ x W[A16] }
type A18 struct{ x W[A17] }
type A19 struct{ x W[A18] }
type A20 struct{ x W[A19] }
type A21 struct{ x W[A20] }
type A22 struct{ x W[A21] }
type A23 struct{ x W[A22] }
type A24 struct{ x W[A23] }
type A25 struct{ x W[A24] }
type A26 struct{ x W[A25] }
type A27 struct{ x W[A26] }
type A28 struct{ x W[A27] }
type A29 struct{ x W[A28] }
type A30 struct{ x W[A29] }
type A31 struct{ x W[A30] }
type A32 struct{ x W[A31] }
type A33 struct{ x W[A32] }
type A34 struct{ x W[A33] }
type A35 struct{ x W[A34] }
type A36 struct{ x W[A35] }
type A37 struct{ x W[A36] }
type A38 struct{ x W[A37] }
type A39 struct{ x W[A38] }
type A40 struct{ x W[A39] }
var Sink A40
EOF
go test -count=1 -v -run TestTestdata/addpkg_typecheck_fanout_generic ./gno.land/pkg/integration/
rm gno.land/pkg/integration/testdata/addpkg_typecheck_fanout_generic.txtarObserved on 088ce87:
# adding the generic fan-out package must fail with a denial-of-service rejection (65.00s)
> ! gnokey maketx addpkg ... -pkgpath gno.land/r/foobar/fanout ...
"gnokey" error: unable to call RPC method abci_query, unable to send request, Post "http://127.0.0.1:...": context deadline exceeded
> stderr 'denial-of-service'
FAIL: testdata/addpkg_typecheck_fanout_generic.txtar:12: no match for `denial-of-service` found in stderr
There was a problem hiding this comment.
Fixed in c049157, by rejecting the instantiation rather than counting it: checkNoGenerics now rejects generics syntactically before go/types runs (Gno targets go1.17; pinning types.Config.GoVersion does not help because go/types reports the version error but still runs the validType walk). Your repro is committed as addpkg_typecheck_fanout_generic.txtar and is now rejected in milliseconds with generic type declarations are not supported (Gno targets go1.17).
| case *ast.InterfaceType: | ||
| total := uint64(1) | ||
| for _, f := range t.Methods.List { | ||
| if len(f.Names) != 0 { | ||
| continue // method: a func signature, not recursed | ||
| } | ||
| total = satAdd(total, cost(f.Type)) // embedded type / type elem | ||
| if total > typeExpansionBudget { | ||
| return total | ||
| } | ||
| } | ||
| return total |
There was a problem hiding this comment.
The guard recurses into struct fields and array elements but not into an interface type-set union. A union term [0]X | [1]X (a BinaryExpr, ~T a UnaryExpr) hits default: return 1 at line 147-148, yet validType walks both sides, so a union-doubling chain blows up unscored. Fix: recurse both sides of a | and the operand of a ~.
repro
gh pr checkout 5826 -R gnolang/gno && git checkout 088ce87
cat > gno.land/pkg/integration/testdata/addpkg_typecheck_fanout_union.txtar <<'EOF'
# Value-containment fan-out through interface type-set unions: each I_n unions
# two array types over I_{n-1}, so validType still doubles per level. The guard's
# InterfaceType arm doesn't recurse the BinaryExpr union term (default returns 1),
# so it must reject this, not hang.
# start a new node
gnoland start
# adding the union fan-out package must fail with a denial-of-service rejection
! gnokey maketx addpkg -pkgdir $WORK/fanout -pkgpath gno.land/r/foobar/fanout -gas-fee 350001ugnot -gas-wanted 20_000_000 -chainid=tendermint_test $test1_user_addr
stderr 'denial-of-service'
-- fanout/gnomod.toml --
module = "gno.land/r/foobar/fanout"
gno = "0.9"
-- fanout/fanout.gno --
package fanout
type I0 interface{ m() }
type I1 interface{ [0]I0 | [1]I0 }
type I2 interface{ [0]I1 | [1]I1 }
type I3 interface{ [0]I2 | [1]I2 }
type I4 interface{ [0]I3 | [1]I3 }
type I5 interface{ [0]I4 | [1]I4 }
type I6 interface{ [0]I5 | [1]I5 }
type I7 interface{ [0]I6 | [1]I6 }
type I8 interface{ [0]I7 | [1]I7 }
type I9 interface{ [0]I8 | [1]I8 }
type I10 interface{ [0]I9 | [1]I9 }
type I11 interface{ [0]I10 | [1]I10 }
type I12 interface{ [0]I11 | [1]I11 }
type I13 interface{ [0]I12 | [1]I12 }
type I14 interface{ [0]I13 | [1]I13 }
type I15 interface{ [0]I14 | [1]I14 }
type I16 interface{ [0]I15 | [1]I15 }
type I17 interface{ [0]I16 | [1]I16 }
type I18 interface{ [0]I17 | [1]I17 }
type I19 interface{ [0]I18 | [1]I18 }
type I20 interface{ [0]I19 | [1]I19 }
type I21 interface{ [0]I20 | [1]I20 }
type I22 interface{ [0]I21 | [1]I21 }
type I23 interface{ [0]I22 | [1]I22 }
type I24 interface{ [0]I23 | [1]I23 }
type I25 interface{ [0]I24 | [1]I24 }
type I26 interface{ [0]I25 | [1]I25 }
type I27 interface{ [0]I26 | [1]I26 }
type I28 interface{ [0]I27 | [1]I27 }
type I29 interface{ [0]I28 | [1]I28 }
type I30 interface{ [0]I29 | [1]I29 }
type I31 interface{ [0]I30 | [1]I30 }
type I32 interface{ [0]I31 | [1]I31 }
type I33 interface{ [0]I32 | [1]I32 }
type I34 interface{ [0]I33 | [1]I33 }
type I35 interface{ [0]I34 | [1]I34 }
type I36 interface{ [0]I35 | [1]I35 }
type I37 interface{ [0]I36 | [1]I36 }
type I38 interface{ [0]I37 | [1]I37 }
type I39 interface{ [0]I38 | [1]I38 }
type I40 interface{ [0]I39 | [1]I39 }
type Use struct{ x I40 }
EOF
go test -count=1 -v -run TestTestdata/addpkg_typecheck_fanout_union ./gno.land/pkg/integration/
rm gno.land/pkg/integration/testdata/addpkg_typecheck_fanout_union.txtarObserved on 088ce87:
# adding the union fan-out package must fail with a denial-of-service rejection (65.00s)
> ! gnokey maketx addpkg ... -pkgpath gno.land/r/foobar/fanout ...
"gnokey" error: unable to call RPC method abci_query, unable to send request, Post "http://127.0.0.1:...": context deadline exceeded
> stderr 'denial-of-service'
FAIL: testdata/addpkg_typecheck_fanout_union.txtar:13: no match for `denial-of-service` found in stderr
There was a problem hiding this comment.
Fixed in c049157: checkNoGenerics rejects exactly the two type-set terms cost() cannot count — union (|) and approximation (~) — before go/types runs. Bare and ;-separated type-set elements stay allowed: they are ordinary containment edges the bound already counts. Your repro is committed as addpkg_typecheck_fanout_union.txtar (rejected with interface type unions are not supported), and 9ec5b86 adds a multi-element interface fan-out case to keep that path counted.
| case *ast.SelectorExpr: | ||
| return 1 // imported type: already validated in its own package |
There was a problem hiding this comment.
The guard counts an imported type pkg.T as a flat 1. But validType re-expands imported types without caching across packages (golang/go#65711), so a doubling chain spread across deployed packages stays under the per-package guard while the walk doubles at every link until a deploy hangs the node. The guard never sees the imported cost. Fix: remember each package's worst-case expansion (e.g. in TypeCheckCache) and add it in for pkg.T.
repro (deploy chain)
p0 is a depth-16 chain (cost 2^16, under budget); p1..p5 each embed the previous package's T four times. Each passes the guard, but validType re-expands the imported chain per deploy:
gh pr checkout 5826 -R gnolang/gno && git checkout 088ce87
cat > gno.land/pkg/integration/testdata/addpkg_typecheck_fanout_imported.txtar <<'EOF'
# Value-containment fan-out split across a deploy chain. Each package passes the
# guard on its own (imported types score as a leaf: SelectorExpr returns 1), but
# validType crosses import boundaries without memoizing, so the walk doubles per
# package and the final deploy kills the node, with no package over budget.
# start a new node
gnoland start
# deploy p0
gnokey maketx addpkg -pkgdir $WORK/p0 -pkgpath gno.land/r/foobar/p0 -gas-fee 350001ugnot -gas-wanted 20_000_000 -chainid=tendermint_test $test1_user_addr
# deploy p1
gnokey maketx addpkg -pkgdir $WORK/p1 -pkgpath gno.land/r/foobar/p1 -gas-fee 350001ugnot -gas-wanted 20_000_000 -chainid=tendermint_test $test1_user_addr
# deploy p2
gnokey maketx addpkg -pkgdir $WORK/p2 -pkgpath gno.land/r/foobar/p2 -gas-fee 350001ugnot -gas-wanted 20_000_000 -chainid=tendermint_test $test1_user_addr
# deploy p3
gnokey maketx addpkg -pkgdir $WORK/p3 -pkgpath gno.land/r/foobar/p3 -gas-fee 350001ugnot -gas-wanted 20_000_000 -chainid=tendermint_test $test1_user_addr
# deploy p4
gnokey maketx addpkg -pkgdir $WORK/p4 -pkgpath gno.land/r/foobar/p4 -gas-fee 350001ugnot -gas-wanted 20_000_000 -chainid=tendermint_test $test1_user_addr
# deploy p5 (passes the guard, but its validType walk re-expands the whole imported chain)
! gnokey maketx addpkg -pkgdir $WORK/p5 -pkgpath gno.land/r/foobar/p5 -gas-fee 350001ugnot -gas-wanted 20_000_000 -chainid=tendermint_test $test1_user_addr
stderr 'denial-of-service'
-- p0/gnomod.toml --
module = "gno.land/r/foobar/p0"
gno = "0.9"
-- p0/p0.gno --
package p0
type t0 struct{ v int }
type t1 struct{ a, b [0]t0 }
type t2 struct{ a, b [0]t1 }
type t3 struct{ a, b [0]t2 }
type t4 struct{ a, b [0]t3 }
type t5 struct{ a, b [0]t4 }
type t6 struct{ a, b [0]t5 }
type t7 struct{ a, b [0]t6 }
type t8 struct{ a, b [0]t7 }
type t9 struct{ a, b [0]t8 }
type t10 struct{ a, b [0]t9 }
type t11 struct{ a, b [0]t10 }
type t12 struct{ a, b [0]t11 }
type t13 struct{ a, b [0]t12 }
type t14 struct{ a, b [0]t13 }
type t15 struct{ a, b [0]t14 }
type T struct{ a, b [0]t15 }
-- p1/gnomod.toml --
module = "gno.land/r/foobar/p1"
gno = "0.9"
-- p1/p1.gno --
package p1
import "gno.land/r/foobar/p0"
type T struct{ a, b, c, d [0]p0.T }
-- p2/gnomod.toml --
module = "gno.land/r/foobar/p2"
gno = "0.9"
-- p2/p2.gno --
package p2
import "gno.land/r/foobar/p1"
type T struct{ a, b, c, d [0]p1.T }
-- p3/gnomod.toml --
module = "gno.land/r/foobar/p3"
gno = "0.9"
-- p3/p3.gno --
package p3
import "gno.land/r/foobar/p2"
type T struct{ a, b, c, d [0]p2.T }
-- p4/gnomod.toml --
module = "gno.land/r/foobar/p4"
gno = "0.9"
-- p4/p4.gno --
package p4
import "gno.land/r/foobar/p3"
type T struct{ a, b, c, d [0]p3.T }
-- p5/gnomod.toml --
module = "gno.land/r/foobar/p5"
gno = "0.9"
-- p5/p5.gno --
package p5
import "gno.land/r/foobar/p4"
type T struct{ a, b, c, d [0]p4.T }
EOF
go test -count=1 -v -run TestTestdata/addpkg_typecheck_fanout_imported ./gno.land/pkg/integration/
rm gno.land/pkg/integration/testdata/addpkg_typecheck_fanout_imported.txtarObserved on 088ce87 (gas flat, wall-clock doubles each hop, then the node dies):
# deploy p0 (0.118s) GAS USED: 3055141
# deploy p1 (0.297s) GAS USED: 4126720
# deploy p2 (0.914s) GAS USED: 4349353
# deploy p3 (3.467s) GAS USED: 4598450
# deploy p4 (13.556s) GAS USED: 4847547
# deploy p5 (28.079s) "gnokey" error: ... Post "http://127.0.0.1:...": EOF <- node down
> stderr 'denial-of-service'
FAIL: testdata/addpkg_typecheck_fanout_imported.txtar:29: no match for `denial-of-service` found in stderr
There was a problem hiding this comment.
Fixed in c049157: the guard now follows value-containment across import boundaries. checkTypeExpansionBoundImports resolves already-deployed dependencies via the importer's getter and memoizes by (package, name), so the cross-package walk validType runs exponentially is computed linearly here, with no extra store gas for the common case. Stdlib imports remain leaves — bounded source that cannot import user packages (rationale documented in 02752d9 and the ADR). Your deploy chain is committed as addpkg_typecheck_fanout_imported.txtar: p0 (2^16, under budget) deploys, and p1 is rejected with denial-of-service at 4×2^16 > 100k.
…n-out Reject go1.18 generics/interface type-sets before go/types runs, and follow value-containment across imports (a memoizing getter keeps store gas unchanged).
…interface fan-out
|
Follow-up, not implemented here: Rationale: |
|
#5892 (per-source-byte gas) is complementary, not overlapping. It prices the linear case; a per-byte charge cannot catch an exponential one, since ~1KB of source causing ~2^40 node visits is not a function of the bytes. They compose: byte-gas gate → this PR's expansion cap → Note #5892 is stacked on #5891, which rewrites the |
|
Superseded — this comment argued against forking The corrected argument — including what a metered fork would buy, and why size and shipping risk still decide against it — is now in the PR description under Why not fork [AI-Assisted] |
|
#5921 is closed as a duplicate — this PR is the sole carrier of the guard. Placement verdict: #5921 (comment). #6059 adds the same rejection to Measured — what still passes this PR's on-chain gate:
None is a DoS vector, so the guard is complete for its purpose. But row 1 is a correctness gap on-chain, not only in dev tools: Pushed here as follow-ups: the |
…n one tx Removing the per-package ceiling was justified by the bound holding per TRANSACTION rather than per message — Tx.Msgs is unbounded and ValidateBasic caps gas, not the message count. Nothing tested that. The existing coverage tests accumulation across a dependency GRAPH within one message (addpkg_typecheck_fanout_deps_gas.txtar) and that the charge is wired at all (TestVMKeeperAddPackage_TypeExpansionGasCharged), but not accumulation across messages, which is the property the ceiling's removal rests on. The new test asserts both halves, since only the pair means anything: four messages each fit the budget alone, and the same four sharing one budget run out partway (2 of 4 land). baseapp gives a tx one basicGasMeter(GasWanted) and then loops over msgs, so driving several AddPackage calls through one ctx and one finite meter is what a multi-message tx actually does. A per-package ceiling could not have caught this: every message here is modest on its own. The fixture holds SOURCE BYTES EQUAL between the two shapes, padding the pointer variant with a comment. That matters: the first version differed by "[0]" vs "*", making the value chain 20 bytes longer, and PreprocessGasPerByte alone carried the assertion — verified by unwiring the meter, where that version still passed. With bytes equal the gap is the expansion charge (1.43M per message, matching a depth-10 chain's ~14k nodes at 100 gas/node), and unwiring the meter now fails this test as well as the existing one.
…ng the removed ceiling
Two loose ends.
The file was named for a bound that no longer exists: nothing in the shipped design
rejects a package for being too complex, it computes a price. A reader looking for
the limit found only gas. typecheck_bound.go -> typecheck_cost.go (plus its two
test files), and parseBoundSrc/makeBoundResolver -> parseCostSrc/makeCostResolver.
leafExpansionBound keeps its name — it genuinely is an upper bound on stdlib
expansion. Git records all three as renames.
The ADR and comments also still narrated the ceiling's history — "an earlier
revision carried 1_000_000", "first per-type at 100_000". That version never
shipped, so the archaeology is noise for anyone reading the result. The ARGUMENT
for having no ceiling stays, since a reviewer needs it; only the history goes. Same
for the rate: the two-step derivation now warns that skipping calibration yields
~25 and a ~4x under-charge, without framing it as something a past revision did.
Also records the limitation this PR does not address: validType is now priced by
structure, but everything else in type-check and preprocess is priced by
PreprocessGasPerByte, which is only right for work proportional to source size —
and nobody has audited the other passes against that. The threshold is lower than
it looks: a merely QUADRATIC pass is already badly under-priced, since 1MB of
O(n^2) work is ~1e12 operations against the ~1.25e9 gas its bytes buy. Lists the
unverified candidates (constant arithmetic, method-set/interface satisfaction,
Identical on deep structural types) so the next person does not start from scratch,
and notes that the generics rejection incidentally closes the other known
super-linear behaviour in go/types.
The per-tx test's comment now also records why it cannot be an integration txtar:
every keyscli command hardcodes Msgs: []std.Msg{msg} (addpkg.go:135, call.go:133,
run.go:144), so gnokey — all a txtar can drive — cannot build a multi-message
transaction at all. The txtars cover the other accumulation, across a dependency
graph within one message.
Verified: guard suite, sdk/vm gas tests, full TestTestdata, golangci-lint 0 issues,
TestFiles unchanged at its 10-failure toolchain-drift baseline.
…after all
Retracts a claim I made in the previous commit. I wrote that this property "cannot
be an integration txtar" because every keyscli command hardcodes
Msgs: []std.Msg{msg}. That is true of maketx and irrelevant: broadcast.go:72-77
reads a JSON file into std.Tx, whose Msgs is a slice, and sign.go:140-152 signs an
arbitrary tx file. addpkg_multi_msg.txtar has been doing exactly a two-message
addpkg all along. The impossibility was asserted from one grep.
addpkg_typecheck_fanout_multi_msg.txtar now covers it over the real path — ante
handler, GasWanted, one basicGasMeter across msgs — via gnokey sign + broadcast on
a hand-written tx. Both halves share an 8,000,000 budget, so the only variable is
how many messages share it:
3 depth-10 value chains in one tx : exceeds 8,000,000, aborts mid-tx
1 identical package on its own : 4,323,324
It discriminates: with GasMeter removed from the keeper's TypeCheckOptions the
three messages cost 6,524,793 and the tx succeeds, failing the assertion. Both
figures are recorded in the header so the budget can be retuned safely.
Also corrects a number I had been quoting. I reported one such package at 1.87M
gas; end-to-end it is 4.32M. The keeper path excludes fee and signature work, so
citing its figure as the cost was misleading — and it is why the first budget I
picked sat below even a single message.
The in-process test stays, with its job stated rather than assumed: it holds source
bytes exactly equal between the two shapes, which a txtar cannot conveniently do,
so the expansion charge is provably the only difference rather than the likely one.
Each test now points at the other.
There was a problem hiding this comment.
Blocking: typeExpansionCost ranges over a map, and for a value-containment cycle the count depends on iteration order — the same package is priced at 3076 or 6136 nodes, 306,000 gas apart. Cause and fix inline.
Forks two ways: ABCIResult.Error is hashed into LastResultsHash, so at a GasWanted between the two totals one validator returns OutOfGasError and another ErrTypeCheck (58/80 vs 22/80 replaying one MsgAddPackage at GasWanted=800_000); and runTx charges GasConsumedToLimit() to the BlockGasMeter.
Only invalid recursive types are affected — I re-scored all 211 stdlib + example packages 15x, none unstable — but the charge lands before go/types rejects them, so one cheap malformed package is enough.
Repro below: six identical packages, GAS USED comes back 1636245 or 1942245 at random. 12/12 runs fail on this head, 12/12 pass with the suggestion.
go test ./gno.land/pkg/integration/ -run 'TestTestdata/addpkg_typecheck_fanout_cyclic_nondet' -count=1
addpkg_typecheck_fanout_cyclic_nondet.txtar
# The type-expansion charge is NOT deterministic for a value-containment cycle.
#
# namedCost returns 1 for a member it is already visiting and correctly does not
# memoize that. But every ANCESTOR on the cycle memoizes a value derived from that
# truncation into the shared c.memo, and which member gets truncated depends on
# which root typeExpansionCost's `range c.declsFor(entryPath)` reaches first. So
# the same source is priced at either 3076 or 6136 nodes -- 306000 gas apart.
#
# All six packages below are identical and all rejected by go/types for the same
# invalid recursive type, but the charge lands BEFORE go/types runs, so each is
# billed at random: GAS USED is 1636245 (1634985 on the last tx, which is 1260
# cheaper for unrelated reasons) or 1942245.
#
# On a real chain that is a divergent DeliverTx result between validators, and
# ABCIResult.Error is hashed into LastResultsHash.
gnoland start
! gnokey maketx addpkg -pkgdir $WORK/cyc1 -pkgpath gno.land/r/foobar/cyc1 -gas-fee 350001ugnot -gas-wanted 40_000_000 -chainid=tendermint_test $test1_user_addr
stdout 'GAS USED:\s+163'
! gnokey maketx addpkg -pkgdir $WORK/cyc2 -pkgpath gno.land/r/foobar/cyc2 -gas-fee 350001ugnot -gas-wanted 40_000_000 -chainid=tendermint_test $test1_user_addr
stdout 'GAS USED:\s+163'
! gnokey maketx addpkg -pkgdir $WORK/cyc3 -pkgpath gno.land/r/foobar/cyc3 -gas-fee 350001ugnot -gas-wanted 40_000_000 -chainid=tendermint_test $test1_user_addr
stdout 'GAS USED:\s+163'
! gnokey maketx addpkg -pkgdir $WORK/cyc4 -pkgpath gno.land/r/foobar/cyc4 -gas-fee 350001ugnot -gas-wanted 40_000_000 -chainid=tendermint_test $test1_user_addr
stdout 'GAS USED:\s+163'
! gnokey maketx addpkg -pkgdir $WORK/cyc5 -pkgpath gno.land/r/foobar/cyc5 -gas-fee 350001ugnot -gas-wanted 40_000_000 -chainid=tendermint_test $test1_user_addr
stdout 'GAS USED:\s+163'
! gnokey maketx addpkg -pkgdir $WORK/cyc6 -pkgpath gno.land/r/foobar/cyc6 -gas-fee 350001ugnot -gas-wanted 40_000_000 -chainid=tendermint_test $test1_user_addr
stdout 'GAS USED:\s+163'
-- cyc1/gnomod.toml --
module = "gno.land/r/foobar/cyc1"
gno = "0.9"
-- cyc1/cyc.gno --
package cyc1
type A struct{ x, y B }
type B struct{ x, y A }
type Z1 struct{ p, q A }
type Z2 struct{ p, q Z1 }
type Z3 struct{ p, q Z2 }
type Z4 struct{ p, q Z3 }
type Z5 struct{ p, q Z4 }
type Z6 struct{ p, q Z5 }
type Z7 struct{ p, q Z6 }
type Z8 struct{ p, q Z7 }
-- cyc2/gnomod.toml --
module = "gno.land/r/foobar/cyc2"
gno = "0.9"
-- cyc2/cyc.gno --
package cyc2
type A struct{ x, y B }
type B struct{ x, y A }
type Z1 struct{ p, q A }
type Z2 struct{ p, q Z1 }
type Z3 struct{ p, q Z2 }
type Z4 struct{ p, q Z3 }
type Z5 struct{ p, q Z4 }
type Z6 struct{ p, q Z5 }
type Z7 struct{ p, q Z6 }
type Z8 struct{ p, q Z7 }
-- cyc3/gnomod.toml --
module = "gno.land/r/foobar/cyc3"
gno = "0.9"
-- cyc3/cyc.gno --
package cyc3
type A struct{ x, y B }
type B struct{ x, y A }
type Z1 struct{ p, q A }
type Z2 struct{ p, q Z1 }
type Z3 struct{ p, q Z2 }
type Z4 struct{ p, q Z3 }
type Z5 struct{ p, q Z4 }
type Z6 struct{ p, q Z5 }
type Z7 struct{ p, q Z6 }
type Z8 struct{ p, q Z7 }
-- cyc4/gnomod.toml --
module = "gno.land/r/foobar/cyc4"
gno = "0.9"
-- cyc4/cyc.gno --
package cyc4
type A struct{ x, y B }
type B struct{ x, y A }
type Z1 struct{ p, q A }
type Z2 struct{ p, q Z1 }
type Z3 struct{ p, q Z2 }
type Z4 struct{ p, q Z3 }
type Z5 struct{ p, q Z4 }
type Z6 struct{ p, q Z5 }
type Z7 struct{ p, q Z6 }
type Z8 struct{ p, q Z7 }
-- cyc5/gnomod.toml --
module = "gno.land/r/foobar/cyc5"
gno = "0.9"
-- cyc5/cyc.gno --
package cyc5
type A struct{ x, y B }
type B struct{ x, y A }
type Z1 struct{ p, q A }
type Z2 struct{ p, q Z1 }
type Z3 struct{ p, q Z2 }
type Z4 struct{ p, q Z3 }
type Z5 struct{ p, q Z4 }
type Z6 struct{ p, q Z5 }
type Z7 struct{ p, q Z6 }
type Z8 struct{ p, q Z7 }
-- cyc6/gnomod.toml --
module = "gno.land/r/foobar/cyc6"
gno = "0.9"
-- cyc6/cyc.gno --
package cyc6
type A struct{ x, y B }
type B struct{ x, y A }
type Z1 struct{ p, q A }
type Z2 struct{ p, q Z1 }
type Z3 struct{ p, q Z2 }
type Z4 struct{ p, q Z3 }
type Z5 struct{ p, q Z4 }
type Z6 struct{ p, q Z5 }
type Z7 struct{ p, q Z6 }
type Z8 struct{ p, q Z7 }
The rest of the guard checks out: cost() never under-counts (diffed against a brute-force validType0 walk over 6,500 random single- and cross-package graphs), dot imports are closed, interface{ (A|B) } / (~A) are unreachable via go/parser, the stdlib leaf bound can't be gamed, and all chain-reachable type-check sites carry the meter.
| if c.visiting[k] { | ||
| // A value-containment cycle: an invalid recursive type that go/types | ||
| // detects and reports itself. Return a finite count so we neither loop | ||
| // nor pre-empt go/types' diagnostic. Deliberately not memoized — the | ||
| // truncated value is only valid inside this walk. | ||
| return 1 | ||
| } | ||
| c.visiting[k] = true | ||
| var best uint64 = 1 | ||
| for _, d := range specs { | ||
| if v := satAdd(1, c.cost(d.spec.Type, k.pkg, d.imports)); v > best { | ||
| best = v | ||
| } | ||
| } | ||
| delete(c.visiting, k) | ||
| c.memo[k] = best |
There was a problem hiding this comment.
Root cause. Returning 1 for a member already being visited, and not memoizing it, is right. But every ancestor on the cycle memoizes a value derived from that truncation into the shared c.memo on line 243 — and which member gets truncated depends on which root typeExpansionCost reaches first. So the charge follows Go map order.
There was a problem hiding this comment.
Confirmed, fixed in dfda73a. Exactly as you describe: the truncated 1 itself was never memoized, but every ancestor memoized a value derived from it, so the charge followed map order. Reproduced locally at exactly 3076 vs 6136 nodes (306k gas apart) before the fix.
| // validType runs once per DECLARATION, so the charge is the sum over them, not | ||
| // over names (a name declared twice is validated twice). Order-independent. | ||
| var total uint64 | ||
| for _, specs := range c.declsFor(entryPath) { | ||
| for _, d := range specs { | ||
| total = satAdd(total, satAdd(1, c.cost(d.spec.Type, entryPath, d.imports))) | ||
| } | ||
| } |
There was a problem hiding this comment.
Order-independent isn't true: the sum is, but c.memo isn't (see above).
Sorting the roots pins which member truncates. Changes no existing number — the calibration scans, -run Gas, and all seven addpkg_typecheck_* txtars still pass:
| // validType runs once per DECLARATION, so the charge is the sum over them, not | |
| // over names (a name declared twice is validated twice). Order-independent. | |
| var total uint64 | |
| for _, specs := range c.declsFor(entryPath) { | |
| for _, d := range specs { | |
| total = satAdd(total, satAdd(1, c.cost(d.spec.Type, entryPath, d.imports))) | |
| } | |
| } | |
| // validType runs once per DECLARATION, so the charge is the sum over them, not | |
| // over names (a name declared twice is validated twice). | |
| // | |
| // Sorted, not map order: namedCost truncates a containment cycle at whichever | |
| // member is reached first and every ancestor memoizes a value derived from that | |
| // truncation, so an unsorted walk prices the same package differently per run. | |
| // This count is charged as gas, so it must be identical on every node. | |
| var total uint64 | |
| decls := c.declsFor(entryPath) | |
| for _, name := range slices.Sorted(maps.Keys(decls)) { | |
| for _, d := range decls[name] { | |
| total = satAdd(total, satAdd(1, c.cost(d.spec.Type, entryPath, d.imports))) | |
| } | |
| } |
Worth a regression test pinning a cyclic package's count; nothing in the suite exercises one today.
There was a problem hiding this comment.
Applied as given in dfda73a, with two regression tests that both fail if the sort is removed: TestTypeExpansionCostCyclicIsDeterministic (300 iterations; unsorted reports the package priced 2 different ways) and your repro committed verbatim as addpkg_typecheck_fanout_cyclic_nondet.txtar. ac814c5 then moved the sort to construction: pkgDecls caches a sorted names slice next to the map and the loop iterates that, so the iteration site no longer touches a map at all — same order, no number moves. 33b0131 adds the remaining cycle topologies.
| "go/types" | ||
| "math" | ||
| "path" | ||
| "strconv" |
There was a problem hiding this comment.
Imports for the suggestion below.
| "go/types" | |
| "math" | |
| "path" | |
| "strconv" | |
| "go/types" | |
| "maps" | |
| "math" | |
| "path" | |
| "slices" | |
| "strconv" |
| continue | ||
| } | ||
| name = imp.Name.Name | ||
| } else if name = c.pkgName(impPath); name == "" { |
There was a problem hiding this comment.
Minor: declsFor runs over allgofs, so pkgName fetches imports appearing only in _test.gno files — which go/types never imports under ProdOnly. Deterministic, just an unintended store-read cost; maybe one line under "Known limits".
There was a problem hiding this comment.
Documented in dfda73a, in the ADR next to the resolver rationale ("Also unnecessary work: declsFor runs over allgofs..."). Left as a known limit rather than filtering here: deterministic and over-charge only, as you note.
| alloc := gnostore.GetAllocator() | ||
| // Per-tx preprocess allocator (see AddPackage for full rationale). | ||
| // Covers both the closure-local Machine that calls RunMemPackage and | ||
| // Covers both the dependency graph-local Machine that calls RunMemPackage and |
There was a problem hiding this comment.
Nit, unrelated to this PR: closure-local Machine became dependency graph-local Machine. Looks like a find/replace artifact — the original was correct.
There was a problem hiding this comment.
Reverted in dfda73a — a find/replace artifact from the earlier pass avoiding the word "closure"; it was the only occurrence. Good catch.
…ent cycles Blocking bug found by @omarsy in review. typeExpansionCost ranged over a map. namedCost truncates a containment cycle at whichever member it is already visiting and correctly does not memoize that 1 — but every ANCESTOR on the cycle memoizes a value DERIVED from the truncation, and which member truncates depends on which root is walked first. The same source was therefore priced at 3076 or 6136 nodes at random, 306,000 gas apart. Reproduced locally at exactly those figures. The count is charged as gas, so a node that disagrees forks: ABCIResult.Error is hashed into LastResultsHash, and runTx charges GasConsumedToLimit() to the BlockGasMeter. Only invalid recursive types diverge and go/types rejects those a moment later, but the charge lands BEFORE it does, so one cheap malformed package was enough. My own comment on that loop read "Order-independent." — I had noticed the truncated value was only valid within its walk and written that down, then failed to follow it one step to the enclosing memo entry. Fix is @omarsy's suggestion as given: walk slices.Sorted(maps.Keys(decls)). Confirmed it moves no existing number — honest maximum still 431 over 211 packages, stdlib maximum still 19, -run Gas unchanged. Two regression tests, both verified to FAIL with the sort removed: - TestTypeExpansionCostCyclicIsDeterministic: 300 iterations, one assertion. Unsorted it reports "priced 2 different ways (map[3076:88 6136:212])". - addpkg_typecheck_fanout_cyclic_nondet.txtar: the reviewer's own repro, included verbatim. Six samples over the real DeliverTx path — 6/6 runs fail unsorted, 5/5 pass sorted. Carries a maintenance note, since its assertions pin a GAS USED prefix that an unrelated gas change would shift; relaxing it to match both values is the bug it detects. Also from the same review: - keeper.go: "closure-local Machine" had become "dependency graph-local Machine", a find/replace artifact from the earlier pass avoiding the word "closure". That usage was correct — it means a Go closure. Reverted; it was the only one. - ADR records that declsFor runs over allgofs, so pkgName resolves imports appearing only in _test.gno files that ProdOnly never lets go/types import. Deterministic and only over-charges, but a store read the deploy would not otherwise make.
The three gno-security*.md docs are written for realm authors. Nothing covers changing the node itself, where the failure mode is not a bug in one contract but a chain split — so an agent or contributor touching GnoVM, the keeper or the tm2 store has no distilled rules to work from. Six bullets, each of which has already cost real debugging: - Nothing reaching consensus may depend on Go map iteration order. Called out explicitly as GO maps, because Gno's own maps iterate in insertion order (gno-data-structures.md) and intuition from writing .gno actively misleads here. - A commutative reduction is not a defence: memoizing a value derived from an early exit launders order-dependence into the total. This shipped in gnolang#5826 — a per-package gas count came out 3076 or 6136 nodes from identical source, 306k gas apart, and was caught in review rather than by a test. - Prove determinism by repetition, since Go randomizes map order per range and a single call proves nothing. - Why it is not a nit: ABCIResult.Error is hashed into LastResultsHash and runTx charges GasConsumedToLimit() to the BlockGasMeter, so two nodes that price a tx differently disagree on the block. - Charging gas before validation is fine, but then malformed input reaches the charge, so the charge must be deterministic for input that never survives. - 1 gas == 1ns is REFERENCE hardware, not the dev machine. gnovm/cmd/calibrate ships paired benchmark output for the conversion; skipping it under-charges by the machine ratio (~3x on Apple silicon). Docs-only, so no ADR. AGENTS.md invites exactly this kind of edit under "Improving This Document".
…ed map range Follow-up to the determinism fix. The charge was already deterministic — this changes how, for two measured reasons. pkgDecls now carries both a lookup map and a sorted []string of its keys, and typeExpansionCost iterates the slice. Two gains: - The iteration path no longer touches a map at all, so a future `range` over pkgDecls cannot silently reintroduce the consensus fork. The type now also has somewhere to state that contract, which a bare map did not. - Measurably faster: 1.50x at chain depth 100 (45.1 -> 30.1 us), 1.54x at 1000 (561 -> 364 us), 1.14x at 5000 (1836 -> 1612 us), medians of 3. The saving is the key-collection pass: slices.Sorted(maps.Keys(...)) walked the whole map through an iterator and grew a fresh slice on every call, whereas the names are now appended during the AST walk declsFor already performs. Where parsing dominates it makes no difference (211-package scan: 0.12s either way). Sorted, not declaration order. Declaration order is equally deterministic and needs no sort at all, but it is layout-sensitive: writing the two members of a containment cycle in the other order changes the price 2x — measured 3076 vs 6136 nodes on the same type graph. Sorting depends only on the set of names, so moving a declaration between files or reordering for readability moves nobody's gas. No number moves. Honest maximum still 431 over 211 packages / 731 types, largest exported stdlib type still 19 (regexp.Regexp), keeper pointer-vs-value delta still 736,055 gas, the cyclic fixture still 3076, and all eight addpkg_typecheck txtars pass including the one pinning a GAS USED prefix. Note this reinstates the pkgDecls struct that the earlier /simplify pass flattened for wrapping a single field. It now holds two, so the wrapper earns its keep.
Two additions to the consensus-safety section, both from reviewing how this class has actually behaved in this repo rather than from one incident. preprocess.go already sorts for the same reason twice: TestInitOrderDeterminism guards variable-initialization order against map iteration over dependency sets, and TestCircDepDeterminism exists because circular-dependency error messages came out in random order until the DFS was sorted. A map, a cycle and a consensus-visible output, in a different pass, predating gnolang#5826. So this is a documented recurrence, not an extrapolation from a single bug — which is the difference between a rule worth reading and a rule someone invented. And the part that let gnolang#5826 survive: determinism has to be tested on the REJECT path. Charging before validation means malformed input reaches the charge, and malformed input is where the order-sensitive branches are. A sweep over only valid fixtures is silent on the whole class, because nothing valid contains an invalid recursive type. Also notes that a determinism test should be checked against a reverted fix, since symmetric fixtures are stable under both orders and pass while the bug is live.
The single cyclic case only covered the shape the reviewer happened to report. Cycle truncation is the ONLY order-sensitive branch in cost() — every other branch is a pure function of its key, and satAdd/satMul saturate to a fixed point — so the topology of that cycle is the whole risk surface, and it deserves a table. Seven cases, verified against a reverted sort. Four detect the regression: 2-cycle, chain on one member 3076 / 6136 the reported one 2-cycle, chains on both members 844 / 1564 cycle across an import boundary 766 / 1510 previously uncovered 3-cycle with a chain 822 / 1578 / 3090 three-way, not binary Three cannot, and are labelled controls in the test: symmetric 2-cycle 32 equal weight either side self-reference by value 751 one type, no ordering choice acyclic control 2537 no cycle at all The cross-package case is the one worth adding on its own merits: visiting is keyed by (package, name), so the truncation can land in either package, and nothing else in the suite exercised that. The symmetric case is kept deliberately, as a documented control. It is stable under both orders, so a first attempt at this test built only from a bare A-B cycle passes while the bug is live — which is exactly what happened when I first tried to reproduce the reviewer's report and got a flat 32 over 200 runs. Both facts are recorded in the test comment, along with a note that a new case must be checked against a reverted sort: a case that cannot fail is documentation, not a test.
|
Revisiting: still needed — #6088 is orthogonal. Why the oracle / per-package angle doesn't remove the need. A per-package bound (gpao's 10s verify budget, or any per-package ceiling) turns exponential-in-depth (one package) into linear-in-count (N packages) — the spike becomes a sum. But:
The right scope is a per-transaction charge against the gas meter — exactly what this PR adds (pre-charged node count, aborting before the walk runs). gpao changes the shape (exp→linear); this PR prices the unit. The linearization needs this PR to be safe, not the reverse. Scope of this fix (what it is and isn't).
Net: needed now (targeted price on the main vector); full go/types metering is a separate mid-term question. |
…aves Merges origin/master (132e9a0, gnolang#6088 "finish the inert-package flow, allowlist MsgRun, delegate params") and closes the hole that change opens in this one. gnolang#6088 added a FOURTH transaction-path type-check, in a new file, unmetered: keeper_inert.go's EnablePackage assembled TypeCheckOptions by hand and omitted the GasMeter, because it predates txTypeCheckOptions. That is exactly the fail-open case the helper exists to prevent — "the default for any future message type is unmetered" — arriving as a real future message type. And it is the path that matters under the inert policy, where AddPackage deliberately does NOT type-check ("the work is deferred, not avoided: MsgEnablePackage type-checks and runs exactly these bytes later"). EnablePackage now goes through txTypeCheckOptions, so all three transaction paths share it; the three remaining call sites are node-local stdlib init and stay unmetered. The merge conflict is the part worth reviewing closest. gnolang#6088 rewrote exactly the lines this PR changes in AddPackage, and master's version of them has no GasMeter, so a mechanical "take theirs" would have compiled, passed most suites, and silently unmetered the main deploy path. Resolved by taking master's inert branch wholesale and swapping only its hand-built options for the helper. Tests, each verified to FAIL with the wiring reverted: - TestVMKeeperEnablePackage_TypeExpansionGasCharged: byte-equal pointer-vs-value chains, 30,274 vs 1,459,487 gas. - TestVMKeeperEnablePackage_TypeExpansionGasAccumulatesPerTx: four enables through one meter, 2 of 4 land. Its first version passed with the meter removed, because sizing the budget from a measured single enable scales with whatever that enable costs; byte-equal shapes fixed it. - keeper_inert_dos_test.go: the submit-park-enable flow plus MsgRun on a chain locked down as gnolang#6088 intends. - contribs/gpao/dosgap_endtoend_test.go: the same end to end — in-memory node, signed transactions over RPC, the ante handler, and gpao's own decision procedure. Measured against master, same files: submit (parks, no type-check) 91ms both trees gpao's simulate probe 8.9s verdictReady vs 1ms willFail enable 9.2s SUCCEEDED, 3.2M gas vs out of gas stranger's MsgRun 9.8s SUCCEEDED, 1.6M gas vs out of gas 30 composed MsgRun 10.0s client timeout vs out of gas Three findings the demonstration makes concrete. gpao does not protect the chain here — its admission check is a SimulateResult, that simulate runs the walk on the node, and unpriced it returns verdictReady in 8.9s, inside its own 10s budget, so it would approve. That probe is an unauthenticated abci_query: no fee, no transaction. And MsgRun bypasses the flow entirely — the fixture asserts run_submitters is empty from genesis, so subtest 4 is the shipped default, not a loosened fixture. Neither suite claims a halt, and both say so: every stall observed ends and the node recovers. A halt cannot be asserted, only observed, since a test proving the walk never finishes would never finish itself. The doc comment carries the manual recipe (raise dosDepth) and records the two gaps left open on purpose — the gpao daemon, and multi-node — with why closing them adds fidelity but no finding. Block.MaxGas is pinned to the production 3e9 in the end-to-end test: TestingMinimalNodeConfig ships 30e9, which would have made a depth-24 walk affordable and quietly voided the test. gpao's own defaultBlockMaxGas confirms the number.
…ell/gno into pr-5826
omarsy
left a comment
There was a problem hiding this comment.
Re-reviewed at ce9afd93. The nondeterministic charge is fixed: 120 runs of the round-2 fixture through VMKeeper.AddPackage give one gas value and one outcome.
Findings inline: one blocking under-count, one smaller one, and three minor notes. The blocking one came out of an independent codex review pass over the diff; I verified and quantified it.
| // last element, the conventional package name. | ||
| name = path.Base(impPath) | ||
| } | ||
| m[name] = impPath |
There was a problem hiding this comment.
Blocking: a duplicate import selector defeats the charge.
Last write wins here, so when two imports bind the same selector this keeps the LAST. go/types reports redeclared in this block but binds the FIRST and still runs the delayed validType; its own diagnostic says so, naming the second import as the unused one. So the guard prices one package while the walk expands the other.
honest and hijack declare the same depth-10 chain over the same heavy.T. hijack adds one line:
import (
"gno.land/r/foobar/heavy"
heavy "gno.land/r/foobar/light" // rebinds the selector
)| entry | gas charged | outcome |
|---|---|---|
| honest | 1,471,529,929 | refused, out of gas |
| hijack | 4,499,116 | walk runs, then rejected for the redeclaration |
Both are ill-typed. That is the point: the walk runs before the rejection, which is the whole reason this charge exists. It scales with depth. At entry depth 14 through AddPackage, 585 bytes of source buy 15.9 seconds of CPU for 40,780,843 gas, and one transaction may carry the full Block.MaxGas of 3e9.
Fix, about 20 lines: return map[string][]string, append instead of overwrite, and take the max over candidates in cost()'s SelectorExpr case. That is the rule declsFor already applies to duplicate type names. hijack is then billed 1,468,922,916 against honest's 1,471,529,929 and refused before go/types runs.
I have that as a txtar (addpkg_typecheck_fanout_import_alias.txtar, fails on this head, passes with the fix) plus the patch. Happy to push both.
There was a problem hiding this comment.
Fixed at 536c135 as you suggested: fileImports keeps every path bound to a selector, cost() prices the heaviest (TestTypeExpansionCostDuplicateImportSelector). 3e75e2b adds your fixture as addpkg_typecheck_fanout_import_alias.txtar with -simulate skip: gnokey simulates at the chain max gas and only synthesizes out-of-gas when no other error came back, so under simulation the redeclaration wins; through DeliverTx's 20M meter both honest and hijack are refused before go/types runs. Fails with last-write-wins restored (hijack 12.4M vs honest 29.5M). 21adf91 pins the same rule for a duplicated type name.
| // Every such case routes through here, so the file has one answer to "what does an | ||
| // unknown name cost" rather than one per call site. | ||
| func unresolvedCost(name string) uint64 { | ||
| if types.Universe.Lookup(name) != nil { |
There was a problem hiding this comment.
error is priced 1 where validType visits 2: types.Universe.Lookup("error") is a *types.Named, so the walk visits the Named and then its underlying interface. At the base of a doubling chain it multiplies. Same chain, same depth, all three billed the identical 1,677,713,400 gas:
| leaf | walk time | ns/gas |
|---|---|---|
int |
829 ms | 0.494 |
error |
1.736 s | 1.035 |
any |
833 ms | 0.497 |
any is unaffected, it unaliases to a bare interface. Suggest an exact entry for error, the way gnoBuiltinShimExpansion handles realm and address.
There was a problem hiding this comment.
Fixed in 3e75e2b: unresolvedCost prices a Universe *types.Named (error, comparable) at 2. TestUnresolvedCostUniverse replays validType0 over every predeclared type name.
| // charge up, so cost() must never UNDER-count a live edge — that is why the guards | ||
| // below reject instead of approximating. 100 = ~40ns/node from BenchmarkValidTypeWalk | ||
| // x ~2.5 calibration to the Xeon that "1 gas == 1ns" means; adr/pr5826_typecheck_dos_guards.md. | ||
| const typeExpansionGasPerNode = 100 |
There was a problem hiding this comment.
Minor: BenchmarkValidTypeWalk only measures fanOutSrc, so this rate is calibrated on one shape. Interface embedding (issue6977) runs 1.5x to 3.3x more expensive per node on the same box, and gets worse with size where the struct shape plateaus (247 vs 75 ns/node at the largest sizes I could complete). Worth deriving the rate from the worse shape.
There was a problem hiding this comment.
3e75e2b adds an interface-embedding shape to BenchmarkValidTypeWalk. Here it is ~1.15x the struct chain at depth 18–22, and by depth 24–26 struct, interface and interface-with-methods all converge on 50 ns/node (×2.5 ≈ 125, inside the ADR's 88–128), so the rate stays at 100; numbers recorded in the ADR. If your 6977 shape still shows 3x on your box, send the source and I will add it.
| gnoland start | ||
|
|
||
| ! gnokey maketx addpkg -pkgdir $WORK/cyc1 -pkgpath gno.land/r/foobar/cyc1 -gas-fee 350001ugnot -gas-wanted 40_000_000 -chainid=tendermint_test $test1_user_addr | ||
| stdout 'GAS USED:\s+163' |
There was a problem hiding this comment.
Nit: this prefix also matches 1630000, so an unrelated gas change can pass it silently rather than failing loudly. GAS USED:\s+163[0-9]{4}\b would pin the width.
There was a problem hiding this comment.
Done in 3e75e2b, GAS USED:\s+163[0-9]{4}\b on all six.
| // and it is the only code-bearing message with no namespace or CLA gate. A | ||
| // second key that is neither the approver nor a listed submitter reaches the | ||
| // same walk on this locked-down chain. | ||
| func TestDoSGapAgainstARealChain(t *testing.T) { |
There was a problem hiding this comment.
Nit: this starts a real node and is not testing.Short()-gated, unlike TestValidTypeWalkIsExponential which skips in short mode.
…ebound Blocking under-count found by omarsy (codex review pass) at ce9afd9. fileImports kept a map[string]string with last-write-wins, so when two imports bind the same selector — `import ("heavy"; heavy "light")` — the guard kept "light". But go/types reports the redeclaration, binds the FIRST import, and still runs the delayed validType walk on it. So the guard priced light while the walk expanded heavy: the walk ran unpriced, then the package was rejected for the redeclaration — after the CPU was already spent, which is the exact thing this charge exists to prevent. Measured: an entry over heavy.T reached through a rebound selector was billed 12 nodes where the honest form billed 1,835,004 — the whole chain, unpriced. At depth 14 through AddPackage that is ~16s of CPU for ~40M gas, and one tx may carry the full Block.MaxGas. Fix, as omarsy suggested: fileImports returns map[string][]string and appends; cost()'s SelectorExpr takes the max namedCost over all candidates. That guarantees the charge covers whichever import go/types actually walks, without depending on which it picks — the same "keep all candidates, take the max" rule declsFor already applies to a duplicated type name. TestTypeExpansionCostDuplicateImportSelector pins it: honest and hijack declare the same depth-16 chain over the same heavy.T, hijack rebinding the selector to a light package. Verified to FAIL with last-write-wins restored (hijack drops to 12 while honest is 1,835,004) and pass with the max.
|
Priority note: this PR is now a stopgap, not urgent. Revising my "still needed" comment above; two premises changed.
The branch stays landable as the interim guard if we want one; otherwise I'd close it when the |
error and comparable are *types.Named in the Universe, so validType visits the Named and its underlying interface; unresolvedCost billed 1. Pinned against a replay of validType0 over every Universe type name. Also from omarsy's second review: BenchmarkValidTypeWalk gains an interface- embedding shape (converges with the struct chain at ~50 ns/node by depth 26, rate unchanged), the cyclic fixture pins its GAS USED width, TestDoSGapAgainstARealChain skips under -short, and a DeliverTx fixture covers the rebound import selector.
keeper_test.go: both sides appended tests, kept both. PackageContentHash now returns an error, so the inert tests go through mustContentHash. The cyclic fixture's GAS USED prefix moves 163 -> 169 with master's gas accounting (gnolang#6164).
… node (#6179) The two blocking findings from my review of #6148, as code, targeted at that PR's own branch so it can be merged in with one click and stays yours. +38 lines of production code, one new test file. Both tests fail against `tbruyelle/feat/gpao-state` and pass with the fix. Verified on the branch: `go test ./...`, `go test -race ./...`, and all seven `oracle/` e2e scenarios — including your own `resume` and `exhausted_purse`. ## 1. A clean shutdown recorded the block it had abandoned `verify()` already classifies parent cancellation correctly (`verifyone.go`): it returns `errVerifyBudget` wrapped in `"shutting down"`, explicitly *"not a verdict about this package"*, and `handleCandidate` files the package `pending` / "will be retried". `runVerifier` then ran `recordVerified` anyway. So the status board said *"will be retried"* about a package the cursor guaranteed would never be reached again: ``` package status = {Status:pending Reason:verification ran out of time; will be retried Attempt:1} recorded cursor = 5 ``` Up to one verify-budget wide per block, on every `systemctl restart gpao` — the workflow the cursor is for. The information was already there; it was just discarded one frame up. Two `ctx.Err()` checks, one inside the candidate loop and one after it. The second is not redundant: a block whose *last* package was the interrupted one has an empty remainder to iterate, so the in-loop guard never runs for it. Errs safe — a re-read is idempotent, and `handleCandidate` asks `isSettled` before paying for anything. ## 2. A catching-up node was read as a chain that had been reset `queryLatestHeight` read only `SyncInfo.LatestBlockHeight`. `SyncInfo.CatchingUp` sits next to it and was never consulted, so a replaying or fast-syncing node — RPC up, height climbing from wherever it restarted — gave `startHeight` a tip below the cursor, which is fatal: > the recorded cursor is at height 26021 but ... is only at 12: this state was written for a chain that has since been reset gpao brought up alongside its node, which is the systemd case in your description, would exit on that. With `Restart=always` it is a crash loop until the node passes the cursor; `StartLimitBurst` can end it for good. Same class `TestRunSurvivesTheBootRace` pins for a failing `Status` call — one startup RPC must not be fatal. Treated as silence, so the caller polls. That also reads correctly against `queryLatestHeight`'s existing contract: a tip that is moving for reasons of the node's own is not a chain height that has settled. The genuine reset case still errors, and the test asserts both halves. I put it in `queryLatestHeight` rather than in `startHeight` so the follow loop gets it too; if you would rather keep the follow loop chasing a replaying node, moving the check up is a small edit and I have no strong view. ## Not included The nil-`o.state` guard from finding 3. `recordVerified` and the `--max-spend` branch deref it unguarded while the package's tests construct bare `&oracle{...}` literals — #5826 adds one that survives only because it never reaches that branch. Left out because it is a judgement call about test ergonomics rather than a bug in this PR, and it is yours to make. Findings 4 (the stale `errAwaitingDependency` comment) and 5 (per-block `O_SYNC` write, the forever-retry on `res.Block == nil`) are also untouched. Merge, cherry-pick, or rewrite — whatever is least disruptive. If you would rather have these as review suggestions you can apply inline, say so and I will re-post them that way.
Charge for the unmetered, exponential
go/typesvalidTypewalk ataddpkg/MsgRunbefore it runs — a consensus DoS — and reject the syntax that makes pricing it unsound.validTypedoes not memoize visited types (commented out as a workaround for golang/go#65711), so a ~40-line "doubling" chain makes it visit ~2^41 nodes, unmetered and uninterruptible.Verified against master: that fixture wedges the node — it never answers, and after 60s
gnokeyfails withabci_query … context deadline exceededatbroadcast.go:245 - simulate tx. Note the read-only simulate path: no fee, no tx inclusion. Here the same input returnsout of gasin ~2s.Approach
typeExpansionCostcomputes the exact node countvalidTypewill visit, with the memoization it lacks (so computing it is linear), and the deploy path charges it toctx.GasMeter()at 100 gas/node — per package, before that package is walked.ConsumeGaspanics on out-of-gas andgo/typesre-panics non-bailoutvalues, so the abort lands before the remaining dependencies are walked; charging at the end would bill for CPU already spent.No ceiling. Any cap sits either below what a sender can pay, refusing packages that were paid for, or above it, where nothing is payable anyway and it only relabels an out-of-gas. It would also have to be per-package, a scope no budget has — whereas charging
ctx.GasMeter()makes the bound hold per transaction, which is what matters:Tx.Msgsis unbounded, and one message re-checks every dependency it imports, whose bytes earlier transactions paid for (measured: a 55-byte package pulls in 321,070 nodes against 68,750 gas of byte charges).Both syntactic rejections are preconditions of the pricing, not language policy: with no ceiling behind the charge, an under-counted edge is under-charging — the same DoS at a discount. So generics/type-sets and dot imports are refused rather than approximated, and unresolvable names are scored high (
leafExpansionBound) so they over-count.Calibration
1 gas == 1nsmeans reference hardware (Intel Xeon Platinum 8168, permachine.go'sOpCPU*table), so the rate needs two steps — an earlier revision shipped 25 by skipping the second, a ~4× under-charge.BenchmarkValidTypeWalkmeasures 30–40 ns/node on an Apple M5 (rising with depth as the working set outgrows cache; a DoS is the deep end), and rerunningcmd/calibrate'sBenchmarkAllocagainst its shipped Xeon output puts the Xeon 2.96× slower over 37 shared cases. 40 × ~2.5 = 100, range 88–128. The check that closes it: a whole block (MaxBlockMaxGas = 3e9) buys 3e7 nodes ≈ 3s ofvalidTypeon reference hardware.regexp.Regexp) →leafExpansionBound = 32.gnobuiltinsrealm/addressaddressis too common in the stdlib API to approximateAgainst the ~5e7
GasWanteda real deploy uses, 43k gas is not a meaningful tax. Three tests hold the wiring, and each fails if the meter is ever unwired again — verified by unwiring it. Two are in-process: one separates pointer-vs-value chains at equal declaration count by 736,055 gas, the other drives four messages through one ctx and one finite meter, where each fits the budget alone but only two land when they share it. Source bytes are held exactly equal there, so the expansion charge is provably the only difference. The third,addpkg_typecheck_fanout_multi_msg.txtar, proves the same per-transaction accumulation over the real path —gnokey sign+gnokey broadcaston a multi-message tx, sincemaketx addpkgonly builds single-msg ones: three depth-10 chains exceed an 8M budget that one identical package clears at 4.3M.Why not fork
go/typesand metervalidTypeinsideThe strongest alternative, and this repo already does it one layer up:
gnovm/pkg/parseris a fork ofgo/parser(5,460 lines) existing for itsParserCallbackmetering hook. It would also be better on soundness, since metering measures where this predicts — a metered fork deletescost(), both rejections' cost role,leafExpansionBound, the shim table, the parse cache, and the "must never under-count" invariant entirely.Rejected on size and shipping risk, not principle.
go/typesis 34,545 lines across 107 files, 6.3× the parser fork, and a type system rather than a token stream, so re-syncing per Go bump is qualitatively harder — which inverts "don't add a layer", since the fork is the larger layer. It would also make consensus-visible error text ours to keep byte-stable. On "go/typesis going away mid-term": this guard is ~450 lines whose lifetime is bounded bygo/types' own, so it is deleted rather than migrated — disposal is symmetric, and if removal slips the fork is the expensive thing to hold.Changed files
typecheck_cost.go(new) — cost model, rate, the two syntactic guards,expansionPkgCache, andexpansionGas, which clamps atMaxInt64becauseint64(math.MaxUint64)is-1and would otherwise refund gas for the worst package a sender could submit.gotypecheck.go—TypeCheckOptions.GasMeter, the three guards,memoizingGetter(store gas unchanged when guard and importer fetch the same package), sharedexpCache. Not computed at all without a meter, so off-chain callers skip resolving the dependency graph.sdk/vm/keeper.go—GasMeter: ctx.GasMeter()forAddPackageandRun.preprocess.go— both dot-import panics share the guard'serrDotImports; the two sites previously disagreed on capitalisation.go2gno.go—TODO(#6059)whereTypeParamsare dropped.AddPackagepath; andTestValidTypeWalkIsExponential, which proves the premise by running the unguarded walk in a subprocess that must be killed at its deadline while the metered path returns.gnovm/adr/pr5826_typecheck_dos_guards.md.Known limits (in the ADR, not fixed here)
gno test/gno lint/gnodev, so a pathological local package churns as long asgo builddoes. Chain-reachable code is bounded by what its deploy paid; CI jobs carrytimeout-minutes: 30.token.FileSetand on in-place AST mutations.go/typesdiagnostics, so such filetests pin two directives. Harness-only.Related
#5921 closed as duplicate; this PR is the sole carrier of
checkNoUncountableGenerics, which must sit beforego/types(verdict). #6059 is a different job: generics completeness belongs inGo2Gno, the traversal every consumer shares includinggno run, while this guard stays narrow. #5892 is complementary — it prices the linear case per source byte, which cannot catch the exponential one; it is stacked on #5891, which rewrites theGetMemPackageseam this resolver reads through, so re-verify on rebase. golang/go#65711 — ifvalidTypeis ever memoized upstream,TestValidTypeWalkIsExponentialfails and says to re-derive the rate.AI-assisted: developed with Claude Code; all measurements are reproducible from the committed tests and benchmarks.