The peerLifecycle channel is sized cfg.MaxPeers*2 because each
peer can emit two lifecycle events (peerAdd followed by peerDone).
Without an inline comment a future reader could "simplify" the
factor to cfg.MaxPeers and tighten back-pressure under normal
connect/disconnect churn, causing peerLifecycleHandler goroutines
to block.
peer.processRemoteVerAckMsg sets verAckReceived=true *before* it
invokes the OnVerAck listener that closes verAckCh. If the peer
disconnects in that window, the peerLifecycleHandler select may
pick Peer.Done() over verAckCh and skip the peerAdd send -- yet
VerAckReceived() still returns true. handleDonePeerMsg would then
call syncManager.DonePeer for a peer that was never NewPeer'd
(sync manager logs a warning and discards).
Add an explicit peerAdded atomic.Bool on serverPeer. Set it in
peerLifecycleHandler after peerAdd has been enqueued. Gate the
syncManager.DonePeer + orphan eviction in handleDonePeerMsg on
peerAdded.Load() so those side effects only fire for peers that
were actually registered.
This tightens the "peerAdd precedes peerDone, or peerAdd is
skipped entirely" invariant from this PR into something explicit
rather than something derived from a flag that races OnVerAck.
The prior select/default+close() guard on verAckCh is correct only
under the invariant that OnVerAck is invoked from a single goroutine
(peer.processRemoteVerAckMsg on the input handler). The two steps
are not atomic: any future change that invokes listeners off the
input goroutine would let two concurrent callers both observe
default and panic on double-close.
Replace the guard with sync.Once. This makes the close-once
contract obviously correct rather than correct-by-distant-invariant
and drops the dead "called more than once" log path.
Prioritize verAckCh in peerLifecycleHandler select to avoid
nondeterministic peerAdd skipping when both channels are ready.
Guard OnVerAck against double-close by checking the channel before
closing, logging an error instead of panicking.
Adjust peerLifecycleEvent comment to reflect that peerAdd may be
skipped when the peer disconnects before or concurrently with verack.
Fix verAckCh field comment formatting.
Address review feedback on the peer add/done race fix:
- Make peerLifecycleHandler (renamed from peerDoneHandler) the sole
sender of both peerAdd and peerDone events for each peer. OnVerAck
now closes a signal channel (verAckCh) instead of sending directly,
and peerLifecycleHandler selects on verAckCh vs peer.Done() to
decide whether to send peerAdd before peerDone. This guarantees
ordering by construction: a single goroutine sends both events
sequentially, eliminating the negotiateTimeout race window.
- Add Done() method to peer.Peer exposing the quit channel read-only,
enabling select-based disconnect detection from server code.
- Remove the now-unused AddPeer method.
- Address style feedback: 80-char line limit, empty lines between
switch cases, break long function calls, use require.GreaterOrEqualf
instead of if+Fatalf, bump syncRaceConcurrency to 300 for
backpressure testing, add TestPreVerackDisconnect for disconnect
prior to verack.
peerDoneHandler ran as a separate goroutine per peer and independently
notified both peerHandler (via donePeers channel) and the sync manager
(via syncManager.DonePeer) about a peer disconnect. Because these two
sends were unsynchronized, the sync manager could observe DonePeer
before NewPeer when a peer connected and disconnected quickly. This
caused the sync manager to log "unknown peer", then later register the
already-dead peer as a sync candidate that was never cleaned up,
potentially leaving it stuck with a dead sync peer.
Two structural changes eliminate the race:
1. Merge the newPeers and donePeers channels into a single
peerLifecycle channel. Since OnVerAck (add) always fires before
WaitForDisconnect returns (done), a single FIFO channel guarantees
peerHandler always processes add before done for a given peer,
removing the select-ambiguity where Go could pick done first.
2. Move the syncManager.DonePeer call and orphan eviction from
peerDoneHandler into handleDonePeerMsg, which runs inside
peerHandler. All sync manager peer lifecycle notifications now
originate from the single peerHandler goroutine and flow into
sm.msgChan in guaranteed add-before-done order.
The preivous usage of two channels - by piping them together to create a
semaphore effect, is difficult to follow and prone to bugs. This commit
now refactors the method to explicitly implement a semaphore.
Prior to this change, we would allow at max 3 concurrent goroutines -
this is now bumped to 5.
In this commit, we refactor the v2 -> v1 downgrade logic to: simplify
the code in the server for reconnection, use a functional option to
avoid breaking the connmgr API, and we also encapsulate the downgrade
state in a new struct so it can be re-used elsewhere.
When ShouldDowngradeToV1() returns true, we'll:
- mark in pendingReconnects that we should attempt v1 bitcoin p2p transport
when the connmgr successfully re-establishes the outbound TCP connection.
- set a bit on the associated *serverPeer that will cause us to tell the
connmgr to reconnect. This is needed because not all outbound peers are
permanent and we might otherwise not reconnect to them if the initial v2
transport negotiation failed.
commit 0b2998b7f279d3aef4d83415dae26948f5a6bdf4
Author: cec489 <173723251+cec489@users.noreply.github.com>
Date: Mon Jun 24 20:01:13 2024 +0000
A cleaner fix is to set the startTime in the server Start() function
which is where the server is actually started.
commit ae6c1256981befb43972e83a086ea663df629873
Author: cec489 <173723251+cec489@users.noreply.github.com>
Date: Mon Jun 24 19:15:23 2024 +0000
Fix the btcctl uptime command by moving the setting of startupTime
This change is part of the effort to add utxocache support to btcd.
utxo cache is now used by the BlockChain struct. By default it's used
and the minimum cache is set to 250MiB. The change made helps speed up
block/tx validation as the cache allows for much faster lookup of utxos.
The initial block download in particular is improved as the db i/o
bottleneck is remedied by the cache.
This change is part of the effort to add pruning support to btcd.
Pruning is now available to the end user via --prune flag. There are
checks in place so that the user doesn't go below the minimum prune
target of 1536 MiB. The minimum is set so that we keep at least 288
blocks per the requirement for NODE_NETWORK_LIMITED nodes specified by
BIP0159. The default value of 0 will disable pruning.
This change is part of the effort to add pruning support to btcd.
Wire now supports the ability to signal NODE_NETWORK_LIMITED which
signals to peers that the node is able to serve the last 288 blocks.
Since archival nodes have all blocks, they can also signal for
NODE_NETWORK_LIMITED. SFNodeNetworkLimited flag is added to the default
services.
In this commit, we update all the btcutil imports to point to the new
sub-module.
In the same commit, we also modify the recently added `btcutil/go.mod`
file as we need to continue pointing to the _old_ version of btcd, until
we merge this PR and push a new tag.
In this commit, we add a new config options that allows one to start
`btcd` in an operating mode that disables the stall detection. This can
be useful in simnet/regtest integration tests settings where it's
important that `btcd` holds on to its possibly sole connection to the
only other node in the test harness.
A new config flag has been added to gate this behavior, which is off by
default.
backport from https://github.com/decred/dcrd/pull/2253
When a peer sends a notfound message, remove the hash from requested
map. Also increase notfound ban score and return early if it
disconnects the peer.
This addresses an issue where the server ends up tracking a peer that
has been disconnected due to it processing a peer's `done` message
before its `add` message.
This was previously done within the OnVersion listener, which should not
be a blocking operation. It turns out that requesting these messages
there can lead to blocking due to peers not being able to process
messages since their message queues have yet to start. Therefore, we'll
now request within handleAddPeerMsg, which should allow it to go
through.
This change is needed as part of requiring peers to also send a verack
message following their version message during protocol negotiation.
Peers were previously added to the SyncManager before their message
queues were started, causing the server to stall if a peer didn't
provide a timely verack response following their version. We now do this
within OnVerAck, which happens shortly before peer message queues are
started.
This makes the logic a bit more unified as previously it was possible we
for us to report the new peer to the SyncManager, but for whatever
reason failed to track the peer in the server internally within AddPeer.
This change ensures this can no longer happen.
Doing so ensures we reach our target number of outbound peers as soon as
possible. This is only necessary after calls to connmgr.Remove, as these
won't request a new peer connection.
The Disconnect method would still attempt to reconnect to the same
peer, which could cause us to reconnect to bad/unstable peers if we came
across them. Instead, we'll now use Remove whenever we intend to remove
a peer that is not persistent.
We do this to ensure the address manager contains live addresses.
Previously, addresses with which we established connections with would
not be marked as connected because it would be done once we disconnect
peers. Given that we don't process all of the disconnect logic when
we're shutting down, addresses of stable and good peers would never be
marked as connected unless the connection was lost during operation.
This modifies the OnVersion handler for server peers to use a local
variable for the remote address of the peer in order to avoid grabbing
the mutex multiple times.
There are no functional changes.
Backported from Decred.
