error(...) has many issues:
return error(...), implying that it has a “fancy” type, creating confusion with util::Result/Error-logsourcelocations does not work with it, because it will pretend the error happened inside of logging.hERROR: , as opposed to [error], like for other errors logged with LogError.Fix all issues by removing it.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | fjahr, stickies-v, ryanofsky |
| Concept ACK | jamesob |
| Stale ACK | epiccurious, Empact |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, this pull request conflicts with the following ones:
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.
error() confuses me basically every time I run into it.
60@@ -60,7 +61,8 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
61 if (fileout.IsNull()) {
62 fileout.fclose();
63 remove(pathTmp);
64- return error("%s: Failed to open file %s", __func__, fs::PathToString(pathTmp));
65+ LogError("%s: Failed to open file %s\n", __func__, fs::PathToString(pathTmp));
nit: it seems the return value from SerializeFileDB is never used to actually shut down the node. In the case of failing to open file, I think LogError is appropriate because that probably should lead to a shutdown. In the case of “Failed to flush file”, I think a LogWarning could be more appropriate.
I think it’s best to leave the PR as is to make progress, bikeshedding over warning vs error categories doesn’t have a huge amount of benefit here. Just leaving this comment for visibility.
Result<void>, and the caller could downgrade the error to a warning and log it? Happy to review such a change, if someone creates it.
Approach ACK fa7658b8d190580eab385691a5e571035e399129
I too have been confused by return error many a times.
Concept ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96.
Fix all issues by removing it.
Any conceivable downsides to removing it?
Any conceivable downsides to removing it?
I can’t see one, apart from the change consuming time to review it.
358@@ -358,14 +359,17 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
359 return false;
LogError not a LogPrintf?
419+ LogError("Proxy failed to accept request\n");
420+ return false;
421 }
422 if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) {
423 // Failures to connect to a peer that are not proxy errors
424 LogPrintf("Socks5() connect to %s:%d failed: %s\n", strDest, port, Socks5ErrorString(pchRet2[1]));
LogError not a LogPrintf?
Tested ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96.
0Options used to compile and link:
1 external signer = yes
2 multiprocess = no
3 with libs = yes
4 with wallet = yes
5 with sqlite = yes
6 with bdb = yes
7 with gui / qt = yes
8 with qr = yes
9 with zmq = yes
10 with test = yes
11 with fuzz binary = yes
12 with bench = yes
13 with upnp = no
14 with natpmp = no
15 use asm = yes
16 USDT tracing = no
17 sanitizers =
18 debug enabled = no
19 gprof enabled = yes
20 werror = yes
Passed test/functional/test_runner.py --extended. Please let me know if any of these skipped tests are relevant.
0feature_bind_port_discover.py | ○ Skipped | 0 s
1feature_bind_port_externalip.py | ○ Skipped | 1 s
2feature_unsupported_utxo_db.py | ○ Skipped | 0 s
3interface_usdt_coinselection.py | ○ Skipped | 0 s
4interface_usdt_mempool.py | ○ Skipped | 0 s
5interface_usdt_net.py | ○ Skipped | 1 s
6interface_usdt_utxocache.py | ○ Skipped | 0 s
7interface_usdt_validation.py | ○ Skipped | 1 s
8mempool_compatibility.py | ○ Skipped | 0 s
9wallet_backwards_compatibility.py --descriptors | ○ Skipped | 1 s
10wallet_backwards_compatibility.py --legacy-wallet | ○ Skipped | 0 s
11wallet_inactive_hdchains.py --legacy-wallet | ○ Skipped | 0 s
12wallet_upgradewallet.py --legacy-wallet | ○ Skipped | 0 s
13
14ALL | ✓ Passed | 6598 s (accumulated)
Code review ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96
This is a nice improvement, and it actually doesn’t change many lines of code, so I don’t think we should be worried about conflicts if we want to make more improvements later, like changing log levels, or adding categories, or switching to util::Result.
One thing I think we probably should revisit after this is the interaction with -logsourcelocations, since it seems like a lot (maybe most?) of the new LogError calls are manually adding “%s: “, __func__ prefixes to the log, which shows the function name in a different format than -logsourcelocations, and also includes the function name twice if -logsourcelocations is enabled. A way to improve this might be to add an option to LogError to make it log the source location even if -logsourcelocations is not turned on. Or maybe it would be better if the error messages were more descriptive and didn’t rely on the function names to provide context.
utACK fa1d4f07c3
There is still a reference to error in the comments in logging.h: https://github.com/fjahr/bitcoin/commit/6457d77e39a8c07fdef798f3bf115fcbb30545c4 But removing it can be kept for a follow-up.
This is required for the next commit to be correct.
This is needed for the next commit.
-BEGIN VERIFY SCRIPT-
# Separate sed invocations to replace one-line, and two-line error(...) calls
sed -i --regexp-extended 's!( +)return (error\(.*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
sed -i --null-data --regexp-extended 's!( +)return (error\([^\n]*\n[^\n]*\);)!\1\2\n\1return false;!g' $( git grep -l 'return error(' )
-END VERIFY SCRIPT-This is needed for the next commit to compile.
This fixes the log output when -logsourcelocations is used.
Also, instead of 'ERROR:', the log will now say '[error]', like other
errors logged with LogError.
-BEGIN VERIFY SCRIPT-
sed -i --regexp-extended 's! error\("([^"]+)"! LogError("\1\\n"!g' $( git grep -l ' error(' ./src/ )
-END VERIFY SCRIPT-rebased.
Should be trivial to re-ACK, as no lines touched by this are affected (only one tangent line)
re-utACK fa391513949b7a3b56321436e2015c7e9e6dac2b
Based on git range-diff master fa1d4f07c36dda3c72fb328918bddd7de062ef96 fa391513949b7a3b56321436e2015c7e9e6dac2b.
Labels
Utils/log/libs
Milestone
28.0