Improve memory consumption during syncing#1129
Open
Fabcien wants to merge 7 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR optimizes the blockchain transaction sync pipeline by introducing snapshot-based dedup filtering for transactions, phase-based batching with producer/consumer drain coordination in the Chronik sync flow, and post-sync cache invalidation for addresses and users. Transaction persistence is refactored to detect and selectively update only changed rows while the Chronik worker coordinates multi-phase batch emissions to manage memory during bulk syncing. ChangesBlockchain Sync Pipeline Optimization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
scripts/run-mark-migration.sh (1)
9-20: ⚡ Quick winAvoid duplicating migration-marking SQL in shell and SQL files.
Lines 10-19 duplicate
scripts/mark-migration-applied.sql(including checksum). If one copy changes and the other doesn’t, migration metadata can drift. Prefer executing the SQL file directly from this script.Refactor to single source of truth
-mysql -h "$MAIN_DB_HOST" -P "$MAIN_DB_PORT" -u "$MAIN_DB_USER" -p"$MAIN_DB_PASSWORD" "$MAIN_DB_NAME" <<EOF -INSERT INTO _prisma_migrations (id, migration_name, applied_steps_count, checksum, started_at, finished_at) -VALUES ( - UUID(), - '20260227100000_add_transaction_indexes', - 1, - 'KVemPS6dYEFuC9uoI21o0vUCNnYWb1odbI5F06YdVJE=', - NOW(3), - NOW(3) -) -ON DUPLICATE KEY UPDATE migration_name = migration_name; -EOF +mysql -h "$MAIN_DB_HOST" -P "$MAIN_DB_PORT" -u "$MAIN_DB_USER" -p"$MAIN_DB_PASSWORD" "$MAIN_DB_NAME" < scripts/mark-migration-applied.sql🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/run-mark-migration.sh` around lines 9 - 20, The embedded INSERT SQL in run-mark-migration.sh duplicates the contents (including checksum) of scripts/mark-migration-applied.sql and should be removed to avoid drift; update run-mark-migration.sh to execute the existing SQL file (scripts/mark-migration-applied.sql) via the mysql client (e.g., mysql ... < scripts/mark-migration-applied.sql) instead of inline heredoc, and delete the duplicated INSERT block so the single source of truth is scripts/mark-migration-applied.sql.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jobs/workers.ts`:
- Around line 21-39: Make cache invalidation best-effort by preventing
individual Redis failures from rejecting the whole job: in
invalidateCachesAfterBlockchainSync, replace the current Promise.all usage with
per-operation error handling (either use Promise.allSettled or wrap each call to
clearPaymentCacheForAddress, clearBalanceCache, and clearDashboardCache inside
try/catch) and log/report any errors instead of throwing; apply the same change
to the other cache invalidation block later in the file so all clear* calls
never bubble up rejections to the caller.
In `@redis/paymentCache.ts`:
- Around line 284-291: clearPaymentCacheForAddress currently relies on
getCachedWeekKeysForAddress which uses redis.keys(...) causing full keyspace
scans when invalidateCachesAfterBlockchainSync calls clearPaymentCacheForAddress
for many addresses after syncMissedTransactions; replace the keys-based lookup
with a bounded SCAN/scanStream-based iteration or maintain an address-scoped
index/set of week keys so you only fetch known keys for that address, then
delete them (use redis.del without an empty callback). Update
getCachedWeekKeysForAddress (or create a new scan-based helper) and ensure
invalidateCachesAfterBlockchainSync/syncMissedTransactions use that bounded
approach to avoid blocking full-keyspace scans.
In `@scripts/resolve-index-migration.sh`:
- Line 26: The echo uses non-POSIX substring expansion (${CHECKSUM:0:20}) which
fails under /bin/sh; change the echo that prints "Calculated checksum" to first
derive the first 20 characters of the CHECKSUM using a portable method (e.g.,
use printf to format/truncate CHECKSUM to 20 chars) and embed that result in the
message; update the echo line referencing CHECKSUM so it no longer relies on
${CHECKSUM:0:20}.
In `@services/chronikService.ts`:
- Around line 1084-1087: The code clears the syncing flag when batch.phase ===
'addresses-synced' by calling setSyncingBatch(batch.addressesSynced, false), but
that only means the fetch finished and there may still be buffered rows in
toCommit waiting for DB_COMMIT_BATCH_SIZE or the 'drain-complete' event; instead
delay clearing the syncing flag until those buffered rows are actually committed
(e.g., after the commit/drain-complete logic finishes), so modify the logic
around setSyncingBatch to only call it after the commit/drain-complete path
completes (or when toCommit is empty), ensuring waitForSyncing() cannot unblock
prematurely and race with mempool upserts.
- Around line 1001-1013: The current matching loop used when runTriggers is true
only checks tuple.row.hash against createdTx.hash, which can misassociate a
transaction that touches multiple watched addresses; update the lookup over
commitTuples (used in the block handling syncResult.inserted) to require both
tuple.row.hash === createdTx.hash and tuple.row.addressId ===
createdTx.addressId so the tuple corresponds to the same persisted address, then
pass the matched tuple.addressString and tuple.raw into
broadcastIncomingTxFromSyncRow as before.
- Around line 858-883: The broadcastIncomingTxFromSyncRow helper currently
zeroes out OP_RETURN by setting stubTx.opReturn = '' which strips
paymentId/message/rawMessage; update stubTx to carry the original op-return from
the persisted row (use createdTx.opReturn or the persisted row's opReturn field)
instead of hardcoding '', ensure the property is passed through into the
TransactionWithAddressAndPrices stub so getSimplifiedTrasaction receives the
real opReturn, and adjust types/null checks if createdTx.opReturn can be
undefined.
In `@services/transactionService.ts`:
- Around line 606-609: The PersistManyTransactionRowsResult currently only
returns inserted rows, so transactions updated for confirmed/timestamp changes
are ignored by post-persist processing; change PersistManyTransactionRowsResult
to include an updated: SyncPersistedTransaction[] (not just updatedCount) and
update the persistManyTransactions/persist path callers to handle both
result.inserted and result.updated (merge them into the same post-persist
processing that refreshes price links, cache and broadcast). Also update the
post-sync repair and any other callers that assumed only inserted (the other
persist/post-persist branches referenced) to process the updated array so
previously-seen mempool txs that later confirm get their price links refreshed
and broadcasted. Ensure functions and tests that used updatedCount are adjusted
to use updated.length where necessary.
---
Nitpick comments:
In `@scripts/run-mark-migration.sh`:
- Around line 9-20: The embedded INSERT SQL in run-mark-migration.sh duplicates
the contents (including checksum) of scripts/mark-migration-applied.sql and
should be removed to avoid drift; update run-mark-migration.sh to execute the
existing SQL file (scripts/mark-migration-applied.sql) via the mysql client
(e.g., mysql ... < scripts/mark-migration-applied.sql) instead of inline
heredoc, and delete the duplicated INSERT block so the single source of truth is
scripts/mark-migration-applied.sql.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ffb3a91-28b9-49bd-8a8b-e3e0c88fffec
📒 Files selected for processing (9)
jobs/workers.tsprisma-local/migrations/20260227100000_add_transaction_indexes/migration.sqlredis/paymentCache.tsscripts/mark-migration-applied.sqlscripts/resolve-index-migration.shscripts/run-mark-migration.shservices/chronikService.tsservices/transactionService.tstests/unittests/chronikService.test.ts
| CHECKSUM=$(cat "$MIGRATION_FILE" | openssl dgst -sha256 -binary | base64 | tr -d '\n' || echo "") | ||
| NOW=$(date -u +"%Y-%m-%d %H:%M:%S.000") | ||
|
|
||
| echo "Calculated checksum: ${CHECKSUM:0:20}..." |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Detect bash-style substring expansion used inside POSIX-sh scripts.
rg -nP '\$\{[A-Za-z_][A-Za-z0-9_]*:[0-9]+' scripts/resolve-index-migration.sh
head -n 1 scripts/resolve-index-migration.shRepository: PayButton/paybutton-server
Length of output: 135
🏁 Script executed:
#!/bin/bash
# Inspect relevant context around line 26 and scan for other common bashisms in this POSIX sh script.
set -euo pipefail
echo "=== Context around line 26 ==="
nl -ba scripts/resolve-index-migration.sh | sed -n '15,40p'
echo "=== Detect bash-style substring expansion in this file ==="
rg -nP '\$\{[A-Za-z_][A-Za-z0-9_]*:[0-9]+' scripts/resolve-index-migration.sh || true
echo "=== Detect other common non-POSIX /bin/sh bashisms ==="
rg -n '(\[\[)|(\]\])|(\(())|(\bdeclare\b)|(\bfunction\b)|(\bsource\b)|(<\(.*\))|(\bexport\b[[:space:]]+[A-Za-z_][A-Za-z0-9_]*=)' scripts/resolve-index-migration.sh || trueRepository: PayButton/paybutton-server
Length of output: 162
🏁 Script executed:
#!/bin/bash
# Inspect relevant context around line 26 and scan for other common bashisms in this script.
echo "=== First 5 lines (shebang) ==="
head -n 5 scripts/resolve-index-migration.sh
echo "=== Lines 20-35 (includes line 26) ==="
sed -n '20,35p' scripts/resolve-index-migration.sh
echo "=== Detect bash-style substring expansion in this file ==="
rg -nP '\$\{[A-Za-z_][A-Za-z0-9_]*:[0-9]+' scripts/resolve-index-migration.sh || true
echo "=== Detect other common non-POSIX bashisms ==="
rg -n '(\[\[)|(\]\])|(\(())|(\bdeclare\b)|(\bfunction\b)|(\bsource\b)|(<\(.*\))' scripts/resolve-index-migration.sh || trueRepository: PayButton/paybutton-server
Length of output: 1793
🏁 Script executed:
#!/bin/bash
# Scan for other parameter expansions that are commonly non-POSIX (besides ${var:0:20})
rg -n '\$\{[A-Za-z_][A-Za-z0-9_]*:[^}]+\}' scripts/resolve-index-migration.sh || true
rg -n '\$\{[A-Za-z_][A-Za-z0-9_]*[#%/][^}]+\}' scripts/resolve-index-migration.sh || trueRepository: PayButton/paybutton-server
Length of output: 125
Fix non-POSIX bash substring expansion in /bin/sh script (scripts/resolve-index-migration.sh:26)
Line 26 uses ${CHECKSUM:0:20}, which is not POSIX sh parameter expansion and will fail under /bin/sh (e.g., dash) with “Bad substitution”.
Portable fix
- echo "Calculated checksum: ${CHECKSUM:0:20}..."
+ echo "Calculated checksum: $(printf '%.20s' "$CHECKSUM")..."📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| echo "Calculated checksum: ${CHECKSUM:0:20}..." | |
| echo "Calculated checksum: $(printf '%.20s' "$CHECKSUM")..." |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 26-26: In POSIX sh, string indexing is undefined.
(SC3057)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/resolve-index-migration.sh` at line 26, The echo uses non-POSIX
substring expansion (${CHECKSUM:0:20}) which fails under /bin/sh; change the
echo that prints "Calculated checksum" to first derive the first 20 characters
of the CHECKSUM using a portable method (e.g., use printf to format/truncate
CHECKSUM to 20 chars) and embed that result in the message; update the echo line
referencing CHECKSUM so it no longer relies on ${CHECKSUM:0:20}.
Don't let prisma handle the duplicates on its own but filter out the duplicate before running the commit which does more and is expensive.
We aim to fix a producer/consumer issue. It happens that we produce more txs to commit to the db than what can be committed to disk, leading to memory explosion. We are going to use a state machine to properly pause the tx producers while the queue of txs to commit is draining. This commit introduces the steps only, no change in behavior yet.
It's not used yet as nothing can set producersPaused to true, so no change in behavior.
Chronik workers fill up the buffer until it has >= 200 txs, then trigger the db commit worker to run the drain cycle. This pauses the producers, blocking them before the next chronik page is read, until the queue is fully drained. Then it resumes, and so on until all addresses are completely synced. This avoids the buffer to be filled faster than it's drained and the app heap usage to grow unbounded.
We start the drain task after 200 txs but it's very likely that some txs are already in the db and shouldn't be committed. Instead of waiting for the full batch of 200 txs, commit immediately so we can release memory asap.
The createMayTransactions path does a lot of things: - insert the txs into the DB - sync the prices - update the cache - run the triggers All that work allocates a ton of memory, and a good chunk of it is not needed during sync: - syncing the price is done anyway after the tx fetching during the syncing process, so it can be avoided during the initial phase - cache update only works if there is a price associated, so better skip it as well during the tx fetching. Similarly, building and caching the payment information per Paybutton is useless at this point. This commit implements a differentiated strategy during sync that skips price update + cache update, and instead only focus on fetching the transactions. Price update happens at the next stage, and the cache will eventually be rebuilt anyway when the user needs it. To enforce this, the cache is cleared after the syncing completed. This significantly reduce the memory usage during syncing at the cost of an inaccurate UI view during that time. On my machine, syncing from an 3 months old prod db with > 12k addresses resulted in OOM (32GB allocated) after ~1300 eCash addresses. After this commit, fetching all txs peaked at ~2,6GB (price fetching needs more).
Collaborator
Author
|
I removed my fix commit for the dump I used in tests, it's irrelevant for prod code |
Don't fail on cache clear issue, reconnect price for updated txs as well, avoid duplicate triggers if several addresses link a single tx, keep op_return, use scanStrean for redis cleanup.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
services/transactionService.ts (1)
765-777: 💤 Low valueConsider using a Map for O(1) lookup instead of O(n²) find.
The nested
.find()inside a loop creates O(n×m) complexity. For large batches, this could slow down processing. Building a Map fromcreatedTxsfirst would give O(1) lookups.♻️ Suggested improvement
+ const createdTxMap = new Map( + createdTxs.map(ct => [`${ct.hash}:${ct.addressId}`, ct]) + ) for (let i = 0; i < newTxs.length; i++) { const row = newTxs[i] - const created = createdTxs.find( - ct => ct.hash === row.hash && ct.addressId === row.addressId - ) + const created = createdTxMap.get(`${row.hash}:${row.addressId}`) if (created != null) { txMap.set(`${row.hash}:${row.addressId}`, { tx: created, inputs: newTxsInputs[i].inputs }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/transactionService.ts` around lines 765 - 777, The current loop uses createdTxs.find(...) inside the newTxs loop causing O(n²); instead build a lookup Map from createdTxs keyed by `${hash}:${addressId}` (e.g., iterate createdTxs and set key -> created) and then iterate newTxs to do O(1) map.get for each row to populate txMap; update the code around txMap/newTxs/newTxsInputs/createdTxs to use that lookup and preserve the same value shape ({ tx: created, inputs: newTxsInputs[i].inputs }).redis/paymentCache.ts (1)
301-303: 💤 Low valueRemove unnecessary callback argument from
redis.del.The ioredis
delmethod accepts variadic key arguments, not a callback. The empty() => {}gets passed as an additional key argument (coerced to string), causing Redis to attempt deleting a non-existent key with that stringified name. This is harmless but wasteful and technically incorrect API usage.This pattern exists elsewhere (line 290), but cleaning it up here would be appropriate.
Suggested fix
await Promise.all( - weekKeys.map(async (key) => await redis.del(key, () => {})) + weekKeys.map(async (key) => await redis.del(key)) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@redis/paymentCache.ts` around lines 301 - 303, The redis.del calls in paymentCache.ts incorrectly pass an empty callback () => {} as a key; remove that callback so del is called with just the key(s) (e.g., call redis.del(key) inside the weekKeys.map or, better, call redis.del(...weekKeys) to delete them in one call). Also update the similar pattern around the other occurrence (the one noted near line 290) to remove the unnecessary callback argument.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@redis/paymentCache.ts`:
- Around line 301-303: The redis.del calls in paymentCache.ts incorrectly pass
an empty callback () => {} as a key; remove that callback so del is called with
just the key(s) (e.g., call redis.del(key) inside the weekKeys.map or, better,
call redis.del(...weekKeys) to delete them in one call). Also update the similar
pattern around the other occurrence (the one noted near line 290) to remove the
unnecessary callback argument.
In `@services/transactionService.ts`:
- Around line 765-777: The current loop uses createdTxs.find(...) inside the
newTxs loop causing O(n²); instead build a lookup Map from createdTxs keyed by
`${hash}:${addressId}` (e.g., iterate createdTxs and set key -> created) and
then iterate newTxs to do O(1) map.get for each row to populate txMap; update
the code around txMap/newTxs/newTxsInputs/createdTxs to use that lookup and
preserve the same value shape ({ tx: created, inputs: newTxsInputs[i].inputs }).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9e93570a-4324-4a49-ba50-fbf6661d4b5a
📒 Files selected for processing (6)
jobs/workers.tsredis/clientInstance.tsredis/paymentCache.tsservices/chronikService.tsservices/transactionService.tstests/unittests/chronikService.test.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This implements a few strategies but is mostly a 2 part fix:
Summary by CodeRabbit
Bug Fixes
Refactor
Tests