interfaces: Describe and follow some code conventions #18278

ryanofsky · 2020-03-06T14:34:39Z

This PR is part of the process separation project.

This PR doesn't change behavior at all, it just cleans up code in src/interfaces to simplify #10102, and documents coding conventions there better

hebasto · 2020-03-06T15:07:25Z

Concept ACK.

practicalswift · 2020-03-06T15:55:01Z

Concept ACK

On behalf of the eternal stream of future generations of Bitcoin Core developers: thank you for cleaning up the code base :)

promag

Code review ACK 2427e93.

DrahtBot · 2020-03-06T16:45:41Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Updated text on send confirmation dialog #18351 (Updated text on send confirmation dialog by dannmat)
Fix wallet unload race condition #18338 (Fix wallet unload race condition by promag)
wip: gui: Refactor to drop client and wallet models setters #18239 (wip: gui: Refactor to drop client and wallet models setters by promag)
wallet: Remove calls to Chain::Lock methods #17954 (wallet: Remove calls to Chain::Lock methods by ryanofsky)
qt, refactor: Make BitcoinUnits::Unit a scoped enum #17877 (qt, refactor: Make enums in BitcoinUnits class scoped by hebasto)
wallet: undo conflicts properly in case of blocks disconnection #17543 (wallet: undo conflicts properly in case of blocks disconnection by ariard)
gui: save and load PSBT #17509 (gui: save and load PSBT by Sjors)
Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" #17463 (Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" by luke-jr)
Native Descriptor Wallets using DescriptorScriptPubKeyMan #16528 (Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101)
qt: Add privacy to the Overview page #16432 (qt: Add privacy to the Overview page by hebasto)
wallet: Make scan / abort status private to CWallet #14942 (wallet: Make scan / abort status private to CWallet by Empact)
Remove CWalletTx merging logic from AddToWallet #9381 (Remove CWalletTx merging logic from AddToWallet by ryanofsky)

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.

ryanofsky · 2020-03-06T23:44:10Z

Rebased 2427e93 -> 60ae4c0 (pr/ipc-conv.3 -> pr/ipc-conv.4, compare) due to conflict with #18241

hebasto

ACK 60ae4c0.

It seems AppVeyor fail is unrelated to this PR changes.

src/interfaces/chain.h

hebasto · 2020-03-07T22:15:19Z

src/interfaces/node.h

@@ -204,7 +204,7 @@ class Node
    virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, std::string& error, std::vector<std::string>& warnings) = 0;

    //! Create a wallet from file
-    virtual WalletCreationStatus createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::vector<std::string>& warnings, std::unique_ptr<Wallet>& result) = 0;
+    virtual std::unique_ptr<Wallet> createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::vector<std::string>& warnings, WalletCreationStatus& status) = 0;


nit: considering a new signature of the refactored function could a more appropriate semantically name be chosen?
newWallet or smth similar?

re: #18278 (comment)

nit: considering a new signature of the refactored function could a more appropriate semantically name be chosen?
newWallet or smth similar?

Unclear why that name is more semantically appropriate, but not knowing this, it seems reasonable for the interface wrapper method name to have the same name as the function it is wrapping

ryanofsky

Thanks for review

Updated 60ae4c0 -> 43c3b39 (pr/ipc-conv.4 -> pr/ipc-conv.5, compare) adding suggested wrapping

src/interfaces/chain.h

ryanofsky · 2020-03-09T19:14:24Z

src/interfaces/node.h

@@ -204,7 +204,7 @@ class Node
    virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, std::string& error, std::vector<std::string>& warnings) = 0;

    //! Create a wallet from file
-    virtual WalletCreationStatus createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::vector<std::string>& warnings, std::unique_ptr<Wallet>& result) = 0;
+    virtual std::unique_ptr<Wallet> createWallet(const SecureString& passphrase, uint64_t wallet_creation_flags, const std::string& name, std::string& error, std::vector<std::string>& warnings, WalletCreationStatus& status) = 0;


re: #18278 (comment)

nit: considering a new signature of the refactored function could a more appropriate semantically name be chosen?
newWallet or smth similar?

Unclear why that name is more semantically appropriate, but not knowing this, it seems reasonable for the interface wrapper method name to have the same name as the function it is wrapping

ryanofsky · 2020-03-09T21:31:08Z

Rebased 43c3b39 -> 25f1a84 (pr/ipc-conv.5 -> pr/ipc-conv.6, compare) due to confilct with #18115

hebasto

re-ACK 25f1a84

Replace by privateKeysDisabled method to avoid need for GUI to reference internal wallet flags. Also remove adjacent WalletModel canGetAddresses wrapper that serves no purpose and make Wallet::canGetAddresses non-const so it can be implemented by IPC classes in bitcoin#10102.

Avoid overloading method name to work more easily with IPC framework

…ther interfaces methods This also simplifies bitcoin#10102 removing overrides needed to deal with inconsistent case convention

Make output argument last argument so it works more easily with IPC framework in bitcoin#10102, and for consistency with other methods

Move output arguments after input arguments for consistency with other methods, and to work more easily with IPC framework in bitcoin#10102

ryanofsky · 2020-03-20T13:30:25Z

Rebased 25f1a84 -> 3dc27a1 (pr/ipc-conv.6 -> pr/ipc-conv.7, compare) due to conflict with #17477

hebasto · 2020-03-23T18:52:58Z

re-ACK 3dc27a1, the only change since the previous review is rebasing.

maflcko · 2020-03-23T20:37:22Z

ACK 3dc27a1 🕍

Show signature and timestamp

Signature:

Timestamp of file with hash a78b9d76a84ed28d935046f7fd167bda964ff54cc7339f5435a9c7c3e22c90ed -

…tions 3dc27a1 doc: Add internal interface conventions to developer notes (Russell Yanofsky) 1dca9dc refactor: Change createWallet, fillPSBT argument order (Russell Yanofsky) 96dfe5c refactor: Change Chain::broadcastTransaction param order (Russell Yanofsky) 6ceb219 refactor: Rename Chain::Notifications methods to be consistent with other interfaces methods (Russell Yanofsky) 1c2ab1a refactor: Rename Node::disconnect methods (Russell Yanofsky) 77e4b06 refactor: Get rid of Wallet::IsWalletFlagSet method (Russell Yanofsky) Pull request description: This PR is part of the [process separation project](https://github.com/bitcoin/bitcoin/projects/10). This PR doesn't change behavior at all, it just cleans up code in [`src/interfaces`](https://github.com/bitcoin/bitcoin/tree/master/src/interfaces) to simplify bitcoin#10102, and [documents](https://github.com/ryanofsky/bitcoin/blob/pr/ipc-conv/doc/developer-notes.md#internal-interface-guidelines) coding conventions there better ACKs for top commit: hebasto: re-ACK 3dc27a1, the only change since the [previous](bitcoin#18278 (review)) review is rebasing. MarcoFalke: ACK 3dc27a1 🕍 Tree-SHA512: 62e6a0f2488e3924e559d2074ed460b92e7a0a5d98eab492221cb20d59d04bbe32aef2a8aeba5e4ea9168cfa91acd5bc973dce6677be0180bd7a919354df53ed

Summary: Replace by privateKeysDisabled method to avoid need for GUI to reference internal wallet flags. Also remove adjacent WalletModel canGetAddresses wrapper that serves no purpose and make Wallet::canGetAddresses non-const so it can be implemented by IPC classes in #10102. Backport of Core [[bitcoin/bitcoin#18278 | PR18278]] part [1/6] : bitcoin/bitcoin@77e4b06 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8019

Summary: Avoid overloading method name to work more easily with IPC framework Backport of Core [[bitcoin/bitcoin#18278 | PR18278]] part [2/6] : bitcoin/bitcoin@1c2ab1a Depends on D8019 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8020

…ther interfaces methods Summary: This also simplifies #10102 removing overrides needed to deal with inconsistent case convention Backport of Core [[bitcoin/bitcoin#18278 | PR18278]] part [3/6] : bitcoin/bitcoin@6ceb219 Depends on D8020 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8021

Summary: Make output argument last argument so it works more easily with IPC framework in #10102, and for consistency with other methods Backport of Core [[bitcoin/bitcoin#18278 | PR18278]] part [4/6] : bitcoin/bitcoin@96dfe5c Depends on D8021 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8022

Summary: Move output arguments after input arguments for consistency with other methods, and to work more easily with IPC framework in #10102 Backport of Core [[bitcoin/bitcoin#18278 | PR18278]] part [5/6] : bitcoin/bitcoin@1dca9dc Depends on D8022 The Qt part of PSBT has not been ported so it is absent from this patch. This shouldn't be a problem, because it is a compilation error and trivial to fix, so it can be don then. Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8023

Summary: Backport of Core [[bitcoin/bitcoin#18278 | PR18278]] part [6/6] : bitcoin/bitcoin@3dc27a1 Depends on D8023 Test Plan: ninja all check-all Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D8024

fanquake added the Refactoring label Mar 6, 2020

ryanofsky force-pushed the pr/ipc-conv branch 2 times, most recently from 671f390 to 2427e93 Compare March 6, 2020 15:21

promag reviewed Mar 6, 2020

View reviewed changes

ryanofsky added this to In progress in Process Separation Mar 6, 2020

ryanofsky force-pushed the pr/ipc-conv branch from 2427e93 to 60ae4c0 Compare March 6, 2020 23:43

ryanofsky mentioned this pull request Mar 6, 2020

Multiprocess bitcoin #10102

Draft

hebasto approved these changes Mar 7, 2020

View reviewed changes

ryanofsky force-pushed the pr/ipc-conv branch from 60ae4c0 to 43c3b39 Compare March 9, 2020 19:22

ryanofsky commented Mar 9, 2020

View reviewed changes

DrahtBot added the Needs rebase label Mar 9, 2020

ryanofsky force-pushed the pr/ipc-conv branch from 43c3b39 to 25f1a84 Compare March 9, 2020 21:28

DrahtBot removed the Needs rebase label Mar 9, 2020

hebasto approved these changes Mar 11, 2020

View reviewed changes

This was referenced Mar 13, 2020

Fix wallet unload race condition #18338

Merged

Updated text on send confirmation dialog #18351

Closed

DrahtBot added the Needs rebase label Mar 19, 2020

ryanofsky added 6 commits March 19, 2020 15:26

refactor: Rename Node::disconnect methods

1c2ab1a

Avoid overloading method name to work more easily with IPC framework

refactor: Rename Chain::Notifications methods to be consistent with o…

6ceb219

…ther interfaces methods This also simplifies bitcoin#10102 removing overrides needed to deal with inconsistent case convention

refactor: Change Chain::broadcastTransaction param order

96dfe5c

Make output argument last argument so it works more easily with IPC framework in bitcoin#10102, and for consistency with other methods

refactor: Change createWallet, fillPSBT argument order

1dca9dc

Move output arguments after input arguments for consistency with other methods, and to work more easily with IPC framework in bitcoin#10102

doc: Add internal interface conventions to developer notes

3dc27a1

ryanofsky force-pushed the pr/ipc-conv branch from 25f1a84 to 3dc27a1 Compare March 20, 2020 13:28

DrahtBot removed the Needs rebase label Mar 20, 2020

maflcko merged commit ac579ad into bitcoin:master Mar 23, 2020

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

ryanofsky moved this from In progress to Done in Process Separation Apr 10, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interfaces: Describe and follow some code conventions #18278

interfaces: Describe and follow some code conventions #18278

ryanofsky commented Mar 6, 2020 •

edited

hebasto commented Mar 6, 2020

practicalswift commented Mar 6, 2020

promag left a comment

DrahtBot commented Mar 6, 2020 •

edited

ryanofsky commented Mar 6, 2020

hebasto left a comment

hebasto Mar 7, 2020

ryanofsky Mar 9, 2020

ryanofsky left a comment

ryanofsky Mar 9, 2020

ryanofsky commented Mar 9, 2020

hebasto left a comment

ryanofsky commented Mar 20, 2020

hebasto commented Mar 23, 2020

maflcko commented Mar 23, 2020

interfaces: Describe and follow some code conventions #18278

interfaces: Describe and follow some code conventions #18278

Conversation

ryanofsky commented Mar 6, 2020 • edited

hebasto commented Mar 6, 2020

practicalswift commented Mar 6, 2020

promag left a comment

Choose a reason for hiding this comment

DrahtBot commented Mar 6, 2020 • edited

Conflicts

ryanofsky commented Mar 6, 2020

hebasto left a comment

Choose a reason for hiding this comment

hebasto Mar 7, 2020

Choose a reason for hiding this comment

ryanofsky Mar 9, 2020

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

ryanofsky Mar 9, 2020

Choose a reason for hiding this comment

ryanofsky commented Mar 9, 2020

hebasto left a comment

Choose a reason for hiding this comment

ryanofsky commented Mar 20, 2020

hebasto commented Mar 23, 2020

maflcko commented Mar 23, 2020

ryanofsky commented Mar 6, 2020 •

edited

DrahtBot commented Mar 6, 2020 •

edited