From 975237fdc79cf704147865a6c7f0ecfbc70477aa Mon Sep 17 00:00:00 2001 From: Brian Ridings Date: Fri, 21 Aug 2026 12:05:15 -0400 Subject: [PATCH 1/5] Reject wrong-length addresses in Address.UnmarshalText --- codec/address.go | 12 ++++++-- codec/address_test.go | 65 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/codec/address.go b/codec/address.go index 474ee6cde3..3069688a57 100644 --- a/codec/address.go +++ b/codec/address.go @@ -48,8 +48,9 @@ func ToAddress(b []byte) (Address, error) { // StringToAddress returns Address with bytes set to the hex decoding // of s. -// StringToAddress uses copy, which copies the minimum of -// either AddressLen or the length of the hex decoded string. +// +// s must decode to exactly [AddressLen] bytes (plus the trailing checksum); +// any other length is rejected rather than being truncated or zero-padded. func StringToAddress(s string) (Address, error) { var a Address if err := a.UnmarshalText([]byte(s)); err != nil { @@ -75,6 +76,11 @@ func (a *Address) UnmarshalText(input []byte) error { return err } + // Check that the decoded address is the expected length + if len(decoded) != AddressLen { + return fmt.Errorf("%w: decoded address is %d bytes, expected %d", ErrInvalidSize, len(decoded), AddressLen) + } + copy(a[:], decoded) return nil } @@ -84,7 +90,7 @@ func encodeWithChecksum(bytes []byte) string { bytesLen := len(bytes) checked := make([]byte, bytesLen+checksumLen) copy(checked, bytes) - copy(checked[AddressLen:], hashing.Checksum(bytes, checksumLen)) + copy(checked[bytesLen:], hashing.Checksum(bytes, checksumLen)) return "0x" + hex.EncodeToString(checked) } diff --git a/codec/address_test.go b/codec/address_test.go index 16850e4863..14cab20508 100644 --- a/codec/address_test.go +++ b/codec/address_test.go @@ -127,3 +127,68 @@ func TestStringToAddress(t *testing.T) { }) } } + +// A valid checksum only proves the payload is intact, not that it is the right +// size. Without a length check, copy would truncate a longer payload or +// zero-pad a shorter one, so many distinct strings would decode to the same +// Address. +func TestStringToAddressRejectsWrongLength(t *testing.T) { + valid := CreateAddress(1, ids.GenerateTestID()) + + tests := []struct { + name string + payload []byte + }{ + { + name: "empty payload", + payload: []byte{}, + }, + { + name: "one byte short", + payload: valid[:AddressLen-1], + }, + { + name: "four byte payload", + payload: []byte{0x01, 0x02, 0x03, 0x04}, + }, + { + name: "one byte long", + payload: append(append([]byte{}, valid[:]...), 0xff), + }, + { + name: "valid address with a long suffix", + payload: append(append([]byte{}, valid[:]...), []byte("extra-suffix-bytes")...), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require := require.New(t) + + got, err := StringToAddress(encodeWithChecksum(tt.payload)) + require.ErrorIs(err, ErrInvalidSize) + require.Equal(EmptyAddress, got) + }) + } + + // the correct length still round-trips + got, err := StringToAddress(valid.String()) + require.NoError(t, err) + require.Equal(t, valid, got) +} + +// encodeWithChecksum must checksum over its argument, not over a fixed length. +func TestEncodeWithChecksumHonorsInputLength(t *testing.T) { + require := require.New(t) + + for _, size := range []int{0, 1, 16, AddressLen, AddressLen + 7} { + payload := make([]byte, size) + for i := range payload { + payload[i] = byte(i + 1) + } + + decoded, err := fromChecksum(encodeWithChecksum(payload)) + require.NoError(err, "size %d", size) + require.Equal(payload, decoded, "size %d", size) + } +} From 381533159aa257185a19d1523aced86823f5fc01 Mon Sep 17 00:00:00 2001 From: Brian Ridings Date: Fri, 21 Aug 2026 12:05:48 -0400 Subject: [PATCH 2/5] Bound action count in SimulateActions --- api/jsonrpc/server.go | 18 +++-- api/jsonrpc/server_test.go | 142 +++++++++++++++++++++++++++++++++++++ 2 files changed, 154 insertions(+), 6 deletions(-) create mode 100644 api/jsonrpc/server_test.go diff --git a/api/jsonrpc/server.go b/api/jsonrpc/server.go index d4cf69327a..93582c9344 100644 --- a/api/jsonrpc/server.go +++ b/api/jsonrpc/server.go @@ -262,6 +262,18 @@ func (j *JSONRPCServer) SimulateActions( ctx, span := j.vm.Tracer().Start(req.Context(), "JSONRPCServer.SimulateActions") defer span.End() + currentTime := time.Now().UnixMilli() + ruleFactory := j.vm.GetRuleFactory() + rules := ruleFactory.GetRules(currentTime) + + // Limit the number of actions to simulate to avoid DoS attacks + if len(args.Actions) == 0 { + return errSimulateZeroActions + } + if maxActionsPerTx := int(rules.GetMaxActionsPerTx()); len(args.Actions) > maxActionsPerTx { + return fmt.Errorf("exceeded max actions per simulation: %d", maxActionsPerTx) + } + txParser := j.vm.GetParser() actions := make([]chain.Action, 0, len(args.Actions)) for _, actionBytes := range args.Actions { @@ -271,9 +283,6 @@ func (j *JSONRPCServer) SimulateActions( } actions = append(actions, action) } - if len(actions) == 0 { - return errSimulateZeroActions - } currentState, err := j.vm.ImmutableState(ctx) if err != nil { return err @@ -287,9 +296,6 @@ func (j *JSONRPCServer) SimulateActions( 0, ) - currentTime := time.Now().UnixMilli() - ruleFactory := j.vm.GetRuleFactory() - rules := ruleFactory.GetRules(currentTime) for _, action := range actions { actionOutput, err := action.Execute( ctx, diff --git a/api/jsonrpc/server_test.go b/api/jsonrpc/server_test.go new file mode 100644 index 0000000000..10565a1875 --- /dev/null +++ b/api/jsonrpc/server_test.go @@ -0,0 +1,142 @@ +// Copyright (C) 2024, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package jsonrpc + +import ( + "context" + "net/http" + "testing" + + "github.com/ava-labs/avalanchego/trace" + "github.com/stretchr/testify/require" + + "github.com/ava-labs/hypersdk/api" + "github.com/ava-labs/hypersdk/chain" + "github.com/ava-labs/hypersdk/chain/chaintest" + "github.com/ava-labs/hypersdk/codec" + "github.com/ava-labs/hypersdk/genesis" + "github.com/ava-labs/hypersdk/state" +) + +// stubVM implements only the methods the simulation endpoints touch. Any other +// call panics on the embedded nil interface, which keeps the stub honest. +type stubVM struct { + api.VM + + ruleFactory chain.RuleFactory + parsed int +} + +func (*stubVM) Tracer() trace.Tracer { return trace.Noop } + +func (s *stubVM) GetRuleFactory() chain.RuleFactory { return s.ruleFactory } + +func (s *stubVM) GetParser() chain.Parser { + s.parsed++ + return chaintest.NewTestParser() +} + +func (*stubVM) ImmutableState(context.Context) (state.Immutable, error) { + return state.ImmutableStorage(map[string][]byte{}), nil +} + +func (*stubVM) ReadState(_ context.Context, keys [][]byte) ([][]byte, []error) { + return make([][]byte, len(keys)), make([]error, len(keys)) +} + +func newTestServer(rules chain.Rules) (*JSONRPCServer, *stubVM) { + v := &stubVM{ruleFactory: &genesis.ImmutableRuleFactory{Rules: rules}} + return NewJSONRPCServer(v), v +} + +func dummyActionBytes(t *testing.T) []byte { + t.Helper() + + return chaintest.NewDummyTestAction().Bytes() +} + +// SimulateActions is unauthenticated and unmetered, so it must bound the action +// count the same way ExecuteActions does. Without this, one request could ask +// the node to parse and execute an arbitrarily long action list against live +// state. +func TestSimulateActionsBoundsActionCount(t *testing.T) { + rules := genesis.NewDefaultRules() + maxActions := int(rules.GetMaxActionsPerTx()) + + t.Run("over the limit is rejected before parsing", func(t *testing.T) { + r := require.New(t) + + server, v := newTestServer(rules) + + actionBytes := dummyActionBytes(t) + actions := make([]codec.Bytes, maxActions+1) + for i := range actions { + actions[i] = actionBytes + } + + err := server.SimulateActions( + &http.Request{}, + &SimulatActionsArgs{Actions: actions}, + &SimulateActionsReply{}, + ) + r.ErrorContains(err, "exceeded max actions per simulation") + r.Zero(v.parsed, "the request should be rejected before any action is parsed") + }) + + t.Run("zero actions is rejected", func(t *testing.T) { + r := require.New(t) + + server, _ := newTestServer(rules) + + err := server.SimulateActions( + &http.Request{}, + &SimulatActionsArgs{}, + &SimulateActionsReply{}, + ) + r.ErrorIs(err, errSimulateZeroActions) + }) + + t.Run("at the limit is accepted", func(t *testing.T) { + r := require.New(t) + + server, _ := newTestServer(rules) + + actionBytes := dummyActionBytes(t) + actions := make([]codec.Bytes, maxActions) + for i := range actions { + actions[i] = actionBytes + } + + reply := &SimulateActionsReply{} + err := server.SimulateActions( + &http.Request{}, + &SimulatActionsArgs{Actions: actions}, + reply, + ) + r.NoError(err) + r.Len(reply.ActionResults, maxActions) + }) +} + +// ExecuteActions already had this bound; keep it covered so the two endpoints +// cannot drift apart again. +func TestExecuteActionsBoundsActionCount(t *testing.T) { + r := require.New(t) + + rules := genesis.NewDefaultRules() + server, _ := newTestServer(rules) + + actionBytes := dummyActionBytes(t) + actions := make([][]byte, int(rules.GetMaxActionsPerTx())+1) + for i := range actions { + actions[i] = actionBytes + } + + err := server.ExecuteActions( + &http.Request{}, + &ExecuteActionArgs{Actions: actions}, + &ExecuteActionReply{}, + ) + r.ErrorContains(err, "exceeded max actions per simulation") +} From 34b9fb6e0d16f6c9cf90676a969c4b07c76e32ef Mon Sep 17 00:00:00 2001 From: Brian Ridings Date: Fri, 21 Aug 2026 12:06:13 -0400 Subject: [PATCH 3/5] Compute fee price update on a 128-bit intermediate to avoid overflow --- internal/fees/manager.go | 27 +++-- internal/fees/manager_test.go | 220 ++++++++++++++++++++++++++++++++++ 2 files changed, 240 insertions(+), 7 deletions(-) diff --git a/internal/fees/manager.go b/internal/fees/manager.go index 27681e2119..cd36460fd7 100644 --- a/internal/fees/manager.go +++ b/internal/fees/manager.go @@ -5,6 +5,7 @@ package fees import ( "encoding/binary" + "math/bits" "sync" "github.com/ava-labs/avalanchego/utils/math" @@ -200,6 +201,16 @@ func (f *Manager) UnitsConsumed() fees.Dimensions { return d } +// mulDiv returns floor(a*b/d). +func mulDiv(a, b, d uint64) uint64 { + hi, lo := bits.Mul64(a, b) + if hi >= d { + return consts.MaxUint64 + } + q, _ := bits.Div64(hi, lo, d) + return q +} + func computeNextPriceWindow( previous window.Window, previousConsumed uint64, @@ -224,9 +235,7 @@ func computeNextPriceWindow( if total > target { // If the parent block used more units than its target, the baseFee should increase. delta := total - target - x := previousPrice * delta - y := x / target - baseDelta := y / changeDenom + baseDelta := mulDiv(previousPrice, delta, target) / changeDenom if baseDelta < 1 { baseDelta = 1 } @@ -239,9 +248,7 @@ func computeNextPriceWindow( } else if total < target { // Otherwise if the parent block used less units than its target, the baseFee should decrease. delta := target - total - x := previousPrice * delta - y := x / target - baseDelta := y / changeDenom + baseDelta := mulDiv(previousPrice, delta, target) / changeDenom if baseDelta < 1 { baseDelta = 1 } @@ -252,7 +259,13 @@ func computeNextPriceWindow( // that has elapsed between the parent and this block. if since > window.WindowSize { // Note: roll/rollupWindow must be greater than 1 since we've checked that roll > rollupWindow - baseDelta *= since / window.WindowSize + scaled, over := math.Mul(baseDelta, since/window.WindowSize) + if over != nil { + // Saturate: an unbounded decrease is clamped to [minPrice] below. + baseDelta = consts.MaxUint64 + } else { + baseDelta = scaled + } } n, under := math.Sub(nextPrice, baseDelta) if under != nil { diff --git a/internal/fees/manager_test.go b/internal/fees/manager_test.go index 99b66a3532..fe02429921 100644 --- a/internal/fees/manager_test.go +++ b/internal/fees/manager_test.go @@ -4,11 +4,14 @@ package fees import ( + "math/big" + "math/rand" "testing" "github.com/stretchr/testify/require" "github.com/ava-labs/hypersdk/fees" + "github.com/ava-labs/hypersdk/internal/window" ) func TestUnitsConsumed(t *testing.T) { @@ -104,3 +107,220 @@ func TestUnitsConsumed(t *testing.T) { }) } } + +// mulDiv must compute floor(a*b/d) exactly whenever the quotient is +// representable, even when the intermediate product exceeds 64 bits, and +// saturate rather than wrap or panic otherwise. +func TestMulDiv(t *testing.T) { + maxU64 := ^uint64(0) + + tests := []struct { + name string + a, b, d uint64 + want uint64 + }{ + { + name: "no overflow", + a: 7, b: 6, d: 4, + want: 10, // floor(42/4) + }, + { + name: "zero numerator", + a: 0, b: 12345, d: 7, + want: 0, + }, + { + // the fee-price case that used to wrap: the product is ~3.3e19 but + // the quotient fits comfortably + name: "product exceeds 64 bits, quotient fits", + a: 359191338483402, b: 93000, d: 1000, + want: 33404794478956386, + }, + { + name: "quotient does not fit, saturates", + a: 1 << 63, b: 1 << 63, d: 1, + want: maxU64, + }, + { + name: "max inputs", + a: maxU64, b: maxU64, d: maxU64, + want: maxU64, + }, + { + name: "zero divisor saturates instead of panicking", + a: 12345, b: 6789, d: 0, + want: maxU64, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + require.Equal(t, tt.want, mulDiv(tt.a, tt.b, tt.d)) + }) + } +} + +type testFeeRules struct { + min, denom, target, max fees.Dimensions +} + +func (r testFeeRules) GetMinUnitPrice() fees.Dimensions { return r.min } +func (r testFeeRules) GetUnitPriceChangeDenominator() fees.Dimensions { return r.denom } +func (r testFeeRules) GetWindowTargetUnits() fees.Dimensions { return r.target } +func (r testFeeRules) GetMaxBlockUnits() fees.Dimensions { return r.max } + +// Under sustained over-target load the unit price must rise monotonically until +// it saturates. Before mulDiv, previousPrice*delta wrapped once the price grew +// past roughly 2^44, which made the price update erratic and left congestion +// pricing under-responding by several multiples. +func TestComputeNextPriceNoOverflowUnderSustainedLoad(t *testing.T) { + require := require.New(t) + + // hypersdk default rules (genesis.NewDefaultRules) + r := testFeeRules{ + min: fees.Dimensions{100, 100, 100, 100, 100}, + denom: fees.Dimensions{48, 48, 48, 48, 48}, + target: fees.Dimensions{20_000_000, 1_000, 1_000, 1_000, 1_000}, + max: fees.Dimensions{1_800_000, 2_000, 2_000, 2_000, 2_000}, + } + const dim = fees.Compute + + m := NewManager(nil) + for i := fees.Dimension(0); i < fees.FeeDimensions; i++ { + m.SetUnitPrice(i, r.min[i]) + } + + // a block every MinBlockGap (100ms), each consuming the full block limit + var ( + tsMs = int64(0) + prev = m.UnitPrice(dim) + ) + for block := 1; block <= 500; block++ { + m.SetLastConsumed(dim, r.max[dim]) + tsMs += 100 + m = m.ComputeNext(tsMs, r) + + price := m.UnitPrice(dim) + require.GreaterOrEqualf(price, prev, + "price decreased at block %d (%d -> %d) under sustained over-target load", block, prev, price) + prev = price + } + + // sustained saturation should have driven the price to its ceiling + require.Equal(^uint64(0), prev) +} + +var refMaxU64 = new(big.Int).SetUint64(^uint64(0)) + +func clampToU64(v *big.Int) uint64 { + if v.Cmp(refMaxU64) > 0 { + return ^uint64(0) + } + if v.Sign() < 0 { + return 0 + } + return v.Uint64() +} + +// refNextPrice models computeNextPriceWindow's price arithmetic exactly, using +// big.Int for the intermediate and saturating only where the implementation +// documents that it does. +func refNextPrice(prevPrice, total, target, changeDenom, minPrice, since uint64) uint64 { + bi := func(v uint64) *big.Int { return new(big.Int).SetUint64(v) } + + baseDelta := func(delta uint64) uint64 { + y := new(big.Int).Div(new(big.Int).Mul(bi(prevPrice), bi(delta)), bi(target)) + bd := clampToU64(y) / changeDenom + if bd < 1 { + bd = 1 + } + return bd + } + + next := prevPrice + switch { + case total > target: + next = clampToU64(new(big.Int).Add(bi(prevPrice), bi(baseDelta(total-target)))) + case total < target: + bd := baseDelta(target - total) + if since > window.WindowSize { + bd = clampToU64(new(big.Int).Mul(bi(bd), bi(since/window.WindowSize))) + } + next = clampToU64(new(big.Int).Sub(bi(prevPrice), bi(bd))) + } + if next < minPrice { + next = minPrice + } + return next +} + +// Differential test across both the increase and decrease branches, including +// the idle-decay scaling path. Guards against the intermediate product wrapping +// at 2^64, which previously made the price update diverge from its intent by +// several multiples once unit prices grew past roughly 2^44. +func TestComputeNextPriceWindowMatchesExactReference(t *testing.T) { + require := require.New(t) + rng := rand.New(rand.NewSource(20260820)) //nolint:gosec // deterministic test input + + var ( + prices = []uint64{0, 1, 100, 1 << 20, 1 << 40, 1 << 44, 1 << 50, 1 << 62, ^uint64(0)} + targets = []uint64{1, 1000, 20_000_000, 1 << 40} + denoms = []uint64{1, 2, 48, 1000} + sinces = []uint64{0, 1, 5, 9, 10, 11, 100, 10_000, 1 << 40} + ) + + const minPrice = 100 + var increase, decrease int + + for _, prevPrice := range prices { + for _, target := range targets { + for _, denom := range denoms { + for _, since := range sinces { + for trial := 0; trial < 6; trial++ { + var w window.Window + for i := 0; i < window.WindowSize; i++ { + var v uint64 + switch trial { + case 0: + v = 0 + case 1: + v = target / uint64(window.WindowSize) + case 2: + v = ^uint64(0) / uint64(window.WindowSize) + default: + v = rng.Uint64() % (2*target + 1) + } + window.Update(&w, i*8, v) + } + prevConsumed := rng.Uint64() % (2*target + 1) + + gotPrice, gotWindow := computeNextPriceWindow( + w, prevConsumed, prevPrice, target, denom, minPrice, since, + ) + + // window.Roll/Update/Sum are unchanged by this fix, so + // deriving the observed total with them is sound + total := window.Sum(gotWindow) + switch { + case total > target: + increase++ + case total < target: + decrease++ + } + + require.Equalf( + refNextPrice(prevPrice, total, target, denom, minPrice, since), + gotPrice, + "prevPrice=%d total=%d target=%d denom=%d since=%d", + prevPrice, total, target, denom, since, + ) + } + } + } + } + } + + // make sure the table actually exercised both branches + require.Positive(increase) + require.Positive(decrease) +} From 259322f73a6ff3bad1d012ed99e80832491eec2b Mon Sep 17 00:00:00 2001 From: Brian Ridings Date: Fri, 21 Aug 2026 12:06:20 -0400 Subject: [PATCH 4/5] Bound WebSocket tx listeners to the validity window --- api/ws/server.go | 43 ++++++++++- api/ws/server_test.go | 168 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 207 insertions(+), 4 deletions(-) create mode 100644 api/ws/server_test.go diff --git a/api/ws/server.go b/api/ws/server.go index ee7b01b174..f52d365809 100644 --- a/api/ws/server.go +++ b/api/ws/server.go @@ -5,7 +5,9 @@ package ws import ( "context" + "errors" "sync" + "time" "github.com/ava-labs/avalanchego/ids" "github.com/ava-labs/avalanchego/trace" @@ -124,8 +126,8 @@ func NewWebSocketServer( return w, w.s } -// Note: no need to have a tx listener removal, this will happen when all -// submitted transactions are cleared. +// AddTxListener registers [c] to be notified about the result of [tx]. +// Note: cleanup relies on expiry eviction; never register a tx expiring past the validity window. func (w *WebSocketServer) AddTxListener(tx *chain.Transaction, c *pubsub.Connection) { w.txL.Lock() defer w.txL.Unlock() @@ -141,6 +143,22 @@ func (w *WebSocketServer) AddTxListener(tx *chain.Transaction, c *pubsub.Connect w.expiringTxs.Add([]*chain.Transaction{tx}) } +// RemoveTxListener unregisters [c] from the listeners for [txID], dropping the +// entry entirely once no connection is waiting on it. +func (w *WebSocketServer) RemoveTxListener(txID ids.ID, c *pubsub.Connection) { + w.txL.Lock() + defer w.txL.Unlock() + + connections, ok := w.txListeners[txID] + if !ok { + return + } + connections.Remove(c) + if connections.Len() == 0 { + delete(w.txListeners, txID) + } +} + func (w *WebSocketServer) expireTx(txID ids.ID) { listeners, ok := w.txListeners[txID] if !ok { @@ -228,11 +246,28 @@ func (w *WebSocketServer) MessageCallback() pubsub.Callback { return } + txID := tx.GetID() + + // Drop txs expiring past the validity window + now := time.Now().UnixMilli() + rules := w.vm.GetRuleFactory().GetRules(now) + if tx.Base.Timestamp > now+rules.GetValidityWindow() { + w.logger.Debug("dropping tx with expiry beyond the validity window", + zap.Stringer("txID", txID), + zap.Int64("timestamp", tx.Base.Timestamp), + ) + return + } + + // Registered before Submit so that a tx accepted into a block + // cannot be reported before the listener exists. w.AddTxListener(tx, c) - // Submit will remove from [txListeners] if it is not added - txID := tx.GetID() if err := w.vm.Submit(ctx, []*chain.Transaction{tx})[0]; err != nil { + // If the tx is already in the mempool, the listener must be retained + if !errors.Is(err, vm.ErrNotAdded) { + w.RemoveTxListener(txID, c) + } w.logger.Error("failed to submit tx", zap.Stringer("txID", txID), zap.Error(err), diff --git a/api/ws/server_test.go b/api/ws/server_test.go new file mode 100644 index 0000000000..49a1f0841e --- /dev/null +++ b/api/ws/server_test.go @@ -0,0 +1,168 @@ +// Copyright (C) 2024, Ava Labs, Inc. All rights reserved. +// See the file LICENSE for licensing terms. + +package ws + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/ava-labs/avalanchego/trace" + "github.com/ava-labs/avalanchego/utils/logging" + "github.com/stretchr/testify/require" + + "github.com/ava-labs/hypersdk/api" + "github.com/ava-labs/hypersdk/chain" + "github.com/ava-labs/hypersdk/chain/chaintest" + "github.com/ava-labs/hypersdk/genesis" + "github.com/ava-labs/hypersdk/vm" +) + +// stubVM implements only the methods the tx message path touches. Any other +// call panics on the embedded nil interface, which keeps the stub honest. +type stubVM struct { + api.VM + + ruleFactory chain.RuleFactory + submitErr error + submitted int +} + +func (*stubVM) Tracer() trace.Tracer { return trace.Noop } + +func (s *stubVM) GetRuleFactory() chain.RuleFactory { return s.ruleFactory } + +func (s *stubVM) Submit(context.Context, []*chain.Transaction) []error { + s.submitted++ + return []error{s.submitErr} +} + +func newTestWebSocketServer(t *testing.T, submitErr error) (*WebSocketServer, *stubVM) { + t.Helper() + + rules := genesis.NewDefaultRules() + v := &stubVM{ + ruleFactory: &genesis.ImmutableRuleFactory{Rules: rules}, + submitErr: submitErr, + } + // a tiny pending-message queue: this test never publishes, and the default + // would allocate hundreds of MiB per connection + w, _ := NewWebSocketServer(v, logging.NoLog{}, trace.Noop, chaintest.NewTestParser(), 8) + return w, v +} + +func newTestTx(t *testing.T, timestamp int64) *chain.Transaction { + t.Helper() + + tx, err := chain.NewTransaction( + chain.Base{Timestamp: timestamp}, + []chain.Action{chaintest.NewDummyTestAction()}, + chaintest.NewDummyTestAuth(), + ) + require.NoError(t, err) + return tx +} + +// encodeTxMessage frames a tx the way api/ws clients do. +func encodeTxMessage(tx *chain.Transaction) []byte { + return append([]byte{TxMode}, tx.Bytes()...) +} + +func (w *WebSocketServer) listenerCount() int { + w.txL.Lock() + defer w.txL.Unlock() + + return len(w.txListeners) +} + +// A tx that fails Submit should release its listener immediately rather than +// holding it until its expiry passes. +func TestMessageCallbackRemovesListenerWhenSubmitFails(t *testing.T) { + r := require.New(t) + + w, v := newTestWebSocketServer(t, errors.New("invalid signature")) + callback := w.MessageCallback() + + // within the validity window, so it reaches Submit + tx := newTestTx(t, time.Now().UnixMilli()+1_000) + callback(encodeTxMessage(tx), nil) + + r.Equal(1, v.submitted, "tx should have reached Submit") + r.Zero(w.listenerCount(), "listener must be removed after Submit fails") +} + +// ErrNotAdded means the tx is already pending in the mempool, so the listener +// has to stay or the client would never be told the outcome. +func TestMessageCallbackKeepsListenerWhenAlreadyInMempool(t *testing.T) { + r := require.New(t) + + w, v := newTestWebSocketServer(t, vm.ErrNotAdded) + callback := w.MessageCallback() + + tx := newTestTx(t, time.Now().UnixMilli()+1_000) + callback(encodeTxMessage(tx), nil) + + r.Equal(1, v.submitted) + r.Equal(1, w.listenerCount(), "listener must be retained for a tx already in the mempool") +} + +// A tx whose expiry is past the validity window can never be executed and can +// never be evicted by setMinTx, so it must be rejected before it is recorded. +// This is the check that actually bounds txListeners: without it a listener is +// retained for the lifetime of the process. +func TestMessageCallbackRejectsExpiryBeyondValidityWindow(t *testing.T) { + r := require.New(t) + + w, v := newTestWebSocketServer(t, nil) + callback := w.MessageCallback() + + rules := genesis.NewDefaultRules() + beyond := time.Now().UnixMilli() + rules.GetValidityWindow() + 10_000 + tx := newTestTx(t, beyond) + callback(encodeTxMessage(tx), nil) + + r.Zero(v.submitted, "tx beyond the validity window should not reach Submit") + r.Zero(w.listenerCount(), "no listener should be recorded") + + // Confirm the eviction path really could not have cleared it: advancing the + // expiry horizon to now leaves a far-future entry in place. + w.setMinTx(time.Now().UnixMilli()) + r.Zero(w.listenerCount()) +} + +// A successfully submitted tx keeps its listener, and it is evictable because +// its expiry is inside the validity window. +func TestMessageCallbackListenerIsEvictable(t *testing.T) { + r := require.New(t) + + w, _ := newTestWebSocketServer(t, nil) + callback := w.MessageCallback() + + expiry := time.Now().UnixMilli() + 1_000 + tx := newTestTx(t, expiry) + callback(encodeTxMessage(tx), nil) + r.Equal(1, w.listenerCount()) + + // once the chain advances past the tx expiry, the listener is cleared + w.setMinTx(expiry + 1) + r.Zero(w.listenerCount(), "listener should be evicted once its expiry passes") +} + +func TestRemoveTxListener(t *testing.T) { + r := require.New(t) + + w, _ := newTestWebSocketServer(t, nil) + tx := newTestTx(t, time.Now().UnixMilli()+1_000) + + w.AddTxListener(tx, nil) + r.Equal(1, w.listenerCount()) + + w.RemoveTxListener(tx.GetID(), nil) + r.Zero(w.listenerCount()) + + // removing an unknown txID is a no-op + w.RemoveTxListener(tx.GetID(), nil) + r.Zero(w.listenerCount()) +} From 30eaddb2e0b7ff219aa2dfafdc788d368edbc2a6 Mon Sep 17 00:00:00 2001 From: Brian Ridings Date: Fri, 21 Aug 2026 12:11:01 -0400 Subject: [PATCH 5/5] Enforce the signed MaxFee cap when charging transaction fees --- chain/errors.go | 1 + chain/pre_executor_test.go | 1 + chain/processor_test.go | 1 + chain/transaction.go | 17 +++++++-- chain/transaction_test.go | 75 +++++++++++++++++++++++++++++++++++++- vm/vm_test.go | 10 ++--- 6 files changed, 96 insertions(+), 9 deletions(-) diff --git a/chain/errors.go b/chain/errors.go index 14acd80727..b20d1e66a0 100644 --- a/chain/errors.go +++ b/chain/errors.go @@ -51,6 +51,7 @@ var ( ErrInvalidActor = errors.New("invalid actor") ErrInvalidSponsor = errors.New("invalid sponsor") ErrTooManyActions = errors.New("too many actions") + ErrFeeExceedsMaxFee = errors.New("fee exceeds max fee") // Execution Correctness ErrBlockTooBig = errors.New("block too big") diff --git a/chain/pre_executor_test.go b/chain/pre_executor_test.go index 06ceeab8df..1803c174bf 100644 --- a/chain/pre_executor_test.go +++ b/chain/pre_executor_test.go @@ -43,6 +43,7 @@ func TestPreExecutor(t *testing.T) { time.Now().UnixMilli(), testRules.GetValidityWindow(), ), + MaxFee: math.MaxUint64, }, }, Auth: chaintest.NewDummyTestAuth(), diff --git a/chain/processor_test.go b/chain/processor_test.go index 70e2950050..c5851985d8 100644 --- a/chain/processor_test.go +++ b/chain/processor_test.go @@ -456,6 +456,7 @@ func TestProcessorExecute(t *testing.T) { testRules.GetMinEmptyBlockGap(), testRules.GetValidityWindow(), ), + MaxFee: math.MaxUint64, }, []chain.Action{}, &auth.ED25519{ diff --git a/chain/transaction.go b/chain/transaction.go index 7460889884..35b7a08a91 100644 --- a/chain/transaction.go +++ b/chain/transaction.go @@ -279,6 +279,11 @@ func (t *Transaction) PreExecute( if err != nil { return err } + + // Check that the calculated fee does not exceed the maximum allowed fee + if fee > t.Base.MaxFee { + return fmt.Errorf("%w: fee (%d) > MaxFee (%d)", ErrFeeExceedsMaxFee, fee, t.Base.MaxFee) + } return bh.CanDeduct(ctx, t.Auth.Sponsor(), im, fee) } @@ -431,10 +436,16 @@ func EstimateUnits(r Rules, actions []Action, authFactory AuthFactory) (fees.Dim ) // Calculate over action/auth + // + // Bandwidth must account for the canoto framing that [SerializeTx] adds + // around each field - a tag plus a length prefix per action and for the + // auth - and not just the payload lengths. Under-counting here would make + // the MaxFee derived from this estimate smaller than the fee derived from + // [Transaction.Units], so a transaction built by [GenerateTransaction] + // would be rejected by its own fee cap. bandwidth += consts.Uint8Len for i, action := range actions { actionBytes := action.Bytes() - actionSize := len(actionBytes) actor := authFactory.Address() stateKeys := action.StateKeys(actor, CreateActionID(ids.Empty, uint8(i))) @@ -442,12 +453,12 @@ func EstimateUnits(r Rules, actions []Action, authFactory AuthFactory) (fees.Dim if !ok { return fees.Dimensions{}, ErrInvalidKeyValue } - bandwidth += uint64(actionSize) + bandwidth += uint64(len(canoto__SerializeTx__Actions__tag)) + canoto.SizeBytes(actionBytes) stateKeysMaxChunks = append(stateKeysMaxChunks, actionStateKeysMaxChunks...) computeOp.Add(action.ComputeUnits(r)) } authBandwidth, authCompute := authFactory.MaxUnits() - bandwidth += authBandwidth + bandwidth += uint64(len(canoto__SerializeTx__Auth__tag)) + canoto.SizeUint(authBandwidth) + authBandwidth sponsorStateKeyMaxChunks := r.GetSponsorStateKeysMaxChunks() stateKeysMaxChunks = append(stateKeysMaxChunks, sponsorStateKeyMaxChunks...) computeOp.Add(authCompute) diff --git a/chain/transaction_test.go b/chain/transaction_test.go index 9a95e07eab..6f09e3f0f0 100644 --- a/chain/transaction_test.go +++ b/chain/transaction_test.go @@ -7,6 +7,7 @@ import ( "context" "encoding/hex" "encoding/json" + "fmt" "testing" "github.com/ava-labs/avalanchego/ids" @@ -391,11 +392,32 @@ func TestPreExecute(t *testing.T) { }(), err: safemath.ErrOverflow, }, + { + name: "fee exceeds max fee", + tx: &chain.Transaction{ + TransactionData: chain.TransactionData{ + // MaxFee of 1 cannot cover the fee computed at a unit price of 1. + Base: chain.Base{MaxFee: 1}, + Actions: []chain.Action{ + chaintest.NewDummyTestAction(), + }, + }, + Auth: chaintest.NewDummyTestAuth(), + }, + fm: func() *fees.Manager { + fm := fees.NewManager([]byte{}) + for i := 0; i < externalfees.FeeDimensions; i++ { + fm.SetUnitPrice(externalfees.Dimension(i), 1) + } + return fm + }(), + err: chain.ErrFeeExceedsMaxFee, + }, { name: "insufficient balance", tx: &chain.Transaction{ TransactionData: chain.TransactionData{ - Base: chain.Base{}, + Base: chain.Base{MaxFee: consts.MaxUint64}, Actions: []chain.Action{ chaintest.NewDummyTestAction(), }, @@ -435,3 +457,54 @@ func TestPreExecute(t *testing.T) { }) } } + +// The MaxFee check in PreExecute is only safe if the MaxFee that +// GenerateTransaction derives from EstimateUnits always covers the fee derived +// from the real Units at the same prices. EstimateUnits previously omitted the +// per-action canoto framing, so the margin shrank by two bandwidth units per +// action and went negative past ~37 actions - transactions the SDK itself built +// would have been rejected by their own fee cap. +func TestEstimateUnitsCoversActualUnits(t *testing.T) { + bh := balance.NewPrefixBalanceHandler([]byte{0}) + authFactory := &chaintest.TestAuthFactory{TestAuth: chaintest.NewDummyTestAuth()} + + priceSets := []externalfees.Dimensions{ + {1, 1, 1, 1, 1}, + {100, 100, 100, 100, 100}, + {7, 13, 29, 31, 37}, + } + + // walk the full range MaxActionsPerTx can express, not just the default + for _, maxActions := range []uint8{1, 16, 37, 38, 64, 255} { + t.Run(fmt.Sprintf("maxActions=%d", maxActions), func(t *testing.T) { + r := require.New(t) + + rules := genesis.NewDefaultRules() + rules.MaxActionsPerTx = maxActions + ruleFactory := &genesis.ImmutableRuleFactory{Rules: rules} + + actions := make([]chain.Action, maxActions) + for i := range actions { + actions[i] = chaintest.NewDummyTestAction() + } + + for _, prices := range priceSets { + tx, err := chain.GenerateTransaction(ruleFactory, prices, 0, actions, authFactory) + r.NoError(err) + + fm := fees.NewManager(nil) + for i := externalfees.Dimension(0); i < externalfees.FeeDimensions; i++ { + fm.SetUnitPrice(i, prices[i]) + } + units, err := tx.Units(bh, rules) + r.NoError(err) + actualFee, err := fm.Fee(units) + r.NoError(err) + + r.GreaterOrEqualf(tx.Base.MaxFee, actualFee, + "GenerateTransaction produced MaxFee %d below the actual fee %d (%d actions, prices %v)", + tx.Base.MaxFee, actualFee, len(actions), prices) + } + }) + } +} diff --git a/vm/vm_test.go b/vm/vm_test.go index c667a4ddb6..0eb04f64ce 100644 --- a/vm/vm_test.go +++ b/vm/vm_test.go @@ -217,7 +217,7 @@ func TestSubmitTx(t *testing.T) { chain.Base{ ChainID: network.ChainID(), Timestamp: utils.UnixRMilli(time.Now().UnixMilli(), 1_000), - MaxFee: 1_000, + MaxFee: 1_000_000, }, []chain.Action{ chaintest.NewDummyTestAction(), @@ -236,7 +236,7 @@ func TestSubmitTx(t *testing.T) { chain.Base{ ChainID: network.ChainID(), Timestamp: 1, - MaxFee: 1_000, + MaxFee: 1_000_000, }, []chain.Action{ chaintest.NewDummyTestAction(), @@ -255,7 +255,7 @@ func TestSubmitTx(t *testing.T) { chain.Base{ ChainID: network.ChainID(), Timestamp: int64(time.Millisecond), - MaxFee: 1_000, + MaxFee: 1_000_000, }, []chain.Action{ chaintest.NewDummyTestAction(), @@ -274,7 +274,7 @@ func TestSubmitTx(t *testing.T) { chain.Base{ ChainID: network.ChainID(), Timestamp: utils.UnixRMilli(time.Now().UnixMilli(), time.Hour.Milliseconds()), - MaxFee: 1_000, + MaxFee: 1_000_000, }, []chain.Action{ chaintest.NewDummyTestAction(), @@ -293,7 +293,7 @@ func TestSubmitTx(t *testing.T) { chain.Base{ ChainID: network.ChainID(), Timestamp: utils.UnixRMilli(time.Now().UnixMilli(), 30_000), - MaxFee: 1_000, + MaxFee: 1_000_000, }, []chain.Action{ chaintest.NewDummyTestAction(),