Scan wallet accounts from earliest account head first #4964
Conversation
The previous version of isAccountUpToDate checked against the wallet's head, but the wallet head is an arbitrary block, so the chain head would be more appropriate. However, I think this is too restrictive to be enforced as a hard requirement at the SDK level, since you might have knowledge that your account only has transactions through a limited number of blocks. A potential downside could be accidentally creating double spends where users would have previously been prevented from creating transactions.
dguenther
changed the title
| @@ -1822,14 +1757,16 @@ export class Wallet { | |||
| return latestHead ? latestHead.hash : null | |||
| } | |||
|
|
|||
| async isAccountUpToDate(account: Account): Promise<boolean> { | |||
| async isAccountUpToDate(account: Account, confirmations?: number): Promise<boolean> { | |||
There was a problem hiding this comment.
it looks like the only place this is used now is in RPC calls to remove an account
i'm not really sure why we require confirmation to remove an account that isn't synced 🤔
There was a problem hiding this comment.
That's a good question... seems like the main case might be when you export, then re-import an account, you might want to make sure the account doesn't have a balance before deleting it, in case you haven't backed it up.
Or if you somehow create an account without a birthday, give someone the public address, they send you tokens, and you delete the account (thinking the transaction didn't work) before fully syncing up.
We could probably re-think how we prompt users about that though 🤔
| @@ -187,6 +187,7 @@ export class Wallet { | |||
| try { | |||
| let hashChanged = false | |||
| do { | |||
| this.chainProcessor.hash = await this.getEarliestHeadHash() | |||
There was a problem hiding this comment.
i don't think that iterating over heads will have an impact relative to the current performance since right now we're checking isAccountUpToDate on every account before decrypting notes
Summary
This PR updates the chain processor's head before every loop in updateHead, rather than loading the earliest account head into the chain processor when the node starts.
Notably, this also removes the idea of a wallet head (previously, chainProcessor.hash), since each account has its own head.
Also notably, sendTransaction was previously gated on whether the account head was the same as the chain processor head, but now it's gated on whether the account head is within
confirmationsblocks of the chain head. I don't think the gate made sense before, but I'm concerned it might be too sensitive now.This PR should make it easier to implement starting/stopping scanning (See #4943), because when scanning is resumed on an account, it'll automatically be picked up by
updateHeadwithout needing to trigger a manual rescan.FIxes IFL-2568
Testing Plan
Did some manual testing of this by adding and removing multiple accounts.
Documentation
Does this change require any updates to the Iron Fish Docs (ex. the RPC API
Reference)? If yes, link a
related documentation pull request for the website.
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and label it with
breaking-change-rpcorbreaking-change-sdk.