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

jonatack · 2021-02-15T07:38:18Z

To not needlessly alarm users upgrading from v0.20 and earlier, log a message rather than an error if the anchors.dat file is missing. "ERROR: DeserializeFileDB" will still be printed if anchors.dat is found but cannot be read. Motivation from this user report.

before

2021-02-14T14:23:46Z ERROR: DeserializeFileDB: Failed to open file ~/.bitcoin/anchors.dat
2021-02-14T14:23:46Z 0 block-relay-only anchors will be tried for connections.

after

2021-02-15T06:53:56Z Could not find anchors file "~/.bitcoin/anchors.dat"
2021-02-15T06:53:56Z 0 block-relay-only anchors will be tried for connections.

hebasto · 2021-02-15T07:41:51Z

Concept ACK. Absence of anchors.dat is not an error.

To not needlessly alarm users upgrading from v0.20 and earlier...

The anchors.dat also could be absent after unclean shut down.

naumenkogs · 2021-02-15T07:48:19Z

Concept ACK

maflcko · 2021-02-15T08:12:28Z

Shouldn't this be done for all files? I am pretty sure it will still print that error for all other files on a fresh start.

jonatack · 2021-02-15T08:40:56Z

For example, this check is done for the asmap file since #17812 (src/init.cpp::1512) but prints an error, as in this case the user may have provided an incorrect filename or path.

maflcko · 2021-02-15T09:14:06Z

Maybe it would help to simply remove the screaming ERROR in all uppercase. It is not really a fatal error (if it was, the program should stop), but more like a debug statement.

2021-02-15T09:14:40Z ERROR: DeserializeFileDB: Failed to open file /tmp/new_datadir/a/regtest/banlist.dat
2021-02-15T09:14:40Z Failed to read fee estimates from /tmp/new_datadir/a/regtest/fee_estimates.dat. Continue anyway.
2021-02-15T09:14:40Z Failed to open mempool file from disk. Continuing anyway.
2021-02-15T09:14:40Z ERROR: DeserializeFileDB: Failed to open file /tmp/new_datadir/a/regtest/peers.dat
2021-02-15T09:14:40Z ERROR: DeserializeFileDB: Failed to open file /tmp/new_datadir/a/regtest/anchors.dat

practicalswift · 2021-02-15T10:43:51Z

Concept ACK

Thanks for removing unnecessary (and thus spammy) logging.

jonatack · 2021-02-15T11:51:43Z

Maybe it would help to simply remove the screaming ERROR in all uppercase.

that would not resolve the issue

@hebasto p2p: if no anchors.dat file, log a message instead of an error #21181 (comment), "Absence of anchors.dat is not an error."
@jnewbery http://www.erisian.com.au/bitcoin-core-dev/log-2021-02-14.html#l-116, "It shouldn't ever be an error condition to not have an anchors.dat file"
@michaelfolkson - http://www.erisian.com.au/bitcoin-core-dev/log-2021-02-13.html#l-300, "why should it be an error message? It is expected behavior if upgrading"

providing anchor.dat logs that distinguish between missing and unreadable anchors.dat seems more useful and helpful to users

This seems to be a clear improvement with a small change similar to what we do for the asmap file. I'm not sure if there are other files that would benefit from the same, but it doesn't need to be done here and I'm not sure there is a need to provide a general abstraction right now unless more cases are identified.

Suggesting this be merged to master and backported to 0.21.x

laanwj · 2021-02-15T11:51:49Z

Concept and Code review ACK 8f51826

Maybe it would help to simply remove the screaming ERROR in all uppercase.

Yes screaming is generally bad but apart from that I think it makes sense to have multiple severities in logging.
E.g. other types of error that happen in DeserializeFileDB (say, specific de-serialization errors) might not be fatal but would ideally trigger someone to take a closer look.
Something @gmaxwell suggested once is to replace it with UNEXPECTED. c-lightning does this as well.

jonatack · 2021-02-15T11:54:14Z

I think it makes sense to have multiple severities in logging.

I would like this indeed (and mention it in #20576).

jnewbery · 2021-02-15T11:58:54Z

Concept ACK.

Maybe it would help to simply remove the screaming ERROR in all uppercase. It is not really a fatal error (if it was, the program should stop), but more like a debug statement.

Agree. It's not an error condition to start without peers.dat, anchors.dat or banlist.dat. I think we could probably resolve all three by just changing the log message?

maflcko · 2021-02-15T12:01:59Z

I think we should be consistent in the way this is treated. Someone will create a pull in x months to fix this for banlist.dat, then for peers.dat in y months. So finally we have 5 different error messages for the same condition.

Failed to read fee estimates from {}. Continue anyway.
Failed to open mempool file from disk. Continuing anyway.
Could not find anchors file {}
Could not find asmap file {}
... Another message for banlist and peers?

jonatack · 2021-02-15T12:03:15Z

Then perhaps move this change into DeserializeFileDB()?

jonatack · 2021-02-15T12:05:12Z

I think we should be consistent in the way this is treated. Someone will create a pull in x months to fix this for banlist.dat, then for peers.dat in y months.

ISTM this is what has been happening with moving the logging from debug to NET, one by one, and no one minded. Maybe I should stay away from logging 😄

jonatack · 2021-02-15T12:08:41Z

To my surprise, this is controversial. I think it will have more success if someone else proposes it.

maflcko · 2021-02-15T12:11:46Z

This has 4 Concept ACK and no NACK, so I don't think anyone sees this as controversial

jnewbery · 2021-02-15T12:14:02Z

This is not at all controversial. I don't understand why this has been closed?

michaelfolkson · 2021-02-15T12:20:50Z

Concept ACK too

jonatack · 2021-02-15T12:27:26Z

I don't understand the suggested changes. We can't fix this by updating the message in ReadAnchors(), it's in DeserializeFileDB(). We can't update that message without affecting the other callers. And suddenly we're talking about gathering consensus on aligning unrelated file messages that were never aligned before AFAIK, so a small fix turns into a wide change. I don't know why is that suddenly necessary (and didn't plan for that) in the context of a small fix.

michaelfolkson · 2021-02-15T12:30:17Z

Keep the scope as limited as you want and provide some direction for others who might want to open additional PRs on top of this one?

It is ok (imo) to say "I see merit in that idea but I want to keep the scope of this PR limited. You are free to open PRs on top of this one if you want to widen the scope" or "I think what you are suggesting is too complicated. I would prefer to keep the scope of this PR limited to what it currently does."

jonatack · 2021-02-15T12:41:08Z

Updated #20576 to be a general RFC on logging improvements and linked to some of the comments/ideas here.

laanwj · 2021-02-15T13:35:57Z

To be clear I'm completely okay with this specific change my comment was only about possible future direction.

jonatack · 2021-02-15T15:07:34Z

That is how I read it (thank you). Re-opening to respect the time people spent reviewing.

vasild

ACK 8f51826

This PR is an improvement over master.
Yes, further improvements are possible, some of which may rewrite the change from this PR, but they do not exist yet and may never be created. So I think this PR should be closed without merge only if another PR exists which clearly supersedes it.

vasild · 2021-02-18T10:59:56Z

src/addrdb.cpp

+    if (!fs::exists(anchors_db_path)) {
+        LogPrintf("Could not find anchors file %s\n", anchors_db_path);
+        anchors.clear();
+    } else if (DeserializeFileDB(anchors_db_path, anchors)) {


There is a race here - the file may exist during the fs::exists check and be deleted after that and before DeserializeFileDB(). Of course this is irrelevant and ok in this case. I just can't stop myself from mentioning it. Apparently not everything I do is in my control, sorry.

I had also noticed this but thought it wise not to comment 😄 (because the race doesn't affect anything but how the error is reported)
But yes if you want to be pedantic about it, it would be to make DeserializeFileDB return a distinguishable error condition when it could not find the anchors file and then log this message, instead of doing a separate check.

maflcko · 2021-02-18T13:17:32Z

Obviously this pull is a fine improvement over master, but I am still not convinced that we need to create several back-and-forth patches to fix a trivial logging issue. Created #21222 as an alternative.

jonatack · 2021-02-18T15:00:58Z

#21222 seems less helpful to users, but you're not happy with this and I'm not keen on having a competition. I was just trying to improve things for our users.

faf48f2 log: Clarify log message when file does not exist (MarcoFalke) Pull request description: Shorter and broader alternative to #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

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

p2p: log a message instead of error on missing anchors.dat

8f51826

fanquake added the P2P label Feb 15, 2021

jonatack mentioned this pull request Feb 15, 2021

Save client version to the settings file on shutdown #21171

Closed

jonatack closed this Feb 15, 2021

jonatack mentioned this pull request Feb 15, 2021

RFC on logging improvements #20576

Closed

jonatack reopened this Feb 15, 2021

vasild approved these changes Feb 18, 2021

View reviewed changes

maflcko mentioned this pull request Feb 18, 2021

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

Merged

jonatack closed this Feb 18, 2021

jonatack deleted the log-message-instead-of-error-if-missing-anchors branch February 18, 2021 15:13

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

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

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

Uh oh!

Conversation

jonatack commented Feb 15, 2021

Uh oh!

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

Uh oh!

Uh oh!

naumenkogs commented Feb 15, 2021

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

laanwj commented Feb 15, 2021

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

jnewbery commented Feb 15, 2021

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

maflcko commented Feb 15, 2021

Uh oh!

jnewbery commented Feb 15, 2021

Uh oh!

michaelfolkson commented Feb 15, 2021

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

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

Uh oh!

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

laanwj commented Feb 15, 2021

Uh oh!

jonatack commented Feb 15, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

vasild Feb 18, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Feb 18, 2021

Uh oh!

jonatack commented Feb 18, 2021

Uh oh!

Uh oh!

hebasto commented Feb 15, 2021 •

edited

Loading

maflcko commented Feb 15, 2021 •

edited

Loading

practicalswift commented Feb 15, 2021 •

edited

Loading

michaelfolkson commented Feb 15, 2021 •

edited

Loading

laanwj Feb 18, 2021 •

edited

Loading