Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

WalletScanner skips accounts with createdAt until createdAt sequence is reached #5652

Draft
wants to merge 1 commit into
base: staging
Choose a base branch
from
Draft

WalletScanner skips accounts with createdAt until createdAt sequence is reached #5652

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 42 additions & 29 deletions ironfish/src/wallet/scanner/walletScanner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ import type { HeadValue } from '../walletdb/headValue'
import { Config } from '../../fileStores'
import { Logger } from '../../logger'
import { Mutex } from '../../mutex'
import { BlockHeader, Transaction } from '../../primitives'
import { BufferUtils, HashUtils } from '../../utils'
import { BlockHeader, GENESIS_BLOCK_SEQUENCE, Transaction } from '../../primitives'
import { HashUtils } from '../../utils'
import { WorkerPool } from '../../workerPool'
import { ChainProcessorWithTransactions } from './chainProcessorWithTransactions'
import { BackgroundNoteDecryptor } from './noteDecryptor'
Expand All @@ -32,8 +32,13 @@ export class WalletScanner {
/**
* A snapshot of the accounts that have `scanningEnabled` set to true. Used
* to tell what accounts should be scanned, and from what block.
* If `scanFrom` is 'always-scan' then the account's head is the same as the
* WalletScanner's head, and blocks should always be added
*/
private scanningAccounts = new Array<{ account: Account; scanFrom: HeadValue | null }>()
private scanningAccounts = new Array<{
account: Account
scanFrom: { head: HeadValue | null } | 'always-scan'
}>()

constructor(options: {
logger: Logger
Expand Down Expand Up @@ -177,34 +182,32 @@ export class WalletScanner {
)
}

const connectOnlyAccounts = new Array<Account>()
const decryptAndConnectAccounts = new Array<Account>()

for (const candidate of this.scanningAccounts) {
if (
!candidate.scanFrom ||
BufferUtils.equalsNullable(candidate.scanFrom.hash, blockHeader.previousBlockHash)
) {
candidate.scanFrom = null

if (
candidate.account.createdAt === null ||
blockHeader.sequence >= candidate.account.createdAt.sequence
) {
decryptAndConnectAccounts.push(candidate.account)
} else {
connectOnlyAccounts.push(candidate.account)
const { account, scanFrom } = candidate

if (scanFrom === 'always-scan') {
continue
}

if (scanFrom.head === null) {
if (account.createdAt === null && blockHeader.sequence === GENESIS_BLOCK_SEQUENCE) {
candidate.scanFrom = 'always-scan'
}

if (account.createdAt && blockHeader.sequence >= account.createdAt.sequence) {
candidate.scanFrom = 'always-scan'
}
Comment on lines +193 to 199
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I think this can be simplified as:

const createdAtSequence = account.createdAt?.sequence ?? GENESIS_BLOCK_SEQUENCE
if (blockHeader.sequence >= createdAtSequence) {
    candidate.scanFrom = 'always-scan'
}

}
}

for (const account of connectOnlyAccounts) {
if (abort?.signal.aborted) {
return
if (scanFrom.head?.hash.equals(blockHeader.previousBlockHash)) {
candidate.scanFrom = 'always-scan'
}
await this.wallet.connectBlockForAccount(account, blockHeader, [], false)
}

const decryptAndConnectAccounts = this.scanningAccounts
.filter(({ scanFrom }) => scanFrom === 'always-scan')
.map(({ account }) => account)

Comment on lines +207 to +210
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like we use 'always-scan' as a way to to divide scanningAccounts into two lists: one list that the walletScanner is currently scanning for (the always-scan accounts), and a list of accounts that are waiting for the walletScanner to get to them.

I wonder if it would be easier to keep the accounts separate or if it would make it harder to follow?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be easier but popping and pushing the accounts across arrays would be more verbose. I can look into something like this though

if (abort?.signal.aborted) {
return
}
Expand All @@ -230,7 +233,7 @@ export class WalletScanner {
this.logger.debug(`AccountHead DEL: ${header.sequence} => ${Number(header.sequence) - 1}`)

const accounts = (await this.getScanningAccountsWithHead()).filter(({ head }) =>
BufferUtils.equalsNullable(head?.hash, header.hash),
head?.hash.equals(header.hash),
)

for (const { account } of accounts) {
Expand All @@ -241,8 +244,12 @@ export class WalletScanner {
}

for (const account of this.scanningAccounts) {
if (account.scanFrom && BufferUtils.equalsNullable(account.scanFrom.hash, header.hash)) {
account.scanFrom = null
if (account.scanFrom === 'always-scan') {
continue
}

if (account.scanFrom?.head?.hash.equals(header.hash)) {
account.scanFrom = 'always-scan'
}
}
}
Expand Down Expand Up @@ -308,20 +315,26 @@ export class WalletScanner {
*/
private async refreshScanningAccounts(): Promise<void> {
this.scanningAccounts = (await this.getScanningAccountsWithHead()).map(
({ account, head }) => ({ account, scanFrom: head }),
({ account, head }) => ({ account, scanFrom: { head } }),
)
}

private getEarliestHead(): HeadValue | null {
let earliestHead = null
for (const { scanFrom: head } of this.scanningAccounts) {
for (const { scanFrom } of this.scanningAccounts) {
if (scanFrom === 'always-scan') {
continue
}
Comment on lines +324 to +327
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that this will have any effect at the moment. We only call getEarliestHead right after calling refreshScanningAccounts, which will set scanFrom back to the head for each account.


const { head } = scanFrom
if (!head) {
return null
}
if (!earliestHead || earliestHead.sequence > head.sequence) {
earliestHead = head
}
}

return earliestHead
}
}