wallet: Change output type globals to members #12408

maflcko · 2018-02-11T02:29:06Z

Output type is used by the wallet when generating addresses or transactions with change, thus it should be a member of CWallet.

Moreover, in light of multiwallet, it makes sense to prepare for per-wallet attributes instead of for-all-wallets globals.

randolf · 2018-02-11T02:43:49Z

Concept ACK. This seems very good in principle, and moving toward multi-wallet support looks like a rational use case to me.

promag · 2018-02-11T10:04:13Z

utACK fab8778.

How do you plan to have different types for different wallets?

jonasschnelli · 2018-02-12T09:08:07Z

Concept ACK

laanwj · 2018-02-12T09:57:26Z

Concept ACK

Sjors

Tested ACK fab87781

Sjors · 2018-02-12T18:37:18Z

src/wallet/wallet.cpp

+        return nullptr;
+    }
+
+


Nit: unnecessary empty line

Sorry. Will keep the white space for now, to not invalidate review.

maflcko · 2018-02-16T18:58:37Z

Trivially rebased and fixed whitespace-nit. Should be easy to re-ACK if you still have the commit locally.

Sjors · 2018-02-20T14:06:54Z

re-ACK fa9d869. I also checked that #12424 didn't come back, since this latest commit undoes that.

instagibbs · 2018-02-27T19:53:25Z

utACK fa9d869

I echo @promag 's question. Out of scope for now, but would be good to know.

ryanofsky

utACK fa9d869d0beec2e6b87c4b88d7226e78864f9e10

Just a suggestion, but I think switching to m_default_address_type instead of m_address_type and GetDefaultAddressType instead of GetWalletAddressType (and similarly for change) might be a little more clear and consistent.

ryanofsky · 2018-03-01T15:58:28Z

src/wallet/wallet.h

@@ -808,6 +804,8 @@ class CWallet final : public CCryptoKeyStore, public CValidationInterface

    void SetNull()
    {
+        m_address_type = OutputType::NONE;


Is there a reason we can't set these to DEFAULT instead of NONE? Using DEFAULT would seem to be better because it would simplify test and test fixture setup in various places. Or if these do have to be NONE, it would be good to have a comment saying why, since the reason isn't obvious.

I think only address type can be DEFAULT, there is use case NONE change type (change_type = g_change_type != OUTPUT_TYPE_NONE ? g_change_type : g_address_type).

ryanofsky · 2018-03-01T16:57:40Z

How do you plan to have different types for different wallets?

I think the answer would depend on what the use case for per-wallet output types is (assuming there is a use case).

If the address/change types for a wallet are supposed to be stable, they should probably be determined from the database, either by being stored explicitly, or by being derived from the wallet version number. In this case the output types should be persistent and controllable by RPC.

Alternately, if the output types should just be runtime settings, we could follow sqlite's example and allow specifying per-file options with uri query parameters, for example: "-wallet=name?addresstype=bech32".

promag · 2018-03-01T17:36:11Z

switching to m_default_address_type instead of m_address_type and GetDefaultAddressType instead of GetWalletAddressType

@ryanofsky 👍

promag · 2018-03-16T17:05:32Z

Needs rebase.

maflcko · 2018-03-17T20:10:18Z

Addressed feedback by removing OutputType::DEFAULT.

laanwj · 2018-03-19T15:37:14Z

utACK fab8a6f

instagibbs · 2018-03-19T15:45:27Z

utACK fab8a6f

fab8a6f wallet: Change output type globals to members (MarcoFalke) Pull request description: Output type is used by the wallet when generating addresses or transactions with change, thus it should be a member of `CWallet`. Moreover, in light of multiwallet, it makes sense to prepare for per-wallet attributes instead of for-all-wallets globals. Tree-SHA512: 4fa397cd82522e5bacf4870160a2a0f5e1f2dc046e4b9e2514dee18b187a0e1724d036315f77fa48e48f85533021d5e5525d798160a92d389d75512f3f9e1405

@sipa

1e46d8a Get rid of ambiguous OutputType::NONE value (Russell Yanofsky) Pull request description: Based on suggestion by @sipa #12119 (comment) After #12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool to indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes. Follows up #12408 by @MarcoFalke Followups for future PRs: - [ ] Add explicit support for specifying "auto" in `ParseOutputType` as suggested by promag and sipa: #12729 (comment) and #12729 (comment) - [ ] Add wallet `AddressChangeType` method to complement `TransactionChangeType`: #12729 (comment). Tree-SHA512: 8b08b272bcb177a0a9e556dcd965840a7fe601ef83ca97938b879c9b1a33b5b3f96939e1bceef11ba7c644ac21bfd6c1dbc6ca715cd1da4ace50475240e4ee48

@sipa

1e46d8a Get rid of ambiguous OutputType::NONE value (Russell Yanofsky) Pull request description: Based on suggestion by @sipa bitcoin#12119 (comment) After bitcoin#12119, the NONE output type was overloaded to refer to either an output type that couldn't be parsed, or to an automatic change output mode. This change drops the NONE enum and uses a simple bool to indicate parse failure, and a new CHANGE_AUTO enum to refer the change output type. This change is almost a pure refactoring except it makes RPCs reject empty string ("") address types instead of treating them like they were unset. This simplifies the parsing code a little bit and could prevent RPC usage mistakes. It's noted in the release notes. Follows up bitcoin#12408 by @MarcoFalke Followups for future PRs: - [ ] Add explicit support for specifying "auto" in `ParseOutputType` as suggested by promag and sipa: bitcoin#12729 (comment) and bitcoin#12729 (comment) - [ ] Add wallet `AddressChangeType` method to complement `TransactionChangeType`: bitcoin#12729 (comment). Tree-SHA512: 8b08b272bcb177a0a9e556dcd965840a7fe601ef83ca97938b879c9b1a33b5b3f96939e1bceef11ba7c644ac21bfd6c1dbc6ca715cd1da4ace50475240e4ee48

maflcko added Wallet Refactoring labels Feb 11, 2018

maflcko force-pushed the Mf1802-walletChangeTypeMember branch from 804f7ba to fab8778 Compare February 11, 2018 02:51

maflcko added this to the 0.16.1 milestone Feb 11, 2018

promag mentioned this pull request Feb 12, 2018

Disable default fallbackfee on mainnet #11882

Merged

Sjors approved these changes Feb 12, 2018

View reviewed changes

promag mentioned this pull request Feb 13, 2018

Fix rescan test failure due to unset g_address_type, g_change_type #12424

Merged

maflcko removed this from the 0.16.1 milestone Feb 14, 2018

maflcko force-pushed the Mf1802-walletChangeTypeMember branch 2 times, most recently from 44ed6d1 to fa9d869 Compare February 16, 2018 18:57

ryanofsky reviewed Mar 1, 2018

View reviewed changes

maflcko force-pushed the Mf1802-walletChangeTypeMember branch 4 times, most recently from f58ae03 to efdc392 Compare March 17, 2018 20:02

wallet: Change output type globals to members

fab8a6f

maflcko force-pushed the Mf1802-walletChangeTypeMember branch from efdc392 to fab8a6f Compare March 17, 2018 20:10

laanwj merged commit fab8a6f into bitcoin:master Mar 19, 2018

maflcko deleted the Mf1802-walletChangeTypeMember branch March 19, 2018 16:17

This was referenced Mar 19, 2018

Rename account to label where appropriate #11536

Merged

scripted-diff: Rename wallet database classes #11851

Merged

Get rid of ambiguous OutputType::NONE value #12729

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

wallet: Change output type globals to members #12408

wallet: Change output type globals to members #12408

Uh oh!

Conversation

maflcko commented Feb 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

randolf commented Feb 11, 2018

Uh oh!

promag commented Feb 11, 2018

Uh oh!

jonasschnelli commented Feb 12, 2018

Uh oh!

laanwj commented Feb 12, 2018

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Feb 12, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko Feb 12, 2018

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Feb 20, 2018

Uh oh!

instagibbs commented Feb 27, 2018

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

promag Mar 1, 2018

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Mar 1, 2018

Uh oh!

promag commented Mar 1, 2018

Uh oh!

promag commented Mar 16, 2018

Uh oh!

maflcko commented Mar 17, 2018

Uh oh!

laanwj commented Mar 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Mar 19, 2018

Uh oh!

Uh oh!

maflcko commented Feb 11, 2018 •

edited

Loading

maflcko commented Feb 16, 2018 •

edited

Loading

laanwj commented Mar 19, 2018 •

edited

Loading