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.

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

Prevent unnecessary wallet rescans. #519

Merged
merged 2 commits into from
Sep 13, 2019
Merged

Conversation

jholdstock
Copy link
Member

This is acheived by adding a new stakepoold RPC. Previously we used a single RPC
to import redeem scripts, regardless of whether they were new or historic. A
rescan was triggered every time a script was imported. This was problematic
because on startup, dcrstakepool performs consistency checks against each
back-end and may import multiple scripts into a single wallet simultaneously.

I have split the ImportScript RPC into two ImportNewScript and
ImportMissingScripts.

ImportNewScript will never trigger a rescan because a new script should have
no associated history. This is used when a new wallet is connected to the VSP.
ImportMissingScripts is used by the consistency checks, and will import
multiple scripts before triggering a single rescan.

Closes #517

This is acheived by adding a new stakepoold RPC. Previously we used a single RPC
to import redeem scripts, regardless of whether they were new or historic. A
rescan was triggered every time a script was imported. This was problematic
because on startup, dcrstakepool performs consistency checks against each
back-end and may import multiple scripts into a single wallet simultaneously.

I have split the `ImportScript` RPC into two `ImportNewScript` and
`ImportMissingScripts`.

`ImportNewScript` will never trigger a rescan because a new script should have
no associated history. This is used when a new wallet is connected to the VSP.
`ImportMissingScripts` is used by the consistency checks, and will import
multiple scripts before triggering a single rescan.
Copy link
Member

@JoeGruffins JoeGruffins left a comment

Choose a reason for hiding this comment

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

Works great! Left some suggestions.

return -1, err
}
return block, nil
}

func (ctx *AppContext) ImportMissingScripts(scripts [][]byte, rescanHeight int) error {

Copy link
Member

Choose a reason for hiding this comment

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

Do you think it's worth checking that len(scripts) != 0 since it could cause a panic? I do realize that you are checking before calling, but someone else may not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, we don't currently have any request validation for the stakepoold RPC server, and I would be reluctant to add it for only one request. We should either add validation to all requests for consistency, or seeing as are controlling both the client and the server code, and seeing as we dont expect any additional clients, I think we could leave it as-is. Obviously we would be a bit more careful if this were a public API with external clients.

Happy to change if others disagree.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Maybe just a comment then?

backend/stakepoold/rpc/rpcserver/context.go Show resolved Hide resolved
backend/stakepoold/rpc/rpcserver/context.go Show resolved Hide resolved
backend/stakepoold/rpc/rpcserver/context.go Outdated Show resolved Hide resolved
stakepooldclient/stakepooldclient.go Show resolved Hide resolved
@dajohi dajohi merged commit 185f8f2 into decred:master Sep 13, 2019
girino added a commit to girino/dcrstakepool that referenced this pull request Sep 19, 2019
* commit '0cec13529c159059eff39dc6eb5b69cde5c1bd2d':
  Add 1.2.0 release note. (decred#509)
  multi: cleanup (decred#527)
  Ensure low fee tickets are detected upon maturation. (decred#524)
  Add RPC automatic reconnections (decred#510)
  Prevent unnecessary wallet rescans. (decred#519)
  enablevoting=0 in dcrwallet conf (decred#520)
  stakepoold: Stop if wallet voting is enabled (decred#523)
jyap808 pushed a commit to ubiq/dcrstakepool that referenced this pull request Dec 16, 2019
* Prevent unnecessary wallet rescans.

This is acheived by adding a new stakepoold RPC. Previously we used a single RPC
to import redeem scripts, regardless of whether they were new or historic. A
rescan was triggered every time a script was imported. This was problematic
because on startup, dcrstakepool performs consistency checks against each
back-end and may import multiple scripts into a single wallet simultaneously.

I have split the `ImportScript` RPC into two `ImportNewScript` and
`ImportMissingScripts`.

`ImportNewScript` will never trigger a rescan because a new script should have
no associated history. This is used when a new wallet is connected to the VSP.
`ImportMissingScripts` is used by the consistency checks, and will import
multiple scripts before triggering a single rescan.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent multiple concurrent wallet rescans
3 participants