Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 3 commits into from
Jan 5, 2019
Merged

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

laanwj merged 3 commits into from
Jan 5, 2019

Conversation

@promag
Copy link
Contributor

@promag promag 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
practicalswift
practicalswift suggested changes Nov 23, 2018
View reviewed changes
@laanwj
Copy link
Member

laanwj 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 2018-11-qtwalletmodel branch from 49f3dd7 to 492156b Compare November 23, 2018 12:06
@promag
Copy link
Contributor Author

promag commented Nov 23, 2018

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

@practicalswift done.

@hebasto
Copy link
Member

hebasto commented Nov 24, 2018

Concept ACK.

@jonasschnelli
Copy link
Contributor

jonasschnelli commented Nov 25, 2018

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

@promag
Copy link
Contributor Author

promag commented Nov 26, 2018

@jonasschnelli what are your concerns?

@laanwj
Copy link
Member

laanwj 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
Copy link
Contributor Author

promag commented Jan 2, 2019

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

ryanofsky
ryanofsky approved these changes Jan 2, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

laanwj 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
Copy link
Contributor Author

promag commented Jan 3, 2019

Yes the RPC identifies the wallet by its name.

@laanwj
Copy link
Member

laanwj commented Jan 3, 2019 •
edited
Loading

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
Copy link
Contributor Author

promag 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
Copy link
Member

laanwj 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
Copy link
Contributor Author

promag 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 2018-11-qtwalletmodel branch from 492156b to ac729ad Compare January 4, 2019 12:18
@promag
Copy link
Contributor Author

promag commented Jan 4, 2019

Rebased and also use WalletModel* in console window.

@promag promag mentioned this pull request Jan 4, 2019
Merged
@laanwj
Copy link
Member

laanwj 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 2018-11-qtwalletmodel branch from ac729ad to dd39ca3 Compare January 4, 2019 15:19
@promag promag force-pushed the 2018-11-qtwalletmodel branch from dd39ca3 to 3f427b4 Compare January 4, 2019 15:22
@promag promag force-pushed the 2018-11-qtwalletmodel branch from 3f427b4 to 91b0c5b Compare January 4, 2019 15:24
@promag
Copy link
Contributor Author

promag commented Jan 4, 2019

Thanks @laanwj, fixed now.

ryanofsky
ryanofsky approved these changes Jan 4, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

promag 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
laanwj added a commit that referenced this pull request Jan 5, 2019
@laanwj
Merge #14784: qt: Use WalletModel* instead of the wallet name as map key
9c71998 
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 2018-11-qtwalletmodel branch January 5, 2019 15:29
linuxsh2 pushed a commit to linuxsh2/dash that referenced this pull request Jul 28, 2021
@laanwj @linuxsh2
Merge bitcoin#14784: qt: Use WalletModel* instead of the wallet name …
9088cd1 
…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
linuxsh2 pushed a commit to linuxsh2/dash that referenced this pull request Jul 30, 2021
@laanwj @linuxsh2
Merge bitcoin#14784: qt: Use WalletModel* instead of the wallet name …
9c05f6d 
…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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+1 more reviewer

@practicalswift practicalswift practicalswift requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

GUI

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@promag @laanwj @hebasto @jonasschnelli @ryanofsky @practicalswift @fanquake