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

Fix gui segfault caused by bitcoin/bitcoin#22216 #361

Merged
merged 1 commit into from
Jun 11, 2021

Conversation

ryanofsky
Copy link
Contributor

Reported by Hennadii Stepanov bitcoin/bitcoin#22216 (comment)

Fixes bitcoin/bitcoin#22227

@hebasto
Copy link
Member

hebasto commented Jun 11, 2021

Why tests and CI did not catch it?

@ryanofsky
Copy link
Contributor Author

ryanofsky commented Jun 11, 2021

Tests would not catch it because tests initialize context differently than the gui app.

I didn't catch it because I only tested the gui with bitcoin/bitcoin#22216 and bitcoin/bitcoin#22219 together. They were initially part of the same PR and bitcoin/bitcoin#22219 also fixes this bug a different way.

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

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

ACK d7f3b1a, tested on Linux Mint 20.1 (Qt 5.12.8).

UPDATE: tested on Windows 10 Pro (20H2, build 19042.1052).

@hebasto hebasto added Bug Something isn't working Wallet labels Jun 11, 2021
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 d7f3b1a

Tested on macOS 11.3, Qt 5.15.2

master:

xyz@xyzs-MBP bitcoin % ./src/qt/bitcoin-qt -testnet
dbus[52399]: Dynamic session lookup supported but failed: launchd did not provide a socket path, verify that org.freedesktop.dbus-session.plist is loaded!
Assertion failed: ("node.args" && check), function operator(), file wallet/init.cpp, line 127.
zsh: abort      ./src/qt/bitcoin-qt -testnet

pr:
Runs and works fine upon manual testing

@hebasto hebasto merged commit a8c8dbc into bitcoin-core:master Jun 11, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 12, 2021
d7f3b1a Fix gui segfault caused by bitcoin#22216 (Russell Yanofsky)

Pull request description:

  Reported by Hennadii Stepanov bitcoin#22216 (comment)

  Fixes bitcoin#22227

ACKs for top commit:
  hebasto:
    ACK d7f3b1a, tested on Linux Mint 20.1 (Qt 5.12.8).
  jarolrod:
    ACK d7f3b1a

Tree-SHA512: d672bfa9f1bcd500a879ec7ed27096086ae93b73ad5da8090f29cc5b6d985c46a76583cc384304d67210f87b6b839c2391f0fcc24fd3588c4a014e540283fdfe
@Rspigler
Copy link
Contributor

Rspigler commented Jul 9, 2021

Tests would not catch it because tests initialize context differently than the gui app.

Is there a way to improve tests so that they initialize in the same way?

@ryanofsky
Copy link
Contributor Author

ryanofsky commented Jul 9, 2021

Is there a way to improve tests so that they initialize in the same way?

One way would be to make the python tests invoke bitcoin-qt. I believe this kind of already works if you set BITCOIND=qt/bitcoin-qt, but this isn't run as part of CI currently

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022
@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.
Labels
Bug Something isn't working Wallet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression in #22216
4 participants