log: Clarify log message when file does not exist #21222

MarcoFalke commented at 1:12 PM on February 18, 2021: member

Shorter and broader alternative to #21181

Rendered diff:

@@ -1,4 +1,4 @@
-Bitcoin Core version v21.99.0-db656db2ed5a (release build)
+Bitcoin Core version v21.99.0-faf48f20f196 (release build)
 Qt 5.15.2 (dynamic), plugin=wayland (dynamic)
 No static plugins.
 Style: adwaita / Adwaita::Style
@@ -24,8 +24,8 @@ scheduler thread start
 Using wallet directory /tmp/test_001/regtest/wallets
 init message: Verifying wallet(s)...
 init message: Loading banlist...
-ERROR: DeserializeFileDB: Failed to open file /tmp/test_001/regtest/banlist.dat
-Invalid or missing banlist.dat; recreating
+Missing or invalid file /tmp/test_001/regtest/banlist.dat
+Recreating banlist.dat
 SetNetworkActive: true
 Failed to read fee estimates from /tmp/test_001/regtest/fee_estimates.dat. Continue anyway.
 Using /16 prefix for IP bucketing
@@ -63,9 +63,9 @@ Bound to [::]:18444
 Bound to 0.0.0.0:18444
 Bound to 127.0.0.1:18445
 init message: Loading P2P addresses...
-ERROR: DeserializeFileDB: Failed to open file /tmp/test_001/regtest/peers.dat
-Invalid or missing peers.dat; recreating
-ERROR: DeserializeFileDB: Failed to open file /tmp/test_001/regtest/anchors.dat
+Missing or invalid file /tmp/test_001/regtest/peers.dat
+Recreating peers.dat
+Missing or invalid file /tmp/test_001/regtest/anchors.dat
 0 block-relay-only anchors will be tried for connections.
 init message: Starting network threads...
 net thread start

vasild approved

vasild commented at 1:23 PM on February 18, 2021: member

ACK fa5a8c30033e67d82494c6eb1113a6563dcf685e

laanwj added the label P2P on Feb 18, 2021

laanwj added the label Utils/log/libs on Feb 18, 2021

jnewbery commented at 1:54 PM on February 18, 2021: member

Is there a reason to not just change the return error(... for a LogPrintf(... and return false?

log: Clarify log message when file does not exist

Also, run clang-format on the function

faf48f20f1

MarcoFalke force-pushed on Feb 18, 2021

MarcoFalke commented at 2:14 PM on February 18, 2021: member

Done (and updated rendered diff in OP)

DrahtBot commented at 2:59 PM on February 18, 2021: 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:

#20966 (banman: save the banlist in a JSON format on disk by vasild)

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.

jnewbery commented at 3:03 PM on February 18, 2021: member

utACK faf48f20f196e418b2eea390a0140db3604cfa15

jonatack commented at 3:17 PM on February 18, 2021: member

I think this is a strictly less helpful user experience, but it's clearly more popular than my proposals so I re-closed.

MarcoFalke commented at 4:09 PM on February 18, 2021: member

I am happy to adjust any part that has issues. The only thing I care about is that we fix this issue once completely, so that it doesn't come up again in the future.

jonatack commented at 5:16 PM on February 18, 2021: member

#21181 addressed the reported issue. I was not aware of other issues filed that necessitated an expanded requirement and scope.

I closed #21181 because you are both the most active maintainer and the most active committer since years and it seems pointless to leave it open if you open a competing patch. Nevertheless, I'm surprised by the expressed all-that-matters imperative to widen the requirement. AFAIK we had one specific issue report. I believe a specific contained fix was a fine response to it.

amitiuttarwar commented at 10:45 PM on February 18, 2021: contributor

utACK faf48f20f1, 👍 for consistency. also checked where we create / load other .dat files, looks good to me.

MarcoFalke commented at 10:09 AM on February 19, 2021: member

AFAIK we had one specific issue report. I believe a specific contained fix was a fine response to it.

Correct, nothing was wrong with that. Though, I overheard several devs (over the years) complain about this particular ERROR that happens on a normal and expected condition. Moreover, three reviewers left a comment in the other PR that it should be fixed for all 3 instances of the same issue (not just one). There is already a huge code churn in the repo and tracing back when a change has been made originally becomes a time-consuming task. While git log -S can deal perfectly fine with move-only, it will abort on every other change. So I think we shouldn't be aiming to make a change, knowing that it will be reverted soon in favour of a broader change.

... the most active maintainer and the most active committer since years and it seems pointless to leave it open if you open a competing patch.

I think it would be very alarming if any patch (or review) is simply preferred based on the number of commits or comments a contributor has made. In the (far) past, horrible consensus failures have been introduced by (presumably) relying on that metric. My impression is that code review and quality assurance have improved significantly since then. I can point to many high-impact bugs introduced in the last three months by "top-10" or "top-20" contributors by commit count and all of them have been caught by code review. I think when evaluating changes to Bitcoin (Core) we should purely take into account how much sense they make and not the reputation of the person voicing them.

in src/addrdb.cpp:119 in faf48f20f1

 118 | +    if (filein.IsNull()) {
 119 | +        LogPrintf("Missing or invalid file %s\n", path.string());
 120 | +        return false;
 121 | +    }
 122 |      return DeserializeDB(filein, data);
 123 |  }

vasild commented at 10:56 AM on February 19, 2021:

I think the word "invalid" implies that something is wrong with the contents of the file, maybe could be misleading to use it for a file open failure.

What about distinguishing a missing file from another open error and staying silent on missing file (since that is normal) and printing a screaming error otherwise? Also print the actual error that has occurred - the user wants to see if it is something like "Disk IO error".

    if (!fs::exists(path)) {
        return false;
    }
    FILE* file = fsbridge::fopen(path, "rb");
    if (file == nullptr) {
        return error("%s: Failed to open file %s: %s", __func__, path.string(),
                     fsbridge::GetErrorReason()); // would require to make GetErrorReason() public
    }
    CAutoFile filein(file, SER_DISK, CLIENT_VERSION);
    return DeserializeDB(filein, data);

MarcoFalke commented at 1:38 PM on February 23, 2021:

Thanks, done in the last commit.

practicalswift commented at 12:42 PM on February 23, 2021: contributor

cr ACK faf48f20f196e418b2eea390a0140db3604cfa15

jnewbery commented at 2:11 PM on February 23, 2021: member

The scope of this PR seems to be creeping. It had three ACKs on the previous version, which simply removed the "ERROR" text from log message. The new version is changing what's exposed by the fs module, which seems unnecessary.

MarcoFalke force-pushed on Feb 23, 2021

MarcoFalke commented at 3:03 PM on February 23, 2021: member

Ok, reverted

MarcoFalke merged this on Feb 23, 2021

MarcoFalke closed this on Feb 23, 2021

MarcoFalke deleted the branch on Feb 23, 2021

sidhujag referenced this in commit c1934b507f on Feb 23, 2021

PastaPastaPasta referenced this in commit b88a0ecef8 on Sep 17, 2021

PastaPastaPasta referenced this in commit e61d959bfc on Sep 19, 2021

thelazier referenced this in commit 7c2a9c5bbc on Sep 25, 2021

gades referenced this in commit fe638cb884 on May 17, 2022

DrahtBot locked this on Aug 16, 2022