interfaces: Describe and follow some code conventions #18278

ryanofsky commented at 2:34 PM on March 6, 2020: contributor

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

fanquake added the label Refactoring on Mar 6, 2020

hebasto commented at 3:07 PM on March 6, 2020: member

Concept ACK.

ryanofsky force-pushed on Mar 6, 2020

practicalswift commented at 3:55 PM on March 6, 2020: contributor

Concept ACK

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

promag commented at 4:24 PM on March 6, 2020: member

Code review ACK 2427e93f5823cffcf1158d1cd389b9d2ef703521.

DrahtBot commented at 4:45 PM on March 6, 2020: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

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

DrahtBot cross-referenced this on Mar 6, 2020 from issue wallet/refactor: refer to CWallet immutably when possible by kallewoof

DrahtBot cross-referenced this on Mar 6, 2020 from issue wip: gui: Refactor to drop client and wallet models setters by promag

DrahtBot cross-referenced this on Mar 6, 2020 from issue wallet: Remove calls to Chain::Lock methods by ryanofsky

DrahtBot cross-referenced this on Mar 6, 2020 from issue qt, refactor: Make BitcoinUnits::Unit a scoped enum by hebasto

DrahtBot cross-referenced this on Mar 6, 2020 from issue wallet: undo conflicts properly in case of blocks disconnection by ariard

DrahtBot cross-referenced this on Mar 6, 2020 from issue gui: save and load PSBT by Sjors

DrahtBot cross-referenced this on Mar 6, 2020 from issue Remove the mempool's NotifyEntryAdded and NotifyEntryRemoved signals by jnewbery

DrahtBot cross-referenced this on Mar 6, 2020 from issue Bugfix: GUI: Restore SendConfirmationDialog button default to "Yes" by luke-jr

DrahtBot cross-referenced this on Mar 6, 2020 from issue Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard

DrahtBot cross-referenced this on Mar 6, 2020 from issue UI external signer support (e.g. hardware wallet) by Sjors

DrahtBot cross-referenced this on Mar 6, 2020 from issue External signer support - Wallet Box edition by Sjors

DrahtBot cross-referenced this on Mar 6, 2020 from issue Native Descriptor Wallets using DescriptorScriptPubKeyMan by achow101

DrahtBot cross-referenced this on Mar 6, 2020 from issue qt: Add privacy to the Overview page by hebasto

DrahtBot cross-referenced this on Mar 6, 2020 from issue wallet: Make scan / abort status private to CWallet by Empact

DrahtBot cross-referenced this on Mar 6, 2020 from issue Remove CWalletTx merging logic from AddToWallet by ryanofsky

ryanofsky added this to the "In progress" column in a project

ryanofsky force-pushed on Mar 6, 2020

ryanofsky commented at 11:44 PM on March 6, 2020: contributor

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

ryanofsky cross-referenced this on Mar 6, 2020 from issue Multiprocess bitcoin by ryanofsky

in src/interfaces/chain.h:160 in 5212bd926b outdated

 153 | @@ -154,7 +154,10 @@ class Chain
 154 |      //! Transaction is added to memory pool, if the transaction fee is below the
 155 |      //! amount specified by max_tx_fee, and broadcast to all peers if relay is set to true.
 156 |      //! Return false if the transaction could not be added due to the fee or for another reason.
 157 | -    virtual bool broadcastTransaction(const CTransactionRef& tx, std::string& err_string, const CAmount& max_tx_fee, bool relay) = 0;
 158 | +    virtual bool broadcastTransaction(const CTransactionRef& tx,
 159 | +        const CAmount& max_tx_fee,
 160 | +        bool relay,
 161 | +        std::string& err_string) = 0;

hebasto commented at 10:00 PM on March 7, 2020:

nit: is this needed to break the line here while keeping this function definition one-liner in chain.cpp?

ryanofsky commented at 7:14 PM on March 9, 2020:

re: #18278 (review)

nit: is this needed to break the line here while keeping this function definition one-liner in chain.cpp?

added same wrapping

in src/interfaces/node.h:207 in c7b9338fd3 outdated

 203 | @@ -204,7 +204,7 @@ class Node
 204 |      virtual std::unique_ptr<Wallet> loadWallet(const std::string& name, std::string& error, std::vector<std::string>& warnings) = 0;
 205 |  
 206 |      //! Create a wallet from file
 207 | -    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;
 208 | +    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;

hebasto commented at 10:15 PM on March 7, 2020:

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

ryanofsky commented at 7:14 PM on March 9, 2020:

re: #18278 (review)

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

hebasto approved

hebasto commented at 10:27 PM on March 7, 2020: member

ACK 60ae4c0756ed14eed31a13c3813e9714cab060d0.

It seems AppVeyor fail is unrelated to this PR changes.

ryanofsky force-pushed on Mar 9, 2020

ryanofsky commented at 7:25 PM on March 9, 2020: contributor

Thanks for review

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

DrahtBot added the label Needs rebase on Mar 9, 2020

ryanofsky force-pushed on Mar 9, 2020

ryanofsky commented at 9:31 PM on March 9, 2020: contributor

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

DrahtBot removed the label Needs rebase on Mar 9, 2020

hebasto approved

hebasto commented at 9:14 AM on March 11, 2020: member

re-ACK 25f1a842929dd73bcec26c5ce213adfc65b9cb2e

DrahtBot cross-referenced this on Mar 13, 2020 from issue Fix wallet unload race condition by promag

DrahtBot cross-referenced this on Mar 14, 2020 from issue Updated text on send confirmation dialog by dannmat

DrahtBot added the label Needs rebase on Mar 19, 2020

refactor: Get rid of Wallet::IsWalletFlagSet method

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.

77e4b06572

refactor: Rename Node::disconnect methods

Avoid overloading method name to work more easily with IPC framework

1c2ab1a6d2

refactor: Rename Chain::Notifications methods to be consistent with other interfaces methods

This also simplifies #10102 removing overrides needed to deal with inconsistent
case convention

6ceb21909c

refactor: Change Chain::broadcastTransaction param order

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

96dfe5ced6

refactor: Change createWallet, fillPSBT argument order

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

1dca9dc4c7

doc: Add internal interface conventions to developer notes 3dc27a1524

ryanofsky force-pushed on Mar 20, 2020

ryanofsky commented at 1:30 PM on March 20, 2020: contributor

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

DrahtBot removed the label Needs rebase on Mar 20, 2020

hebasto commented at 6:52 PM on March 23, 2020: member

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

MarcoFalke commented at 8:37 PM on March 23, 2020: member

ACK 3dc27a15242a22b5301904375e5880372e9b7f4d 🕍

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash a78b9d76a84ed28d935046f7fd167bda964ff54cc7339f5435a9c7c3e22c90ed -

</details>

MarcoFalke merged this on Mar 23, 2020

MarcoFalke closed this on Mar 23, 2020

sidhujag referenced this in commit d630e78524 on Mar 28, 2020

MarkLTZ cross-referenced this on Apr 4, 2020 from issue Bitcoin PR tracking by MarkLTZ

ryanofsky moved this from the "In progress" to the "Done" column in a project

jasonbcox referenced this in commit b4ad0581f6 on Oct 21, 2020

jasonbcox referenced this in commit 4f3e6494f3 on Oct 21, 2020

jasonbcox referenced this in commit a44de4c634 on Oct 21, 2020

deadalnix referenced this in commit ba43817291 on Oct 21, 2020

jasonbcox referenced this in commit 24128015c5 on Oct 21, 2020

jasonbcox referenced this in commit 1f00ddc620 on Oct 21, 2020

bitcoin locked this on Feb 15, 2022