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.

Sign up for GitHub

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

Jump to bottom

Fix regression in "Encrypt Wallet" menu item #393

Merged
merged 1 commit into from
Aug 5, 2021
Merged

Fix regression in "Encrypt Wallet" menu item #393

merged 1 commit into from
Aug 5, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 3, 2021 •
edited
Loading

Fix #392.

Adding a new item to the m_wallet_selector must follow the establishment of a connection between the WalletView::encryptionStatusChanged signal and the BitcoinGUI::updateWalletStatus slot.

This was a regression introduced in bitcoin/bitcoin@20e2e24 (#29).

An encrypted wallet being auto-loaded at the GUI startup:

Screenshot from 2021-08-03 22-38-49

  • with this PR

Screenshot from 2021-08-03 22-34-58

@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
d54d949 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.
@hebasto hebasto mentioned this pull request Aug 3, 2021
Closed
@hebasto hebasto added this to the 22.0 milestone Aug 3, 2021
@hebasto hebasto added Bug Something isn't working UX All about "how to get things done" Needs backport (22.x) labels Aug 3, 2021
@hebasto
Copy link
Member Author

hebasto commented Aug 3, 2021

cc @katesalazar @jarolrod @meshcollider @achow101 @promag

@achow101
Copy link
Member

achow101 commented Aug 3, 2021

ACK d54d949

Tested that this fixes the issue.

For those wondering why this fixes the issue, m_wallet_selector->addItem ends up calling a lot of setup functions and emits several signals. One of those signals is encryptionStatusChanged. However previously this signal was not connected to its slot so when that signal was emitted, it did not have the intended effect of greying out the Encrypt Wallet option. By moving addItem to after all of the signals have been connected, the signal will be correctly handled.

@hebasto hebasto added the Wallet label Aug 4, 2021
jarolrod
jarolrod reviewed Aug 5, 2021
View reviewed changes
Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

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

ACK d54d949

Tested on macOS 12 and Ubuntu 20.4.02

The related code is a bit entangled, but I have ensured that this is correct in its implementation and is a good way to fix the issue. Below are screenshots comparing master and this PR:

Master. PR.
master pr

This was tested by starting with an already encrypted wallet. On the master branch, we can notice the issue described in #392. On the PR branch, we can see that this is fixed.

@hebasto hebasto merged commit e9472e6 into bitcoin-core:master Aug 5, 2021
@hebasto hebasto deleted the 210803-encrypt branch August 5, 2021 06:49
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 5, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
98369d8 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
@hebasto hebasto mentioned this pull request Aug 5, 2021
Merged
@hebasto
Copy link
Member Author

hebasto commented Aug 5, 2021

Backported in bitcoin/bitcoin#22629.

@hebasto hebasto mentioned this pull request Aug 5, 2021
Closed
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 5, 2021
@hebasto
Merge bitcoin-core/gui#393: Fix regression in "Encrypt Wallet" menu item
a4f8ab0 
d54d949 qt: Fix regression in "Encrypt Wallet" menu item (Hennadii Stepanov)

Pull request description:

  Fix #392.

  Adding a new item to the `m_wallet_selector` must follow the establishment of a connection between the `WalletView::encryptionStatusChanged` signal and the `BitcoinGUI::updateWalletStatus` slot.

  This was a regression introduced in bitcoin@20e2e24 (#29).

  ---

  An _encrypted_ wallet being auto-loaded at the GUI startup:
  - on master (eaf09bd)

  ![Screenshot from 2021-08-03 22-38-49](https://user-images.githubusercontent.com/32963518/128075837-cdbb2047-5327-43ea-b2d5-2dcdef67cdc0.png)

  - with this PR

  ![Screenshot from 2021-08-03 22-34-58](https://user-images.githubusercontent.com/32963518/128075572-cb727652-ad44-4b85-bf64-edcd19f9dea1.png)

ACKs for top commit:
  achow101:
    ACK d54d949
  jarolrod:
    ACK d54d949

Tree-SHA512: 669615ec8e1517c2f4cdf59bd11a7c85be793ba0dda112361cf95e6c2f0636215fed331d26a86dc9b779a49defae1b248232f98dab449584376c111c288e87bb
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 5, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
f20bbdc 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 6, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
c7a0030 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 6, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
a2ac4f0 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 6, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
0d7f06d 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 7, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
8e67850 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 9, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
d0010f5 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 9, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
0ae7dd8 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 19, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
76f3e87 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 20, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
eb67772 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 20, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
0ffa79a 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
hebasto added a commit to hebasto/bitcoin that referenced this pull request Aug 20, 2021
@hebasto
qt: Fix regression in "Encrypt Wallet" menu item
e9b4487 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
laanwj added a commit to bitcoin/bitcoin that referenced this pull request Aug 26, 2021
@laanwj
Merge #22629: [22.x] rc3 backports
4a25e39 
32e1424 Fix build with Boost 1.77.0 (Rafael Sadowski)
cb34a0a qt: Handle new added plurals in bitcoin_en.ts (Hennadii Stepanov)
068985c doc: Mention the flat directory structure for uploads (Andrew Chow)
27d43e5 guix: Don't include directory name in SHA256SUMS (Andrew Chow)
88fb7e3 test: fix bug in 22686 (S3RK)
63fec7e clientversion: No suffix #if CLIENT_VERSION_IS_RELEASE (Carl Dong)
dfaffbe test: Test for ApproximateBestSubset edge case with too little fees (Andrew Chow)
e86b023 wallet: Assert that enough was selected to cover the fees (Andrew Chow)
ffc81e2 wallet: Use GetSelectionAmount for target value calculations (Andrew Chow)
ce77b45 release: Release with separate SHA256SUMS and sig files (Carl Dong)
cb491bd guix-verify: Non-zero exit code when anything fails (Carl Dong)
6a611d2 gui: ensure external signer option remains disabled without signers (Andrew Chow)
e9b4487 qt: Fix regression in "Encrypt Wallet" menu item (Hennadii Stepanov)
57fce06 consensus/params: simplify ValidDeployment check to avoid gcc warning (Anthony Towns)
e9d30fb ci: Run fuzzer task for the master branch only (Hennadii Stepanov)

Pull request description:

  Backported:

  1) #22730
  1) bitcoin-core/gui#393
  1) #22597
  1) bitcoin-core/gui#396
  1) #22643
  1) #22642
  1) #22685
  1) #22686
  1) #22654
  1) #22742
  1) bitcoin-core/gui#406
  1) #22713

ACKs for top commit:
  laanwj:
    Code list-of-backported-PRs review ACK 32e1424

Tree-SHA512: f5e2dd1be6cdcd39368eeb5d297b3ff4418d0bf2e70c90e59ab4ba1dbf16f773045d877b4997511de58c3aca75a978dcf043e338bad23951557e2a27ccc845f6
fujicoin added a commit to fujicoin/fujicoin-22.0 that referenced this pull request Aug 27, 2021
@fujicoin
qt: Fix regression in "Encrypt Wallet" menu item
e63c997 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d94959869b0c363939163b99ba0475751dcb6
gwillen pushed a commit to gwillen/elements that referenced this pull request Jul 27, 2022
@hebasto @gwillen
qt: Fix regression in "Encrypt Wallet" menu item
980a8b8 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
gwillen pushed a commit to gwillen/elements that referenced this pull request Aug 1, 2022
@hebasto @gwillen
qt: Fix regression in "Encrypt Wallet" menu item
61e1689 
Adding a new item to the m_wallet_selector must follow the establishment
of signal-slot connections.

Github-Pull: bitcoin-core/gui#393
Rebased-From: d54d949
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jarolrod jarolrod jarolrod left review comments

Assignees
No one assigned
Labels
Bug Something isn't working UX All about "how to get things done" Wallet
Projects
None yet
Milestone
22.0
Development

Successfully merging this pull request may close these issues.

Control changes from clickable to unclickable upon canceling.
3 participants
@hebasto @achow101 @jarolrod