net: preserve anchors when network is disabled #34213

brunoerg commented at 9:26 PM on January 6, 2026: contributor

Problem

When a node is shut down while network activity is disabled (e.g. via -networkactive=0 or setnetworkactive false), anchors.dat is overwritten with an empty list. This happens because StopNodes() collects anchors via GetCurrentBlockRelayOnlyConns(), which returns nothing when there are no active connections. The node then loses its anchors and cannot reconnect to them on the next startup.

Fix

In SetNetworkActive(false), save the current block-relay-only connections into m_anchors before the connections are closed, so the anchor list is preserved even after the network is deactivated.
In StopNodes(), if the network is inactive and m_anchors is non-empty, use those stored anchors as the source for anchors.dat instead of the (empty) live connection list. Also skip DumpAnchors entirely when the network is inactive and there is nothing to write.

Also, log that "X anchors will be tried for connections" only if network is active.

DrahtBot added the label P2P on Jan 6, 2026

DrahtBot commented at 9:26 PM on January 6, 2026: contributor

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34213.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept ACK	waketraindev, danielabrozzoni
Stale ACK	bensig, Bortlesboat

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32394 (net: make m_nodes_mutex non-recursive by vasild)
#32065 (i2p: make a time gap between creating transient sessions and using them 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.

brunoerg force-pushed on Jan 6, 2026

DrahtBot added the label CI failed on Jan 6, 2026

DrahtBot commented at 9:31 PM on January 6, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task lint: https://github.com/bitcoin/bitcoin/actions/runs/20762589891/job/59621113321</sub> <sub>LLM reason (✨ experimental): Linting failed: ruff detected errors (trailing whitespace) in Python code, causing the CI to fail.</sub>

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

brunoerg force-pushed on Jan 6, 2026

DrahtBot removed the label CI failed on Jan 6, 2026

in src/net.cpp:3348 in 903a37c1e1 outdated

3344 | @@ -3345,7 +3345,7 @@ bool CConnman::Start(CScheduler& scheduler, const Options& connOptions)
3345 |          std::shuffle(seed_nodes.begin(), seed_nodes.end(), FastRandomContext{});
3346 |      }
3347 |  
3348 | -    if (m_use_addrman_outgoing) {
3349 | +    if (m_use_addrman_outgoing && fNetworkActive) {

waketraindev commented at 12:21 AM on January 7, 2026:

I start some nodes with network inactive.

micro nit (in both cases):

    if (fNetworkActive && m_use_addrman_outgoing) {

brunoerg commented at 6:25 PM on January 7, 2026:

I'll leave as-is for now.

luke-jr commented at 8:27 PM on January 22, 2026:

If the user subsequently uses setnetworkactive to enable network, should we restore the anchors then?

brunoerg commented at 12:36 PM on January 27, 2026:

Yes, I think so. I will implement it in a follow-up.

waketraindev commented at 12:22 AM on January 7, 2026: contributor

Concept ACK

waketraindev commented at 1:50 AM on January 7, 2026: contributor

Have you considered also adding a DumpAnchors() if anchors_to_dump.size() > 1 when setting network active from true to false?

That would persist the anchors for nodes that do a setnetworkactive false and after shut down and skip dumping them again at shutdown.

brunoerg commented at 6:26 PM on January 7, 2026: contributor

Have you considered also adding a DumpAnchors() if anchors_to_dump.size() > 1 when setting network active from true to false?

Yes, but would leave it for a follow-up, it will require more changes than the simple ones to cover the -networkactive behavior.

bensig commented at 6:40 PM on January 7, 2026: contributor

ACK 903a37c1e142b4ee6b83fee99735ab69716084ee

Tested, feature_anchors.py passes

DrahtBot requested review from waketraindev on Jan 7, 2026

w0xlt commented at 8:59 PM on January 7, 2026: contributor

This seems like a reasonable change, but it may be better to document that setting fNetworkActive to false via the setnetworkactive RPC can prevent anchors from being preserved.

brunoerg commented at 7:48 PM on January 8, 2026: contributor

This seems like a reasonable change, but it may be better to document that setting fNetworkActive to false via the setnetworkactive RPC can prevent anchors from being preserved.

Sounds good, I will add this information in the setnetworkactive RPC.

brunoerg commented at 1:07 PM on January 9, 2026: contributor

Force-pushed addressing #34213#pullrequestreview-3636779749

danielabrozzoni commented at 4:39 PM on January 13, 2026: member

Concept ACK. I think this is a good improvement, makes sense to have, and I can’t find any additional surface of attack introduced here. To recap, this is how we use anchors at the moment:

Anchors are used to protect from eclipse attacks. When our node cleanly shutdown, it saves our outbound block relay only connections to the anchors.dat file. The attack this is preventing is: if an attacker can manipulate our addrman to make sure that, on restart, we will only connect to the attacker nodes, and we happen to restart our node, the attacker can eclipse us. Anchors protect us because on restart we would connect to some of our previous block relay only peer, and one honest peer is sufficient for us not to become eclipsed.
We do not persist anchors if we uncleanly shutdown (power outage, software crash, etc.), because if one of our anchor is malicious and figures out a way to crash our node, and on restart we will reconnect to it, the attacker can keep crashing our node. See: #17428 (comment)

I started thinking along the lines of the last bullet point, to see whether the code introduced in this PR could be dangerous in any way, but I couldn’t think of anything. While I couldn’t find a way to exploit the bug this PR is fixing (if an attacker can figure out how to manipulate our bitcoind setting to set networkactive=0, we likely have bigger problems!), I still think it makes sense to fix this, as it’s low risk, and would avoid unnecessarily losing anchors.

However, while reviewing it I started thinking about the anchor logic, and I wonder if we should harden it a bit to make sure that we don’t delete anchors if networkactive is not set, but the user is not connected to the internet. I would appreciate feedback from someone who has thought about this area more deeply than me :)

At the moment, the PR works as intended if we set -networkactive=0, but if we simply disconnect the wifi between restarts, Bitcoin Core is still deleting anchors, similarly to master.

For example, I tried on my own node:

Start the node, keep it running until there’s block only connections, shut it down and check that the anchors.dat file is present
Disconnect internet
Restart the node

From the logs, we can see that the node reads the anchors, can’t connect to them, and at the time of flushing to disk, it writes 0 addresses.

2026-01-13T11:48:49Z Loaded 2 addresses from "anchors.dat"
2026-01-13T11:48:49Z 2 block-relay-only anchors will be tried for connections.
2026-01-13T11:53:26Z DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat started
2026-01-13T11:53:26Z DumpAnchors: Flush 0 outbound block-relay-only peer addresses to anchors.dat completed (0.00s)

The same happens if we start the node, disconnect the wifi, wait for long enough that all the peers are disconnected.

This happens because fNetworkActive only refers to whether -networkactive is set or not.

I think it would be beneficial to fix this edge case, because if an attacker can figure out how to disconnect us from the internet, then we would lose the anchors and be vulnerable to an eclipse attack. Of course, finding a way to disconnect a node from the internet is not an easy task! I suppose an attacker could try with a BGP attack, but I’m not entirely sure. I haven’t looked into how we would implement this, and I’m not sure it’s entirely doable.

brunoerg commented at 12:27 PM on January 14, 2026: contributor

I started thinking along the lines of the last bullet point, to see whether the code introduced in this PR could be dangerous in any way, but I couldn’t think of anything. While I couldn’t find a way to exploit the bug this PR is fixing (if an attacker can figure out how to manipulate our bitcoind setting to set networkactive=0, we likely have bigger problems!), I still think it makes sense to fix this, as it’s low risk, and would avoid unnecessarily losing anchors.

To clarify, I don't think this is exploitable, it is mostly about the own user restarting the node (which happened to me) disabling the network and ending up losing the anchors. Also, since the network is disabled it makes sense to not perform any network activity like this.

waketraindev commented at 6:32 PM on January 14, 2026: contributor

still wish anchors would have been persisted if available when network activity is turned off instead just on exit.

edit: shouldn't anchors be loaded at startup even if networkactive is false and maybe just skip saving them if network is inactive?

With this case both loading and saving will be skipped if node is started with network active false

    if (m_use_addrman_outgoing && fNetworkActive) {
        // Load addresses from anchors.dat
        m_anchors = ReadAnchors(gArgs.GetDataDirNet() / ANCHORS_DATABASE_FILENAME);
        if (m_anchors.size() > MAX_BLOCK_RELAY_ONLY_ANCHORS) {
            m_anchors.resize(MAX_BLOCK_RELAY_ONLY_ANCHORS);
        }
        LogInfo("%i block-relay-only anchors will be tried for connections.\n", m_anchors.size());
    }

How I see it:

Anchors should be loaded on startup regardless if network activity is disabled (it might be turned on after)
Anchors should not be popped back, attempted connection, if network activity is disabled
Anchors should be saved on exit from GetCurrentBlockRelayOnlyConns() if populated or m_anchors if empty

in src/rpc/net.cpp:894 in a52689837b

 889 | @@ -890,7 +890,8 @@ static RPCHelpMan setnetworkactive()
 890 |  {
 891 |      return RPCHelpMan{
 892 |          "setnetworkactive",
 893 | -        "Disable/enable all p2p network activity.\n",
 894 | +        "Disable/enable all p2p network activity.\n"
 895 | +        "Disabling it can prevent anchors from being preserved.\n",

luke-jr commented at 8:26 PM on January 22, 2026:

This seems like a bug... Maybe we should re-save the original anchors if there's no new ones at a clean shutdown? Perhaps update the "original anchors" list just before disabling network activity?

brunoerg commented at 12:36 PM on January 27, 2026:

Maybe we should re-save the original anchors if there's no new ones at a clean shutdown?

I don't think so. What if these anchors are not available anymore? I mean, we tried to connect to them but they are not available anymore.

ajtowns commented at 9:52 AM on March 20, 2026:

I think maintaining m_anchors as something like:

    auto cutoff = now() - 1m;
    vector<CAddress> anchors;
    set<CAddress> old{m_anchors};
    for (const CNode* pnode : m_nodes) {
        if (!pnode->IsBlockOnlyConn()) continue;
        if (!pnode->fSuccessfullyConnected) continue;
        if (pnode->m_connected > cutoff) continue;
        anchors.push_back(pnode->addr);
        old.erase(pnode->addr);
    }
    for (auto& o : old) {
        if (m_nodes.size() >= 2) break;
        anchors.push_back(o);
    }
    m_anchors.swap(anchors);

could work? ie update it with the address of current blocks-only connections that have been working, but if there aren't any (or enough) such connections, keep the old addresses.

brunoerg commented at 6:14 PM on March 20, 2026:

Would it prevent us to not save addresses that we tried to connect to and didn't work?

Perhaps when disabling the network, we could save the anchors (we're connected to) to m_anchors and use it in StopNodes if GetCurrentBlockRelayOnlyConns doesn't return any peer.

sedited requested review from danielabrozzoni on Mar 16, 2026

Bortlesboat commented at 3:01 PM on March 18, 2026: none

ACK a52689837bfa210e4426092e34a5d2c6ea24f351

Ran feature_anchors.py locally — passes on both transports.

DrahtBot requested review from waketraindev on Mar 18, 2026

Bortlesboat commented at 3:04 PM on March 18, 2026: none

Re the open question about whether anchors should be loaded at startup even with -networkactive=0: I think skipping the load is correct. Anchors protect against eclipse attacks on restart — if the node starts with the network off, there is no connection activity to eclipse. If the user later calls setnetworkactive true, the node has its addrman for peer diversity. The anchors.dat file stays on disk, so the next clean restart with network active loads them normally.

l0rinc commented at 3:07 PM on March 18, 2026: contributor

@Bortlesboat, please point your bot to a different repo, this isn't useful...

fanquake commented at 3:08 PM on March 18, 2026: member

cc @willcl-ark given #34486.

sedited removed review request from waketraindev on Mar 19, 2026

sedited requested review from ajtowns on Mar 19, 2026

DrahtBot requested review from waketraindev on Mar 20, 2026

brunoerg force-pushed on Mar 27, 2026

brunoerg renamed this:
~~net: do not read/dump anchors if network is not active~~
net: preserve anchors when network is disabled
on Mar 27, 2026

DrahtBot added the label CI failed on Mar 27, 2026

brunoerg commented at 2:52 PM on March 27, 2026: contributor

I just changed the approach of this PR and changed PR title and description. It addresses concerns like #34213 (review). Now this PR addresses cases involving disabling network via RPC and starting the node with the network disabled via flag.

cc @danielabrozzoni @luke-jr @ajtowns

DrahtBot removed the label CI failed on Mar 27, 2026

in test/functional/feature_anchors.py:140 in 71d3aa1957 outdated

 136 | @@ -137,6 +137,10 @@ def run_test(self):
 137 |              new_data_hash = hash256(new_data)
 138 |              file_handler.write(new_data + new_data_hash)
 139 |  
 140 | +        self.log.info("Check that disabling network does not affect the anchors")

danielabrozzoni commented at 4:09 PM on April 6, 2026:

In 71d3aa1957d42d79e075c3380b73f3c7bae37f2e: I think this test should be moved in the next commit (a6d411664e967b4fa06c4a6249e492b9185766b9, "test: disabling network activity should not affect anchors").

Both test additions are testing networkactive=0, this one having -networkactive=0 passed in as a bitcoind argument, and the other one calling self.nodes[0].setnetworkactive(False). Can we move these two closer to each other? This way it's clear that we're testing both ways of setting the network to inactive.

brunoerg commented at 4:42 PM on April 13, 2026:

Done. I think it makes sense to have both tests together.

in src/net.cpp:2718 in dce37280c3 outdated

2714 | @@ -2715,7 +2715,8 @@ void CConnman::ThreadOpenConnections(const std::vector<std::string> connect, std
2715 |          // block-relay-only peer (to confirm our tip is current, see below) or the next_feeler
2716 |          // timer to decide if we should open a FEELER.
2717 |  
2718 | -        if (!m_anchors.empty() && (nOutboundBlockRelay < m_max_outbound_block_relay)) {
2719 | +        const bool have_anchors{WITH_LOCK(m_anchors_mutex, return !m_anchors.empty())};

danielabrozzoni commented at 4:12 PM on April 6, 2026:

In dce37280c3a2cdebf4a97c99b67fba110b2b84f2: I think this commit should be the first one, so in the git history we never access m_anchors from multiple threads without having a mutex.

danielabrozzoni commented at 3:29 PM on April 10, 2026:

Same as other comments, you should add a AssertLockNotHeld at the start of ThreadOpenConnections

in src/net.cpp:3368 in 9a7c4970cd

3363 | @@ -3368,6 +3364,10 @@ void CConnman::SetNetworkActive(bool active)
3364 |  
3365 |      fNetworkActive = active;
3366 |  
3367 | +    if (!fNetworkActive) {
3368 | +        m_anchors = GetCurrentBlockRelayOnlyConns();

danielabrozzoni commented at 11:15 AM on April 10, 2026:

Right now m_anchors is used for:

keeping track of the anchors that were in the file upon startup and we haven't connected to yet or
keeping track of the anchors we were connected to when we disable the network

The docs of m_anchors should be updated to reflect this.

Also, it would make sense here to append to m_anchors, instead of substituting (and then resizing m_anchors so it's not bigger than MAX_BLOCK_RELAY_ONLY_CONNECTIONS). I think it's a bit cleaner, because we wouldn't lose any information about anchors that were in the file, and we hadn't connected to yet.

In this way, point 2 would become:

"keeping track of the anchors we were connected to and were about to connect to when we disable the network"

Right now, this could happen:

we start our node, it reads 2 anchors from the file, anchor1 and anchor2, puts them in m_anchors
the node starts our first block only connection, pops anchor1 from m_anchors
we set network active to 0: this saves anchor1 to m_anchors, anchor2 is forgotten.

brunoerg commented at 4:45 PM on April 13, 2026:

I've changed the documentation of m_anchors according to the new behavior - it is now contains addresses saved from previous clean shutdown or when the network was deactivated.

Also, it would make sense here to append to m_anchors, instead of substituting (and then resizing m_anchors so it's not bigger than MAX_BLOCK_RELAY_ONLY_CONNECTIONS). I think it's a bit cleaner, because we wouldn't lose any information about anchors that were in the file, and we hadn't connected to yet.

Left this as is for now, tried to not change too much. But happy to change it whether I have to touch it again.

in src/net.cpp:3671 in 3aa8484020

3668 | +                LOCK(m_anchors_mutex);
3669 | +                if (!fNetworkActive && !m_anchors.empty()) {
3670 | +                    anchors_to_dump = m_anchors;
3671 | +                }
3672 | +            }
3673 | +            if (fNetworkActive || !anchors_to_dump.empty()) {

danielabrozzoni commented at 1:51 PM on April 10, 2026:

nit: can be if(!anchors_to_dump.empty()): even if the network is active, but anchors_to_dump is empty, you can skip the dump

in src/net.cpp:3365 in 9a7c4970cd

3363 | @@ -3368,6 +3364,10 @@ void CConnman::SetNetworkActive(bool active)
3364 |  
3365 |      fNetworkActive = active;

danielabrozzoni commented at 2:06 PM on April 10, 2026:

claude code caught this, but I verified manually and I think it's correct: currently there is a data race, you should first save the anchors, and only then set fNetworkActive = active, like this:

if (!active) {
    auto anchors = GetCurrentBlockRelayOnlyConns();
    LOCK(m_anchors_mutex);
    m_anchors = std::move(anchors);
}
fNetworkActive = active;  // flip AFTER capturing

The reason is: right now you set fNetworkActive to false, then call GetCurrentBlockRelayOnlyConns(). Between these two steps there is a small window where ThreadSocketHandler might call DisconnectNodes, which will lock m_nodes_mutex and disconnect all nodes if the network is not active, and clear m_nodes:

https://github.com/bitcoin/bitcoin/blob/58dccd27e11de68f810145fc30631a2c446f3bf5/src/net.cpp#L1923-L1944

GetCurrentBlockRelayOnlyConns needs a lock on m_nodes_mutex, so if DisconnectNodes runs first, it will have already marked all nodes as fDisconnect = true and removed them from m_nodes. By the time GetCurrentBlockRelayOnlyConns acquires the lock, m_nodes is empty and it returns nothing.

brunoerg commented at 4:45 PM on April 13, 2026:

Nice find, going to address it.

in src/net.cpp:3664 in dce37280c3 outdated

3659 | @@ -3648,8 +3660,11 @@ void CConnman::StopNodes()
3660 |          if (m_use_addrman_outgoing) {
3661 |              // Anchor connections are only dumped during clean shutdown.
3662 |              std::vector<CAddress> anchors_to_dump = GetCurrentBlockRelayOnlyConns();
3663 | -            if (!fNetworkActive && !m_anchors.empty()) {
3664 | -                anchors_to_dump = m_anchors;
3665 | +            {
3666 | +                LOCK(m_anchors_mutex);

danielabrozzoni commented at 3:27 PM on April 10, 2026:

You should add a AssertLockNotHeld at the start of the function, similarly to AssertLockNotHeld(m_reconnections_mutex);

danielabrozzoni commented at 3:30 PM on April 10, 2026: member

Review up to 3aa8484020

Thanks for updating! This is shaping up nicely, I left a couple of comments

brunoerg force-pushed on Apr 13, 2026

DrahtBot added the label CI failed on Apr 13, 2026

brunoerg commented at 4:54 PM on April 13, 2026: contributor

Thanks, @danielabrozzoni for reviewing. Force-pushed - major changes:

Updated m_anchors documentation to reflect the new behavior;
Added missing AssertLockNotHeld for m_anchors;
Fixed data race when disabling the network and saving the anchors.

net: guard m_anchors with mutex

m_anchors can be read and written from different threads.
Introduce m_anchors_mutex, guard the vector with GUARDED_BY,
and update EXCLUSIVE_LOCKS_REQUIRED annotations on all callers.

50cc19d625

net: store anchors in SetNetworkActive

When disabling the network activity, store anchors on m_anchors.

2826ef66e0

net: preserve anchors on shutdown when network is inactive

During StopNodes(), anchors are collected via GetCurrentBlockRelayOnlyConns().
When the network has been deactivated, that call returns an empty list since
there are no active connections, causing anchors.dat to be overwritten with no
entries and losing the anchors that were saved at deactivation time (by
SetNetworkActive).

Fix this by falling back to m_anchors as the source when the network is
inactive and m_anchors is non-empty. Also skip DumpAnchors entirely if the
network is inactive and there is nothing to write, avoiding unnecessary
file I/O.

bf0425fbad

test: disabling network activity should not affect anchors 874378f9eb

net: log that anchors will be tried for connections only if network is active 133cfae47d

brunoerg force-pushed on Apr 13, 2026

DrahtBot removed the label CI failed on Apr 13, 2026