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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For detailed information about the code coverage, see the test coverage report.
<!--021abf342d371248e50ceaed478a90ca-->
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.
<!--174a7506f384e20aa4161008e828411d-->
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.
Big Concept ACK. 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.
Yeah, but "error" is still applicable within the addrdb functions, as they can not continue and must return. So I think, ideally they'd return 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;
nit - Noticed this when reviewing the changes. Since we're already changing the file - should Line 357/358 be a LogError not a LogPrintf?
If there are changes to other log lines, I'd say it would be best to do them in a separate pull request. The goal here is to be as close to a refactor as possible.
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]));
nit - Also noticed this when reviewing the changes. Since we're already changing the file - should Line 413/422 be a LogError not a LogPrintf?
Why? This line is a failure to connect to a peer and the above are proxy errors. So keeping this as-is makes most sense. In any case, if there is a reason to change something unrelated in other lines, a separate pull request would be ideal.
utACK fa1d4f07c36dda3c72fb328918bddd7de062ef96.
Tested ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96.
Options used to compile and link:
external signer = yes
multiprocess = no
with libs = yes
with wallet = yes
with sqlite = yes
with bdb = yes
with gui / qt = yes
with qr = yes
with zmq = yes
with test = yes
with fuzz binary = yes
with bench = yes
with upnp = no
with natpmp = no
use asm = yes
USDT tracing = no
sanitizers =
debug enabled = no
gprof enabled = yes
werror = yes
Passed test/functional/test_runner.py --extended. Please let me know if any of these skipped tests are relevant.
feature_bind_port_discover.py | â—‹ Skipped | 0 s
feature_bind_port_externalip.py | â—‹ Skipped | 1 s
feature_unsupported_utxo_db.py | â—‹ Skipped | 0 s
interface_usdt_coinselection.py | â—‹ Skipped | 0 s
interface_usdt_mempool.py | â—‹ Skipped | 0 s
interface_usdt_net.py | â—‹ Skipped | 1 s
interface_usdt_utxocache.py | â—‹ Skipped | 0 s
interface_usdt_validation.py | â—‹ Skipped | 1 s
mempool_compatibility.py | â—‹ Skipped | 0 s
wallet_backwards_compatibility.py --descriptors | â—‹ Skipped | 1 s
wallet_backwards_compatibility.py --legacy-wallet | â—‹ Skipped | 0 s
wallet_inactive_hdchains.py --legacy-wallet | â—‹ Skipped | 0 s
wallet_upgradewallet.py --legacy-wallet | â—‹ Skipped | 0 s
ALL | ✓ Passed | 6598 s (accumulated)
ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96
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.
Code review ACK fa1d4f07c36dda3c72fb328918bddd7de062ef96
I think this is RFM?
It conflicts with something that will be merged after branch-off.
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.
re-ACK fa391513949b7a3b56321436e2015c7e9e6dac2b, no changes since 4a903741b0
Code review ACK fa391513949b7a3b56321436e2015c7e9e6dac2b. Just rebase since last review
Milestone
28.0