log: Nuke error(...) #29236

maflcko · 2024-01-11T18:29:58Z

error(...) has many issues:

It is often used in the context of 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.h
The log line contains ERROR: , as opposed to [error], like for other errors logged with LogError.

Fix all issues by removing it.

DrahtBot · 2024-01-11T18:30:01Z

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29538 (refactor: Improve naming of CBlock::GetHash() -> GetHeaderHash() by fjahr)
#29158 (PoC: fuzz chainstate and block managers by darosior)
#28955 (index: block filters sync, reduce disk read operations by caching last header by furszy)
#28945 (sync: improve CCoinsViewCache ReallocateCache by martinus)
#27006 (reduce cs_main scope, guard block index 'nFile' under a local mutex by furszy)

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.

jamesob · 2024-01-11T20:05:57Z

Big Concept ACK. error() confuses me basically every time I run into it.

stickies-v

Approach ACK fa7658b

I too have been confused by return error many a times.

stickies-v · 2024-01-16T13:13:21Z

src/addrdb.cpp

@@ -60,7 +61,8 @@ bool SerializeFileDB(const std::string& prefix, const fs::path& path, const Data
    if (fileout.IsNull()) {
        fileout.fclose();
        remove(pathTmp);
-        return error("%s: Failed to open file %s", __func__, fs::PathToString(pathTmp));
+        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.

epiccurious · 2024-02-11T03:18:56Z

Concept ACK fa1d4f0.

Fix all issues by removing it.

Any conceivable downsides to removing it?

maflcko · 2024-02-11T12:08:47Z

Any conceivable downsides to removing it?

I can't see one, apart from the change consuming time to review it.

epiccurious

utACK fa1d4f0.

epiccurious · 2024-02-11T16:07:07Z

src/netbase.cpp

@@ -358,14 +359,17 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
            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.

epiccurious · 2024-02-11T16:22:48Z

src/netbase.cpp

            }
        }
        if (pchRet2[0] != SOCKSVersion::SOCKS5) {
-            return error("Proxy failed to accept request");
+            LogError("Proxy failed to accept request\n");
+            return false;
        }
        if (pchRet2[1] != SOCKS5Reply::SUCCEEDED) {
            // Failures to connect to a peer that are not proxy errors
            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.

epiccurious · 2024-02-11T22:16:21Z

Tested ACK fa1d4f0.

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)

stickies-v

ACK fa1d4f0

ryanofsky

Code review ACK fa1d4f0

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.

fjahr · 2024-02-18T17:48:15Z

utACK fa1d4f0

There is still a reference to error in the comments in logging.h: fjahr@6457d77 But removing it can be kept for a follow-up.

Empact

Code review ACK fa1d4f0

stickies-v · 2024-03-01T14:20:54Z

I think this is RFM?

fanquake · 2024-03-01T14:48:44Z

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-

maflcko · 2024-03-11T13:14:45Z

rebased.

Should be trivial to re-ACK, as no lines touched by this are affected (only one tangent line)

fjahr · 2024-03-11T13:35:54Z

re-utACK fa39151

Based on git range-diff master fa1d4f07c36dda3c72fb328918bddd7de062ef96 fa391513949b7a3b56321436e2015c7e9e6dac2b.

stickies-v · 2024-03-11T13:47:47Z

re-ACK fa39151, no changes since 4a90374

ryanofsky

Code review ACK fa39151. Just rebase since last review

d0e6564 log: Remove error() reference (Fabian Jahr) Pull request description: Mini-followup to #29236 that was just merged. Removes a reference to `error()` that was missed in a comment. ACKs for top commit: ryanofsky: Code review ACK d0e6564. Just dropped LogPrintf reference since last review stickies-v: ACK d0e6564 Empact: ACK d0e6564 Tree-SHA512: 8abe4895951013c2ceca9a57743aacabaf8af831d07eee9ae8372c121c16e88b7226f0e537200c3464792e19ac7e03b57ba0be31f43add8802753972b0aefc48

DrahtBot added the Utils/log/libs label Jan 11, 2024

DrahtBot added the CI failed label Jan 11, 2024

maflcko force-pushed the 2401-log-error- branch from fa3b64f to fa6d7ed Compare January 12, 2024 07:46

DrahtBot removed the CI failed label Jan 12, 2024

DrahtBot mentioned this pull request Jan 12, 2024

reduce cs_main scope, guard block index 'nFile' under a local mutex #27006

Draft

DrahtBot added the CI failed label Jan 15, 2024

maflcko force-pushed the 2401-log-error- branch from fa6d7ed to fa7658b Compare January 15, 2024 10:32

DrahtBot removed the CI failed label Jan 15, 2024

stickies-v reviewed Jan 16, 2024

View reviewed changes

DrahtBot added the Needs rebase label Jan 31, 2024

maflcko force-pushed the 2401-log-error- branch from fa7658b to fa1d4f0 Compare February 1, 2024 08:37

DrahtBot removed the Needs rebase label Feb 1, 2024

epiccurious approved these changes Feb 11, 2024

View reviewed changes

DrahtBot requested review from stickies-v and jamesob February 11, 2024 16:23

stickies-v approved these changes Feb 15, 2024

View reviewed changes

ryanofsky approved these changes Feb 15, 2024

View reviewed changes

maflcko added this to the 28.0 milestone Feb 22, 2024

maflcko mentioned this pull request Feb 27, 2024

build: Enable -Wshadow #29485

Closed

Empact approved these changes Feb 27, 2024

View reviewed changes

DrahtBot mentioned this pull request Feb 28, 2024

Improve new LogDebug/Trace/Info/Warning/Error Macros #29256

Open

DrahtBot mentioned this pull request Mar 3, 2024

refactor: Improve naming of CBlock::GetHash() -> GetHeaderHash() #29538

Closed

DrahtBot added the Needs rebase label Mar 9, 2024

MarcoFalke added 5 commits March 11, 2024 13:49

refactor: Add missing {} around error() calls

fa9a5e8

This is required for the next commit to be correct.

refactor: Make error() return type void

fa808fb

This is needed for the next commit to compile.

refactor: Remove unused error()

fa39151

maflcko force-pushed the 2401-log-error- branch from fa1d4f0 to fa39151 Compare March 11, 2024 12:50

DrahtBot requested review from epiccurious, Empact, stickies-v and ryanofsky March 11, 2024 13:35

DrahtBot requested review from epiccurious and removed request for stickies-v and epiccurious March 11, 2024 13:47

DrahtBot removed the Needs rebase label Mar 11, 2024

ryanofsky reviewed Mar 11, 2024

View reviewed changes

fanquake merged commit 31be1a4 into bitcoin:master Mar 12, 2024
15 of 16 checks passed

maflcko deleted the 2401-log-error- branch March 12, 2024 10:38

fjahr mentioned this pull request Mar 12, 2024

log: Remove error() reference #29633

Merged

achow101 mentioned this pull request Mar 12, 2024

rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block #26415

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

log: Nuke error(...) #29236

log: Nuke error(...) #29236

maflcko commented Jan 11, 2024 •

edited

DrahtBot commented Jan 11, 2024 •

edited

jamesob commented Jan 11, 2024

stickies-v left a comment

stickies-v Jan 16, 2024

maflcko Feb 13, 2024

epiccurious commented Feb 11, 2024

maflcko commented Feb 11, 2024

epiccurious left a comment

epiccurious Feb 11, 2024

maflcko Feb 13, 2024

epiccurious Feb 11, 2024

maflcko Feb 13, 2024

epiccurious commented Feb 11, 2024 •

edited

stickies-v left a comment

ryanofsky left a comment •

edited

fjahr commented Feb 18, 2024

Empact left a comment

stickies-v commented Mar 1, 2024

fanquake commented Mar 1, 2024

maflcko commented Mar 11, 2024

fjahr commented Mar 11, 2024

stickies-v commented Mar 11, 2024

ryanofsky left a comment

		@@ -358,14 +359,17 @@ bool Socks5(const std::string& strDest, uint16_t port, const ProxyCredentials* a
		return false;

log: Nuke error(...) #29236

log: Nuke error(...) #29236

Conversation

maflcko commented Jan 11, 2024 • edited

DrahtBot commented Jan 11, 2024 • edited

Code Coverage

Reviews

Conflicts

jamesob commented Jan 11, 2024

stickies-v left a comment

Choose a reason for hiding this comment

stickies-v Jan 16, 2024

Choose a reason for hiding this comment

maflcko Feb 13, 2024

Choose a reason for hiding this comment

epiccurious commented Feb 11, 2024

maflcko commented Feb 11, 2024

epiccurious left a comment

Choose a reason for hiding this comment

epiccurious Feb 11, 2024

Choose a reason for hiding this comment

maflcko Feb 13, 2024

Choose a reason for hiding this comment

epiccurious Feb 11, 2024

Choose a reason for hiding this comment

maflcko Feb 13, 2024

Choose a reason for hiding this comment

epiccurious commented Feb 11, 2024 • edited

stickies-v left a comment

Choose a reason for hiding this comment

ryanofsky left a comment • edited

Choose a reason for hiding this comment

fjahr commented Feb 18, 2024

Empact left a comment

Choose a reason for hiding this comment

stickies-v commented Mar 1, 2024

fanquake commented Mar 1, 2024

maflcko commented Mar 11, 2024

fjahr commented Mar 11, 2024

stickies-v commented Mar 11, 2024

ryanofsky left a comment

Choose a reason for hiding this comment

maflcko commented Jan 11, 2024 •

edited

DrahtBot commented Jan 11, 2024 •

edited

epiccurious commented Feb 11, 2024 •

edited

ryanofsky left a comment •

edited