iron-fish / ironfish

A novel cryptocurrency focused on privacy and accessibility.
https://ironfish.network
Mozilla Public License 2.0
964 stars 577 forks source link

Maximize decryption parallelism during wallet scans #5058

Closed andiflabs closed 1 week ago

andiflabs commented 3 weeks ago

Summary

A wallet scan currently works like this:

graph LR;
  r[read block]-->d[decrypt notes]
  d-->u[update accounts]
  u-.->r

read block and update accounts are primarily IO-bound operations, and they run in the main thread only. This means that while read block and update accounts are running, the CPUs are under-utilized.

To maximize CPU utilization (and therefore improve scan performance), the scan has been changed to look like this:

graph LR;
  r[read block]-.->r
  r-->q[queue]
  q-->d[decrypt notes]
  d-->u[update accounts]

With this new algorithm, read block will try to read as many blocks as possible, without stopping, and place them on a queue. The queue is consumed and notes are decrypted in parallel inside the worker threads. Because read block is significantly faster than decrypt notes, the queue will normally be almost full, and so the worker threads are fully utilized.

Testing Plan

Documentation

N/A

Breaking Change

N/A

NullSoldier commented 1 week ago

After the latest change, I'm starting to see these errors emmitted from the logs.

Could not parse payload from request: (jobId: 66, type: JobAborted, body: '')

Reproduction

NullSoldier commented 1 week ago

When running the CLI command ironfish wallet:rescan I got this error on my node

/Users/jasonspafford/projects/fish/ironfish/src/assert.ts:20
      throw new Error(message || `Expected value not to be undefined`)
            ^

Error: Expected value not to be undefined
    at Function.isNotUndefined (/Users/jasonspafford/projects/fish/ironfish/src/assert.ts:20:13)
    at WalletDB.getHead (/Users/jasonspafford/projects/fish/ironfish/src/wallet/walletdb/walletdb.ts:399:12)
    at <anonymous> (/Users/jasonspafford/projects/fish/ironfish/src/wallet/scanner/walletScanner.ts:293:21)
    at async Promise.all (index 69)
    at WalletScanner.refreshScanningAccounts (/Users/jasonspafford/projects/fish/ironfish/src/wallet/scanner/walletScanner.ts:287:29)
    at WalletScanner.scan (/Users/jasonspafford/projects/fish/ironfish/src/wallet/scanner/walletScanner.ts:78:7)
    at Wallet.scan (/Users/jasonspafford/projects/fish/ironfish/src/wallet/wallet.ts:190:18)
andiflabs commented 1 week ago

After the latest change, I'm starting to see these errors emmitted from the logs.

Could not parse payload from request: (jobId: 66, type: JobAborted, body: '')

It happens rarely for me, but I was able to observe it. It's caused by Job.abort() sending a JobAbortedMessage:

this.worker.send(new JobAbortedMessage(this.id))

which is a message of type JobAborted:

super(WorkerMessageType.JobAborted, jobId)

which, according to Worker, should never be sent:

case WorkerMessageType.JobAborted:
  throw new Error('JobAbort should not be sent as a request')

My guess is that this bug has been hidden for a while because perhaps this is the first use of .abort() that we have. In any case, I'll work on getting it fixed, but if you have some historical context on the intentions of this code, please share.

andiflabs commented 1 week ago

https://github.com/iron-fish/ironfish/pull/5093 should fix the "Could not parse payload from request" problem