p2p: delete anchors.dat after trying to connect to that peers #24034

brunoerg · 2022-01-11T13:08:59Z

In the current scenario, anchors.dat is deleted right after reading it, however, if my node stops before trying to connect to the anchors peers, it will create an empty anchors.dat. This PR changes it to delete anchors.dat after trying to connect to the peers from it. So, if my node stops (in a clean way) before trying to connect to that peers, the anchors.dat file will be preserved and a dump won't happen.

jonatack

A couple of comments after skimming the diff, feel free to ignore for now; will review.

src/net.cpp

src/addrdb.cpp

jonatack · 2022-01-11T14:06:43Z

Perhaps remove "refactor" from the PR title as there seems to be a change of behavior.

src/addrdb.cpp

brunoerg · 2022-01-11T14:12:19Z

Perhaps remove "refactor" from the PR title as there seems to be a change of behavior.

Done!

brunoerg · 2022-01-11T16:39:16Z

force-pushed addressing @jonatack's comments

src/addrdb.h

brunoerg · 2022-01-21T19:01:09Z

cc: @hebasto

src/addrdb.h

hebasto · 2022-01-24T18:47:22Z

So, if my node stops before trying to connect to that peers, the anchors.dat file will be preserved.

Do you mean the normal shutdown, i.e., stop RPC or Quit in the GUI, right?

hebasto · 2022-01-24T18:50:33Z

src/net.cpp

+        } else if (m_anchors.size() == 0) {
+            DeleteAnchorsFile(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME);


Why this change?

Because if there aren't anchors we can delete the file at that point, no need to wait to try connect them.

We talked about it earlier but I just remembered I can't remove it because in https://github.com/bitcoin/bitcoin/pull/24034/files#diff-00021eed586a482abdb09d6cdada1d90115abe988a91421851960e26658bed02R2029 if m_anchors is empty the anchors.dat won't be deleted.

brunoerg · 2022-02-01T18:49:21Z

Do you mean the normal shutdown, i.e., stop RPC or Quit in the GUI, right?

Yes.

stickies-v

I'm not sure the current approach works for the goal you're trying to achieve.

In case of clean shutdown, CConMan::Close() is always (I think?) going to be called, which means that DumpAnchors() is called too. DumpAnchors() will just overwrite any existing anchors.dat file, regardless of whether or not that file exists. If no block-only connections have been initialized yet, DumpAnchors() will simply create an empty file. So I think in case of clean shutdown, you need to control when DumpAnchors() is called instead of deleting anchors.dat.

In case of forced shutdown, I think this PR moves in the right direction by not immediately removing anchors.dat, because then DumpAnchors() may not be called and it could be helpful to restart from the previously existing anchors.dat. However, I'm unsure (from a security or resilience perspective) if that's something to be desired or not, I don't fully understand the big picture yet here. On the one hand, you don't want to make it too easy for an attacker to purge existing anchors. However, you should properly handle e.g. a corrupt anchors file too.

src/net.cpp

stickies-v · 2022-02-15T13:27:50Z

src/addrdb.cpp

    return anchors;
 }
+
+void DeleteAnchorsFile(const fs::path& anchors_db_path) { fs::remove(anchors_db_path); }


What do you think about making anchors_db_path optional? If you apply the same pattern to DumpAnchors, ReadAnchors and DeleteAnchorsFile, you can move the definition of ANCHORS_DATABASE_FILENAME to addrdb.cpp. Personally I think it's more logical to define the default path logic within these functions since they're called DeleteAnchorsFile, not DeleteFile.

Suggested change

void DeleteAnchorsFile(const fs::path& anchors_db_path) { fs::remove(anchors_db_path); }

void DeleteAnchorsFile(std::optional<const fs::path> anchors_db_path)

{

fs::path path{ anchors_db_path.value_or(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME) };

fs::remove(path);

}

brunoerg · 2022-02-21T13:56:49Z

Thanks, @stickies-v.

Added a flag to control if a connection with all peers from anchors.dat was tried. So, if the connections with all of them have not been tried yet we don't allow DumpAnchors.

E.g:

Starts bitcoind
Read anchors.dat
2 block-relay-only anchors will be tried for connections
Shutdown node before trying to connect to them
Preserve anchors.dat and don't allow call to DumpAnchors

theStack

Light Concept ACK

This seems to be a definitive improvement, but there might be unthought drawbacks or consequences that I'm not aware of yet. Looking forward to today's review club to shed some more light on this.

src/net.cpp

LarryRuane

It seems arbitrary that we preserve our block-relay-only peers across restarts if the node shuts down cleanly but not if the node shuts down uncleanly. Would a better approach be to never delete anchors.dat but instead rewrite it (truncate and write) immediately whenever the set of block-relay-only peers changes (that is, make it a write-through cache), instead of only on a clean shutdown?

If this set of peers gets updated multiple times close in time, then for better performance, use CScheduler.scheduleFromNow() to implement a write-back cache (check every few seconds if anchors.dat is out of date, and if so, rewrite it).

lsilva01

Approach ACK, but I think the code can be simplified.

delete_anchors_file and tried_connect_anchors may not be needed.
And there may also be no need to change CConnman::Start.

I added some suggestions.

src/net.cpp

mzumsande

It seems arbitrary that we preserve our block-relay-only peers across restarts if the node shuts down cleanly but not if the node shuts down uncleanly. Would a better approach be to never delete anchors.dat but instead rewrite it (truncate and write) immediately whenever the set of block-relay-only peers changes (that is, make it a write-through cache), instead of only on a clean shutdown?

There was a lot of discussion on this point in the original PR #17428 which introduced the anchors, see e.g. this summary of some of the aspects.

If I understand it correctly, this PR changes behavior in the special case where a clean shutdown is happening in the time window between startup and connecting to the anchors.
Is there a realistic scenario for how this could happen? If the shutdown was caused maliciously (by DoS, power shutdown, etc.), I think it would likely not be clean.

The only scenario I could think of is that the node operator themselves initiate a clean shutdown (maybe they forgot to specify a startup option, send a SIGINT and try again).

brunoerg · 2022-03-15T23:37:08Z

@mzumsande

If I understand it correctly, this PR changes behavior in the special case where a clean shutdown is happening in the time window between startup and connecting to the anchors.

This PR covers both scenarios (clean and unclean). See the functional test this PR modifies, I just added (last force-pushed) a coverage for un clean shutdown using the same logic as feature_init.

Is there a realistic scenario for how this could happen? If the shutdown was caused maliciously (by DoS, power shutdown, etc.), I think it would likely not be clean.

The only scenario I could think of is that the node operator themselves initiate a clean shutdown (maybe they forgot to specify a startup option, send a SIGINT and try again).

Yes, you're right, I don't think there is a realistic/explicit scenario as an unclean has. It could be caused by a node operator, maybe a social engineering, but there are many more scenarios for unclean shutdown as you mentioned (DoS, power shutdown, etc.).

DrahtBot · 2022-09-23T13:50:24Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	theStack
Approach ACK	lsilva01

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:

#27071 (Handle CJDNS from LookupSubNet() by vasild)
#25284 (net: Use serialization parameters for CAddress serialization by MarcoFalke)

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.

brunoerg · 2022-11-30T14:41:23Z

Just discovered another thing this PR may fix.

Start bitcoind
Wait 2 block-relay-only connections
Shutdown it and start bitcoind with -stopafterblockimport

Since there is no enough time to establish the 2 connections from anchors.dat (because it's gonna stop after block import), maybe there is time for just one, it's gonna clean up that peer from anchors.dat.

andrewtoth · 2022-12-25T14:18:26Z

src/addrdb.cpp

+void DeleteAnchorsFile()
+{
+    const fs::path path{gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME};
+    fs::remove(path);


Right now if we fail to remove the file we crash fairly early on startup, so the user can investigate why they are unable to remove it. With this change we will get pretty far into normal operation before crashing. Should we catch and log any error if we fail to remove, or is it a fatal error if we don't remove?

DrahtBot · 2023-09-03T10:20:12Z

There hasn't been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2023-09-07T11:54:45Z

🐙 This pull request conflicts with the target branch and needs rebase.

DrahtBot · 2023-12-06T01:23:55Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2024-03-05T00:18:35Z

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

DrahtBot · 2024-03-05T00:18:36Z

🤔 There hasn't been much activity lately and the CI seems to be failing.

If no one reviewed the current pull request by commit hash, a rebase can be considered. While the CI failure may be a false positive, the CI hasn't been running for some time, so there may be a real issue hiding as well. A rebase triggers the latest CI and makes sure that no silent merge conflicts have snuck in.

fanquake · 2024-03-06T13:51:36Z

Closing for now. 4 pings and 0 followup. From the discussion here it's not even clear that there is buy-in from other contributers that this is a change we want/is ultimately beneficial.

brunoerg marked this pull request as draft January 11, 2022 13:39

brunoerg force-pushed the 2022-02-anchors-delete branch from 55cf0d2 to c9893a6 Compare January 11, 2022 13:50

jonatack reviewed Jan 11, 2022

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/addrdb.cpp Outdated Show resolved Hide resolved

jonatack reviewed Jan 11, 2022

View reviewed changes

src/addrdb.cpp Show resolved Hide resolved

brunoerg marked this pull request as ready for review January 11, 2022 14:10

brunoerg changed the title ~~p2p, refactor: delete anchors.dat after trying to connect to that peers~~ p2p: delete anchors.dat after trying to connect to that peers Jan 11, 2022

brunoerg force-pushed the 2022-02-anchors-delete branch 2 times, most recently from 69d4184 to c24194c Compare January 11, 2022 14:36

DrahtBot added the P2P label Jan 11, 2022

brunoerg force-pushed the 2022-02-anchors-delete branch from c24194c to fb0c332 Compare January 11, 2022 16:38

luke-jr suggested changes Jan 12, 2022

View reviewed changes

src/addrdb.h Show resolved Hide resolved

brunoerg force-pushed the 2022-02-anchors-delete branch from fb0c332 to 831377b Compare January 12, 2022 10:40

ghost reviewed Jan 21, 2022

View reviewed changes

src/addrdb.h Show resolved Hide resolved

hebasto reviewed Jan 24, 2022

View reviewed changes

stickies-v reviewed Feb 18, 2022

View reviewed changes

brunoerg force-pushed the 2022-02-anchors-delete branch 2 times, most recently from ab509a0 to fe350d6 Compare February 21, 2022 13:53

theStack reviewed Mar 2, 2022

View reviewed changes

src/net.cpp Show resolved Hide resolved

LarryRuane reviewed Mar 2, 2022

View reviewed changes

lsilva01 reviewed Mar 3, 2022

View reviewed changes

src/net.cpp Show resolved Hide resolved

src/net.cpp Show resolved Hide resolved

mzumsande reviewed Mar 15, 2022

View reviewed changes

brunoerg force-pushed the 2022-02-anchors-delete branch from ac11edf to e495983 Compare October 17, 2022 21:21

brunoerg marked this pull request as draft October 17, 2022 21:27

brunoerg force-pushed the 2022-02-anchors-delete branch from e495983 to 41ae2b4 Compare October 17, 2022 21:32

DrahtBot mentioned this pull request Oct 18, 2022

Rework logging timer #24923

Closed

brunoerg marked this pull request as ready for review October 18, 2022 13:08

brunoerg added 7 commits November 30, 2022 10:46

p2p: Add DeleteAnchorsFile()

20e8aaf

p2p: delete anchors.dat after trying to connect

7a2152e

p2p: delete anchors.dat if it is empty

e8155bb

test: check anchors.dat with wait_until

03a20a1

p2p: add tried_connect_anchors

68c12a7

test: stop node unclearly and test if anchors.dat has been preserved

665b45e

move ANCHORS_DATABASE_FILENAME to addrdb

658cfb1

brunoerg force-pushed the 2022-02-anchors-delete branch from 41ae2b4 to 658cfb1 Compare November 30, 2022 14:37

DrahtBot mentioned this pull request Dec 16, 2022

net: Use serialization parameters for CAddress serialization #25284

Merged

andrewtoth reviewed Dec 25, 2022

View reviewed changes

DrahtBot mentioned this pull request Feb 10, 2023

Handle CJDNS from LookupSubNet() #27071

Merged

DrahtBot mentioned this pull request Mar 31, 2023

p2p: skip netgroup diversity of new connections for tor/i2p/cjdns #27374

Merged

brunoerg marked this pull request as draft April 25, 2023 15:35

DrahtBot added the CI failed label Aug 6, 2023

DrahtBot mentioned this pull request Sep 2, 2023

test: Use pathlib over os path #28392

Merged

DrahtBot added the Needs rebase label Sep 7, 2023

fanquake closed this Mar 6, 2024

bitcoin locked and limited conversation to collaborators Mar 6, 2025

		} else if (m_anchors.size() == 0) {
		DeleteAnchorsFile(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME);

p2p: delete anchors.dat after trying to connect to that peers #24034

p2p: delete anchors.dat after trying to connect to that peers #24034

Uh oh!

Conversation

brunoerg commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jonatack commented Jan 11, 2022

Uh oh!

Uh oh!

brunoerg commented Jan 11, 2022

Uh oh!

brunoerg commented Jan 11, 2022

Uh oh!

Uh oh!

brunoerg commented Jan 21, 2022

Uh oh!

Uh oh!

hebasto commented Jan 24, 2022

Uh oh!

hebasto Jan 24, 2022

Choose a reason for hiding this comment

Uh oh!

brunoerg Jan 24, 2022 • 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!

brunoerg Jan 25, 2022 • 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!

brunoerg commented Feb 1, 2022

Uh oh!

stickies-v left a comment • 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!

Uh oh!

stickies-v Feb 15, 2022

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Feb 21, 2022

Uh oh!

theStack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

LarryRuane left a comment

Choose a reason for hiding this comment

Uh oh!

lsilva01 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mzumsande left a comment

Choose a reason for hiding this comment

Uh oh!

brunoerg commented Mar 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Sep 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Conflicts

Uh oh!

brunoerg commented Nov 30, 2022

Uh oh!

andrewtoth Dec 25, 2022

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Sep 3, 2023

Uh oh!

DrahtBot commented Sep 7, 2023

Uh oh!

brunoerg commented Jan 11, 2022 •

edited

Loading

brunoerg Jan 24, 2022 •

edited

Loading

brunoerg Jan 25, 2022 •

edited

Loading

stickies-v left a comment •

edited

Loading

brunoerg commented Mar 15, 2022 •

edited

Loading

DrahtBot commented Sep 23, 2022 •

edited

Loading