Limit the number of txs per address#1117
Conversation
Avoid requiring too much memory by syncing very large history.
📝 WalkthroughWalkthroughThe changes introduce a new constant Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/chronikService.ts`:
- Around line 313-315: The new throw in the txsPage.numTxs check can bypass
cleanup in syncTransactionsForAddress; wrap the main loop/processing in a
try/finally so setSyncing(address.address, false) always runs and only call
updateLastSynced(address.address) when the sync actually completed successfully
(use a completed flag set after the loop). Ensure the thrown Error from the
MAX_TXS_PER_ADDRESS check is allowed to propagate but does not prevent the
syncing flag from being cleared.
| if (txsPage.numTxs > MAX_TXS_PER_ADDRESS) { | ||
| throw new Error(`Address ${addressString} has too many txs to sync (${txsPage.numTxs} > ${MAX_TXS_PER_ADDRESS}).`) | ||
| } |
There was a problem hiding this comment.
Ensure thrown tx-limit errors cannot leave sync state dirty.
Line 313 introduces a predictable throw path, but syncTransactionsForAddress does cleanup outside a finally. If this error propagates there, setSyncing(false) may be skipped, leaving the address stuck in syncing state.
Proposed fix
+class AddressTxLimitExceededError extends Error {
+ constructor (
+ public readonly address: string,
+ public readonly numTxs: number,
+ public readonly limit: number
+ ) {
+ super(`Address ${address} has too many txs to sync (${numTxs} > ${limit}).`)
+ this.name = 'AddressTxLimitExceededError'
+ }
+}
public async getPaginatedTxs (addressString: string, page: number, pageSize: number): Promise<Tx[]> {
const { type, hash160 } = toHash160(addressString)
const txsPage = (await this.chronik.script(type, hash160).history(page, pageSize))
// If there are too many txs, this might be too expensive to sync. Raise an
// error to skip this address.
if (txsPage.numTxs > MAX_TXS_PER_ADDRESS) {
- throw new Error(`Address ${addressString} has too many txs to sync (${txsPage.numTxs} > ${MAX_TXS_PER_ADDRESS}).`)
+ throw new AddressTxLimitExceededError(addressString, txsPage.numTxs, MAX_TXS_PER_ADDRESS)
}
return txsPage.txs
}// In syncTransactionsForAddress(), ensure state cleanup is always executed.
let completed = false
try {
while (true) {
// existing loop
}
completed = true
} finally {
await setSyncing(address.address, false)
if (completed) {
await updateLastSynced(address.address)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/chronikService.ts` around lines 313 - 315, The new throw in the
txsPage.numTxs check can bypass cleanup in syncTransactionsForAddress; wrap the
main loop/processing in a try/finally so setSyncing(address.address, false)
always runs and only call updateLastSynced(address.address) when the sync
actually completed successfully (use a completed flag set after the loop).
Ensure the thrown Error from the MAX_TXS_PER_ADDRESS check is allowed to
propagate but does not prevent the syncing flag from being cleared.
Klakurka
left a comment
There was a problem hiding this comment.
Good idea putting an error throw on it.
Avoid requiring too much memory by syncing very large history.
Summary by CodeRabbit