wallet: Avoid translating RPC errors #18699

pull MarcoFalke wants to merge 4 commits into bitcoin:master from MarcoFalke:1908-walletErrorSegfault changing 26 files +260 −221
  1. MarcoFalke commented at 6:04 pm on April 18, 2020: member

    Common errors and warnings should be translated when displayed in the GUI, but not translated when displayed elsewhere. The wallet method CreateWalletFromFile does not know its caller, so this commit changes it to return a bilingual_str to the caller.

    Fixes #17072

  2. DrahtBot added the label GUI on Apr 18, 2020
  3. DrahtBot added the label RPC/REST/ZMQ on Apr 18, 2020
  4. DrahtBot added the label Utils/log/libs on Apr 18, 2020
  5. DrahtBot added the label Wallet on Apr 18, 2020
  6. MarcoFalke removed the label GUI on Apr 18, 2020
  7. MarcoFalke removed the label RPC/REST/ZMQ on Apr 18, 2020
  8. MarcoFalke removed the label Utils/log/libs on Apr 18, 2020
  9. MarcoFalke force-pushed on Apr 18, 2020
  10. MarcoFalke commented at 6:57 pm on April 18, 2020: member

    Can be tested with:

    0XDG_SESSION_TYPE=""  QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=bitcoin-qt ./test/functional/wallet_basic.py

    The gui popups should be translated, but the json rpc errors should not.

  11. hebasto commented at 7:47 pm on April 18, 2020: member
    Concept ACK.
  12. DrahtBot commented at 10:16 pm on April 18, 2020: member

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #18836 (wallet: upgradewallet fixes and additional tests by achow101)
    • #18790 (gui: Improve thread naming by hebasto)
    • #18740 (Remove g_rpc_node global by ryanofsky)
    • #18654 (rpc: separate bumpfee’s psbt creation function into psbtbumpfee by achow101)
    • #18618 (gui: Drop RecentRequestsTableModel dependency to WalletModel by promag)
    • #18608 (refactor: Remove CAddressBookData::destdata by ryanofsky)
    • #18592 (rpc: replace raw pointers with shared_ptrs by brakmic)
    • #18531 (rpc: Assert that RPCArg names are equal to CRPCCommand ones by MarcoFalke)
    • #18354 (Protect wallet by using shared pointers by bvbfan)
    • #17526 (Use Single Random Draw In addition to knapsack as coin selection fallback by achow101)
    • #17331 (Use effective values throughout coin selection by achow101)
    • #17211 (Allow fundrawtransaction and walletcreatefundedpsbt to take external inputs by achow101)
    • #16910 (wallet: reduce loading time by using unordered maps by achow101)
    • #16224 (gui: Bilingual GUI error messages by hebasto)
    • #15719 (Drop Chain::requestMempoolTransactions method by ryanofsky)
    • #14582 (wallet: always do avoid partial spends if fees are within a specified range by kallewoof)

    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.

  13. MarcoFalke force-pushed on Apr 18, 2020
  14. MarcoFalke force-pushed on Apr 19, 2020
  15. hebasto commented at 10:29 am on April 21, 2020: member

    @MarcoFalke

    Can be tested with:

    0XDG_SESSION_TYPE=""  QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=bitcoin-qt ./test/functional/wallet_basic.py

    The gui popups should be translated, but the json rpc errors should not.

    On Linux Mint 19.3 having a timeout:

     0$ QT_LOGGING_RULES="qt5ct.debug=false" XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LC_ALL=ru_RU.UTF-8 BITCOIND=/home/hebasto/GitHub/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py
 12020-04-21T10:25:52.532000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_ya2ujqgn
 22020-04-21T10:25:53.484000Z TestFramework (INFO): Mining blocks...
 32020-04-21T10:26:54.210000Z TestFramework (ERROR): Assertion failed
 4Traceback (most recent call last):
 5  File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_framework.py", line 112, in main
 6    self.run_test()
 7  File "./test/functional/wallet_basic.py", line 65, in run_test
 8    self.sync_all(self.nodes[0:3])
 9  File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_framework.py", line 490, in sync_all
10    self.sync_blocks(nodes, **kwargs)
11  File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/test_framework.py", line 484, in sync_blocks
12    sync_blocks(nodes or self.nodes, **kwargs)
13  File "/home/hebasto/GitHub/bitcoin/test/functional/test_framework/util.py", line 413, in sync_blocks
14    raise AssertionError("Block sync timed out:{}".format("".join("\n  {!r}".format(b) for b in best_hash)))
15AssertionError: Block sync timed out:
16  '25fa4cf68bbf80d0b9cf47795f8eff272b7230d16c94f0b1a82602aadf6398a2'
17  '0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206'
18  '0f9188f13cb7b2c71f2a335e3a4fc328bf5beb436012afca590b1a11466e2206'
192020-04-21T10:26:54.266000Z TestFramework (INFO): Stopping nodes
202020-04-21T10:26:54.726000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_ya2ujqgn
212020-04-21T10:26:54.726000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_ya2ujqgn/test_framework.log
222020-04-21T10:26:54.727000Z TestFramework (ERROR): Hint: Call /home/hebasto/GitHub/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_ya2ujqgn' to consolidate all logs
  16. hebasto commented at 10:40 am on April 21, 2020: member
    Should CheckUniqueFileid() errors be translated for GUI (possibly in other PR)?
  17. hebasto approved
  18. hebasto commented at 11:03 am on April 21, 2020: member

    ACK fa5ba7e472b90707f7ae13ba2ab9e2b106a9c743, tested on Linux Mint 19.3 by manually running commands in the GUI and in the console window.

    Noted that it is not possible to confirm that all touched in this PR error messages are translated in the GUI as translation files are not updated for now, e.g., https://github.com/bitcoin/bitcoin/blob/fadc12f29e17451a090fc7fa03bdf4603ab874ba/src/wallet/wallet.cpp#L157

  19. MarcoFalke commented at 11:22 am on April 21, 2020: member

    Noted that it is not possible to confirm that all touched in this PR error messages are translated in the GUI as translation files are not updated for now, e.g.,

    This is always true, as translations may be missing. You reminded me to remove trailing whitespace from translations, since whitespace is hard to translate and easy to forget. Added a new commit, no changes to the commit you already reviewed.

  20. MarcoFalke commented at 11:23 am on April 21, 2020: member

    On Linux Mint 19.3 having a timeout:

    Is there an error popup? You might have to close it. You can also try without the QT_QPA_PLATFORM and manually close all error popups.

  21. hebasto commented at 11:31 am on April 21, 2020: member

    On Linux Mint 19.3 having a timeout:

    Is there an error popup? You might have to close it. You can also try without the QT_QPA_PLATFORM and manually close all error popups.

    No error popups. Without the QT_QPA_PLATFORM I can see three windows but no error popups.

  22. in src/wallet/feebumper.cpp:225 in fa5a95c678 outdated 
    219@@ -218,9 +220,9 @@ Result CreateRateBumpTransaction(CWallet& wallet, const uint256& txid, const CCo
220     CTransactionRef tx_new = MakeTransactionRef();
221     CAmount fee_ret;
222     int change_pos_in_out = -1; // No requested location for change
223-    std::string fail_reason;
224+    bilingual_str fail_reason;
225     if (!wallet.CreateTransaction(*locked_chain, recipients, tx_new, fee_ret, change_pos_in_out, fail_reason, new_coin_control, false)) {
226-        errors.push_back("Unable to create transaction: " + fail_reason);
227+        errors.push_back(_("Unable to create transaction.") + fail_reason);
    hebasto commented at 11:43 am on April 21, 2020:
    fa5a95c678ba2a4ebdbaad084a9fe18ba5b64629 Hereinafter, it should looks like https://github.com/bitcoin/bitcoin/blob/fa5a95c678ba2a4ebdbaad084a9fe18ba5b64629/src/wallet/wallet.cpp#L199
    MarcoFalke commented at 11:58 am on April 21, 2020:
    Thanks, good catch.
  23. MarcoFalke force-pushed on Apr 21, 2020
  24. hebasto commented at 12:19 pm on April 21, 2020: member

    On Ubuntu 18.04.4 having another kind of exception:

     0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py
 12020-04-21T12:15:56.635000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_r5a4hpii
 22020-04-21T12:16:16.255000Z TestFramework (INFO): Mining blocks...
 32020-04-21T12:16:17.987000Z TestFramework (INFO): test gettxout
 42020-04-21T12:16:21.593000Z TestFramework (INFO): test gettxout (second part)
 5--- Logging error ---
 6Traceback (most recent call last):
 7  File "/home/hebasto/bitcoin/test/functional/test_framework/test_framework.py", line 112, in main
 8    self.run_test()
 9  File "./test/functional/wallet_basic.py", line 395, in run_test
10    test_address(self.nodes[0], addr, labels=[label])
11  File "/home/hebasto/bitcoin/test/functional/test_framework/wallet_util.py", line 99, in test_address
12    raise AssertionError("key {} value {} did not match expected value {}".format(key, addr_info[key], value))
13AssertionError: key labels value [{'name': '\u0440\u044b\u0431\u0430', 'purpose': 'receive'}] did not match expected value ['\u0440\u044b\u0431\u0430']
14
15During handling of the above exception, another exception occurred:
16
17Traceback (most recent call last):
18  File "/usr/lib/python3.6/logging/__init__.py", line 996, in emit
19    stream.write(msg)
20UnicodeEncodeError: 'ascii' codec can't encode characters in position 600-603: ordinal not in range(128)
21Call stack:
22  File "./test/functional/wallet_basic.py", line 536, in <module>
23    WalletTest().main()
24  File "/home/hebasto/bitcoin/test/functional/test_framework/test_framework.py", line 120, in main
25    self.log.exception("Assertion failed")
26Message: 'Assertion failed'
27Arguments: ()
282020-04-21T12:17:41.175000Z TestFramework (INFO): Stopping nodes
292020-04-21T12:17:48.063000Z TestFramework (WARNING): Not cleaning up dir /tmp/bitcoin_func_test_r5a4hpii
302020-04-21T12:17:48.063000Z TestFramework (ERROR): Test failed. Test logging available at /tmp/bitcoin_func_test_r5a4hpii/test_framework.log
312020-04-21T12:17:48.065000Z TestFramework (ERROR): Hint: Call /home/hebasto/bitcoin/test/functional/combine_logs.py '/tmp/bitcoin_func_test_r5a4hpii' to consolidate all logs
  25. MarcoFalke commented at 12:38 pm on April 21, 2020: member
    Did you set PYTHONUTF8=1?
  26. hebasto commented at 12:46 pm on April 21, 2020: member

    Did you set PYTHONUTF8=1?

    0$ PYTHONUTF8=1 XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py

    didn’t help ((

  27. hebasto commented at 12:52 pm on April 21, 2020: member

    Can be tested with:

    0XDG_SESSION_TYPE=""  QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=bitcoin-qt ./test/functional/wallet_basic.py

    The gui popups should be translated, but the json rpc errors should not.

    On Ubuntu 18.04.4, before test crashing I can see the GUI popups: Screenshot from 2020-04-21 15-49-03 Screenshot from 2020-04-21 15-49-20

    Should they be in German, no?

  28. MarcoFalke commented at 1:06 pm on April 21, 2020: member

    Screenshot from 2020-04-21 09-03-06

    Screenshot from 2020-04-21 09-03-09

    Anyway, if this is not a regression introduced in this pull, it should be reported as a new issue.

  29. hebasto commented at 1:35 pm on April 21, 2020: member
    @MarcoFalke What system are you testing on?
  30. MarcoFalke commented at 1:39 pm on April 21, 2020: member
    Fedora 32
  31. hebasto approved
  32. hebasto commented at 3:45 pm on April 21, 2020: member

    ACK fa8068acc34f60536f6b55e7326df807f2059a53, tested manually and with scripts:

    • on Fedora 31 (using LC_ALL=de_DE.UTF-8):
    0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py
    • on Ubuntu 18.04.4 (using LANGUAGE=de):
    0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LANGUAGE=de BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py

    On master (b470c758470fd97ccb42d0348a32942afa2a3ec8) scripts fail with an error:

    0AssertionError: Expected substring not found in error message:
1substring: 'Insufficient funds'
2error message: 'Ungenügendes Guthaben'.
  33. hebasto approved
  34. hebasto commented at 3:55 pm on April 21, 2020: member

    ACK fa8068acc34f60536f6b55e7326df807f2059a53, tested manually and with scripts:

    • on Fedora 31 (using LC_ALL=de_DE.UTF-8):
    0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py
    • on Ubuntu 18.04.4 (using LANGUAGE=de):
    0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LANGUAGE=de BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py

    On master (b470c758470fd97ccb42d0348a32942afa2a3ec8) scripts fail with an error:

    0AssertionError: Expected substring not found in error message:
1substring: 'Insufficient funds'
2error message: 'Ungenügendes Guthaben'.
  35. hebasto approved
  36. hebasto commented at 4:15 pm on April 21, 2020: member

    ACK fa8068acc34f60536f6b55e7326df807f2059a53, tested manually and with scripts:

    • on Fedora 31 (using LC_ALL=de_DE.UTF-8):
    0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py
    • on Ubuntu 18.04.4 (using LANGUAGE=de):
    0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LANGUAGE=de BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py

    On master (b470c758470fd97ccb42d0348a32942afa2a3ec8) scripts fail with an error:

    0AssertionError: Expected substring not found in error message:
1substring: 'Insufficient funds'
2error message: 'Ungenügendes Guthaben'.
  37. hebasto approved
  38. hebasto commented at 4:31 pm on April 21, 2020: member

    ACK fa8068acc34f60536f6b55e7326df807f2059a53, tested manually and with scripts:

    • on Fedora 31 (using LC_ALL=de_DE.UTF-8):
    0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LC_ALL=de_DE.UTF-8 BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py
    • on Ubuntu 18.04.4 (using LANGUAGE=de):
    0$ XDG_SESSION_TYPE="" QT_QPA_PLATFORM=minimal LANGUAGE=de BITCOIND=/home/hebasto/bitcoin/src/qt/bitcoin-qt ./test/functional/wallet_basic.py

    On master (b470c758470fd97ccb42d0348a32942afa2a3ec8) scripts fail with an error:

    0AssertionError: Expected substring not found in error message:
1substring: 'Insufficient funds'
2error message: 'Ungenügendes Guthaben'.
  39. hebasto commented at 4:35 pm on April 21, 2020: member
    @MarcoFalke due to the recent GH glitch my single comment becomes multiple clones; please remove redundant ones.
  40. MarcoFalke force-pushed on Apr 21, 2020
  41. MarcoFalke commented at 6:53 pm on April 21, 2020: member
    Sorry, had to force push the last commit again to update the tests
  42. MarcoFalke force-pushed on Apr 21, 2020
  43. hebasto approved
  44. hebasto commented at 9:49 pm on April 21, 2020: member
    ACK fafca4ad4957defd2aef3f2af07fd3926adbadd8, all unit and functional tests passed locally.
  45. laanwj commented at 11:31 am on April 22, 2020: member

    Concept ACK, however this adds a fair number of translation messages that shouldn’t be there IMO because translators will have difficulty with them due to technicality, ambiguous wording, or spilling internal technical details:

    0+ QT_TRANSLATE_NOOP("bitcoin-core", ""
1+"Transaction needs a change address, but we can't generate it. Please call "
2+"keypoolrefill first."),
3+QT_TRANSLATE_NOOP("bitcoin-core", "Error: Keypool ran out, please call keypoolrefill first"),
4+QT_TRANSLATE_NOOP("bitcoin-core", "Invalid or non-wallet transaction id"),
5+QT_TRANSLATE_NOOP("bitcoin-core", "Transaction has been mined, or is conflicted with a mined transaction"),
6+QT_TRANSLATE_NOOP("bitcoin-core", "Transaction has descendants in the mempool"),
7+QT_TRANSLATE_NOOP("bitcoin-core", "Transaction has descendants in the wallet"),

    I think if any of these appear in the GUI they’re not helpful in the first place? Do users manually do keypoolrefill for example? mempool is not a GUI concept at all, it isn’t reflected anywhere. What do you have to do if “Invalid or non-wallet transaction id”? (can this even happen through the GUI?)

  46. MarcoFalke force-pushed on Apr 22, 2020
  47. luke-jr commented at 9:10 pm on April 24, 2020: member
    Concept ACK
  48. promag commented at 11:48 pm on April 26, 2020: member
    Concept ACK.
  49. DrahtBot added the label Needs rebase on Apr 27, 2020
  50. MarcoFalke force-pushed on Apr 27, 2020
  51. DrahtBot removed the label Needs rebase on Apr 27, 2020
  52. MarcoFalke commented at 1:10 pm on April 28, 2020: member
    Ok, I marked everything that wasn’t translated before as Untranslated(). Should be easier to review now.
  53. MarcoFalke force-pushed on Apr 29, 2020
  54. laanwj added this to the "Blockers" column in a project
  55. DrahtBot added the label Needs rebase on May 1, 2020
  56. wallet: Avoid translating RPC errors when loading wallets 
    Common errors and warnings should be translated when displayed in the
GUI, but not translated when displayed elsewhere. The wallet method
CreateWalletFromFile does not know its caller, so this commit changes it
to return a bilingual_str to the caller.
    fae51a5c6f
  57. wallet: Avoid translating RPC errors when creating txs 
    Also, mark feebumper bilingual_str as Untranslated

They are technical and have previously not been translated either.
It is questionable whether they can even appear in the GUI.
    fae7776690
  58. wallet: Report full error message in wallettool fa59cc1c97
  59. wallet: Remove trailing whitespace from potential translation strings 
    If the potential translation strings are translated in the future,
trailing whitespace is going to make translation effort harder.
    fa2cce4391
  60. MarcoFalke force-pushed on May 1, 2020
  61. DrahtBot removed the label Needs rebase on May 1, 2020
  62. MarcoFalke commented at 1:46 pm on May 1, 2020: member
    Rebased
  63. hebasto approved
  64. hebasto commented at 0:30 am on May 2, 2020: member

    ACK fa2cce4391b0b1bda325f695bb45f7b565c8e8ea

    Ok, I marked everything that wasn’t translated before as Untranslated().

    Verified by code reviewing that no new translations added.

  65. MarcoFalke commented at 0:32 am on May 2, 2020: member
    Can also be verified with (cd src && make translate) && git diff before and after (diff of diff should be empty). :)
  66. laanwj commented at 2:29 pm on May 4, 2020: member
    ACK fa2cce4391b0b1bda325f695bb45f7b565c8e8ea, checked that no new translation messages are added compared to master.
  67. laanwj merged this on May 4, 2020
  68. laanwj closed this on May 4, 2020
  69. MarcoFalke deleted the branch on May 4, 2020
  70. in src/util/translation.h:33 in fa2cce4391 
    28+/** Mark a bilingual_str as untranslated */
29+inline static bilingual_str Untranslated(std::string original) { return {original, original}; }
30+/** Unary operator to return the original */
31+inline static std::string OpOriginal(const bilingual_str& b) { return b.original; }
32+/** Unary operator to return the translation */
33+inline static std::string OpTranslated(const bilingual_str& b) { return b.translated; }
    hebasto commented at 4:45 pm on May 4, 2020:
    @MarcoFalke What is the rationale to declare these functions with static?
    MarcoFalke commented at 5:20 pm on May 4, 2020:
    I think this is a leftover. They only need to be inline to avoid linker errors. static might have no effect.
    MarcoFalke commented at 5:22 pm on May 4, 2020:
    Feel free to remove in your pull
  71. sidhujag referenced this in commit a2b5fc738d on May 4, 2020
  72. laanwj removed this from the "Blockers" column in a project
  73. jasonbcox referenced this in commit cfbeff3426 on Oct 1, 2020
  74. jasonbcox referenced this in commit fd902a90a6 on Oct 1, 2020
  75. jasonbcox referenced this in commit f5b557323f on Oct 1, 2020
  76. jasonbcox referenced this in commit e3f1ac00d4 on Oct 1, 2020
  77. DrahtBot locked this on Feb 15, 2022


MarcoFalke hebasto DrahtBot laanwj luke-jr promag

Labels
Wallet

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me