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

brunoerg commented at 1:08 PM on January 11, 2022: contributor

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.

brunoerg marked this as a draft on Jan 11, 2022

brunoerg force-pushed on Jan 11, 2022

in src/net.cpp:2026 in c9893a6751 outdated

2022 | @@ -2023,11 +2023,13 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
2023 |  
2024 |          int64_t nANow = GetAdjustedTime();
2025 |          int nTries = 0;
2026 | +        bool fDeleteAnchors = false;

jonatack commented at 2:01 PM on January 11, 2022:

naming suggestion

        bool delete_anchors_file{false};

brunoerg commented at 2:11 PM on January 11, 2022:

I prefer delete_anchors_file, didn't do it just to follow the same pattern of other vars in this file.

jonatack commented at 2:18 PM on January 11, 2022:

Not a blocker but I believe it's preferred to use the current style guidelines for new code.

brunoerg commented at 2:36 PM on January 11, 2022:

Done! Thanks!

in src/addrdb.cpp:229 in c9893a6751 outdated

 225 |  }
 226 | +
 227 | +void DeleteAnchorsFile(const fs::path& anchors_db_path)
 228 | +{
 229 | +    fs::remove(anchors_db_path);
 230 | +}

jonatack commented at 2:03 PM on January 11, 2022:

Maybe inline this function definition into the declaration, as it is only one line of code (otherwise, missing EOF newline).

brunoerg commented at 2:15 PM on January 11, 2022:

Same reason of delete_anchors_file. I also prefer inline, but there are other functions in this file that is not following this pattern (e.g. ReadFromStream).

jonatack commented at 2:20 PM on January 11, 2022:

ISTM that this is a per-function (performance and LOC/code organization) decision, not a style one (no need to follow what other functions are doing).

brunoerg commented at 2:34 PM on January 11, 2022:

Great, going to change it.

jonatack commented at 2:04 PM on January 11, 2022: member

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

jonatack commented at 2:06 PM on January 11, 2022: member

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

in src/addrdb.cpp:233 in 3da7f8dd50 outdated

 219 | @@ -220,7 +220,6 @@ std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
 220 |          anchors.clear();
 221 |      }
 222 |  
 223 | -    fs::remove(anchors_db_path);

jonatack commented at 2:09 PM on January 11, 2022:

It looks like this commit isn't standalone and should be combined so as to maintain the file deletion functionality.

brunoerg commented at 2:22 PM on January 11, 2022:

Done, thank you!

brunoerg marked this as ready for review on Jan 11, 2022

brunoerg renamed this:
~~p2p, refactor: delete anchors.dat after trying to connect to that peers~~
p2p: delete anchors.dat after trying to connect to that peers
on Jan 11, 2022

brunoerg commented at 2:12 PM on January 11, 2022: contributor

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

Done!

brunoerg force-pushed on Jan 11, 2022

DrahtBot added the label P2P on Jan 11, 2022

brunoerg force-pushed on Jan 11, 2022

brunoerg commented at 4:39 PM on January 11, 2022: contributor

force-pushed addressing @jonatack's comments

in src/addrdb.h:66 in fb0c33236a outdated

  59 | @@ -60,10 +60,15 @@ void DumpAnchors(const fs::path& anchors_db_path, const std::vector<CAddress>& a
  60 |  
  61 |  /**
  62 |   * Read the anchor IP address database (anchors.dat)
  63 | - *
  64 | - * Deleting anchors.dat is intentional as it avoids renewed peering to anchors after
  65 | - * an unclean shutdown and thus potential exploitation of the anchor peer policy.

luke-jr commented at 7:57 AM on January 12, 2022:

This property is being lost here in the case where the anchor causes a crash between reading it and making a connection successfully.

brunoerg commented at 10:37 AM on January 12, 2022:

Yes, make sense.

luke-jr changes_requested

brunoerg force-pushed on Jan 12, 2022

brunoerg commented at 7:01 PM on January 21, 2022: contributor

cc: @hebasto

in src/addrdb.h:62 in 831377b4ec outdated

  62 | - * Read the anchor IP address database (anchors.dat)
  63 | - *
  64 | - * Deleting anchors.dat is intentional as it avoids renewed peering to anchors after
  65 | - * an unclean shutdown and thus potential exploitation of the anchor peer policy.
  66 | - */
  67 | +/** Read the anchor IP address database (anchors.dat) */

unknown commented at 8:24 PM on January 21, 2022:

This PR looks to improve things based on the concept however I could not understand why is the file deleted? Why not just read and write?

Dump block-relay-only peers to anchors.dat on exit
Read anchors.dat on start
Repeat 1

Then I read the comment which is removed by the PR:

Deleting anchors.dat is intentional as it avoids renewed peering to anchors after an unclean shutdown and thus potential exploitation of the anchor peer policy

So maybe this comment should not be removed or add relevant information.

/**
* Read the anchor IP address database (anchors.dat)
*
* Deleting anchors.dat after trying to connect to block-relay-only peers is intentional as...
 */

brunoerg commented at 10:06 PM on January 21, 2022:

Just read and write? Why keep in anchors.dat addresses we could not be able to connect, for example? Supposing we are on your step 2, what would happen if that peers were offline?

unknown commented at 11:36 PM on January 21, 2022:

Replace with new peers? I don't have issues with deleting, if we still delete anchors.dat maybe comment can be changed and not completely removed.

brunoerg commented at 12:58 AM on January 22, 2022:

I understood the point. I removed it after #24034 (review), did you check that? what do you think?

hebasto commented at 6:47 PM on January 24, 2022: member

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?

in src/net.cpp:2308 in fc734a4bff outdated

2555 | @@ -2556,6 +2556,8 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
2556 |          m_anchors = ReadAnchors(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME);
2557 |          if (m_anchors.size() > MAX_BLOCK_RELAY_ONLY_ANCHORS) {
2558 |              m_anchors.resize(MAX_BLOCK_RELAY_ONLY_ANCHORS);
2559 | +        } else if (m_anchors.size() == 0) {
2560 | +            DeleteAnchorsFile(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME);

hebasto commented at 6:50 PM on January 24, 2022:

Why this change?

brunoerg commented at 6:57 PM on January 24, 2022:

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

brunoerg commented at 12:27 AM on January 25, 2022:

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 commented at 6:49 PM on February 1, 2022: contributor

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

Yes.

in src/net.cpp:2032 in 831377b4ec outdated

2027 |          while (!interruptNet)
2028 |          {
2029 |              if (anchor && !m_anchors.empty()) {
2030 |                  const CAddress addr = m_anchors.back();
2031 |                  m_anchors.pop_back();
2032 | +                if (m_anchors.size() == 0) delete_anchors_file = true;

stickies-v commented at 1:01 PM on February 15, 2022:

nit: we use m_anchors.empty() a few lines higher up, would suggest to stay consistent?

                if (m_anchors.empty()) delete_anchors_file = true;

brunoerg commented at 1:44 PM on February 21, 2022:

Agreed, done!

in src/addrdb.cpp:227 in 831377b4ec outdated

 219 | @@ -220,6 +220,7 @@ std::vector<CAddress> ReadAnchors(const fs::path& anchors_db_path)
 220 |          anchors.clear();
 221 |      }
 222 |  
 223 | -    fs::remove(anchors_db_path);
 224 |      return anchors;
 225 |  }
 226 | +
 227 | +void DeleteAnchorsFile(const fs::path& anchors_db_path) { fs::remove(anchors_db_path); }

stickies-v commented at 1:27 PM on February 15, 2022:

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.

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);
}

stickies-v commented at 10:02 PM on February 18, 2022: contributor

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.

brunoerg force-pushed on Feb 21, 2022

brunoerg commented at 1:56 PM on February 21, 2022: contributor

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

in src/net.cpp:1769 in fe350d62d9 outdated

2052 | @@ -2053,7 +2053,7 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
2053 |              if (anchor && !m_anchors.empty()) {
2054 |                  const CAddress addr = m_anchors.back();
2055 |                  m_anchors.pop_back();
2056 | -                if (m_anchors.size() == 0) delete_anchors_file = true;
2057 | +                if (m_anchors.empty()) delete_anchors_file = true;

theStack commented at 1:31 PM on March 2, 2022:

nit: in order to create minimal diffs (and save reviewers time), I'd suggest to stick to either .size() == 0 or .empty() from the very first commit that introduces it and not change it after.

theStack commented at 1:39 PM on March 2, 2022: contributor

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.

LarryRuane commented at 10:07 PM on March 2, 2022: contributor

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#WRITE-THROUGH)), 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).

in src/net.cpp:2447 in fe350d62d9 outdated

2722 | @@ -2713,7 +2723,10 @@ void CConnman::StopNodes()
2723 |              if (anchors_to_dump.size() > MAX_BLOCK_RELAY_ONLY_ANCHORS) {
2724 |                  anchors_to_dump.resize(MAX_BLOCK_RELAY_ONLY_ANCHORS);
2725 |              }
2726 | -            DumpAnchors(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME, anchors_to_dump);
2727 | +
2728 | +            if (tried_connect_anchors) {

lsilva01 commented at 3:19 PM on March 3, 2022:

If anchors_to_dump is empty, there is nothing to dump.

            if (!anchors_to_dump.empty()) {

brunoerg commented at 8:01 PM on March 10, 2022:

What if my node connected to one node from anchors.dat but a connection to the other one has not been tried yet? In this case, anchors_to_dump wouldn't be empty.

in src/net.cpp:1763 in fe350d62d9 outdated

2046 | @@ -2047,11 +2047,13 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect)
2047 |  
2048 |          int64_t nANow = GetAdjustedTime();
2049 |          int nTries = 0;
2050 | +        bool delete_anchors_file{false};

lsilva01 commented at 3:51 PM on March 3, 2022:

If this code is added, I think the delete_anchors_file variable can be removed and the DeleteAnchorsFile in CConnman::Start can also be removed.

EDIT: The suggestion preview is not accurate. The suggested code should be inside while (!interruptNet).

        if (m_anchors.empty()) DeleteAnchorsFile(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME);

brunoerg commented at 8:23 PM on October 17, 2022:

No, I think we can't do it. Because I just want to delete anchors.dat after trying the connection what's not the scenario in L2050.. That's why it sets delete_anchors_file=true and at the end we check it to know if it's time to delete the file.

lsilva01 commented at 3:54 PM on March 3, 2022: contributor

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.

mzumsande commented at 10:09 PM on March 15, 2022: contributor

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#WRITE-THROUGH)), 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 commented at 11:37 PM on March 15, 2022: contributor

@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 commented at 1:50 PM on September 23, 2022: contributor

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.

achow101 commented at 6:55 PM on October 12, 2022: member

Are you still working on this?

brunoerg commented at 5:01 PM on October 17, 2022: contributor

Are you still working on this?

Yes! Moving it this week.

brunoerg force-pushed on Oct 17, 2022

brunoerg marked this as a draft on Oct 17, 2022

brunoerg force-pushed on Oct 17, 2022

brunoerg marked this as ready for review on Oct 18, 2022

p2p: Add DeleteAnchorsFile() 20e8aaf754

p2p: delete anchors.dat after trying to connect 7a2152ebe7

p2p: delete anchors.dat if it is empty e8155bbcb5

test: check anchors.dat with wait_until 03a20a1ebb

p2p: add tried_connect_anchors 68c12a7b37

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

move `ANCHORS_DATABASE_FILENAME` to `addrdb` 658cfb1aa9

brunoerg force-pushed on Nov 30, 2022

brunoerg commented at 2:41 PM on November 30, 2022: contributor

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.

in src/addrdb.cpp:244 in 658cfb1aa9

 245 |  }
 246 | +
 247 | +void DeleteAnchorsFile()
 248 | +{
 249 | +    const fs::path path{gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME};
 250 | +    fs::remove(path);

andrewtoth commented at 2:18 PM on December 25, 2022:

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?

brunoerg marked this as a draft on Apr 25, 2023

DrahtBot added the label CI failed on Aug 6, 2023

DrahtBot commented at 10:20 AM on September 3, 2023: contributor

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 commented at 11:54 AM on September 7, 2023: contributor

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

DrahtBot added the label Needs rebase on Sep 7, 2023

DrahtBot commented at 1:23 AM on December 6, 2023: contributor

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 commented at 12:18 AM on March 5, 2024: contributor

⌛ 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 commented at 12:18 AM on March 5, 2024: contributor

🤔 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 commented at 1:51 PM on March 6, 2024: member

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.

fanquake closed this on Mar 6, 2024

bitcoin locked this on Mar 6, 2025