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

refactor: Optimize signal-slot connections logic #29

Merged
merged 3 commits into from
Jun 5, 2021
Merged

refactor: Optimize signal-slot connections logic #29

merged 3 commits into from
Jun 5, 2021

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Jul 10, 2020

This PR:

@hebasto
Copy link
Member Author

hebasto commented Jul 10, 2020

This PR was originally opened in the https://github.com/bitcoin/bitcoin repo: bitcoin/bitcoin#17966 has a concept ACK from @jonasschnelli.

@hebasto hebasto closed this Jul 14, 2020
@hebasto hebasto reopened this Jul 14, 2020
@jonasschnelli
Copy link
Contributor

Concept ACK

@hebasto hebasto marked this pull request as draft October 24, 2020 09:30
maflcko pushed a commit that referenced this pull request May 15, 2021
Merge bitcoin/bitcoin#21948: test: Fix off-by-one in mockscheduler te…
c857148 
…st RPC

fa2e614 test: Fix off-by-one in mockscheduler test RPC (MarcoFalke)

Pull request description:

  Fixes:

  ```
  fuzz: scheduler.cpp:83: void CScheduler::MockForward(std::chrono::seconds): Assertion `delta_seconds.count() > 0 && delta_seconds < std::chrono::hours{1}' failed.
  ==1059066== ERROR: libFuzzer: deadly signal
      #0 0x558f75449c10 in __sanitizer_print_stack_trace (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x5fec10)
      #1 0x558f753f32b8 in fuzzer::PrintStackTrace() fuzzer.o
      #2 0x558f753d68d3 in fuzzer::Fuzzer::CrashCallback() fuzzer.o
      #3 0x7f4a3cbbb3bf  (/lib/x86_64-linux-gnu/libpthread.so.0+0x153bf)
      #4 0x7f4a3c7ff18a in raise (/lib/x86_64-linux-gnu/libc.so.6+0x4618a)
      #5 0x7f4a3c7de858 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x25858)
      #6 0x7f4a3c7de728  (/lib/x86_64-linux-gnu/libc.so.6+0x25728)
      #7 0x7f4a3c7eff35 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x36f35)
      #8 0x558f7588a913 in CScheduler::MockForward(std::chrono::duration<long, std::ratio<1l, 1l> >) scheduler.cpp:83:5
      #9 0x558f75b0e5b1 in mockscheduler()::$_7::operator()(RPCHelpMan const&, JSONRPCRequest const&) const rpc/misc.cpp:435:30
      #10 0x558f75b0e5b1 in std::_Function_handler<UniValue (RPCHelpMan const&, JSONRPCRequest const&), mockscheduler()::$_7>::_M_invoke(std::_Any_data const&, RPCHelpMan const&, JSONRPCRequest const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
      #11 0x558f7587a141 in std::function<UniValue (RPCHelpMan const&, JSONRPCRequest const&)>::operator()(RPCHelpMan const&, JSONRPCRequest const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
      #12 0x558f7587a141 in RPCHelpMan::HandleRequest(JSONRPCRequest const&) const rpc/util.cpp:565:26
      #13 0x558f756c0086 in CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)::operator()(JSONRPCRequest const&, UniValue&, bool) const ./rpc/server.h:110:91
      #14 0x558f756c0086 in std::_Function_handler<bool (JSONRPCRequest const&, UniValue&, bool), CRPCCommand::CRPCCommand(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, RPCHelpMan (*)())::'lambda'(JSONRPCRequest const&, UniValue&, bool)>::_M_invoke(std::_Any_data const&, JSONRPCRequest const&, UniValue&, bool&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:285:9
      #15 0x558f756b8592 in std::function<bool (JSONRPCRequest const&, UniValue&, bool)>::operator()(JSONRPCRequest const&, UniValue&, bool) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
      #16 0x558f756b8592 in ExecuteCommand(CRPCCommand const&, JSONRPCRequest const&, UniValue&, bool) rpc/server.cpp:480:20
      #17 0x558f756b8592 in ExecuteCommands(std::vector<CRPCCommand const*, std::allocator<CRPCCommand const*> > const&, JSONRPCRequest const&, UniValue&) rpc/server.cpp:444:13
      #18 0x558f756b8017 in CRPCTable::execute(JSONRPCRequest const&) const rpc/server.cpp:464:13
      #19 0x558f7552457a in (anonymous namespace)::RPCFuzzTestingSetup::CallRPC(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&) test/fuzz/rpc.cpp:50:25
      #20 0x558f7552457a in rpc_fuzz_target(Span<unsigned char const>) test/fuzz/rpc.cpp:354:28
      #21 0x558f7544cf0f in std::_Function_handler<void (Span<unsigned char const>), void (*)(Span<unsigned char const>)>::_M_invoke(std::_Any_data const&, Span<unsigned char const>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:300:2
      #22 0x558f75c05197 in std::function<void (Span<unsigned char const>)>::operator()(Span<unsigned char const>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/9/../../../../include/c++/9/bits/std_function.h:688:14
      #23 0x558f75c05197 in LLVMFuzzerTestOneInput test/fuzz/fuzz.cpp:74:5
      #24 0x558f753d8073 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) fuzzer.o
      #25 0x558f753c1f72 in fuzzer::RunOneTest(fuzzer::Fuzzer*, char const*, unsigned long) fuzzer.o
      #26 0x558f753c7d6a in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) fuzzer.o
      #27 0x558f753f3a92 in main (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x5a8a92)
      #28 0x7f4a3c7e00b2 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
      #29 0x558f7539cc9d in _start (/root/fuzz_dir/scratch/fuzz_gen/code/src/test/fuzz/fuzz+0x551c9d)

ACKs for top commit:
  practicalswift:
    cr ACK fa2e614

Tree-SHA512: cfa120265261f0ad019b46c426b915c1c007806b37aecb27016ce780a0ddea5e6fc9b09065fd40684b11183dcd3bf543558d7a655e604695021653540266baf7
@hebasto hebasto mentioned this pull request May 15, 2021
Merged
@hebasto hebasto force-pushed the 200710-walletframe branch from 3eb6d5b to e7c7fbe Compare May 15, 2021 21:46
@hebasto hebasto marked this pull request as ready for review May 15, 2021 21:49
@hebasto
Copy link
Member Author

hebasto commented May 15, 2021

Updated 3eb6d5b -> e7c7fbe (pr29.01 -> pr29.02):

hebasto added 3 commits May 29, 2021 15:49
@hebasto
qt: Move WalletView connections from WalletFrame to BitcoinGUI
20e2e24 
This changes remove some pointers to the BitcoinGUI instance that is
required for the next commits.

This commit does not change behavior.
@hebasto
qt: Move CreateWalletActivity connection from WalletFrame to BitcoinGUI
f73e5c9 
This changes remove some pointers to the BitcoinGUI instance that is
required for the next commits.

This commit does not change behavior.
@hebasto
qt: Drop BitcoinGUI* WalletFrame data member
62cb8d9 
This changes removes bitcoingui->walletframe->bitcoingui circular
dependency.

This commit does not change behavior.
@hebasto hebasto force-pushed the 200710-walletframe branch from e7c7fbe to 62cb8d9 Compare May 29, 2021 12:49
@hebasto
Copy link
Member Author

hebasto commented May 29, 2021

Rebased e7c7fbe -> 62cb8d9 (pr29.02 -> pr29.03) on top of the recent CI changes.

promag
promag reviewed Jun 2, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Code review ACK 62cb8d9. Will build and test.

Nice to see you picked bitcoin/bitcoin#17500 (comment) suggestion.

promag
promag reviewed Jun 3, 2021
View reviewed changes
Copy link
Contributor

@promag promag left a comment

Choose a reason for hiding this comment

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

Tested ACK 62cb8d9 on macos 11.2.3 with depends build.

Ran with QT_FATAL_WARNINGS=0, didn't observe behavior changes.

src/qt/bitcoingui.cpp
@@ -680,6 +688,18 @@ void BitcoinGUI::addWallet(WalletModel* walletModel)
}
const QString display_name = walletModel->getDisplayName();
m_wallet_selector->addItem(display_name, QVariant::fromValue(walletModel));

connect(wallet_view, &WalletView::outOfSyncWarningClicked, walletFrame, &WalletFrame::outOfSyncWarningClicked);
Copy link
Contributor

Choose a reason for hiding this comment

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

nit, maybe this should stay in WalletFrame?

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing in this way keeps compatibility with #333.

jarolrod
jarolrod reviewed Jun 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 62cb8d9

Tested that there are no weird side-effects. Also used @promag instructions to look for behavior changes aside from manual testing. Ran on Arch Linux Qt 5.15.2, macOS 11.3 Qt 5.15.2 and cross-compile to Windows 10.

Overall, I think the changes in this PR provide a clearer definition of the functionality the walletframe should be responsible for performing vs being aware of. Thanks for working on this.

@hebasto hebasto merged commit e033ca1 into bitcoin-core:master Jun 5, 2021
@hebasto hebasto deleted the 200710-walletframe branch June 5, 2021 22:09
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 6, 2021
@hebasto
Merge bitcoin-core/gui#29: refactor: Optimize signal-slot connections…
1308761 
… logic
hebasto added a commit that referenced this pull request Jun 12, 2021
@hebasto
Merge #333: refactor: Signal-slot connections cleanup
6f3fbc0 
f507681 qt: Connect WalletView signal to BitcoinGUI slot directly (Hennadii Stepanov)
bd50ff9 qt: Drop redundant OverviewPage::handleOutOfSyncWarningClicks slot (Hennadii Stepanov)
793f195 qt: Drop redundant WalletView::requestedSyncWarningInfo slot (Hennadii Stepanov)

Pull request description:

  This PR:
  - removes slots whose only job is to emit a signal, since we can use the signal as a slot
  - connects the`WalletView::outOfSyncWarningClicked` signal to the `BitcoinGUI::showModalOverlay` slot directly, and removes intermediate `WalletFrame` slot and signal
  - split from #29

  This PR does not change behavior.

ACKs for top commit:
  Talkless:
    tACK f507681, tested on Debian Sid with Qt 5.15.2, no any behavioral changes noticed.
  promag:
    Code review ACK f507681.

Tree-SHA512: cd636a7e61881b2cbee84d5425d2107a8e39683b8eb32d79dc9ea942db55d5c1979be2f70da1660eaee5de622d10ed5a92f11fc2351de21b84324b10b23d0c96
@hebasto hebasto mentioned this pull request Aug 3, 2021
Merged
hebasto added a commit that referenced this pull request Aug 5, 2021
@hebasto
Merge #393: Fix regression in "Encrypt Wallet" menu item
e9472e6 
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/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
gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@apoelstra
Merge e033ca1 into merged_master (Bitcoin PR bitcoin-core/gui#29)
69fd180
@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

@promag promag promag left review comments

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@hebasto @jonasschnelli @promag @jarolrod @maflcko @DrahtBot