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

maflcko · 2021-02-18T13:12:40Z

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

ACK fa5a8c3

jnewbery · 2021-02-18T13:54:32Z

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

Also, run clang-format on the function

maflcko · 2021-02-18T14:14:49Z

Done (and updated rendered diff in OP)

DrahtBot · 2021-02-18T14:59:20Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

banman: save the banlist in a JSON format on disk #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 · 2021-02-18T15:03:21Z

utACK faf48f2

jonatack · 2021-02-18T15:17:10Z

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

maflcko · 2021-02-18T16:09:33Z

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 · 2021-02-18T17:16:08Z

#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 · 2021-02-18T22:45:15Z

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

maflcko · 2021-02-19T10:09:14Z

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.

src/addrdb.cpp

practicalswift · 2021-02-23T12:42:27Z

cr ACK faf48f2

jnewbery · 2021-02-23T14:11:42Z

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.

maflcko · 2021-02-23T15:03:28Z

Ok, reverted

faf48f2 log: Clarify log message when file does not exist (MarcoFalke) Pull request description: Shorter and broader alternative to bitcoin#21181 Rendered diff: ```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 ACKs for top commit: jnewbery: utACK faf48f2 amitiuttarwar: utACK faf48f2, 👍 for consistency. also checked where we create / load other `.dat` files, looks good to me. practicalswift: cr ACK faf48f2 Tree-SHA512: 697a728ef2b9f203363ac00b03eaf23ddf80bee043ecd3719265a0d884e8cfe88cd39afe946c86ab849edd1c836f05ec51125f052bdc14fe184b84447567756f

maflcko mentioned this pull request Feb 18, 2021

p2p: if no anchors.dat file, log a message instead of an error #21181

Closed

vasild approved these changes Feb 18, 2021

View reviewed changes

laanwj added P2P Utils/log/libs labels Feb 18, 2021

log: Clarify log message when file does not exist

faf48f2

Also, run clang-format on the function

maflcko force-pushed the 2102-logFile branch from fa5a8c3 to faf48f2 Compare February 18, 2021 14:09

DrahtBot mentioned this pull request Feb 18, 2021

banman: save the banlist in a JSON format on disk #20966

Merged

vasild reviewed Feb 19, 2021

View reviewed changes

src/addrdb.cpp Show resolved Hide resolved

maflcko force-pushed the 2102-logFile branch from faccdc2 to faf48f2 Compare February 23, 2021 14:59

maflcko merged commit 78effb3 into bitcoin:master Feb 23, 2021

maflcko deleted the 2102-logFile branch February 23, 2021 15:07

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

maflcko commented Feb 18, 2021 •

edited

Loading

Uh oh!

vasild left a comment

Uh oh!

jnewbery commented Feb 18, 2021

Uh oh!

maflcko commented Feb 18, 2021

Uh oh!

DrahtBot commented Feb 18, 2021

Uh oh!

jnewbery commented Feb 18, 2021

Uh oh!

jonatack commented Feb 18, 2021

Uh oh!

maflcko commented Feb 18, 2021

Uh oh!

jonatack commented Feb 18, 2021

Uh oh!

amitiuttarwar commented Feb 18, 2021

Uh oh!

maflcko commented Feb 19, 2021

Uh oh!

Uh oh!

practicalswift commented Feb 23, 2021

Uh oh!

jnewbery commented Feb 23, 2021

Uh oh!

maflcko commented Feb 23, 2021

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

maflcko commented Feb 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Feb 18, 2021

Uh oh!

maflcko commented Feb 18, 2021

Uh oh!

DrahtBot commented Feb 18, 2021

Conflicts

Uh oh!

jnewbery commented Feb 18, 2021

Uh oh!

jonatack commented Feb 18, 2021

Uh oh!

maflcko commented Feb 18, 2021

Uh oh!

jonatack commented Feb 18, 2021

Uh oh!

amitiuttarwar commented Feb 18, 2021

Uh oh!

maflcko commented Feb 19, 2021

Uh oh!

Uh oh!

practicalswift commented Feb 23, 2021

Uh oh!

jnewbery commented Feb 23, 2021

Uh oh!

maflcko commented Feb 23, 2021

Uh oh!

Uh oh!

maflcko commented Feb 18, 2021 •

edited

Loading