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/{core,db}: headless options #1568

Merged
merged 4 commits into from
Dec 15, 2022
Merged

client/{core,db}: headless options #1568

merged 4 commits into from
Dec 15, 2022

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Apr 7, 2022

These options are aimed at improving startup and shutdown speed of headless clients by avoiding unneeded work.

client/{core,db}: options for core startup/shutdown

This adds two new core.Config options aimed at improving startup
and shutdown depending on the user's use case:
 - NoAutoWalletLock instructs Core to skip locking the wallet on
   shutdown. This can be helpful if the user wants the wallet to remain
   unlocked. e.g. They started with the wallet unlocked, or they intend
   to start Core again and wish to avoid the time to unlock a locked
   the wallet on startup.
 - NoAutoDBBackup instructs the DB to skip the creation of a backup DB
   file on shutdown. This is useful if the consumer is using the
   BackupDB method, or simply creating manual backups of the DB file
   after shutdown.

These options are intended for direct Core consumers, such as headless
applications that drive Core directly instead of via the browser UI.

These options are created so that the zero-value corresponds to legacy
behavior. That is, the defaults are the existing behavior. However, in
client/db/bolt, the BackupOnShutdown is reversed from Core's
NoAutoDBBackup flag. The NewDB constructor accepts an optional db.Opts
struct, and if it is not provided, a defaultOpts is used that has
BackupOnShutdown set to true.
client/{asset,core}: add UnlockCoinsOnLogin option

This adds the core.Config.UnlockCoinsOnLogin option that indicates that
on wallet connect during login, or on creation of a new wallet, all
coins with the wallet should be unlocked.

To support this "unlock all" with the wallets, the ReturnCoins method
of the asset.Wallet interface now interprets a nil Coins slice to mean
that all coins should be unlocked. This is similar to the lockunspent
RPC's semantics.

The individual implementations of ReturnCoins are modified to recognize
a nil unspent input slice and accordingly request the wallet backend
unlock all coins.

@chappjc chappjc changed the title client/{core,db}: options client/{core,db}: headless options May 6, 2022
client/core/core.go Outdated Show resolved Hide resolved
client/core/core.go Show resolved Hide resolved
@chappjc chappjc force-pushed the core-opts branch 2 times, most recently from 850720a to 3cc70c3 Compare December 1, 2022 18:52
@chappjc chappjc marked this pull request as ready for review December 1, 2022 18:52
@chappjc
Copy link
Member Author

chappjc commented Dec 1, 2022

Working as intended. Added dexc command line switches, but the intended use is directly via Core (Go consumers)

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

Can't seem to get --no-wallet-lock to work.

  1. Client running with flag, connected to dcr rpc alpha node
  2. Unlock wallet.
  3. In harness, ./alpha walletislocked returns false
  4. Shut down dexc
  5. In harness, ./alpha walletislocked returns true

Comment on lines 2335 to 2336
btc.fundingCoins = make(map[outPoint]*utxo)
return btc.node.lockUnspent(true, nil)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe

if err := btc.node.lockUnspent(true, nil); err != nil {
	return err
}
btc.fundingCoins = make(map[outPoint]*utxo)
return nil

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. going to apply that in the selective utxo loop below too.

Comment on lines +2282 to +2363
if c.cfg.UnlockCoinsOnLogin {
if err = wallet.ReturnCoins(nil); err != nil {
c.log.Errorf("Failed to unlock all %s wallet coins: %v", unbip(wallet.AssetID), err)
}
}

Copy link
Member

Choose a reason for hiding this comment

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

On wallet creation too?

Copy link
Member Author

@chappjc chappjc Dec 5, 2022

Choose a reason for hiding this comment

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

Correct. It's in the docs and help for the cli switch. You mean the field name should change? UnlockCoinsOnInitialAccess? The most routine use is login of course.

I didn't really want to wire this in to the connect helpers.

@chappjc

This comment was marked as resolved.

@chappjc
Copy link
Member Author

chappjc commented Dec 5, 2022

Hmm, That's strange. I did verify that with the flag, the message c.log.Infof("Locking %s wallet", symb) was not printed out. Will check if there's locking going on elsewhere. Did you note the same change in the logs?

Oh dear, we've always been locking wallets twice it seems. Once on Run shutdown and once in Logout as well. :/

@chappjc
Copy link
Member Author

chappjc commented Dec 5, 2022

I'm undecided about the last commit ab7660f. Has potential for gotchas, and it's code that hasn't been giving us trouble thus far. If the work in this PR finds it's way back to release-v0.5, I don't want it with mods to that part of the code.

@chappjc chappjc modified the milestones: 0.5.7, 0.5.8 Dec 7, 2022
This adds two new core.Config options aimed at improving startup
and shutdown depending on the user's use case:
 - NoAutoWalletLock instructs Core to skip locking the wallet on
   shutdown. This can be helpful if the user wants the wallet to remain
   unlocked. e.g. They started with the wallet unlocked, or they intend
   to start Core again and wish to avoid the time to unlock a locked
   wallet on startup.
 - NoAutoDBBackup instructs the DB to skip the creation of a backup DB
   file on shutdown. This is useful if the consumer is using the
   BackupDB method, or simply creating manual backups of the DB file
   after shutdown.

These options are intended for direct Core consumers, such as headless
applications that drive Core directly instead of via the browser UI.

These options are created so that the zero-value corresponds to legacy
behavior. That is, the defaults are the existing behavior. However, in
client/db/bolt, the BackupOnShutdown is reversed from Core's
NoAutoDBBackup flag. The NewDB constructor accepts an optional db.Opts
struct, and if it is not provided, a defaultOpts is used that has
BackupOnShutdown set to true.
This adds the core.Config.UnlockCoinsOnLogin option that indicates that
on wallet connect during login, or on creation of a new wallet, all
coins with the wallet should be unlocked.

To support this "unlock all" with the wallets, the ReturnCoins method
of the asset.Wallet interface now interprets a nil Coins slice to mean
that all coins should be unlocked. This is similar to the lockunspent
RPC's semantics.

The individual implementations of ReturnCoins are modified to recognize
a nil unspent input slice and accordingly request the wallet backend
unlock all coins.
      --no-wallet-lock        Disable locking of wallets on shutdown.
      Use this if you want your external wallets to stay unlocked after
      closing the DEX app.
      --no-db-backup          Disable creation of a database backup on
      shutdown.
      --release-wallet-coins  On login or wallet creation, instruct the
      wallet to release any coins that it may have locked.
@chappjc chappjc merged commit 391f2fd into decred:master Dec 15, 2022
@chappjc chappjc deleted the core-opts branch December 15, 2022 01:58
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.

3 participants