limit the number of tokens a transaction retrieves and locks - #1707
HayimShaul wants to merge 1 commit into
Conversation
dda132d to
142170c
Compare
|
Hi @HayimShaul , should these changes apply also to |
0a0b6ca to
2662935
Compare
|
@HayimShaul , don't we need to enforce limit also at the fetcher level A more general question, if we say that we can only load 100 tokens at the time from the storage, what if for these 100 tokens the selection request cannot be satisfied, but if had picked the tokens differently we would have. We might reject requests that can be satisfied. |
done |
4ac2576 to
079ec93
Compare
cb59a41 to
3778cd1
Compare
39378e4 to
1bdbe20
Compare
2b91b1f to
8f6abe6
Compare
9c40136 to
e61dda0
Compare
bf26e34 to
a004852
Compare
a004852 to
43f5b6e
Compare
|
I would like to ask your help to review this PR. Regards, |
adecaro
left a comment
adecaro
left a comment
There was a problem hiding this comment.
Review: does this fully address #1641?
Yes — every Blocking finding from the earlier review round is fixed at 694ad64f7, on both the sherdlock (default) and simple drivers; what's left is test-coverage depth, not correctness.
Checked out 1641_bound_work_by_request at 694ad64f7 and verified:
CI checks / utest (race + regression) / itest (~50 legs incl. fabricx-dlog-t3) / bench-check / cgo-check / CodeQL — all SUCCESS
DCO — both commits signed off
make lint (full repo) — 17 pre-existing revive findings, none in this PR's changed files
Read in full: sherdlock/{selector,manager,bounded_locker,fetcher,service}.go, simple/{selector,manager}.go,
simple/inmemory/locker.go, selector/config/driver.go, sdk/network/selector.go, token/selector.go
git merge-tree HEAD origin/main — 1 real conflict (dlogx/dlog_test.go), 2 clean auto-merges
I re-checked every item @AkramBitar's prior review flagged as Blocking against the current source rather than trusting the commit message's claims at face value: the sherdlock SQL LIMIT placeholder bug, StubbornSelector dropping locks before backoff, pre-dedup row limiting, the O(n²) in-memory locker scan, the cache/Close() race, fmt.Errorf usage, Validate()'s unreachable checks, the dead MaxRetryCycles field, the sqlite downgrade, sherdlock's missing lock-count bound, and the timeout/sufficient-funds mislabeling. All ten check out fixed by reading the code, not just the diff — details and file:line for each are in the local write-up. I'm not repeating any of those threads below.
CI is fully green, including the fabricx-dlog-t3 leg the final two commits specifically target.
What it fixes
| # | Finding | Verified |
|---|---|---|
| 1641.1 | Bound token-iteration depth per Select |
✅ sherdlock/selector.go (tokensIterated > s.maxTokensPerSelection) and simple/selector.go (tokensIteratedCount), both abort with a distinct error before further work |
| 1641.2 | Bound lock attempts per Select |
✅ maxLockAttempts counters in both selectors |
| 1641.3 | Bound retry/back-off cycles | ✅ MaxRetriesAfterBackOff (sherdlock) / Limits.MaxRetries (simple), plus SelectionTimeout as a wall-clock backstop |
| 1641.4 | Bound lock-store growth, on the default driver too | ✅ sherdlock/bounded_locker.go, wired to the real SQL lock store at service.go:157 — this was the specific gap Akram's review caught at an earlier commit, now closed |
| 1641.5 | Abort early and release acquired locks on limit breach | ✅ confirmed via the resolved UnlockAll-on-cancellation thread and reading the defer path in selectWithoutMetrics |
All five rows check out against the current source.
Non-blocking
- Attribution nit: the final commit message credits "a real-database exercise ... on both SQLite and PostgreSQL" to the new limit tests, but
tokens_limit_test.goissqlmock-based — the actual dual-backend coverage lives indbtest/tokens.go'sTUnspentTokensIteratorByLimit. Worth fixing the attribution so the next reader knows where the guarantee actually lives. - Coverage gap:
sherdlock/bounded_locker_test.goonly exercises the wrapper againstmocks.FakeLocker; nothing drivesmaxLocksPerTxthrough the real SQL-backed lock store end-to-end. The wiring inservice.gois correct today, but an integration-style test here would catch a future wiring regression that the wrapper unit tests can't see. - Rebase needed:
git merge-treeagainst currentmain(5 commits ahead) shows one real conflict inintegration/token/fungible/dlogx/dlog_test.go;wallets.goandauditor_test.goalso moved onmainbut auto-merge cleanly.
Recommendation
No correctness blockers. Rebase past the dlogx/dlog_test.go conflict, and consider the two test-depth nits above if there's time before merge — neither should hold this up.
|
Thanks a lot for the effort you've put into this — bounding the work a single request can trigger is the right thing to be doing, and there's a lot of careful plumbing and test coverage here. Below is what I think needs addressing before merge. I've split it into things I'd consider blocking and smaller ones that are quick fixes. Blocking1. token.selector.limits.maxLockAttempts: 100
# maxTokensPerSelection left unset
2. 3. The new DB-side row limit never takes effect on the default driver. 4. Concurrent I derived reachability from Quick fixes5. The reclaim path leaks the previous holder's 6. A freshly created counter is born stale, so a live reservation can be evicted. Non-blocking, worth a look
|
|
adecaro
left a comment
adecaro
left a comment
There was a problem hiding this comment.
Review: does this fully address #1641?
This is a much stronger patch than the version I (and others) reviewed earlier — approve, with two should-fix comments.
Checked out this branch at 14d0930 and verified:
go build ./... # clean
go vet ./token/... # clean
go build ./integration/... # clean
go test -count=1 ./token/services/selector/... # pass
go test -count=1 ./token/services/storage/db/sql/common/... # pass
go test -count=1 ./token/... # pass (full tree)
grep -rn "fmt.Errorf" token/services/selector/ # no matches
I re-checked every issue raised in the prior review rounds against this commit rather than trusting
the PR description. Thirteen of them are genuinely fixed, not just superficially patched — the
O(n²) lock counting is now O(1) via shard.txLocks, the Close()/iterator race is closed with a
drain-lock (selectMu), the timeout is a distinct sentinel instead of being conflated with
"sufficient but locked funds," Validate() now checks resolved rather than raw config values so a
partially-specified limits block can't sneak through, and the bounded locker finally has a test
against the real SQL-backed store instead of only a fake.
CI is fully green, including utest (unit-tests-race) which actually ran for 28 minutes and
passed (not just compiled), and every itest leg across dlog/fabtoken/fabricx/dloghsm/interop/
update/dvp/nft.
What it fixes
| # | Finding | Verified |
|---|---|---|
| 1 | Limit silently dropped on default (sherdlock) driver | Now enforced in-process via tokensIterated/lockAttempts counters in selectInternal — but see Blocking-adjacent note below, the DB query itself is still unbounded for this driver |
| 2 | O(n²) lock counting in inmemory locker |
shard.txLocks map[string]int, O(1) |
| 3 | Race between Close() and in-flight selection |
selectMu sync.RWMutex drain lock |
| 4 | Timeout conflated with locked-funds error | Distinct token.SelectorTimedOut sentinel |
| 5 | fmt.Errorf usage |
Removed, confirmed via grep |
| 6 | Scattered constructor config | Consolidated into sherdlock.Config |
| 7 | Rate-limit denial mishandled by selector | Hard-stops via errors.Is(lockErr, token.SelectorRateLimited) |
| 8 | Validate() checked raw config |
Now validates GetLimits() (resolved) values |
| 9 | Bounded locker only tested against a fake | New bounded_locker_sql_test.go against real SQL store |
All nine rows above check out against the current code, not just the diff.
Should fix 1 — security doc overstates DB-level enforcement for the default driver
docs/security/selector_resource_limits.md:50-57 says the token-iteration limit is enforced "at
two levels," including a LIMIT ? SQL clause, and claims ~10x faster queries as a result. That's
accurate for the simple driver (simple/selector.go:144 does pass the limit into the query), but
it's false for sherdlock — the default driver. All three sherdlock fetchers document that they
ignore the limit parameter (fetcher.go:106-107, 140-141, 339-340), and cachedFetcher.update()
does an unfiltered SpendableTokensIteratorBy(ctx, "", "") full-table scan on every cache refresh
(fetcher.go:260). maxTokensPerSelection only bounds the in-process counting loop over whatever
that unbounded fetch already returned — it doesn't stop a wallet with millions of tokens from being
pulled into memory in the first place, which is exactly the attack this doc's own threat model
section describes.
Should fix 2 — sherdlock's new abort logic has zero direct test coverage
selectInternal's two new limit checks (selector.go:320-334) aren't exercised by any test —
grep -rn "exceeded max token iteration\|exceeded max lock attempts" token/services/selector/sherdlock/
matches only the production file. manager_unit_test.go:33 wires MaxTokensPerSelection into a
Config but never drives a selection past it. The simple driver has exactly this coverage
(simple/selector_limits_test.go's TestSelector_TokenIterationLimit/TestSelector_LockAttemptLimit),
but sherdlock's cache-swap-on-retry path (selector.go:301-309) is different enough that passing
simple's tests says nothing about sherdlock's behavior here.
Non-blocking
- Doc nit: the same file's YAML examples and prose (lines 158, 186, 200, 213, 226) use a key
maxRetryCyclesthat doesn't exist — the actual field ismaxRetries. Config unmarshalling
silently ignores unknown keys, so anyone copying these examples gets the default retry count
regardless of what they set. - Doc nit:
config.Config(driver.go:55) is missing a Godoc comment; theLimitsstruct
right above it has one.
Recommendation
Good to merge once the security doc is corrected to scope the DB-level LIMIT claim to the simple
driver (or the claim is made accurate for sherdlock too) — that's the one thing here with a real
chance of misleading an operator's security posture. The test-coverage gap for sherdlock's abort
paths is worth a fast follow if not blocking. Everything else checks out.
Bound the work a single token selection can do, so one request can no longer scan or lock an unbounded portion of the wallet. Selection limits: - Add maxTokensPerSelection and maxLockAttempts to the selector config, applied per retry cycle rather than cumulatively, and push the row limit down into the UnspentTokensIteratorBy query so the database stops producing rows once the budget is reached. - config.Validate() checks the resolved limits from GetLimits() instead of the raw c.Limits.* fields, so a partial config (e.g. maxLockAttempts set while maxTokensPerSelection keeps its 10000 default) is rejected up front instead of aborting every selection at runtime. - A non-positive maxTokensPerSelection / maxLockAttempts means "unlimited", matching withSelectionTimeout, so a Config built without these limits is usable. Likewise a non-positive selectionTimeout means "no timeout" rather than an already-expired context, and the default is derived so it outlasts the default retry budget. - On Validate() failure, reset only cfg.Limits rather than discarding the whole config, so one mistyped limit no longer silently drops retryInterval, leaseExpiry, fetcher-cache and rateLimit* settings. Query correctness: - The limited query appended a literal "?" placeholder while every other parameter is emitted as $N by the query builder — a syntax error on PostgreSQL (SQLSTATE 42601). Since simple/selector.go always passes a non-zero limit, 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. - The row limit was applied before the Go-side dedup, and a directly-owned token matches both UNION ALL branches, so a limit of N surfaced roughly N/2 distinct tokens and the selector read that as an empty wallet. Use UNION on the limited path so LIMIT counts distinct rows. Contention and locking: - StubbornSelector.Select releases its partial locks before backing off again. Without that, two selections each holding part of the funds both exhausted their retry budget and both reported insufficient funds while funds were available; it also makes the surrounding log message true again. - Report the in-memory locker's per-transaction lock ceiling as SelectorRateLimited, so the selection fails fast instead of looking like contention. A full page with insufficient funds reports the limit instead of retrying a query that cannot change. - Lock cleanup no longer resets live per-transaction counters (which made the ceiling per-tick rather than per-transaction), counters are reclaimed on every replica instead of only the cleanup leader, and the reclaim path uses deleteLocked() so the previous holder's txLocks counter is decremented — it previously leaked a dead entry per reclaimed token on a hot shard. - sherdlock boundedLocker.counter() stamps lastLock at construction so a fresh counter is not born stale, closing the race where a concurrent EvictStaleTxState between the reservation and the first touch() could evict a live reservation and hand the tx a fresh budget. - Restore the swapCache helper that closes the iterator it displaces; the selectInternal rewrite replaced s.cache on every immediate retry and leaked a database cursor and its pooled connection per retry. - Guard the selectMu drain on the default path: StubbornSelector overrides Select, so Close() could otherwise close the iterator mid-iteration. Integration tests: - Restore the 3-second readiness wait in fabricxTestSuite.Setup(), mirroring integration.TestSuite.Setup(). The custom suite introduced in 1c1607c dropped it, so BeforeEach began running the issue view before the issuer node's view client was up and public params were installed. That produced "cannot retrieve public params for [default,testchannel,token_chaincode]" in the issue view, and the fire-and-forget InstallPublicParams goroutine then hit the shutting-down connection and panicked, killing the whole test binary. - Relax the CHF1 contention test's retry budget. It spins up 300 concurrent goroutines (3 replicas x 100 requests) contending for 2 tokens; the herd took ~18s to drain on a slow CI runner while each goroutine had only ~10 backoff cycles (~12s), so a few aborted with SelectorInsufficientFunds even though funds were available. - Keep lib-p2p-bootstrap-node out of TMS membership in the fungible topology, as the nft/interop/mixed topologies already do. Also adds regression coverage for each fixed path, including a real-database exercise of the limited query on both SQLite and PostgreSQL; corrects the stale maxRetries default in the docs (3 -> 10) with a migration note on the resulting worst-case selection-latency change; and reformats two files with gofmt 1.27, which no longer over-indents composite literals in a multi-value return (go.mod now requires go 1.27.1, so CI resolves that gofmt). Signed-off-by: Hayim.Shaul@ibm.com <hayimsha@fhe03.vpc.cloud9.ibm.com> Signed-off-by: Effi-S <effi.szt@gmail.com> Signed-off-by: AkramBitar <akram@il.ibm.com>
4 participants
Fixes #1641
Imposes hard, configurable upper bounds on every dimension of work reachable from a
Selector.Selectcall, so that a request cannot consume unbounded memory, CPU, storage or wall-clock time no matter how it is crafted. A request that would exceed a bound is aborted and its locks released.Bounds
All under the
token.selector.limitsconfig key:maxTokensPerSelectionmaxLockAttemptsmaxRetriesmaxLocksPerTransactionselectionTimeoutThe row limit is pushed into the query rather than applied in Go, so a wallet with millions of tokens no longer streams all of them into the view service. Tokens come back largest-first (
ORDER BY amount DESC) so a selection reaches its target in the fewest rows.Adds the
token.SelectorTimedOutsentinel, so callers can tell a timeout apart from a permanent failure.token.SelectorRateLimited(pre-existing) is now what both built-in lockers return when a transaction hitsmaxLocksPerTransaction, making that a fail-fast condition rather than something callers retry.Configuration is validated at startup; an invalid or unparseable
token.selectorblock logs and falls back to defaults.Behaviour changes reviewers should know about
selectionTimeoutdefault is no longer a flat 30s. It is30s + maxRetries * retryInterval, i.e. 80s with the defaults. A flat 30s expires while the retries meant to resolve contention are still backing off, turning ordinary contention into a timeout the caller cannot resolve. An explicitly configured value is used verbatim. A non-positive value means no timeout.SelectorSufficientButLockedFunds) reaches the caller instead of being replaced by a generic limit error on a later cycle.Docs
docs/security/selector_resource_limits.mddocuments each limit, its default, when it triggers, and tuning guidance.Testing
Unit tests per limit, plus a
dbtestcase that exercises the limited query against both SQLite and PostgreSQL — the two dialects emit placeholders differently, so the shared case is what keeps the pushed-down limit honest on both.Follow-up commit
d1cb0422addresses the review in this comment: the pushed-down limit was emitted with a?placeholder (invalid on PostgreSQL), was counted before de-duplication, and the release-before-back-off inStubbornSelectorhad been dropped. It also restores the two test relaxations those regressions required.