Skip to content

Commit 43f5b6e

Browse files
AkramBitarEffi-S
authored andcommitted
fix(selector): correct the token retrieval limit and restore contention behaviour
Follow-up to the token retrieval/lock limit work, addressing review findings. Blocking: - The limited UnspentTokensIteratorBy query appended a literal "?" placeholder while every other parameter is emitted as $N by the query builder. That is a syntax error on PostgreSQL (SQLSTATE 42601), and simple/selector.go always passes a non-zero limit, so every token selection failed on a Postgres-backed node. Build the LIMIT through the builder instead. It only worked on SQLite because SQLite assigns $1-style names sequential indices. - StubbornSelector.Select no longer released its partial locks before backing off, so two selections each holding part of the funds both exhausted their retry budget and both reported insufficient funds while funds were available. Restore the release, which also makes the surrounding log message true again. - The row limit was applied before the Go-side dedup. A directly-owned token matches both UNION branches, so a limit of N surfaced about N/2 distinct tokens and the selector read that as an empty wallet. Use UNION on the limited path so LIMIT counts distinct rows. Also: - Guard the selectMu drain on the default path: StubbornSelector overrides Select, so Close() could still close the iterator mid-iteration. - Treat a non-positive selectionTimeout as "no timeout" rather than an already-expired context. - Apply the token iteration budget per retry cycle; the cumulative counter made the effective budget maxTokensPerSelection/maxRetries and masked the typed failure reason. A full page with insufficient funds now reports the limit instead of retrying a query that cannot change. - Report the in-memory locker's per-transaction lock ceiling as SelectorRateLimited so the selection fails fast instead of looking like contention. - Stop lock cleanup from resetting live per-transaction counters, which made the ceiling per-tick rather than per-transaction, and reclaim counters on every replica instead of only the cleanup leader. - Return a usable Config when the selector config fails to parse, and actually fall back to defaults when validation rejects it; validate in sherdlock too. - Derive the default selectionTimeout so it outlasts the default retry budget. - Keep lib-p2p-bootstrap-node out of TMS membership in the fungible topology, as the nft/interop/mixed topologies already do. Restores the two test relaxations that the regressions above required, and adds regression coverage for each fixed path, including a real-database exercise of the limited query that runs on both SQLite and PostgreSQL. Signed-off-by: AkramBitar <akram@il.ibm.com> Signed-off-by: Effi-S <effi.szt@gmail.com>
1 parent 8832708 commit 43f5b6e

24 files changed

Lines changed: 1129 additions & 70 deletions

docs/security/selector_resource_limits.md

Lines changed: 29 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,16 @@ token:
127127

128128
**When it triggers**: When a transaction tries to acquire more locks than configured
129129

130-
**Error message**: `"lock limit exceeded: transaction TX already holds 5000 locks (max: 5000)"`
130+
**Error message**: `"lock limit exceeded: transaction TX already holds 5000 locks (max: 5000)"`,
131+
wrapping `token.SelectorRateLimited` so the selection aborts immediately instead
132+
of treating the denial as contention and retrying.
133+
134+
The count is tracked per transaction for the whole life of that transaction: it
135+
is released when the transaction's locks are released (`UnlockByTxID`) or when
136+
its selector is closed, and stale counters are reclaimed once a transaction has
137+
been idle longer than `leaseExpiry`. Periodic lock cleanup does **not** reset
138+
live counters, so the ceiling cannot be circumvented by waiting for a cleanup
139+
tick.
131140

132141
**Tuning guidance**:
133142
- Should be ≤ `maxTokensPerSelection` (validation enforced)
@@ -138,7 +147,17 @@ token:
138147

139148
**What it limits**: Maximum time allowed for entire selection operation
140149

141-
**Default**: 30 seconds
150+
**Default**: 30 seconds *plus* the worst-case retry budget, i.e.
151+
`30s + maxRetryCycles * retryInterval`. With the default 10 retry cycles and a
152+
5s `retryInterval` the effective default is 80s.
153+
154+
The retry budget is added because both selectors sleep for up to
155+
`retryInterval` between cycles: a fixed 30s ceiling would expire while the
156+
retries that are meant to resolve contention are still in progress, turning
157+
ordinary contention into a timeout that the caller cannot resolve by retrying.
158+
An explicitly configured `selectionTimeout` is always used as-is.
159+
160+
A non-positive value means **no timeout**.
142161

143162
**Configuration**:
144163
```yaml
@@ -150,11 +169,15 @@ token:
150169

151170
**When it triggers**: When selection takes longer than configured timeout
152171

153-
**Error message**: `"token selection aborted: exceeded timeout (30s) after examining X tokens and Y lock attempts"`
172+
**Error message**: `"token selection aborted: exceeded timeout (30s) after examining X tokens and Y lock attempts"`,
173+
wrapping the `token.SelectorTimedOut` sentinel so callers can distinguish a
174+
timeout from a permanent failure.
154175

155176
**Tuning guidance**:
156177
- **Increase** for slow databases or bulk operations
157178
- **Decrease** for faster failure detection
179+
- Keep it above `maxRetryCycles * retryInterval`, or the timeout will fire
180+
before the retry budget is spent
158181
- Consider database query performance when setting
159182

160183
## Configuration Examples
@@ -348,7 +371,9 @@ type TokenLockStore interface {
348371
```
349372

350373
The built-in in-memory locker and the SQL-backed `TokenLockStore` accept `walletID`
351-
but do not act on it — they apply no rate limiting or quota.
374+
but do not act on it — they apply no per-wallet rate limiting or quota. They do
375+
enforce the per-transaction `maxLocksPerTransaction` ceiling described above, and
376+
report it through this same contract.
352377

353378
### The fail-fast contract
354379

integration/token/fungible/topology/topology.go

Lines changed: 21 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -181,13 +181,17 @@ func Topology(opts common.Opts) []api.Topology {
181181
}
182182
}
183183

184-
// Add bootstrap node before creating node list
184+
// Create the bootstrap node before the token topology is assembled, but
185+
// keep it out of the TMS member lists below: it only performs libp2p
186+
// bootstrapping and has no token role, so giving it TMS membership would
187+
// generate crypto material and wallet configuration it never uses.
185188
bootstrapNode := fscTopology.AddNodeByName("lib-p2p-bootstrap-node")
186189
fscTopology.SetBootstrapNode(bootstrapNode)
190+
tmsNodes := nodesExcluding(fscTopology.ListNodes(), bootstrapNode)
187191

188192
tokenTopology := token.NewTopology()
189193
tokenTopology.TokenSelector = opts.TokenSelector
190-
tms := tokenTopology.AddTMS(fscTopology.ListNodes(), backendTopology, backendChannel, opts.DefaultTMSOpts.TokenSDKDriver)
194+
tms := tokenTopology.AddTMS(tmsNodes, backendTopology, backendChannel, opts.DefaultTMSOpts.TokenSDKDriver)
191195
tms.SetNamespace("token_chaincode")
192196
common.SetDefaultParams(tms, opts.DefaultTMSOpts)
193197
if !opts.DefaultTMSOpts.Aries {
@@ -208,7 +212,7 @@ func Topology(opts common.Opts) []api.Topology {
208212
} else {
209213
fabric2.SetOrgs(tms, "Org1")
210214
}
211-
nodeList := fscTopology.ListNodes()
215+
nodeList := tmsNodes
212216

213217
if !opts.NoAuditor {
214218
tms.AddAuditor(auditor)
@@ -285,3 +289,17 @@ func Topology(opts common.Opts) []api.Topology {
285289

286290
return []api.Topology{backendTopology, tokenTopology, fscTopology}
287291
}
292+
293+
// nodesExcluding returns nodes without the given one. Used to keep the libp2p
294+
// bootstrap node out of TMS membership: it is part of the FSC topology so that
295+
// peers can bootstrap through it, but it plays no token role.
296+
func nodesExcluding(nodes []*node.Node, excluded *node.Node) []*node.Node {
297+
kept := make([]*node.Node, 0, len(nodes))
298+
for _, n := range nodes {
299+
if n != excluded {
300+
kept = append(kept, n)
301+
}
302+
}
303+
304+
return kept
305+
}

integration/token/fungible/views/transfer.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,10 @@ func (t *TransferWithSelectorView) Call(context view.Context) (any, error) {
290290
if err != nil && t.Retry &&
291291
(errors.HasCause(err, token2.SelectorSufficientButLockedFunds) ||
292292
errors.HasCause(err, token2.SelectorSufficientButNotCertifiedFunds) ||
293-
errors.HasCause(err, token2.SelectorSufficientFundsButConcurrencyIssue)) {
293+
errors.HasCause(err, token2.SelectorSufficientFundsButConcurrencyIssue) ||
294+
// A selection that ran out of wall-clock time under
295+
// contention is transient in exactly the same way.
296+
errors.HasCause(err, token2.SelectorTimedOut)) {
294297
time.Sleep(10 * time.Second)
295298

296299
continue
@@ -309,6 +312,8 @@ func (t *TransferWithSelectorView) Call(context view.Context) (any, error) {
309312
assert.NoError(err, "mandarin")
310313
case errors.HasCause(err, token2.SelectorSufficientFundsButConcurrencyIssue):
311314
assert.NoError(err, "peach")
315+
case errors.HasCause(err, token2.SelectorTimedOut):
316+
assert.NoError(err, "papaya")
312317
default:
313318
assert.NoError(err, "system failure")
314319
}

token/services/selector/config/driver.go

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -58,12 +58,16 @@ type Config struct {
5858
NumRetries int `yaml:"numRetries,omitempty"`
5959
}
6060

61-
// New returns a SelectorConfig with the values from the token.selector key
61+
// New returns a SelectorConfig with the values from the token.selector key.
62+
//
63+
// On error the returned *Config is a usable zero value rather than nil, so the
64+
// callers that log the error and carry on with defaults do not dereference nil.
65+
// A partially unmarshalled struct is discarded: half-applied values are worse
66+
// than defaults.
6267
func New(config configService) (*Config, error) {
6368
c := &Config{}
64-
err := config.UnmarshalKey("token.selector", c)
65-
if err != nil {
66-
return nil, errors.Wrap(err, "invalid config for key [token.selector]: expected retryInterval (duration) and numRetries (integer))")
69+
if err := config.UnmarshalKey("token.selector", c); err != nil {
70+
return &Config{}, errors.Wrap(err, "invalid config for key [token.selector]: expected retryInterval (duration) and numRetries (integer))")
6771
}
6872

6973
return c, nil
@@ -152,6 +156,18 @@ func (c *Config) GetLimits() Limits {
152156
}
153157
if limits.SelectionTimeout <= 0 {
154158
limits.SelectionTimeout = defaultSelectionTimeout
159+
160+
// Keep the default wall-clock timeout from binding before the retry
161+
// budget it is meant to bound. Both selectors sleep for up to
162+
// retryInterval between cycles, so a contended selection can spend
163+
// maxRetries * retryInterval backing off before it has used up its
164+
// retries. With the defaults (10 retries, 5s interval) that averages
165+
// ~25s and reaches 50s, so a fixed 30s ceiling would turn ordinary
166+
// contention into SelectorTimedOut — which callers cannot resolve by
167+
// retrying — instead of letting the retry budget play out.
168+
if budget := time.Duration(limits.MaxRetries)*c.GetRetryInterval() + defaultSelectionTimeout; budget > limits.SelectionTimeout {
169+
limits.SelectionTimeout = budget
170+
}
155171
}
156172

157173
return limits
Lines changed: 88 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,88 @@
1+
/*
2+
Copyright IBM Corp. All Rights Reserved.
3+
4+
SPDX-License-Identifier: Apache-2.0
5+
*/
6+
7+
package config
8+
9+
import (
10+
"testing"
11+
"time"
12+
13+
"github.com/hyperledger-labs/fabric-smart-client/pkg/utils/errors"
14+
"github.com/stretchr/testify/assert"
15+
"github.com/stretchr/testify/require"
16+
)
17+
18+
// failingConfigService fails every UnmarshalKey, standing in for a malformed
19+
// token.selector block.
20+
type failingConfigService struct{}
21+
22+
func (failingConfigService) UnmarshalKey(string, any) error {
23+
return errors.New("malformed token.selector block")
24+
}
25+
26+
// TestNewReturnsUsableConfigOnError pins that New never hands back a nil
27+
// *Config. Every caller logs the error and carries on with defaults, so a nil
28+
// would panic on the first Get*/Validate call — i.e. a malformed
29+
// token.selector block would take the node down instead of falling back.
30+
func TestNewReturnsUsableConfigOnError(t *testing.T) {
31+
cfg, err := New(failingConfigService{})
32+
require.Error(t, err)
33+
require.NotNil(t, cfg, "callers log the error and keep using cfg")
34+
35+
// Every accessor must work and return the documented default.
36+
assert.NotPanics(t, func() {
37+
require.NoError(t, cfg.Validate())
38+
assert.Equal(t, defaultDriver, cfg.GetDriver())
39+
assert.Equal(t, defaultMaxTokensPerSelection, cfg.GetMaxTokensPerSelection())
40+
assert.Equal(t, defaultMaxLockAttempts, cfg.GetMaxLockAttempts())
41+
assert.Equal(t, defaultMaxLocksPerTransaction, cfg.GetMaxLocksPerTransaction())
42+
assert.Equal(t, defaultMaxRetries, cfg.GetNumRetries())
43+
assert.Equal(t, defaultRetryInterval, cfg.GetRetryInterval())
44+
assert.Positive(t, cfg.GetSelectionTimeout())
45+
})
46+
}
47+
48+
// TestGetLimitsDefaultTimeoutClearsRetryBudget pins the relationship between the
49+
// two defaults: the wall-clock timeout has to outlast the worst-case retry
50+
// budget, or contention that the retries would have resolved is reported as
51+
// SelectorTimedOut instead.
52+
func TestGetLimitsDefaultTimeoutClearsRetryBudget(t *testing.T) {
53+
for _, tc := range []struct {
54+
name string
55+
cfg *Config
56+
minimumBudget time.Duration
57+
}{
58+
{
59+
name: "defaults",
60+
cfg: &Config{},
61+
minimumBudget: time.Duration(defaultMaxRetries) * defaultRetryInterval,
62+
},
63+
{
64+
name: "explicit retry interval",
65+
cfg: &Config{RetryInterval: 10 * time.Second},
66+
minimumBudget: time.Duration(defaultMaxRetries) * 10 * time.Second,
67+
},
68+
{
69+
name: "explicit retry count",
70+
cfg: &Config{Limits: Limits{MaxRetries: 25}},
71+
minimumBudget: 25 * defaultRetryInterval,
72+
},
73+
} {
74+
t.Run(tc.name, func(t *testing.T) {
75+
limits := tc.cfg.GetLimits()
76+
assert.Greater(t, limits.SelectionTimeout, tc.minimumBudget,
77+
"default timeout must outlast %d retries of up to %v each",
78+
limits.MaxRetries, tc.cfg.GetRetryInterval())
79+
})
80+
}
81+
}
82+
83+
// TestGetLimitsExplicitTimeoutIsHonoured verifies the derivation above only
84+
// fills in a missing value and never overrides an operator's own timeout.
85+
func TestGetLimitsExplicitTimeoutIsHonoured(t *testing.T) {
86+
cfg := &Config{Limits: Limits{SelectionTimeout: 3 * time.Second, MaxRetries: 100}}
87+
assert.Equal(t, 3*time.Second, cfg.GetLimits().SelectionTimeout)
88+
}

token/services/selector/config/driver_limits_test.go

Lines changed: 13 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,12 @@ func TestConfig_GetLimits(t *testing.T) {
2323
assert.Equal(t, defaultMaxLockAttempts, limits.MaxLockAttempts)
2424
assert.Equal(t, defaultMaxRetries, limits.MaxRetries)
2525
assert.Equal(t, defaultMaxLocksPerTransaction, limits.MaxLocksPerTransaction)
26-
assert.Equal(t, defaultSelectionTimeout, limits.SelectionTimeout)
26+
// The default timeout is widened to clear the default retry budget
27+
// (maxRetries backoffs of up to retryInterval each), so that ordinary
28+
// contention plays out through the retries instead of tripping
29+
// SelectorTimedOut.
30+
assert.Equal(t, defaultTimeoutFor(defaultMaxRetries, defaultRetryInterval), limits.SelectionTimeout)
31+
assert.Greater(t, limits.SelectionTimeout, time.Duration(defaultMaxRetries)*defaultRetryInterval)
2732
})
2833

2934
t.Run("returns configured values when set", func(t *testing.T) {
@@ -61,7 +66,7 @@ func TestConfig_GetLimits(t *testing.T) {
6166
assert.Equal(t, defaultMaxLockAttempts, limits.MaxLockAttempts)
6267
assert.Equal(t, defaultMaxRetries, limits.MaxRetries)
6368
assert.Equal(t, defaultMaxLocksPerTransaction, limits.MaxLocksPerTransaction)
64-
assert.Equal(t, defaultSelectionTimeout, limits.SelectionTimeout)
69+
assert.Equal(t, defaultTimeoutFor(defaultMaxRetries, defaultRetryInterval), limits.SelectionTimeout)
6570
})
6671

6772
t.Run("backward compatibility: uses deprecated NumRetries", func(t *testing.T) {
@@ -186,3 +191,9 @@ func TestConfig_DefaultValues(t *testing.T) {
186191
"maxLocksPerTransaction should be <= maxTokensPerSelection")
187192
})
188193
}
194+
195+
// defaultTimeoutFor mirrors the derivation in GetLimits: the fixed default plus
196+
// the worst-case time the selector may spend backing off between retries.
197+
func defaultTimeoutFor(maxRetries int, retryInterval time.Duration) time.Duration {
198+
return time.Duration(maxRetries)*retryInterval + defaultSelectionTimeout
199+
}

token/services/selector/config/driver_test.go

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -285,7 +285,12 @@ func TestNew(t *testing.T) {
285285

286286
if tt.expectError {
287287
require.Error(t, err)
288-
assert.Nil(t, cfg)
288+
// New returns a usable zero-value Config alongside the error:
289+
// callers log it and continue with defaults, so a nil here
290+
// would panic on the first Get*/Validate call.
291+
require.NotNil(t, cfg)
292+
assert.Equal(t, &Config{}, cfg, "a failed unmarshal must not leave partial values")
293+
assert.Equal(t, defaultMaxTokensPerSelection, cfg.GetMaxTokensPerSelection())
289294
} else {
290295
require.NoError(t, err)
291296
assert.NotNil(t, cfg)

0 commit comments

Comments
 (0)