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

asset: create btcwallet+neutrino log dir and file without initialization #1946

Merged
merged 6 commits into from
Dec 2, 2022

Conversation

ukane-philemon
Copy link
Contributor

This fixes an issue where wallet recovery deletes the existing neutrino log file but logging initialization is a one-time operation. Closes #1944.

client/asset/dcr/spv.go Outdated Show resolved Hide resolved
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.

Looks fixed to me.

client/asset/bch/spv.go Outdated Show resolved Hide resolved
@buck54321
Copy link
Member

buck54321 commented Nov 10, 2022

Looks like the spvWallet.dir is

dir: filepath.Join(cfg.WalletCFG.DataDir, cfg.ChainParams.Name, "spv"),

and the log file is created in that directory.

If we instead created the log files in, say,

filepath.Join(cfg.WalletCFG.DataDir, cfg.ChainParams.Name, "spvlogs")

this wouldn't be an issue.

@ukane-philemon
Copy link
Contributor Author

If we instead created the log files in, say,
filepath.Join(cfg.WalletCFG.DataDir, cfg.ChainParams.Name, "spvlogs")
this wouldn't be an issue.

I think the issue is that only the log dir is created while the neutrino.log file is not, causing an error when trying to retrieve the log file. This PR allows rotator.New(...) to create the neutrino.log file when the wallet is created so that calls to download wallet logs will find the neutrino.log file.

@buck54321
Copy link
Member

My read might be wrong, but it seems to me that the problem is that (*spvWallet).moveWalletData is moving log files in the first place.

@ukane-philemon
Copy link
Contributor Author

My read might be wrong, but it seems to me that the problem is that (*spvWallet).moveWalletData is moving log files in the first place.

What is the expected behaviour for new wallets(the create wallet method does not know it's a recovery)? And does it mean the wallet log should be untouched in any scenario?

@buck54321
Copy link
Member

I don't see a reason to start new log files when we perform recovery. We'll want the record to be continuous, no?

@ukane-philemon
Copy link
Contributor Author

I don't see a reason to start new log files when we perform recovery. We'll want the record to be continuous, no?

Hmm, maybe not, because we ought to keep all details regarding the old wallet including the logs on the assetdb-backup directory. So the new wallet will have to start clean?

@chappjc
Copy link
Member

chappjc commented Nov 14, 2022

I think Buck is saying we should just not delete the logs. We can still copy them.

Also with the current PR, dcrwallet loggers keep using the old log rotator. I don't see how it's working.

@ukane-philemon
Copy link
Contributor Author

ukane-philemon commented Nov 14, 2022

I think Buck is saying we should just not delete the logs. We can still copy them.

Okay, I had assumed the expected behaviour for recovery is to remove everything related to the previous wallet(including logs) from the main wallet dir. However, @buck54321 suggestion(#1946 (comment)) will help keep changes minimal, I think.

@ukane-philemon
Copy link
Contributor Author

@buck54321, RE: #1946 (comment), I had to move some things around in 1e56f6d

client/asset/btc/spv_wrapper.go Outdated Show resolved Hide resolved
client/asset/bch/spv.go Outdated Show resolved Hide resolved
client/asset/dcr/spv.go Outdated Show resolved Hide resolved
client/asset/btc/spv.go Outdated Show resolved Hide resolved
- This moves existing spv wallet logs to a new log path(logs -> spvlogs),
  which fixes an issue where wallet recovery removes the existing spv log
  file. Now an spv wallet log file is never deleted. Wallet receovery may
  only copy the wallet log file.
- Fix typos
@chappjc chappjc self-requested a review December 2, 2022 16:39
Copy link
Member

@chappjc chappjc left a comment

Choose a reason for hiding this comment

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

Works so that the wallet file is accessible (and continuous) after recover.

Just need to rework the file copy code so it is buffered, not loading a whole file into memory.

client/asset/btc/spv_wrapper.go Outdated Show resolved Hide resolved
client/asset/btc/spv_wrapper.go Outdated Show resolved Hide resolved
@chappjc chappjc merged commit 4685064 into decred:master Dec 2, 2022
@chappjc chappjc added this to the 0.6 milestone Dec 27, 2022
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.

wallets/internal: Cannot view logs after recover.
4 participants