qt: Use WalletModel* instead of the wallet name as map key

promag commented at 1:17 pm on November 22, 2018: member

This a small refactor that doesn’t change behavior. This is also necessary if in the future we allow renaming wallets.

fanquake added the label GUI on Nov 22, 2018

in src/qt/walletframe.h:40 in 49f3dd713c outdated

36@@ -37,8 +37,8 @@ class WalletFrame : public QFrame
37     void setClientModel(ClientModel *clientModel);
38 
39     bool addWallet(WalletModel *walletModel);
40-    bool setCurrentWallet(const QString& name);
41-    bool removeWallet(const QString &name);
42+    bool setCurrentWallet(WalletModel* walletModel);

practicalswift commented at 6:15 am on November 23, 2018:

Make sure parameter names match between declaration and definition :-)

in src/qt/walletframe.h:41 in 49f3dd713c outdated

36@@ -37,8 +37,8 @@ class WalletFrame : public QFrame
37     void setClientModel(ClientModel *clientModel);
38 
39     bool addWallet(WalletModel *walletModel);
40-    bool setCurrentWallet(const QString& name);
41-    bool removeWallet(const QString &name);
42+    bool setCurrentWallet(WalletModel* walletModel);
43+    bool removeWallet(WalletModel* walletModel);

practicalswift commented at 6:16 am on November 23, 2018:

Same here :-)

practicalswift changes_requested

laanwj commented at 9:07 am on November 23, 2018: member

Concept ACK.

Small nit for clarity’s sake: would make sense to word the PR title and commit as “use WalletModel pointer as map key” instead of what is not used, this tells reviewers more.

promag renamed this:
~~qt: Don't use the wallet name as map key~~
qt: Use WalletModel* instead of the wallet name as map key
on Nov 23, 2018

promag force-pushed on Nov 23, 2018

promag commented at 12:06 pm on November 23, 2018: member

@laanwj sure, actually the commit was already along that. @practicalswift done.

hebasto commented at 6:01 pm on November 24, 2018: member

Concept ACK.

jonasschnelli commented at 5:44 am on November 25, 2018: contributor

I’m not entirely convince that holding raw pointers in a list is an improvement.

promag commented at 2:25 pm on November 26, 2018: member

@jonasschnelli what are your concerns?

laanwj commented at 1:19 pm on January 2, 2019: member

@jonasschnelli what are your concerns?

I think the concern, versus using another kind of id, is that when using a pointer an inconsistency between the different data structures will cause crashes / use after free.

promag commented at 1:25 pm on January 2, 2019: member

Must be removed before free and we should be confident about that.

ryanofsky approved

ryanofsky commented at 8:03 pm on January 2, 2019: member

utACK 492156b4b908ae1aa99b947a2efb314295d70749. This change looks like an improvement to me. Both pointers and strings should be unique but it seems easier to screw up string formatting and comparison than pointer equality checking.

In theory if there were use-after-free bugs we didn’t know about, string comparisons could allow the code to limp along further without crashing, but I don’t see where this could happen here because the pointers seem to be used for map lookups without getting dereferenced. Maybe you could replace WalletModel* with intptr_t to officially prevent dereferencing, though I think the WalletModel* is more obvious and easy to understand.

laanwj commented at 7:12 am on January 3, 2019: member

So is there no other suitable unique identifier except the pointer?

I ask this because a remote RPC client would have to do with only the name too, right?

promag commented at 7:25 am on January 3, 2019: member

Yes the RPC identifies the wallet by its name.

laanwj commented at 7:30 am on January 3, 2019: member

This is also necessary if in the future we allow renaming wallets.

I guess there’s something to be said, then, for not ever changing that identifier while the wallet is loaded to avoid confusing remote clients, even if, say, the on-disk name or location changes. (to be clear this is not an argument to not change this to WalletModel* if that helps, but I don’t think renaming should work like that)

promag commented at 8:20 am on January 3, 2019: member

@laanwj my point is if there is a rename the WalletModel is the same. I’m not saying anything regarding RPC :)

laanwj commented at 8:30 am on January 3, 2019: member

No, you don’t say anything about RPC, but that the identifier should probably be constant while a wallet is loaded. Or does RPC use a different identifier for wallets than the GUI does?

promag commented at 1:52 pm on January 3, 2019: member

The public unique identifier is the wallet name but here WalletModel* is used as the internal identifier in the GUI.

Maybe I shouldn’t bring up “wallet rename”.. I just think using the WalletModel* around is more convenient than looking up model by wallet name.

promag force-pushed on Jan 4, 2019

promag commented at 12:19 pm on January 4, 2019: member

Rebased and also use WalletModel* in console window.

laanwj commented at 2:55 pm on January 4, 2019: member

travis fail:

0qt/libbitcoinqt.a(qt_libbitcoinqt_a-rpcconsole.o): In function `RPCConsole::on_lineEdit_returnPressed()':
1/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/src/qt/rpcconsole.cpp:928: undefined reference to `WalletModel::getWalletName() const'
2qt/libbitcoinqt.a(qt_libbitcoinqt_a-rpcconsole.o): In function `RPCConsole::on_lineEdit_returnPressed()':
3/home/travis/build/bitcoin/bitcoin/build/bitcoin-x86_64-unknown-linux-gnu/src/qt/rpcconsole.cpp:928: undefined reference to `WalletModel::getWalletName() const'
4clang: error: linker command failed with exit code 1 (use -v to see invocation)

promag force-pushed on Jan 4, 2019

qt: Factor out WalletModel::getDisplayName() d2a1adffeb

qt: Use WalletModel* instead of wallet name in main window b2ce86c3ad

promag force-pushed on Jan 4, 2019

qt: Use WalletModel* instead of wallet name in console window 91b0c5b096

promag force-pushed on Jan 4, 2019

promag commented at 4:15 pm on January 4, 2019: member

Thanks @laanwj, fixed now.

ryanofsky approved

ryanofsky commented at 4:34 pm on January 4, 2019: member

utACK 91b0c5b09616bb8c5ef8107efb432ce4c7c9a383. Changes since last review: rebase, adding new rpc console commit, and adding copyright year updates to earlier commits.

Not a big deal, but I think rpc console commit 91b0c5b09616bb8c5ef8107efb432ce4c7c9a383 goes a little too far in passing WalletModel to parse/execute functions which really only need a wallet name to pass along to the RPC engine, and not a full WalletModel instance. It might make this code slightly more difficult to separate and test.

promag commented at 5:09 pm on January 4, 2019: member

@ryanofsky I had like that but RPC console already has ENABLE_WALLET macro to conditionally build the wallet endpoint. I think that a future refactor is to build the endpoint outside and inject it instead of passing the wallet.

laanwj merged this on Jan 5, 2019

laanwj closed this on Jan 5, 2019

laanwj referenced this in commit 9c71998771 on Jan 5, 2019

promag deleted the branch on Jan 5, 2019

linuxsh2 referenced this in commit 52f0cd7368 on Jul 27, 2021

linuxsh2 referenced this in commit 3f063f8f99 on Jul 27, 2021

linuxsh2 referenced this in commit e011fac5ce on Jul 27, 2021

linuxsh2 referenced this in commit ebffbe090b on Jul 27, 2021

linuxsh2 referenced this in commit 6fc997f670 on Jul 27, 2021

linuxsh2 referenced this in commit 9805096d23 on Jul 28, 2021

linuxsh2 referenced this in commit 13b243cb9c on Jul 28, 2021

linuxsh2 referenced this in commit 4d29d40f10 on Jul 28, 2021

linuxsh2 referenced this in commit 701be9a535 on Jul 28, 2021

linuxsh2 referenced this in commit 9088cd1fc4 on Jul 28, 2021

linuxsh2 referenced this in commit 549d60dd07 on Jul 29, 2021

linuxsh2 referenced this in commit 9c05f6d568 on Jul 30, 2021

Munkybooty referenced this in commit 6d40e6be8a on Aug 3, 2021

DrahtBot locked this on Dec 16, 2021

qt: Use WalletModel* instead of the wallet name as map key #14784