refactor: Error message bilingual_str consistency #19176

pull laanwj wants to merge 3 commits into bitcoin:master from laanwj:2020_06_bilingual_str changing 13 files +57 −47
  1. laanwj commented at 12:32 pm on June 5, 2020: member

    A straightforward and hopefully uncontroversial refactor to improve consistency.

    • Move the decision whether to translate an individual error message to where it is defined. This simplifies call sites: no more InitError(Untranslated(SomeFunction(...))).

    • Make all functions in util/error.h consistently return a bilingual_str. We’ve decided to use this as error message type so let’s roll with it.

    This has no functional changes: no messages are changed, no new translation messages are defined.

    Also make a function static that can be static.

  2. laanwj added the label Refactoring on Jun 5, 2020
  3. practicalswift commented at 12:36 pm on June 5, 2020: contributor
    Concept ACK
  4. DrahtBot commented at 1:24 pm on June 5, 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:

    • #19192 (doc: Extract net permissions doc by MarcoFalke)
    • #19191 (net: Extract download permission from noban by MarcoFalke)
    • #19183 ([WIP DONOTMERGE] Replace boost with C++17 by MarcoFalke)
    • #19160 (multiprocess: Add basic spawn and IPC support by ryanofsky)
    • #19101 (refactor: remove ::vpwallets and related global variables by ryanofsky)
    • #19099 (refactor: Move wallet methods out of chain.h and node.h by ryanofsky)
    • #19098 (test: Remove duplicate NodeContext hacks by ryanofsky)
    • #19031 (Implement ADDRv2 support (part of BIP155) by vasild)
    • #18972 (WIP: net: Add blockfilters white{bind,list} permission flag by luke-jr)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    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.

  5. hebasto commented at 1:28 pm on June 5, 2020: member
    Concept ACK.
  6. jonatack commented at 1:31 pm on June 5, 2020: member
    ACK f40dddfde231550e76043011d4211ca2fb17143d
  7. in src/net_permissions.cpp:12 in f40dddfde2 outdated 
     8@@ -9,7 +9,7 @@
 9 #include <util/translation.h>
10 
11 // The parse the following format "perm1,perm2@xxxxxx"
12-bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, std::string& error)
13+static bool TryParsePermissionFlags(const std::string str, NetPermissionFlags& output, size_t& readen, bilingual_str& error)
  8. hebasto approved
  9. hebasto commented at 1:56 pm on June 5, 2020: member
    ACK f40dddfde231550e76043011d4211ca2fb17143d, tested on Linux Mint 19.3 (x86_64).
  10. in src/util/error.h:35 in 2b9114768b outdated 
    31@@ -32,9 +32,9 @@ enum class TransactionError {
32     MAX_FEE_EXCEEDED,
33 };
34 
35-std::string TransactionErrorString(const TransactionError error);
36+bilingual_str TransactionErrorString(const TransactionError error);
    MarcoFalke commented at 2:07 pm on June 5, 2020:

    in commit 2b9114768bb842a82ef82f317d7f0408267e7dc5 :

    I can no longer compile the fuzz tests:

    0test/fuzz/kitchen_sink.cpp:24:11: error: calling 'TransactionErrorString' with incomplete return type 'bilingual_str'
1
2    (void)TransactionErrorString(transaction_error);

    It might be possible to fix this by including util/translation.h in the fuzz test that fails to compile

    jonatack commented at 2:25 pm on June 5, 2020:

    confirmed here as well

    0test/fuzz/kitchen_sink.cpp:24:11: error: calling 'TransactionErrorString' with incomplete return type 'bilingual_str'
1    (void)TransactionErrorString(transaction_error);
2          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
3./util/error.h:35:15: note: 'TransactionErrorString' declared here
4bilingual_str TransactionErrorString(const TransactionError error);
5              ^
6./util/error.h:20:8: note: forward declaration of 'bilingual_str'
7struct bilingual_str;
8       ^
91 error generated.

    and verified that your suggestion allows the test to build and run correctly.

  11. MarcoFalke approved
  12. MarcoFalke commented at 2:08 pm on June 5, 2020: member
    ACK. This should also fix the failing tests when running the net permission tests in the gui
  13. jonatack commented at 3:02 pm on June 5, 2020: member

    Probably unrelated or a bad build, but with two different debug builds on this branch and not on master, I’m seeing seeing this shortly after launching the GUI. Testing further with rebase on master.

    0Assertion failed: detected inconsistent lock order at sync.cpp:128, details in debug log.
1Aborted

     02020-06-05T14:38:04Z Bitcoin Core version v0.20.99.0-f40dddfde2-dirty (debug build)
 12020-06-05T14:38:04Z Qt 5.11.3 (dynamic), plugin=xcb (dynamic)
 22020-06-05T14:38:04Z System: PureOS, x86_64-little_endian-lp64
 32020-06-05T14:38:04Z Screen: HDMI-1 3840x1600, pixel ratio=1.0
 42020-06-05T14:38:04Z Screen: eDP-1 1920x1080, pixel ratio=1.0
 52020-06-05T14:38:04Z Assuming ancestors of block 0000000000000000000f2adce67e49b0b6bdeb9de8b7c3d7e93b21e7fc1e819d have valid signatures.
 62020-06-05T14:38:04Z Setting nMinimumChainWork=00000000000000000000000000000000000000000e1ab5ec9348e9f4b8eb8154
 72020-06-05T14:38:04Z Using the 'sse4(1way),sse41(4way),avx2(8way)' SHA256 implementation
 82020-06-05T14:38:04Z Using RdSeed as additional entropy source
 92020-06-05T14:38:04Z Using RdRand as an additional entropy source
102020-06-05T14:38:04Z Default data directory /home/jon/.bitcoin
112020-06-05T14:38:04Z Using data directory /home/jon/.bitcoin
122020-06-05T14:38:04Z Config file: /home/jon/.bitcoin/bitcoin.conf
132020-06-05T14:38:04Z Config file arg: addresstype="bech32"
142020-06-05T14:38:04Z Config file arg: avoidpartialspends="1"
152020-06-05T14:38:04Z Config file arg: blocksonly="0"
162020-06-05T14:38:04Z Config file arg: dbcache="10240"
172020-06-05T14:38:04Z Config file arg: logips="1"
182020-06-05T14:38:04Z Config file arg: maxconnections="512"
192020-06-05T14:38:04Z Config file arg: maxmempool="1000"
202020-06-05T14:38:04Z Config file arg: maxuploadtarget="0"
212020-06-05T14:38:04Z Config file arg: par="0"
222020-06-05T14:38:04Z Config file arg: persistmempool="1"
232020-06-05T14:38:04Z Config file arg: rpcpassword=****
242020-06-05T14:38:04Z Config file arg: rpcuser=****
252020-06-05T14:38:04Z Config file arg: server="1"
262020-06-05T14:38:04Z Config file arg: txconfirmtarget="10"
272020-06-05T14:38:04Z Config file arg: txindex="1"
282020-06-05T14:38:04Z Config file arg: walletbroadcast="1"
29
30...
31
322020-06-05T14:38:30Z opencon thread start
332020-06-05T14:38:30Z msghand thread start
342020-06-05T14:38:31Z GUI: Platform customization: "other"
352020-06-05T14:38:31Z New outbound peer connected: version: 70015, blocks=633204, peer=0, peeraddr=[2a01:4f8:1c17:7ac6::1]:8333 (full-relay)
362020-06-05T14:38:32Z New outbound peer connected: version: 70015, blocks=633204, peer=1, peeraddr=136.32.20.188:8333 (full-relay)
372020-06-05T14:38:32Z Imported mempool transactions from disk: 1447 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
382020-06-05T14:38:36Z UpdateTip: new best=0000000000000000000280a397cbe44cbfb9469f098922dbec3ebb7b01546968 height=633198 version=0x20000000 log2_work=92.005832 tx=536190097 date='2020-06-05T13:50:17Z' progress=0.999982 cache=2.0MiB(15058txo) warning='64 des 100 derniers blocs ont une version inattendue'
392020-06-05T14:38:36Z POTENTIAL DEADLOCK DETECTED
402020-06-05T14:38:36Z Previous lock order was:
412020-06-05T14:38:36Z  m_cs_chainstate validation.cpp:2868 (in thread msghand)
422020-06-05T14:38:36Z  (1) cs_main validation.cpp:2883 (in thread msghand)
432020-06-05T14:38:36Z  ::mempool.cs validation.cpp:2883 (in thread msghand)
442020-06-05T14:38:36Z  (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 (in thread msghand)
452020-06-05T14:38:36Z Current lock order is:
462020-06-05T14:38:36Z  (2) m_cached_tip_mutex qt/clientmodel.cpp:119 (in thread main)
472020-06-05T14:38:36Z  (1) ::cs_main interfaces/node.cpp:200 (in thread main)

     02020-06-05T14:39:32Z P2P peers available. Skipped DNS seeding.
 12020-06-05T14:39:32Z dnsseed thread exit
 22020-06-05T14:39:38Z New outbound peer connected: version: 70015, blocks=633204, peer=10, peeraddr=138.229.205.237:8333 (full-relay)
 32020-06-05T14:39:56Z UpdateTip: new best=0000000000000000000280a397cbe44cbfb9469f098922dbec3ebb7b01546968 height=633198 version=0x20000000 log2_work=92.005832 tx=536190097 date='2020-06-05T13:50:17Z' progress=0.999982 cache=2.1MiB(15165txo) warning='64 des 100 derniers blocs ont une version inattendue'
 42020-06-05T14:39:56Z POTENTIAL DEADLOCK DETECTED
 52020-06-05T14:39:56Z Previous lock order was:
 62020-06-05T14:39:56Z  m_cs_chainstate validation.cpp:2868 (in thread msghand)
 72020-06-05T14:39:56Z  (1) cs_main validation.cpp:2883 (in thread msghand)
 82020-06-05T14:39:56Z  ::mempool.cs validation.cpp:2883 (in thread msghand)
 92020-06-05T14:39:56Z  (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 (in thread msghand)
102020-06-05T14:39:56Z Current lock order is:
112020-06-05T14:39:56Z  (2) m_cached_tip_mutex qt/clientmodel.cpp:119 (in thread main)
122020-06-05T14:39:56Z  (1) ::cs_main interfaces/node.cpp:200 (in thread main)

     02020-06-05T14:41:51Z net thread start
 12020-06-05T14:41:51Z opencon thread start
 22020-06-05T14:41:51Z msghand thread start
 32020-06-05T14:41:51Z GUI: Platform customization: "other"
 42020-06-05T14:41:52Z New outbound peer connected: version: 70015, blocks=633204, peer=0, peeraddr=173.249.2.209:8333 (full-relay)
 52020-06-05T14:41:52Z Imported mempool transactions from disk: 1447 succeeded, 0 failed, 0 expired, 0 already there, 0 waiting for initial broadcast
 62020-06-05T14:41:58Z UpdateTip: new best=0000000000000000000280a397cbe44cbfb9469f098922dbec3ebb7b01546968 height=633198 version=0x20000000 log2_work=92.005832 tx=536190097 date='2020-06-05T13:50:17Z' progress=0.999981 cache=2.0MiB(15058txo) warning='64 des 100 derniers blocs ont une version inattendue'
 72020-06-05T14:41:58Z POTENTIAL DEADLOCK DETECTED
 82020-06-05T14:41:58Z Previous lock order was:
 92020-06-05T14:41:58Z  m_cs_chainstate validation.cpp:2868 (in thread msghand)
102020-06-05T14:41:58Z  (1) cs_main validation.cpp:2883 (in thread msghand)
112020-06-05T14:41:58Z  ::mempool.cs validation.cpp:2883 (in thread msghand)
122020-06-05T14:41:58Z  (2) clientmodel->m_cached_tip_mutex qt/clientmodel.cpp:255 (in thread msghand)
132020-06-05T14:41:58Z Current lock order is:
142020-06-05T14:41:58Z  (2) m_cached_tip_mutex qt/clientmodel.cpp:119 (in thread main)
152020-06-05T14:41:58Z  (1) ::cs_main interfaces/node.cpp:200 (in thread main)
  14. MarcoFalke commented at 3:03 pm on June 5, 2020: member
    @jonatack This issue has been fixed on master
  15. jonatack commented at 3:05 pm on June 5, 2020: member
    Thanks, suspected as much. I just verified with a build of this branch rebased on master that the issue is gone.
  16. DrahtBot added the label Needs rebase on Jun 8, 2020
  17. refactor: Error message bilingual_str consistency 
    - Move the decision whether to translate an error message to where it is
  defined. This simplifies call sites: no more `InitError(Untranslated(...))`.

- Make all functions in `util/error.h` consistently return a
  `bilingual_str`. We've decided to use this as error message type so
  let's roll with it.

This has no functional changes: no messages are changed, no new
translation messages are defined.
    77b79fa6ef
  18. refactor: Put`TryParsePermissionFlags` in anonymous namespace 
    It's only used inside `net_permissions.cpp`.
    425e7cb8cf
  19. refactor: Change Node::initError to take bilingual_str 
    Make it consistent with `Chain::initError`.
    6fe989054f
  20. laanwj force-pushed on Jun 9, 2020
  21. laanwj commented at 1:41 pm on June 9, 2020: member
    • Rebased
    • Use anonymous namespace instead of static
    • Fuzzing tests should compile again
  22. MarcoFalke commented at 3:01 pm on June 9, 2020: member

    Tested before:

    0LC_ALL=de_DE.UTF-8 QT_QPA_PLATFORM=xcb BITCOIND=bitcoin-qt ./test/functional/p2p_permissions.py --tracerpc
1...
2AssertionError: [node 1] Expected message "Invalid P2P permission" does not partially match stderr:
3"Error: Ungültige P2P Genehmigung: 'oopsie'"

    Tested after:

    0# same test
1...
22020-06-09T14:57:47.148000Z TestFramework (INFO): Tests successful

    ACK 6fe989054f0ad9308e8a25f7123d9e5dd67f1164 🔣

    Signature:

     0-----BEGIN PGP SIGNED MESSAGE-----
 1Hash: SHA512
 2
 3ACK 6fe989054f0ad9308e8a25f7123d9e5dd67f1164 🔣
 4-----BEGIN PGP SIGNATURE-----
 5
 6iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
 7pUigZgv/aQzZaHqcIzdNovyxkfR+qQOTJCXkEDUY+kITIOyKIwc970A+qvldJRhW
 87fUT7TUM4zl8GglIT9QVUPLTtLSWl75iNFphGkqBEQcXFcUApqrSP5iWITDzEqAr
 9WqdUkTxtCdyxdw8GpBW8cFrRPdvIE2HmLfHDMtdvjRa3pkajy6u15l9xOm6jvMeT
104icO1axpuwnCPfLMOckzTQVhxUtJ1QR4gsl49PjidRqK7BGx1iiEHSgSkfUIBTma
11hQrVrzPtEkHQhkNUADSzRLofKjkRH83QBEezulKOyaZUnNWQa7UFjxSTWo6dUmG1
12cdKFUJ9nFTcGi2K5PuBYx/El05MkNlJ5XsJ5yB1irukHjdZ/d8AkVoC5mB0i1bhn
13UGtM0KWvAIOZOiOmmBewgUlR6kbVIjwFhDOmUiJX/w9GIkheM1c8A90APOZVK0d5
14mOfCJwlQ5q8Q5JcRyibcqgpCfAkpdJmJJhB+xHET+w0SkzkoQ7r7bsXUVjj29GaF
15MhL92C/x
16=2qDJ
17-----END PGP SIGNATURE-----

    Timestamp of file with hash 71e8aa88cba09cc7b0e920f846122ece50689e066e9939b354e8102915106d61 -

  23. DrahtBot removed the label Needs rebase on Jun 9, 2020
  24. hebasto commented at 5:02 pm on June 9, 2020: member

    @MarcoFalke

    Tested before:

    0LC_ALL=de_DE.UTF-8 QT_QPA_PLATFORM=xcb BITCOIND=bitcoin-qt ./test/functional/p2p_permissions.py --tracerpc
1...
2AssertionError: [node 1] Expected message "Invalid P2P permission" does not partially match stderr:
3"Error: Ungültige P2P Genehmigung: 'oopsie'"

    Tested after:

    0# same test
1...
22020-06-09T14:57:47.148000Z TestFramework (INFO): Tests successful

    Trying to reproduce your tests, and got:

    0AssertionError: [node 1] Unable to connect to bitcoind after 60s

    What am I doing wrong?

  25. hebasto approved
  26. hebasto commented at 5:11 pm on June 9, 2020: member
    ACK 6fe989054f0ad9308e8a25f7123d9e5dd67f1164, tested on Linux Mint 19.3 (x86_64). Also verified that it compiles successfully with the --enable-fuzz option.
  27. MarcoFalke commented at 5:17 pm on June 9, 2020: member

    What am I doing wrong?

    You’ll need to manually close the GUI dialogs. The python test framework can’t do that for you (yet).

  28. MarcoFalke merged this on Jun 9, 2020
  29. MarcoFalke closed this on Jun 9, 2020
  30. fanquake referenced this in commit 948f1134bc on Jun 10, 2020
  31. sidhujag referenced this in commit d15420294f on Jun 10, 2020
  32. sidhujag referenced this in commit 58b0b9a648 on Jun 10, 2020
  33. fanquake referenced this in commit 20e9531379 on Jun 10, 2020
  34. sidhujag referenced this in commit 0582e63d8e on Jun 10, 2020
  35. stackman27 referenced this in commit 54704b6d05 on Jun 26, 2020
  36. stackman27 referenced this in commit f3896b13b7 on Jul 4, 2020
  37. Fabcien referenced this in commit 29d51c3416 on Dec 14, 2020
  38. DrahtBot locked this on Feb 15, 2022


laanwj practicalswift DrahtBot hebasto jonatack MarcoFalke

Labels
Refactoring

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-12-18 15:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me