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

pull jonatack wants to merge 1 commits into bitcoin:master from jonatack:log-message-instead-of-error-if-missing-anchors changing 1 files +4 −1
  1. jonatack commented at 7:38 am on February 15, 2021: member

    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

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

    after

    02021-02-15T06:53:56Z Could not find anchors file "~/.bitcoin/anchors.dat"
12021-02-15T06:53:56Z 0 block-relay-only anchors will be tried for connections.
  2. p2p: log a message instead of error on missing anchors.dat 8f51826d33
  3. fanquake added the label P2P on Feb 15, 2021
  4. hebasto commented at 7:41 am on February 15, 2021: member

    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.

  5. naumenkogs commented at 7:48 am on February 15, 2021: member
    Concept ACK
  6. MarcoFalke commented at 8:12 am on February 15, 2021: member
    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.
  7. jonatack commented at 8:40 am on February 15, 2021: member
    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.
  8. MarcoFalke commented at 9:14 am on February 15, 2021: member

    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.

    02021-02-15T09:14:40Z ERROR: DeserializeFileDB: Failed to open file /tmp/new_datadir/a/regtest/banlist.dat
12021-02-15T09:14:40Z Failed to read fee estimates from /tmp/new_datadir/a/regtest/fee_estimates.dat. Continue anyway.
22021-02-15T09:14:40Z Failed to open mempool file from disk. Continuing anyway.
32021-02-15T09:14:40Z ERROR: DeserializeFileDB: Failed to open file /tmp/new_datadir/a/regtest/peers.dat
42021-02-15T09:14:40Z ERROR: DeserializeFileDB: Failed to open file /tmp/new_datadir/a/regtest/anchors.dat
  9. practicalswift commented at 10:43 am on February 15, 2021: contributor

    Concept ACK

    Thanks for removing unnecessary (and thus spammy) logging.

  10. jonatack commented at 11:51 am on February 15, 2021: member

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

    1. that would not resolve the issue
    1. 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

  11. laanwj commented at 11:51 am on February 15, 2021: member

    Concept and Code review ACK 8f51826d3391ed78dc6f2428d9c556929687d7ea

    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.

  12. jonatack commented at 11:54 am on February 15, 2021: member

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

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

  13. jnewbery commented at 11:58 am on February 15, 2021: member

    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?

  14. MarcoFalke commented at 12:01 pm on February 15, 2021: member

    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?
  15. jonatack commented at 12:03 pm on February 15, 2021: member
    Then perhaps move this change into DeserializeFileDB()?
  16. jonatack commented at 12:05 pm on February 15, 2021: member

    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 :smile:

  17. jonatack commented at 12:08 pm on February 15, 2021: member
    To my surprise, this is controversial. I think it will have more success if someone else proposes it.
  18. jonatack closed this on Feb 15, 2021
  19. MarcoFalke commented at 12:11 pm on February 15, 2021: member
    This has 4 Concept ACK and no NACK, so I don’t think anyone sees this as controversial
  20. jnewbery commented at 12:14 pm on February 15, 2021: member
    This is not at all controversial. I don’t understand why this has been closed?
  21. michaelfolkson commented at 12:20 pm on February 15, 2021: contributor
    Concept ACK too
  22. jonatack commented at 12:27 pm on February 15, 2021: member
    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.
  23. michaelfolkson commented at 12:30 pm on February 15, 2021: contributor

    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.”

  24. jonatack commented at 12:41 pm on February 15, 2021: member
    Updated #20576 to be a general RFC on logging improvements and linked to some of the comments/ideas here.
  25. laanwj commented at 1:35 pm on February 15, 2021: member
    To be clear I’m completely okay with this specific change my comment was only about possible future direction.
  26. jonatack commented at 3:07 pm on February 15, 2021: member
    That is how I read it (thank you). Re-opening to respect the time people spent reviewing.
  27. jonatack reopened this on Feb 15, 2021
  28. in src/addrdb.cpp:173 in 8f51826d33 
    166@@ -167,7 +167,10 @@ void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& a
167 std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
168 {
169     std::vector<CAddress> anchors;
170-    if (DeserializeFileDB(anchors_db_path, anchors)) {
171+    if (!fs::exists(anchors_db_path)) {
172+        LogPrintf("Could not find anchors file %s\n", anchors_db_path);
173+        anchors.clear();
174+    } else if (DeserializeFileDB(anchors_db_path, anchors)) {
    vasild commented at 10:59 am on February 18, 2021:
    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.
    laanwj commented at 11:50 am on February 18, 2021:
    I had also noticed this but thought it wise not to comment :smile: (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.
  29. vasild approved
  30. vasild commented at 11:04 am on February 18, 2021: member

    ACK 8f51826d3391ed78dc6f2428d9c556929687d7ea

    • 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.
  31. MarcoFalke commented at 1:17 pm on February 18, 2021: member
    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.
  32. jonatack commented at 3:00 pm on February 18, 2021: member
    #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.
  33. jonatack closed this on Feb 18, 2021
  34. jonatack deleted the branch on Feb 18, 2021
  35. MarcoFalke referenced this in commit 78effb37f3 on Feb 23, 2021
  36. sidhujag referenced this in commit c1934b507f on Feb 23, 2021
  37. PastaPastaPasta referenced this in commit b88a0ecef8 on Sep 17, 2021
  38. PastaPastaPasta referenced this in commit e61d959bfc on Sep 19, 2021
  39. thelazier referenced this in commit 7c2a9c5bbc on Sep 25, 2021
  40. gades referenced this in commit fe638cb884 on May 17, 2022
  41. DrahtBot locked this on Aug 16, 2022


jonatack hebasto naumenkogs MarcoFalke practicalswift laanwj jnewbery michaelfolkson vasild

Labels
P2P

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 13:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me