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

i1722-avoid-wallet-rescan-after-init #1723

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

pragmaxim
Copy link
Collaborator

@pragmaxim pragmaxim commented May 30, 2022

Closes #1722

Introduction of WalletPhase into state is necessary, I reverted a few alternative approaches but in the end I think that it must be decided in case ScanOnChain(newBlock) whether to rescan or not and for that we need WalletPhase information kept in state.

@@ -297,6 +299,8 @@ class ErgoWalletServiceSpec
val pass = Random.nextString(10)
val initializedState = walletService.initWallet(walletState, settings, SecretString.create(pass), Option.empty).get._2

initializedState.getWalletHeight shouldBe 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kushti this test is passing only locally, there seem to be some racing condition on CI, but we rely on the fact that freshly init wallet has waletHeight=0

Copy link
Member

Choose a reason for hiding this comment

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

ah I see, so let's remove this condition? Now it should not be true for wallet just initialized

@pragmaxim pragmaxim changed the title i1722-avoid-wallet-rescan-after-init [Testing] i1722-avoid-wallet-rescan-after-init May 31, 2022
@pragmaxim pragmaxim requested a review from kushti May 31, 2022 12:18
@pragmaxim pragmaxim changed the base branch from master to v4.0.36 July 27, 2022 19:10
@kushti kushti changed the base branch from v4.0.36 to master July 30, 2022 18:11
@@ -265,7 +265,8 @@ class ErgoWalletActor(settings: ErgoSettings,
case ScanOnChain(newBlock) =>
if (state.secretIsSet(settings.walletSettings.testMnemonic)) { // scan blocks only if wallet is initialized
val nextBlockHeight = state.expectedNextBlockHeight(newBlock.height, settings.nodeSettings.isFullBlocksPruned)
if (nextBlockHeight == newBlock.height) {
// we want to scan a block either when it is its turn or when wallet is freshly created (no need to load the past)
if (nextBlockHeight == newBlock.height || (state.walletState == WalletPhase.Created && state.getWalletHeight == 0)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@kushti I've been through this PR one more time, I once found this solution after many thoughts. We basically want the first block applied to chain to be the first block added to freshly created wallet (out of order in this special case). We cannot manually change the digest of the wallet as that can be done only by applying modifiers to it. So this is the best place to do that.

@kushti kushti changed the base branch from master to v5.0.3 November 4, 2022 17:48
@@ -267,7 +267,9 @@ class ErgoWalletActor(settings: ErgoSettings,
case ScanOnChain(newBlock) =>
if (state.secretIsSet(settings.walletSettings.testMnemonic)) { // scan blocks only if wallet is initialized
val nextBlockHeight = state.expectedNextBlockHeight(newBlock.height, settings.nodeSettings.isFullBlocksPruned)
if (nextBlockHeight == newBlock.height) {
// we want to scan a block either when it is its turn or when wallet is freshly created (no need to load the past)
val walletFreshlyCreated = state.walletPhase == WalletPhase.Created && state.getWalletHeight == 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If I remember correctly, only state.getWalletHeight == 0 is not enough as restored wallet has also state.getWalletHeight == 0, that's why the WalletPhase introduction

@@ -563,6 +564,7 @@ class ErgoWalletServiceImpl(override val ergoSettings: ErgoSettings) extends Erg
block,
state.outputsFilter,
dustLimit,
state.walletPhase == WalletPhase.Created,
Copy link
Member

Choose a reason for hiding this comment

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

Why always Created here?

Copy link
Collaborator Author

@pragmaxim pragmaxim Nov 8, 2022

Choose a reason for hiding this comment

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

The scanBlockTransactions method takes walletCreated: Boolean parameter as there is check inside that requires information whether it was restored or created ...

Copy link
Collaborator Author

@pragmaxim pragmaxim Nov 8, 2022

Choose a reason for hiding this comment

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

I could pass WalletPhase argument and it would be probably more readable ... gimme an hour

@@ -252,7 +252,7 @@ class WalletRegistry(store: LDBVersionedStore)(ws: WalletSettings) extends Score

// and update wallet digest
updateDigest(bag3) { case WalletDigest(height, wBalance, wTokensSeq) =>
if (height + 1 != blockHeight) {
if (height + 1 != blockHeight && !walletCreated) {
log.error(s"Blocks were skipped during wallet scanning, from $height until $blockHeight")
Copy link
Collaborator Author

@pragmaxim pragmaxim Nov 8, 2022

Choose a reason for hiding this comment

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

This is the only reason why the walletPhase is being propagated down the WalletRegistry and WalletScanLogic, so if we want to preserve this check, I have to do it ...

@pragmaxim pragmaxim requested a review from kushti November 8, 2022 10:44
@kushti kushti changed the base branch from v5.0.3 to v5.0.4 November 8, 2022 20:35
Base automatically changed from v5.0.4 to master December 6, 2022 12:11
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.

Do not wallet-rescan blockchain from height 0 after wallet initialization
2 participants