Skip to content

gui: Do not create payment server if -disablewallet option provided #16436

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
Merged
laanwj merged 1 commit into from
Jul 29, 2019
Merged

gui: Do not create payment server if -disablewallet option provided #16436

laanwj merged 1 commit into from
Jul 29, 2019

Conversation

@hebasto
Copy link
Member

@hebasto hebasto commented Jul 22, 2019

This PR makes early check for the -disablewallet option.

If -disablewallet=1, objects PaymentServer and WalletController are nor created.

@hebasto hebasto changed the title Do not create payment server if -disablewallet option provided wallet: Do not create payment server if -disablewallet option provided Jul 22, 2019
@maflcko maflcko changed the title wallet: Do not create payment server if -disablewallet option provided gui: Do not create payment server if -disablewallet option provided Jul 22, 2019
@maflcko maflcko added the GUI label Jul 22, 2019
@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 22, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

promag
promag reviewed Jul 23, 2019
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.

Concept ACK, makes sense to not instantiate these objects when -disablewallet.

It would be nice to remove all ENABLE_WALLET and other similar checks from src/qt/bitcoin.cpp.

nit, commit could also have prefix.

src/qt/bitcoin.cpp Outdated
Copy link
Contributor

@promag promag Jul 23, 2019

Choose a reason for hiding this comment

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

This is not necessary, it is safe to delete nullptr.

Copy link
Contributor

@promag promag Jul 23, 2019

Choose a reason for hiding this comment

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

Or maybe you want to get rid of these delete as these objects are owned by BitcoinApplication instance.

Copy link
Member Author

@hebasto hebasto Jul 23, 2019

Choose a reason for hiding this comment

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

Or maybe you want to get rid of these delete as these objects are owned by BitcoinApplication instance.

Exactly!

src/qt/bitcoin.cpp Outdated
Copy link
Contributor

@promag promag Jul 23, 2019

Choose a reason for hiding this comment

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

This could be assert(paymentServer)?

Copy link
Member Author

@hebasto hebasto Jul 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Failing to create a PaymentServer object is not critical, IMO.

Copy link
Member

@laanwj laanwj Jul 29, 2019

Choose a reason for hiding this comment

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

Also new will never return a nullptr anyway.

@hebasto hebasto force-pushed the 20190722-payment-server branch from 20ef0d5 to 4057b7a Compare July 23, 2019 12:45
@hebasto
Copy link
Member Author

hebasto commented Jul 23, 2019

@promag Thank you for your review. Your comments have been addressed.

It would be nice to remove all ENABLE_WALLET and other similar checks from src/qt/bitcoin.cpp.

Could you elaborate your suggestion?

@fanquake
Copy link
Member

fanquake commented Jul 29, 2019

Concept ACK

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Jul 29, 2019

utACK 4057b7a
Removes three #ifdefs, probably very tiny ressource reduction when not running with the wallet.

@laanwj
Copy link
Member

laanwj commented Jul 29, 2019

ACK 4057b7a

@laanwj laanwj merged commit 4057b7a into bitcoin:master Jul 29, 2019
laanwj added a commit that referenced this pull request Jul 29, 2019
@laanwj
Merge #16436: gui: Do not create payment server if -disablewallet opt…
2a7c3bc 
…ion provided

4057b7a wallet: Recognize -disablewallet option early (Hennadii Stepanov)

Pull request description:

  This PR makes early check for the `-disablewallet` option.

  If `-disablewallet=1`, objects `PaymentServer` and `WalletController` are  nor created.

ACKs for top commit:
  jonasschnelli:
    utACK 4057b7a
  laanwj:
    ACK 4057b7a

Tree-SHA512: 74633cd1eacd0914c73712e6dff190255b5378595cfee7eaeb91e17671fc9120928034739f4ae1c53b86f46c4b400390877241384376b2fc534de326d3ab0944
@hebasto hebasto deleted the 20190722-payment-server branch July 31, 2019 14:43
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Oct 12, 2020
@hebasto @PiRK
wallet: Recognize -disablewallet option early
227c112 
Summary:
PR description:
> This PR makes early check for the -disablewallet option.
> If `-disablewallet=1`, objects PaymentServer and WalletController are nor created.

Backport of Core [[bitcoin/bitcoin#16436 | PR16436]]

Test Plan:
`ninja && ninja check`
`src/qt/bitcoin-qt -disablewallet`

Reviewers: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Reviewed By: O1 Bitcoin ABC, #bitcoin_abc, deadalnix

Subscribers: deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D7889
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@laanwj laanwj laanwj left review comments

+1 more reviewer

@promag promag promag left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

GUI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@hebasto @DrahtBot @fanquake @jonasschnelli @laanwj @promag @maflcko