cryptosim speedup - #4185
cryptosim speedup#4185cody-littley wants to merge 5 commits into
Conversation
PR SummaryMedium Risk Overview Cryptosim now builds each block with Gigasim gets the same measurement fix: writes are staged at block generation, execution is reads-only, Reviewed by Cursor Bugbot for commit a6c92cc. Bugbot is set up for automated code reviews on this repo. Configure here. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4185 +/- ##
==========================================
- Coverage 66.70% 65.56% -1.14%
==========================================
Files 2204 2083 -121
Lines 169825 157877 -11948
==========================================
- Hits 113279 103517 -9762
+ Misses 56405 54219 -2186
Partials 141 141
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit d5bdbc2. Configure here.
There was a problem hiding this comment.
The parallel block-build refactor is well documented and largely sound (fork-per-range randomness, range dispatch, changeset built off the critical path), but the new account-minting cadence has two correctness defects: a cadence hit that lands on a hot selection is silently dropped while its account ID stays reserved, which makes the next block re-issue IDs a fork already used; and the selection counter restarts every block, so the configured cadence is not what the benchmark actually applies.
Findings: 2 blocking | 4 non-blocking | 6 posted inline
Blockers
- None at the file/PR level.
- 2 blocking issue(s) flagged inline on specific lines.
Non-blocking
- None at the file/PR level.
- 4 suggestion(s)/nit(s) flagged inline on specific lines.
|
Default runs can re-issue account IDs across blocks (blocking) Default config is exactly the dangerous combination: HotAccountProbability: 0.1, BlockBuildWorkers: 8, cadence 1111, 1024 txns/block → two reserved IDs in different workers (selection 0 and 1111). |
I've fixed how account creation is handled. |
|
@seidroid review |
There was a problem hiding this comment.
The parallel build, the generation-time changeset and the deterministic selection patterns are a coherent design, and the previous review's two blockers are genuinely fixed. One worker-count dependence remains: the cold-selection window is sized from a counter each fork mutates, so a block's contents still depend on BlockBuildWorkers once NewAccountDormancyProbability < 1, and the new invariant test's fixture cannot see it.
Findings: 1 blocking | 3 non-blocking | 4 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- None at the file/PR level.
- 3 suggestion(s)/nit(s) flagged inline on specific lines.
|
@seidroid review |
There was a problem hiding this comment.
The parallel block build, the generation-time changeset and the "reads always reach the DB" rule are a coherent design, and last round's cold-window blocker is genuinely fixed by the coldAccountsVisibleInBlock freeze. Two non-blocking items remain: the even-spread selection pattern still puts every claimed selection on an odd count for both shipped defaults (so hot/minting selections only ever land on a transfer's destination), and the worker-independence fixture still mints on every selection, so the cold paths the new freeze protects are never exercised.
Findings: 0 blocking | 2 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- None at the file/PR level.
- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| func (d *DataGenerator) selectionIsHot(selection int64) bool { | ||
| share := int64(math.Round(d.config.HotAccountProbability * selectionPatternCycle)) | ||
| position := selection % selectionPatternCycle | ||
| return (position+1)*share/selectionPatternCycle > position*share/selectionPatternCycle |
There was a problem hiding this comment.
[suggestion] Still present after the latest push, so re-reporting. The even-spread condition claims a selection only when (position+1)*share/cycle > position*share/cycle, which for an integer cycle/share puts every claimed position at p ≡ (cycle/share - 1) mod (cycle/share) — odd whenever cycle/share is even, which both shipped defaults are.
- cryptosim,
HotAccountProbability = 0.1:share = 100_000, so hot positions arep ≡ 9 (mod 10)— all odd. - gigasim
selectionInShare(account_model.go:316),NewAccountProbability = 0.001:claimed = 1000, so mint positions arep ≡ 999 (mod 1000)— all odd;HotAccountProbability = 0.1givesp ≡ 9 (mod 10), also all odd.
BuildTransaction / buildTransaction select source first and destination second, so a transaction's selection counts are txIndex*2 (source, even) and txIndex*2+1 (destination, odd). Consequences at the defaults:
- cryptosim: no source account is ever hot; every hot selection is a destination.
- gigasim: no source is ever hot and no source is ever minted, so a transfer's source is unconditionally drawn from the cold population while ~20% of destinations are hot.
The aggregate shares are preserved, so the key set the store sees is close to unchanged, but the per-transaction composition is not the one the previous per-selection draws produced — and since the source and destination reads are the ones the read caches serve, that shifts the locality this benchmark exists to measure. Nothing in the docs or tests records the new shape (TestHotSelectionsFollowTheConfiguredShare only checks the aggregate count over a contiguous run, which holds either way).
Keying the pattern on a hash of the selection count, or offsetting it by the selection's position within the transaction, restores the share on both sides while keeping it a pure function of the count — which is all the range reservation in buildBlockRanges needs.
| t.Helper() | ||
| builder := newTestBuilder(t, 64) | ||
| builder.config.BlockBuildWorkers = workers | ||
| builder.config.SelectionsPerNewAccount = 1 |
There was a problem hiding this comment.
[suggestion] TestBlockContentsIndependentOfWorkerCount builds off this fixture, and with SelectionsPerNewAccount = 1 on top of newTestBuilder's HotAccountProbability = 1.0 and the default NewAccountDormancyProbability = 1.0, every selection creates (creating wins the tie). So the invariant test exercises only the create branch of RandomAccount:
selectionIsHot's branch never runs,- the cold-window branch (
highestSafeAccountIDInBlock/coldAccountsVisibleInBlock) never runs, and isCold = rand.Float64() >= 1.0is effectively never true, soColdAccountsMinted()is always 0 and thecoldAccountsMintedterm ofAdoptForkResultsis never non-zero in any test.
That means the coldAccountsVisibleInBlock freeze added this round — the fix for the previous review's worker-dependence blocker — has no test that can see it: deleting the two lines in Fork/ReportEndOfBlock that hold the window still leaves the suite green. TestABlockCreatesExactlyTheReservedAccountIDs does drive the cold branch (via newMixedSelectionTestBuilder), but it only compares created addresses against the reserved run and leaves NewAccountDormancyProbability at 1.0, so neither cold selection nor cold minting is compared across worker counts.
Adding a worker-independence case with NewAccountDormancyProbability < 1, HotAccountProbability < 1 and a cadence above 1 would make all three selection kinds and the cold-mint tally live in the comparison — this is the same case requested last round.
Superseded: latest AI review found no blocking issues.

Describe your changes and provide context
Optimize crytposim. At high throughput, block generation actually can become a bottleneck.
Previous cryptosim strategy was that each simulated transaction did a number of reads, then produced a number of key-value pairs that changed. This led to overhead in the benchmark framework, as managing and processing hundreds of thousands of key-value pairs adds nontrivial overhead.
In this branch, I change it so that we generate the writes for each transaction at the same time we are generating the "action plan" for the transaction. This produces equivalent data from the DB's perspective, but allows us to move the accounting work off of the main thread.
The crytposim suite is basically asking the question, "how fast can the DB go if the rest of the system is infinitely fast". In reality we will have transaction execution overhead, but that's not something that this benchmark wants to measure. So this optimization is a reasonable thing to do.