Skip to content
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.

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

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

Merged
merged 3 commits into from Jan 5, 2019

Conversation

Projects
None yet
7 participants
@promag
Copy link
Member

commented Nov 22, 2018

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

@fanquake fanquake added the GUI label Nov 22, 2018

Show resolved Hide resolved src/qt/walletframe.h Outdated
Show resolved Hide resolved src/qt/walletframe.h Outdated
@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

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 promag changed the title qt: Don't use the wallet name as map key qt: Use WalletModel* instead of the wallet name as map key Nov 23, 2018

@promag promag force-pushed the promag:2018-11-qtwalletmodel branch Nov 23, 2018

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 23, 2018

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

@practicalswift done.

@hebasto

This comment has been minimized.

Copy link
Member

commented Nov 24, 2018

Concept ACK.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Nov 25, 2018

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

@promag

This comment has been minimized.

Copy link
Member Author

commented Nov 26, 2018

@jonasschnelli what are your concerns?

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

@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

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

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

@ryanofsky
Copy link
Contributor

left a comment

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

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

Yes the RPC identifies the wallet by its name.

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

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

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 3, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Jan 3, 2019

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 promag force-pushed the promag:2018-11-qtwalletmodel branch Jan 4, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

Rebased and also use WalletModel* in console window.

@promag promag referenced this pull request Jan 4, 2019

Merged

gui: Add WalletController #15101

@laanwj

This comment has been minimized.

Copy link
Member

commented Jan 4, 2019

travis fail:

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

@promag promag force-pushed the promag:2018-11-qtwalletmodel branch Jan 4, 2019

@promag promag force-pushed the promag:2018-11-qtwalletmodel branch Jan 4, 2019

@promag promag force-pushed the promag:2018-11-qtwalletmodel branch to 91b0c5b Jan 4, 2019

@promag

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

Thanks @laanwj, fixed now.

@ryanofsky
Copy link
Contributor

left a comment

utACK 91b0c5b. 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 91b0c5b 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

This comment has been minimized.

Copy link
Member Author

commented Jan 4, 2019

@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 laanwj merged commit 91b0c5b into bitcoin:master Jan 5, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jan 5, 2019

Merge #14784: qt: Use WalletModel* instead of the wallet name as map key
91b0c5b qt: Use WalletModel* instead of wallet name in console window (João Barbosa)
b2ce86c qt: Use WalletModel* instead of wallet name in main window (João Barbosa)
d2a1adf qt: Factor out WalletModel::getDisplayName() (João Barbosa)

Pull request description:

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

Tree-SHA512: 1820d0ff28e84b1d862097f1f55b52f94520fa50c9b1939d235a448a48159748c3bbf99b19e4cb1ff4f91efc008c0971b4c25a91f645f9d43792c8aeaa93cf9e

@promag promag deleted the promag:2018-11-qtwalletmodel branch Jan 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.