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

client/ui/btc: Recover BTC SPV wallet #1507

Merged
merged 1 commit into from May 20, 2022
Merged

Conversation

martonp
Copy link
Contributor

@martonp martonp commented Mar 4, 2022

Adds the ability to delete all the files from the BTC SPV wallet and recreate it from scratch.

  • client/asset/interface: A Recoverer trait is added that includes three functions
    • GetRecoveryCfg: Attempts to retrieve a map[string]string from the existing wallet that would help in putting the
      wallet back to the state is was in before all the wallet files were deleted.
    • Destroy: deletes all the wallet files
    • ApplyRecoveryCfg: passes the settings that were retrieved using GetRecoveryCfg to a newly created wallet.
  • client/asset/btc: Implements the Recoverer interface. The recovery configuration includes the number of internal and number of external addresses that the wallet generated. This amount of addresses are regenerated in the new instance of the wallet. If this was not done and there were active orders being managed when the user recovered the wallet, the new wallet would not be able to redeem and swaps because it wouldn't have access to the required private keys.
  • client/core: A Recover function is added that destroys the old wallet and creates a new one. It contains a force parameter similar to Rescan which unless is set to true, will not allow the user to recover a wallet if there are currently active deals requiring the wallet. An activeOrdersErr is returned from the wallet if force=false and there are currently active deals.
  • app: A button is added on the wallet's page to allow users to recover a wallet if the wallet has the Recoverer trait. The "force" workflow is implemented for both rescan and recover. If either of these requests return the activeOrdersErr, another popup will show up prompting the user to confirm that they would like to take this action even though there are active orders. If the user confirms, the same request will be resubmitted with force=true.

Closes #1438.

@JoeGruffins
Copy link
Member

closes #1438 ?

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 well without active order, but if you have some can't redeem with [ERR] CORE: 127.0.0.1:17273 tick: {redeemMatches order 39f776f7ae672bd2ca7fcad3ff7ad6b2f8700f5aa30d32fa7e5348db89ee08d5 - {error sending redeem transaction: unable to find key for addr bcrt1qk2aq3m7hl2k5shc33d7hujg5qh8qngvjua04j6}}

I don't understand why this is, it is just an external address owned by the wallet?

client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
client/webserver/api.go Outdated Show resolved Hide resolved
client/webserver/locales/en-us.go Outdated Show resolved Hide resolved
@martonp martonp marked this pull request as draft March 10, 2022 16:32
@martonp
Copy link
Contributor Author

martonp commented Mar 10, 2022

I've founds some issues with this, putting it back into draft for now.

@martonp martonp force-pushed the recoverWallet branch 3 times, most recently from d7320eb to 3d9e7a4 Compare April 14, 2022 03:15
@martonp martonp marked this pull request as ready for review April 14, 2022 03:32
client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/btc/spv.go Outdated Show resolved Hide resolved
client/asset/interface.go Outdated Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
client/core/core.go Outdated Show resolved Hide resolved
@chappjc chappjc added this to the 0.5 milestone Apr 23, 2022
@chappjc chappjc self-requested a review April 30, 2022 13:59
@buck54321
Copy link
Member

This approach seems off to me. Why not handle this more generally in Core and leave the wallets alone? Seems like we're relying on a wallet that has proven useless to fix itself for us.

@martonp
Copy link
Contributor Author

martonp commented May 4, 2022

@buck54321 GetRecoveryCfg and ApplyRecoveryCfg are not required to work, but they make a best effort to recover some useful things from the wallet. If we are able to at least know how many addresses the wallet had created, then we can avoid reusing any addresses in the recovered wallet. Also, if there were active deals when the wallet was recovered, the wallet will have created the addresses needed to do redemptions. If these functions fail, then there's still no problem.. the wallet files are just deleted and the wallet is recreated from scratch. I think it's good to make an effort to try to recover what can be recovered though.

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!

Deleted my neutrino db and then started a trade. Recovered in the middle of a trade and it finished fine!

I'm not so sure about the naming of the button. "Recover" sounds to me like you want to recover from seed or something. I don't have a better suggestion at the moment though.

Also, I think this screen should also have an "abort/cancel" button:
image

client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
Comment on lines 3155 to 3158
if err != nil {
btc.log.Errorf("Failed to get peer count: %v", err)
btc.peersChange(0)
}
Copy link
Member

Choose a reason for hiding this comment

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

Should return I think. wallet will be nil below.

Comment on lines 1244 to 1273
// deleteWalletData will delete all files except peers.json from the
// wallet's directory.
func (w *spvWallet) deleteWalletData() error {
Copy link
Member

Choose a reason for hiding this comment

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

I would suppose it's likely this sometimes fails if the wallet is borked and unable to shut down. The OS (windows?) could be holding these files preventing any io actions until the process is hard stopped. Any maybe Linux will continue writing stuff after it's deleted? Not sure though. Maybe there are OS specific ways of killing the process? Nothing needs to be done right now imo, just commenting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If there's any process holding locks on the files, then it would be the dexc processs wouldn't it? Is it true that the process holding a lock on a file is allowed to delete the file?

Copy link
Member

@chappjc chappjc May 10, 2022

Choose a reason for hiding this comment

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

I have not reviewed this PR yet (very sorry), but I would not use os.Remove(anything) on a file that's still opened. What would happen to the file handle that dexc current has on the db? Become in valid? Note that there's no file handle arg on that, it's just a syscall. Before deleting anything I would close it.

Copy link
Member

Choose a reason for hiding this comment

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

It would probably be better to use a new database folder and not delete anything, if that is doable.

Or move/rename the old folder to something like oldName.save_date? Then can always recover from the recover.

Here's some info: https://www.baeldung.com/linux/open-file-handle-after-move-delete

Seems to suggest, on linux, that both deleting and renaming to a file on the same disc would do what we want. The file handles will still be pointing to the old files, even if they do not exist. Do not know what happens on Windows, but I suspect deletion or renaming would fail because file is in use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's some info: https://www.baeldung.com/linux/open-file-handle-after-move-delete

That's a good read. spvWallet.stop() closes all the file handles, and it is called before Destroy. Are you saying that in case spvWallet.stop() fails, I should find any file handles that were left and close them before calling os.Remove?

A backup folder is definitely a good idea though.

Copy link
Member

Choose a reason for hiding this comment

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

I should find any file handles that were left and close them before calling os.Remove?

If this was directed to me... no. It looks like from reading that that it doesn't matter if you rename or delete the file. The new folder will not be the same thing internally for linux, and Windows will probably just fail whatever you do.

The wallet could potentially not run if it is trying to listen on the same ports though, and the bad instance is still using those.

client/asset/btc/spv.go Outdated Show resolved Hide resolved
client/asset/btc/btc.go Outdated Show resolved Hide resolved
@chappjc
Copy link
Member

chappjc commented May 10, 2022

This approach seems off to me. Why not handle this more generally in Core and leave the wallets alone? Seems like we're relying on a wallet that has proven useless to fix itself for us.

I've reached the same conclusion, but I think we're talking about different aspects (getting the recovery data does not bother me). When I look at (*Core).RecoverWallet, it's pretty much what I would do, but the stuff I would avoid is the changes to client/asset/btc.baseWallet.

Very roughly speaking, RecoverWallet is very logical, and it does what I think is enough and would leave alone baseWallet for the most part:

  • grab the account indexes for faithful recovery, if possible
  • shutdown the wallet (the xcWallet.Wallet via the connector field's Disconnect)
  • clobber the old assetdb stuff
  • create a new assetdb folder via CreateWallet (and I wonder if the recovery settings map could just be merged with the map used here)
  • load and connect the new one
  • patch up all the dc.trades wallet pointers

AFAICT, there's no strong reason for the nodeAccessor business in baseWallet. Core just needs to remove it from the Core.wallets map during the recovery process to prevent it from being reconnected automatically by a caller. This would be just like what happens at the end of (*Core).Run on shutdown.

@buck54321
Copy link
Member

  • grab the account indexes for faithful recovery, if possible
  • shutdown the wallet (the xcWallet.Wallet via the connector field's Disconnect)
  • clobber the old assetdb stuff
  • create a new assetdb folder via CreateWallet (and I wonder if the recovery settings map could just be merged with the map used here)
  • load and connect the new one
  • patch up all the dc.trades wallet pointers

Exactly the suggestion I was going to write up today. Thanks, @chappjc.

@martonp
Copy link
Contributor Author

martonp commented May 11, 2022

@chappjc @buck54321 Without the nodeAccessor in the wallet, I think there would need to be some concurrency control in the trackedTrade, because even if a wallet is disconnected and does not call tipChange anymore, there will still be calls to tick from a few other places. When tick is called, some methods on the trackedTrade.fromWallet and trackedTrade.toWallet are called, which causes panics because the files those methods need to use will no longer exist. So I think either there will need to be some added complexity in the trackedTrade or in the wallet implementation.

@chappjc
Copy link
Member

chappjc commented May 11, 2022

When tick is called, some methods on the trackedTrade.fromWallet and trackedTrade.toWallet are called, which causes panics because the files those methods need to use will no longer exist

I'll toy with it, but I think there will just be errors to the effect of not connected. The nodeAccessor goes to a lot of trouble to return an "recovery in progress error". Either way, just an error. If we really are concerned about this, we can swap out the walletSet with a stub before pulling the plug.

@chappjc
Copy link
Member

chappjc commented May 11, 2022

I have done no testing at all on the following, it's just for illustration: chappjc@94a2f08

@martonp
Copy link
Contributor Author

martonp commented May 14, 2022

@chappjc Those were nice changes, everything worked except Unmapify didn't work properly when embedding the RecoveryCfg inside WalletConfig. Calling Unmapify twice using both works though.

It's definitely nicer with NodeAccessor gone. I guess the error messages aren't a big deal.

@martonp
Copy link
Contributor Author

martonp commented May 14, 2022

Screen Shot 2022-05-14 at 9 12 15 PM

I've added a cancel button.

@chappjc
Copy link
Member

chappjc commented May 14, 2022

@chappjc Those were nice changes, everything worked except Unmapify didn't work properly when embedding the RecoveryCfg inside WalletConfig. Calling Unmapify twice using both works though.

It's definitely nicer with NodeAccessor gone. I guess the error messages aren't a big deal.

Oh I was thinking json tags not ini. That's annoying. It might have an inline or embed directive (like omitempty), but whatever I don't like relying on quirks of imported community packages.

Copy link
Member

@buck54321 buck54321 left a comment

Choose a reason for hiding this comment

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

Good stuff. Tests well.

client/asset/btc/spv.go Outdated Show resolved Hide resolved
client/asset/btc/spv.go Outdated Show resolved Hide resolved
client/asset/btc/spv.go Outdated Show resolved Hide resolved
defer encode.ClearBytes(seed)
defer encode.ClearBytes(pw)

if recoveryCfg, err := oldWallet.GetRecoveryCfg(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

This is the only place that (*xcWallet).GetRecoveryCfg is used, and you could just as well grab the asset.Recoverer when you check isRecoverer just a few lines up. I guess you're technically checking twice whether this is a asset.Recoverer.

I would personally remove (*xcWallet).GetRecoveryCfg, grab the asset.Recoverer above and use its GetRecoveryCfg method directly.

Comment on lines 2097 to 2100
// Unseeded wallets shouldn't implement the Recoverer interface. This
// is just an additional check for safety.
if !walletDef.Seeded {
return fmt.Errorf("cannot only recover a seeded wallet")
Copy link
Member

Choose a reason for hiding this comment

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

cannot → can, but I don't know that we need to impose this arbitrary limitation. Even unseeded wallets are free to use the DataDir for local storage, so there could potentially be some cleanup to do during a recovery.

Copy link
Member

Choose a reason for hiding this comment

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

This check is getting out ahead of the error you'd hit with asset.CreateWallet a few lines down

client/asset/driver.go:

// CreateWallet creates a new wallet. Only use Create for seeded wallet types.
func CreateWallet(assetID uint32, seedParams *CreateWalletParams) error {

client/asset/btc/btc.go:

// Create creates a new SPV wallet.
func (d *Driver) Create(params *asset.CreateWalletParams) error {
if params.Type != walletTypeSPV {
	return fmt.Errorf("SPV is the only seeded wallet type. required = %q, requested = %q", walletTypeSPV, params.Type)
}
if len(params.Seed) == 0 {
	return errors.New("wallet seed cannot be empty")
}

If "recovery" is modified to do something other than Create, I would agree it's an unnecessarily limitation, but it's presently central to the definition of recover. And because we call Destroy before doing Create, the consequences for not checking this first are potentially severe.

client/webserver/site/src/html/wallets.tmpl Outdated Show resolved Hide resolved
client/webserver/site/src/html/wallets.tmpl Show resolved Hide resolved
this.closePopups()
} else {
page.recoverWalletErr.textContent = res.msg
Doc.show(page.recoverWalletErr)
Copy link
Member

Choose a reason for hiding this comment

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

You'll want to Doc.hide this at the top of recoverWallet too, as a lingering message for an e.g. password typo might be confusing while the request is being processed.

@@ -67,12 +69,22 @@ export default class WalletsPage extends BasePage {
openAsset: number
walletAsset: number
reconfigAsset: number
forms: PageElement[]
forceReq: any
Copy link
Member

Choose a reason for hiding this comment

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

Let's define a type here.

interface RescanRecoveryRequest {
  assetID: number
  appPW?: string
  force?: boolean
}

Comment on lines 344 to 345
// Destroy will delete all the wallet files so the wallet can be recreated.
Destroy() error
Copy link
Member

Choose a reason for hiding this comment

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

I wish that Destroy wasn't needed. *Core should really just delete the DataDir. We can't though, because *Core doesn't set the network (mainnet/testnet/simnet) directory and relies on the wallets to do that part. Dang.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest we change this to Move() at least, I think that is better practice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated it to Move().

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

Working well for me.

Only thing I noticed, which may just be from the bootstrap 5 migration, is that the col-14 lacks left and right padding now. From what I can tell, columns no longer have position: relative or a padding. No worries if this is an unrelated thing... I'm just noticing it now is all.

image
(testnet obv)

vs 0.4.3 where it had 15 pixels

image

Maybe I'm just noticing what's on master now.

Adds the ability to delete all the files of the BTC SPV wallet and
recreate it from scratch.
@chappjc chappjc merged commit 05276d7 into decred:master May 20, 2022
@martonp martonp deleted the recoverWallet branch January 20, 2024 13:16
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.

similar to Rescan, need a way to Recreate a wallet for recovery
4 participants