util::Result from WalletLoader methods
#25616
This PR adds a method that implement common logic to WalletLoader methods and change them to return BResult<std::unique_ptr<Wallet>>.
Motivation: #25594 changed restoreWallet to return BResult but this method shares a common pattern with createWallet and loadWallet. This PR keeps the same pattern to all WalletLoader methods.
349+ auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
350
351- if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
352+ m_error_message = wallet ? bilingual_str{} : wallet.GetError();
353+
354+ if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
0auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
1
2if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
3else m_error_message = wallet.GetError();
266+ auto wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
267
268- if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
269+ m_error_message = wallet ? bilingual_str{} : wallet.GetError();
270+
271+ if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
0auto res_wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
1
2if (res_wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(res_wallet.ReleaseObj());
3else m_error_message = res_wallet.GetError();
343@@ -343,9 +344,10 @@ void OpenWalletActivity::open(const std::string& path)
344 tr("Opening Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
345
346 QTimer::singleShot(0, worker(), [this, path] {
347- std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().loadWallet(path, m_error_message, m_warning_message);
348+ auto wallet{node().walletLoader().loadWallet(path, m_warning_message)};
349
350- if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
351+ if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
352+ else m_error_message = wallet.GetError();
Below I used m_error_message = wallet ? bilingual_str{} : wallet.GetError();
It would be good to use the same code consistently everywhere.
I think the code change was not correctly pushed. The commit 7cbfa1335bd89d5831e916672e57aa8601b12bb4 still displays the line:
0else m_error_message = wallet.GetError();
and not:
0m_error_message = wallet ? bilingual_str{} : wallet.GetError();
@shaavan I said the suggestion was addressed because I understand it’s about keeping all code consistent across all commits.
I had originally defined all m_error_message same way as done in #25594: m_error_message = wallet ? bilingual_str{} : wallet.GetError();.
So #25616 (review) and #25616 (review) suggested changing them to else m_error_message = res_wallet.GetError(); but I had forgotten to change it to restoreWallet(). https://github.com/bitcoin/bitcoin/commit/7cbfa1335bd89d5831e916672e57aa8601b12bb4 fixed this.
If I understand the BResult interface correctly, it seems to be the clearest option, as BResult<T>::GetError() will return the default bilingual_str{} : value if T is not set.
However, both approaches work. If the reviewers have a strong opinion about one of them, I can change it.
BResult is essentially an std::variant wrapper. It stores one object OR the other (succeed or failure). Not both.
So, it is redundant to ask m_error_message = wallet ? bilingual_str{} : wallet.GetError(); first and then ask if succeeded in another if block.
m_error_message = wallet ? bilingual_str{} : wallet.GetError(); was intentional to reset the member that stores the error, instead of leaving the previous value, if no error occurred. However, I am fine changing it to something else.
Oh, I missed the comment, sorry Marko.
Is it possible to load a wallet with an error and then load a wallet without error on the same walletcontroller?
You mean, restoring or opening a wallet?
Because if that is the case, there shouldn’t be any problem. Every time that we restore or open a wallet we create a new WalletControllerActivity subclass which encapsulates the error and warning messages (it’s a single shot class. It gets deleted as soon as it finishes processing the action).
0- if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
1- else m_error_message = wallet.GetError();
2+ if (wallet) {
3+ m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
4+ } else {
5+ m_error_message = wallet.GetError();
6+ }
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
GetReservedDestination methods by theStack)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.
548@@ -549,8 +549,15 @@ class WalletLoaderImpl : public WalletLoader
549 void stop() override { return StopWallets(m_context); }
550 void setMockTime(int64_t time) override { return SetMockTime(time); }
551
552+ //! Return a wallet interface object or an error wrapped in a BResult instance
553+ BResult<std::unique_ptr<Wallet>> makeWallet(const std::shared_ptr<CWallet>& _wallet, bilingual_str& error)
makeWallet be renamed to makeWalletResult or something else? Having both createWallet and makeWallet is a bit confusing.
ACK 7cbfa1335bd89d5831e916672e57aa8601b12bb4. I think this is a good use-case of BResult.
nit: Shouldn’t the commits be merged into one as the changes are small and related?
Concept ACK
Is it possible to load a wallet with an error and then load a wallet without an error on the same walletcontroller?
ACK 087959d6e92ed37b72b86dbbc9b0ff052a6c5a75
Changes since my last review:
makeWallet -> makeWalletResult.As per @furszy explanation in this comment, since the WalletControllerActivity class gets deleted after the loading of a wallet finishes, it is not possible to have an error message stored from a previous erroneous loading of a wallet.
Hence it is not necessary to manually reset bilingual_str{}.
Thanks, @furszy, for the explanation.
261@@ -262,9 +262,10 @@ void CreateWalletActivity::createWallet()
262 }
263
264 QTimer::singleShot(500ms, worker(), [this, name, flags] {
265- std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().createWallet(name, m_passphrase, flags, m_error_message, m_warning_message);
266+ auto res_wallet{node().walletLoader().createWallet(name, m_passphrase, flags, m_warning_message)};
267
268- if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
269+ if (res_wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(res_wallet.ReleaseObj());
270+ else m_error_message = res_wallet.GetError();
0- if (res_wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(res_wallet.ReleaseObj());
1- else m_error_message = res_wallet.GetError();
2+ if (res_wallet) {
3+ m_wallet_model = m_wallet_controller->getOrCreateWallet(res_wallet.ReleaseObj());
4+ } else {
5+ m_error_message = res_wallet.GetError();
6+ }
Any reason why you’ve changed the variable name from wallet to res_wallet here? It’s wallet in the identical code sections below.
394@@ -393,8 +395,8 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri
395 QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
396 auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
397
398- m_error_message = wallet ? bilingual_str{} : wallet.GetError();
399 if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
400+ else m_error_message = wallet.GetError();
0- if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
1- else m_error_message = wallet.GetError();
2+ if (wallet) {
3+ m_wallet_model = m_wallet_controller->getOrCreateWallet(wallet.ReleaseObj());
4+ } else {
5+ m_error_message = wallet.GetError();
6+ }
87@@ -88,7 +88,7 @@ class Wallet
88 virtual std::string getWalletName() = 0;
89
90 // Get a new address.
91- virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string label) = 0;
92+ virtual util::Result<CTxDestination> getNewDestination(const OutputType type, const std::string& label) = 0;
The wallet method still creates a copy?
0$ git grep GetNewDestination -- '*wallet.h'
1src/wallet/wallet.h: util::Result<CTxDestination> GetNewDestination(const OutputType type, const std::string label);
576 DatabaseStatus status;
577 ReadDatabaseArgs(*m_context.args, options);
578 options.require_existing = true;
579- return MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings));
580+ bilingual_str error;
581+ util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, LoadWallet(m_context, name, true /* load_on_start */, options, status, error, warnings))};
createWallet() above, while touching this line can use Clang-tidy named arg format (/*load_on_start=*/true) like in restoreWallet() below.
565+ bilingual_str error;
566+ util::Result<std::unique_ptr<Wallet>> wallet{MakeWallet(m_context, CreateWallet(m_context, name, true /* load_on_start */, options, status, error, warnings))};
567+ if (!wallet) {
568+ return util::Error{error};
569+ }
570+ return wallet;
return wallet ? wallet : util::Error{error}; (feel free to ignore)
That would be good, but applying this change will result in the error below. I’m not sure about the reason for the error.
0wallet/interfaces.cpp:577:25: error: call to implicitly-deleted copy constructor of 'util::Result<std::unique_ptr<Wallet>>'
1 return wallet ? wallet : util::Error{error};
2 ^~~~~~
3./util/result.h:38:36: note: copy constructor of 'Result<std::unique_ptr<interfaces::Wallet>>' is implicitly deleted because field 'm_variant' has a deleted copy constructor
4 std::variant<bilingual_str, T> m_variant;
std::unique_ptr is move constructible and move assignable but not copy constructible or copy assignable. So this would work: return wallet ? std::move(wallet) : util::Error{error};
Thanks. Done in https://github.com/bitcoin/bitcoin/pull/25616/commits/466374f6c1a032d7540b1c9307631f56fe555fcf.
So return wallet moves the object and return wallet ? wallet : util::Error{error}; copies the object. Interesting.
It might be that return with a ternary doesn’t benefit from RVO (return value optimization) by the compiler in the same way as with an if statement.
https://en.cppreference.com/w/cpp/language/copy_elision
Edit: benefitting from RVO could (possibly, unsure) be an argument for using the more verbose if statement.
Concept ACK
nit: Same as done in the commit message, the PR title and description should also be adapted to the latest rebase (s/BResult/util::Result/).
CI error seems unrelated.
interface_usdt_validation.py worked fine on my machine.
0@@ -0,0 +1,81 @@
1+{
918@@ -919,10 +919,9 @@ BOOST_FIXTURE_TEST_CASE(wallet_sync_tx_invalid_state_test, TestingSetup)
919 // Add tx to wallet
920 const auto& op_dest = wallet.GetNewDestination(OutputType::BECH32M, "");
921 BOOST_ASSERT(op_dest);
922- const CTxDestination& dest = *op_dest;
923
924 CMutableTransaction mtx;
925- mtx.vout.push_back({COIN, GetScriptForDestination(dest)});
926+ mtx.vout.push_back({COIN, GetScriptForDestination(*op_dest)});
1769@@ -1770,7 +1770,7 @@ bool DescriptorScriptPubKeyMan::GetReservedDestination(const OutputType type, bo
1770 } else {
1771 error = util::ErrorString(op_dest);
1772 }
1773- return bool(op_dest);
1774+ return op_dest.has_value();
267
268- if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(wallet));
269+ m_error_message = util::ErrorString(wallet);
270+ if (wallet) {
271+ m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
272+ }
why the revert here? (and in the others as well). the result could be an object or an error, not both.
0if (wallet) m_wallet_model = something;
1else m_error_message = util::ErrorString(wallet);
Thinking that might be good to add an assertion inside util::ErrorString, callers should always check that the result is an error prior to convert it.
Maybe assert(std::holds_alternative<bilingual_str>(result.m_variant)); inside util::ErrorString ?
However this assertion cause a compile-time error error: static_assert failed due to requirement '__detail::__variant::__exactly_once<bilingual_str, bilingual_str, bilingual_str>' "T must occur exactly once in alternatives"
assert(!result.has_value()); there should work fine.
I applied the suggestion above (if (wallet) m_wallet_model = something;) in https://github.com/bitcoin/bitcoin/pull/25616/commits/ac7479953bc95baf6ef4b35fa25c27dd4100fe6a.
But regarding the assertion, I think a follow-up PR is better because it also requires changing the src/test/result_tests.cpp which expects the result of util::ErrorString be {} if the object has value This would increase the scope of this PR.
yep, aren’t we having a single line statement here?
0if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));
1else m_error_message = util::ErrorString(wallet);
319@@ -320,31 +320,31 @@ class WalletLoader : public ChainClient
320 {
321 public:
322 //! Create new wallet.
323- virtual std::unique_ptr<Wallet> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, bilingual_str& error, std::vector<bilingual_str>& warnings) = 0;
324+ virtual util::Result<std::unique_ptr<Wallet>> createWallet(const std::string& name, const SecureString& passphrase, uint64_t wallet_creation_flags, std::vector<bilingual_str>& warnings) = 0;
re: #25616 (review)
Would probably be best to also wrap the warnings in the result? Are you working on this @ryanofsky ?
It needs rebase, but yes I did this in #25722
Result<void>) that isn’t needed for simply passing warnings
400@@ -393,8 +401,11 @@ void RestoreWalletActivity::restore(const fs::path& backup_file, const std::stri
401 QTimer::singleShot(0, worker(), [this, backup_file, wallet_name] {
402 auto wallet{node().walletLoader().restoreWallet(backup_file, wallet_name, m_warning_message)};
403
404- m_error_message = util::ErrorString(wallet);
405- if (wallet) m_wallet_model = m_wallet_controller->getOrCreateWallet(std::move(*wallet));