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

GUI: Change the receive button to respond to keypool state changing #15225

Merged
merged 2 commits into from Jan 31, 2019

Conversation

Projects
None yet
7 participants
@achow101
Copy link
Member

commented Jan 21, 2019

Currently the Receive button in the GUI is displayed enabled or disabled by the initial state of the wallet when the wallet is first loaded. The button is only enabled or disabled depending on whether the disable private keys flag is set when the wallet is loaded. However, future changes to the wallet means that this initial state and check may no longer be accurate. #14938 introduces empty wallets which do not have private keys. An empty wallet that is loaded should have the Receive button disabled, and then it should become enabled once sethdseed is used so that a keypool can be generated and new keys generated. Likewise, with #14075, a wallet can be loaded with no keypool initially, so the button should be disabled. Later, public keys can be imported into the keypool, at which time the button should become enabled. When the keypool runs out again (no new keys are generated as the keypool only consists of imports), the button should become disabled.

This PR makes it so that the button becomes enabled and disabled as the keypool state changes. The check for whether to enable or disable the receive button has changed to checking whether it is possible to get new keys. It now checks for whether the wallet has an HD seed and, if not, whether the private keys are disabled. When an action happens which would make it possible for a new address to be retrieved or make it possible for a no more addresses to be retrieved, a signal is emitted which has the GUI recheck the conditions for the Receive button. These actions are setting a new HD seed, topping up the keypool, retrieving a key from the keypool, and returning a key to the keypool.

@achow101 achow101 force-pushed the achow101:receive-button branch 2 times, most recently Jan 21, 2019

@achow101 achow101 force-pushed the achow101:receive-button branch Jan 22, 2019

@fanquake fanquake added the GUI label Jan 22, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15226 (Allow creating blank (empty) wallets (alternative) by achow101)
  • #15006 (Add option to create an encrypted wallet by achow101)
  • #14075 (Import watch only pubkeys to the keypool if private keys are disabled by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 22, 2019

Looks good.
Concept ACK

A minor not very relevant question would be, if it makes sense to transport the work "keypool" upwards to the GUI... since the GUI don't need to understand the concept of keypool,... just if addresses can be fetched or not.

@Sjors
Copy link
Member

left a comment

Concept ACK.

I agree with @jonasschnelli that the GUI merely needs to know that a new address can be generated. That's already done at the wallet level WalletModel::canGetAddresses(), and I'm fine with renaming the other stuff later. E.g. handleKeypoolChanged -> handleFreshAddressAvailable / handleCanGetAddresses.

See inline feedback on 2795d95bfc6810fd91e6f94943c93ec247b25662

Show resolved Hide resolved src/qt/receivecoinsdialog.cpp Outdated
Show resolved Hide resolved src/qt/receivecoinsdialog.cpp Outdated
Show resolved Hide resolved src/qt/walletmodel.cpp

@achow101 achow101 force-pushed the achow101:receive-button branch Jan 22, 2019

@achow101

This comment has been minimized.

Copy link
Member Author

commented Jan 22, 2019

I've renamed the signal and handlers to be CanGetAddresses instead of Keypool.

@promag
Copy link
Member

left a comment

utACK f76822d, just some comments for your consideration.

Show resolved Hide resolved src/qt/walletmodel.cpp Outdated
Show resolved Hide resolved src/qt/walletmodel.cpp Outdated
Show resolved Hide resolved src/qt/walletmodel.h Outdated
Show resolved Hide resolved src/qt/receivecoinsdialog.cpp Outdated
Notify the GUI that the keypool has changed to set the receive button
Whenever the keypool changes (new keys generated, new seed set,
keypool runs out, etc.), notify the GUI that the keypool has changed. The
receive button can then be enabled and disabled as necessary.

@achow101 achow101 force-pushed the achow101:receive-button branch to 2bc4c3e Jan 23, 2019

@laanwj laanwj added this to Blockers in High-priority for review Jan 24, 2019

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Jan 29, 2019

utACK 2bc4c3e
I think hard to protect against regression as it is (not functional exposed). Ideally test coverage follows in #15226.

@Sjors

This comment has been minimized.

Copy link
Member

commented Jan 30, 2019

Semi tested ACK 2bc4c3e. Based on testing #15226, but they're currently rebased out of sync.

@laanwj laanwj merged commit 2bc4c3e into bitcoin:master Jan 31, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 31, 2019

Merge #15225: GUI: Change the receive button to respond to keypool st…
…ate changing

2bc4c3e Notify the GUI that the keypool has changed to set the receive button (Andrew Chow)
14bcdbe Check for more than private keys disabled to show receive button (Andrew Chow)

Pull request description:

  Currently the Receive button in the GUI is displayed enabled or disabled by the initial state of the wallet when the wallet is first loaded. The button is only enabled or disabled depending on whether the disable private keys flag is set when the wallet is loaded. However, future changes to the wallet means that this initial state and check may no longer be accurate. #14938 introduces empty wallets which do not have private keys. An empty wallet that is loaded should have the Receive button disabled, and then it should become enabled once `sethdseed` is used so that a keypool can be generated and new keys generated. Likewise, with #14075, a wallet can be loaded with no keypool initially, so the button should be disabled. Later, public keys can be imported into the keypool, at which time the button should become enabled. When the keypool runs out again (no new keys are generated as the keypool only consists of imports), the button should become disabled.

  This PR makes it so that the button becomes enabled and disabled as the keypool state changes. The check for whether to enable or disable the receive button has changed to checking whether it is possible to get new keys. It now checks for whether the wallet has an HD seed and, if not, whether the private keys are disabled. When an action happens which would make it possible for a new address to be retrieved or make it possible for a no more addresses to be retrieved, a signal is emitted which has the GUI recheck the conditions for the Receive button. These actions are setting a new HD seed, topping up the keypool, retrieving a key from the keypool, and returning a key to the keypool.

Tree-SHA512: eff15a5337f4c64ecd7169414fb47053c04f6a0f0130341b6dd9799ac4d79f451e25284701c668971fca33f0909d5352a474a2c12349375bedfdb59b63077d50

@fanquake fanquake removed this from Blockers in High-priority for review Jan 31, 2019

meshcollider added a commit that referenced this pull request Feb 10, 2019

Merge #15226: Allow creating blank (empty) wallets (alternative)
7687f78 [wallet] Support creating a blank wallet (Andrew Chow)

Pull request description:

  Alternative (kind of) to #14938

  This PR adds a `blank` parameter to the `createwallet` RPC to create a wallet that has no private keys initially. `sethdseed` can then be used to make a clean wallet with a custom seed. `encryptwallet` can also be used to make a wallet that is born encrypted.

  Instead of changing the version number as done in #14938, a wallet flag is used to indicate that the wallet should be blank. This flag is set at creation, and then unset when the wallet is no longer blank. A wallet becomes non-blank when a HD seed is set or anything is imported. The main change to create a blank wallet is primarily taken from #14938.

  Also with this, the term "blank wallet" is used instead of "empty wallet" to avoid confusion with wallets that have balance which would also be referred to as "empty".

  This is built on top of #15225 in order to fix GUI issues.

Tree-SHA512: 824d685e11ac2259a26b5ece99c67a7bda94a570cd921472c464243ee356b7734595ad35cc439b34357135df041ed9cba951e6edac194935c3a55a1dc4fcbdea
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.