wallet: Use PACKAGE_NAME to mention our software #23311

hebasto commented at 9:10 PM on October 19, 2021: member

This PR replaces "bitcoin" and "bitcoind" with PACKAGE_NAME in wallet log and error messages.

in src/wallet/bdb.cpp:135 in 16fad5b415 outdated

 131 | @@ -132,7 +132,7 @@ bool BerkeleyEnvironment::Open(bilingual_str& err)
 132 |      fs::path pathIn = fs::PathFromString(strPath);
 133 |      TryCreateDirectories(pathIn);
 134 |      if (!LockDirectory(pathIn, ".walletlock")) {
 135 | -        LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of bitcoin may be using it.\n", strPath);
 136 | +        LogPrintf("Cannot obtain a lock on wallet directory %s. Another instance of %s may be using it.\n", strPath, PACKAGE_NAME);

luke-jr commented at 9:14 PM on October 19, 2021:

There's no reason to assume it's necessarily Bitcoin Core, though.

hebasto commented at 9:16 PM on October 19, 2021:

Leave it as is?

MarcoFalke commented at 7:27 AM on October 20, 2021:

Maybe make it general: "Another instance may be using it.".

hebasto commented at 6:32 PM on October 20, 2021:

Thanks! Updated.

DrahtBot added the label Wallet on Oct 19, 2021

shaavan commented at 12:42 PM on October 20, 2021: contributor

Concept ACK

This change helps in the generalization of strings and makes them robust in case of any possible future change.

I was just curious. What do you think about adding the word “instance” followed by the package name?

For example:

throw std::runtime_error("SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another " PACKAGE_NAME "?\n");

Changed to

throw std::runtime_error("SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another " PACKAGE_NAME " instance?\n");

This suggestion is valid for all the strings that have been updated except the one in the src/wallet/bdb.cpp file

laanwj commented at 2:22 PM on October 20, 2021: member

Concept ACK

I was just curious. What do you think about adding the word “instance” followed by the package name?

Yes, I think adding "instance" makes the sentences better.

jarolrod commented at 4:03 PM on October 20, 2021: member

concept ACK

wallet: Use PACKAGE_NAME to mention our software da791c7f66

in src/wallet/sqlite.cpp:231 in 16fad5b415 outdated

 227 | @@ -228,7 +228,7 @@ void SQLiteDatabase::Open()
 228 |      // Now begin a transaction to acquire the exclusive lock. This lock won't be released until we close because of the exclusive locking mode.
 229 |      int ret = sqlite3_exec(m_db, "BEGIN EXCLUSIVE TRANSACTION", nullptr, nullptr, nullptr);
 230 |      if (ret != SQLITE_OK) {
 231 | -        throw std::runtime_error("SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another bitcoind?\n");
 232 | +        throw std::runtime_error("SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another " PACKAGE_NAME "?\n");

jonatack commented at 4:30 PM on October 20, 2021:

        throw std::runtime_error("SQLiteDatabase: Unable to obtain an exclusive lock on the database, is it being used by another instance of " PACKAGE_NAME "?\n");

hebasto commented at 6:32 PM on October 20, 2021:

Thanks! Updated.

hebasto force-pushed on Oct 20, 2021

hebasto commented at 6:31 PM on October 20, 2021: member

Updated 16fad5b415efc803eab53a5c8eddd129637a78aa -> da791c7f66243080177f92ce5f38c49305da63dc (pr23311.01 -> pr23311.02, diff).

Thanks to all reviewers, your comments are addressed :tiger2:

lsilva01 approved

lsilva01 commented at 12:00 AM on October 21, 2021: contributor

Tested ACK da791c7 on Ubuntu 20.04.

jonatack commented at 1:23 PM on October 21, 2021: member

ACK da791c7f66243080177f92ce5f38c49305da63dc

Verified that changing the test assertions to just "Bitcoin Core" is equivalent.

Would it make sense to also update these 3 lines?

src/wallet/dump.cpp:147:        error = strprintf(_("Error: Dumpfile version is not supported. This version of bitcoin-wallet only supports version 1 dumpfiles. Got dumpfile with version %s"), version_value);
test/functional/tool_wallet.py:362:        self.assert_raises_tool_error('Error: Dumpfile version is not supported. This version of bitcoin-wallet only supports version 1 dumpfiles. Got dumpfile with version 0', '-wallet=badload', '-dumpfile={}'.format(bad_ver_wallet_dump), 'createfromdump')
test/functional/tool_wallet.py:367:        self.assert_raises_tool_error('Error: Dumpfile version is not supported. This version of bitcoin-wallet only supports version 1 dumpfiles. Got dumpfile with version 2', '-wallet=badload', '-dumpfile={}'.format(bad_ver_wallet_dump), 'createfromdump')

brunoerg approved

brunoerg commented at 3:56 PM on October 22, 2021: member

tACK da791c7f66243080177f92ce5f38c49305da63dc

stratospher commented at 1:53 PM on October 23, 2021: contributor

Tested ACK da791c7

The generic error messages introduced by this PR in src/wallet/bdb.cpp and src/wallet/sqlite.cpp, which reflect that there may be other instances(not necessarily a bitcoin instance or a bitcoind instance) using the lock is a nice change!

shaavan approved

shaavan commented at 12:59 PM on October 25, 2021: contributor

ACK da791c7f66243080177f92ce5f38c49305da63dc

Changes since my last review:

Removed “of bitcoin” in the string in the src/wallet/bdb.cpp file to increase the generality of this string message.
Added “instance of” in all other (updated) strings to correct the sentences.

MarcoFalke commented at 1:23 PM on October 25, 2021: member

I still think we can remove PACKAGE_NAME, because it is not possible to say whether the other instance is Bitcoin Core or Bitcoin Knots, or bitcoind, or bitcoin-qt, or ...

But :shrug:

MarcoFalke merged this on Oct 25, 2021

MarcoFalke closed this on Oct 25, 2021

sidhujag referenced this in commit 581c489479 on Oct 25, 2021

hebasto deleted the branch on Oct 25, 2021

DrahtBot locked this on Oct 30, 2022