bugfix: make LoadWallet assigns status always #19919

AkioNak commented at 4:46 PM on September 8, 2020: contributor

In my enviroment, test/functional/wallet_multiwallet.py failed in line 237 for master( 147d50d63 ). It got an expected rpc-error-message, but error code was not (-4) but (-18).

This is because that although loadwallet() in rpcwallet.cpp assumes LoadWallet() always assign some value to the 'status', but LoadWallet() does not do so in some situation.

This PR intends to fix above and prevends loadwallet() returns ambiguous error code.

in src/wallet/db.h:217 in f32ddd8126 outdated

 213 | @@ -214,6 +214,7 @@ enum class DatabaseStatus {
 214 |      FAILED_CREATE,
 215 |      FAILED_VERIFY,
 216 |      FAILED_ENCRYPT,
 217 | +    FAILED,

ryanofsky commented at 6:06 PM on September 8, 2020:

In commit "bugfix: make LoadWallet assigns status always" (f32ddd81267cfca9f3c07006cef7c80f79a48527)

Can FAILED be renamed to FAILED_LOAD and moved immediately after FAILED_CREATE above? FAILED_LOAD and FAILED_CREATE would be the most generic failures, but it'd be good to distinguish between loading an existing wallet and creating a new one.

in src/wallet/wallet.cpp:215 in f32ddd8126 outdated

 211 | @@ -212,6 +212,7 @@ std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, const std:
 212 |          std::shared_ptr<CWallet> wallet = CWallet::Create(chain, name, std::move(database), options.create_flags, error, warnings);
 213 |          if (!wallet) {
 214 |              error = Untranslated("Wallet loading failed.") + Untranslated(" ") + error;
 215 | +            status = DatabaseStatus::FAILED_CREATE;

ryanofsky commented at 6:09 PM on September 8, 2020:

In commit "bugfix: make LoadWallet assigns status always" (f32ddd81267cfca9f3c07006cef7c80f79a48527)

Better to use DatabaseStatus::FAILED_LOAD here because the error happens when loading an existing wallet not creating a new one. (CWallet::Create name refers to creating a CWallet object not creating a new database).

in src/wallet/wallet.cpp:227 in f32ddd8126 outdated

 223 | @@ -223,6 +224,7 @@ std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, const std:
 224 |          return wallet;
 225 |      } catch (const std::runtime_error& e) {
 226 |          error = Untranslated(e.what());
 227 | +        status = DatabaseStatus::FAILED;

ryanofsky commented at 6:11 PM on September 8, 2020:

In commit "bugfix: make LoadWallet assigns status always" (f32ddd81267cfca9f3c07006cef7c80f79a48527)

Would be good to use DatabaseStatus::FAILED_LOAD here too

in src/wallet/wallet.cpp:238 in f32ddd8126 outdated

 234 | @@ -233,6 +235,7 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string&
 235 |      auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(name));
 236 |      if (!result.second) {
 237 |          error = Untranslated("Wallet already being loading.");
 238 | +        status = DatabaseStatus::FAILED_BAD_PATH;

ryanofsky commented at 6:11 PM on September 8, 2020:

In commit "bugfix: make LoadWallet assigns status always" (f32ddd81267cfca9f3c07006cef7c80f79a48527)

Would be good to use DatabaseStatus::FAILED_LOAD here too

ryanofsky changes_requested

ryanofsky commented at 6:12 PM on September 8, 2020: member

Thanks for catching an fixing this!

DrahtBot added the label Wallet on Sep 8, 2020

haggleburt1972 approved

promag commented at 9:20 PM on September 8, 2020: member

This only affects master?

ryanofsky commented at 9:30 PM on September 8, 2020: member

This only affects master?

The behavior change probably happened with #19619. I haven't tried to reproduce this, but the status variables this is assigning didn't exist before #19619

AkioNak force-pushed on Sep 9, 2020

ryanofsky approved

ryanofsky commented at 1:46 AM on September 9, 2020: member

Code review ACK 1728059730abef04f3fa84de0b6e20044be7a9d6. Thanks for the fix! Just suggested status code tweaks since last review

AkioNak commented at 2:05 AM on September 9, 2020: contributor

@ryanofsky @haggleburt1972 Thank you for reviewing this!

hebasto approved

hebasto commented at 6:11 AM on September 9, 2020: member

ACK 1728059730abef04f3fa84de0b6e20044be7a9d6, I have reviewed the code and it looks OK, I agree it can be merged.

But I failed to reproduce test/functional/wallet_multiwallet.py fails on master (tried some merge commits deep in history).

promag commented at 7:03 AM on September 9, 2020: member

It is a good practice to have a DatabaseStatus::UNKNOWN as default value and assert it's != after the call?

AkioNak commented at 7:46 AM on September 9, 2020: contributor

@hebasto Thanks. FWIW, my enviroment is: macOS Catalina 10.15.6 and configure with --enable-debug.

% uname -mrsvp
Darwin 19.6.0 Darwin Kernel Version 19.6.0: Thu Jun 18 20:49:00 PDT 2020; root:xnu-6153.141.1~1/RELEASE_X86_64 x86_64 i386


% clang --version
Apple clang version 11.0.3 (clang-1103.0.32.62)
Target: x86_64-apple-darwin19.6.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin

AkioNak commented at 7:50 AM on September 9, 2020: contributor

@promag Thanks. I will address (both loadwallet() and createwallet() that refer the status) .

AkioNak force-pushed on Sep 9, 2020

ryanofsky commented at 1:50 PM on September 9, 2020: member

5c4fd19dabd27fe64f7cb66387e00a582c99135d is ok but I preferred 1728059730abef04f3fa84de0b6e20044be7a9d6. I think UNKNOWN/INVALID error codes are bad practice and source of bugs on their own. If you want to handle unset values better to use Optional<DatabaseStatus> type to be unambiguous and actually force all calls to deal with unset values instead of having some callers add unknown/check_nonfatal wrapping and leaving other calls unchecked.

I guess this is bikeshedding though. Prefer 1728059730abef04f3fa84de0b6e20044be7a9d6 but ACK 5c4fd19dabd27fe64f7cb66387e00a582c99135d

promag commented at 2:04 PM on September 9, 2020: member

@ryanofsky yup, I've asked if it makes sense some approach to prevent unset values. The current approach is similarly bad because nothing forces settings the default to UNKNOWN in the first place. In this case I'd drop the "output" arguments and return a LoadResult structure, something that could be used like:

return LoadResult(wallet); // for success
return LoadResult(DatabaseStatus::FAILED_LOAD, Untranslated("Wallet already being loading."))

This would ensure a result is fully set.

Anyway, I agree with @ryanofsky that the latest version isn't an improvement.

AkioNak commented at 3:09 PM on September 9, 2020: contributor

@ryanofsky @promag Sorry for bother you caused by my misunderstanding. I think I should at least revert these latest improvement-less changes , right?

bugfix: make LoadWallet assigns status always

Although loadwallet() in rpcwallet.cpp assumes LoadWallet() always
assign some value to the 'status', but LoadWallet() does not do so
in some situation.

This fixes above and prevends loadwallet() returns ambiguous error code.

8b39a87558

AkioNak force-pushed on Sep 9, 2020

AkioNak commented at 3:59 PM on September 9, 2020: contributor

Revered to same as 1728059 .

hebasto approved

hebasto commented at 4:29 PM on September 9, 2020: member

re-ACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59, that is the same as 1728059730abef04f3fa84de0b6e20044be7a9d6.

ryanofsky approved

ryanofsky commented at 1:45 PM on September 11, 2020: member

This seems mergeable

Code review ACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59 (same as previous)

meshcollider commented at 11:49 PM on September 12, 2020: contributor

utACK 8b39a875581bed1c2f40a7d9616bdb7cc642bf59

meshcollider merged this on Sep 13, 2020

meshcollider closed this on Sep 13, 2020

sidhujag referenced this in commit 24ffd8351c on Sep 13, 2020

AkioNak deleted the branch on Sep 14, 2020

deadalnix referenced this in commit 0ddd20024b on Oct 5, 2021

DrahtBot locked this on Feb 15, 2022