Skip to content

p2p: Batch send NOTFOUND messages #26486

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

p2p: Batch send NOTFOUND messages #26486

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 38 additions & 24 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ static constexpr auto NONPREF_PEER_TX_DELAY{2s};
static constexpr auto OVERLOADED_PEER_TX_DELAY{2s};
/** How long to wait before downloading a transaction from an additional peer */
static constexpr auto GETDATA_TX_INTERVAL{60s};
/** Limit to avoid sending big packets. Not used in processing incoming GETDATA for compatibility */
/** Limit to avoid sending big packets (getdata, notfound). Not used in processing incoming GETDATA for compatibility */
static const unsigned int MAX_GETDATA_SZ = 1000;
/** Number of blocks that can be requested at any given time from a single peer. */
static const int MAX_BLOCKS_IN_TRANSIT_PER_PEER = 16;
Expand Down Expand Up @@ -316,6 +316,10 @@ struct Peer {

/** A vector of addresses to send to the peer, limited to MAX_ADDR_TO_SEND. */
std::vector<CAddress> m_addrs_to_send GUARDED_BY(NetEventsInterface::g_msgproc_mutex);

/** A vector of notfounds to send to the peer, limited to MAX_PEER_TX_ANNOUNCEMENTS. */
std::vector<CInv> m_notfounds_to_send GUARDED_BY(NetEventsInterface::g_msgproc_mutex);

/** Probabilistic filter to track recent addr messages relayed with this
* peer. Used to avoid relaying redundant addresses to this peer.
*
Expand Down Expand Up @@ -890,7 +894,7 @@ class PeerManagerImpl final : public PeerManager
CTransactionRef FindTxForGetData(const CNode& peer, const GenTxid& gtxid, const std::chrono::seconds mempool_req, const std::chrono::seconds now) LOCKS_EXCLUDED(cs_main);

void ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic<bool>& interruptMsgProc)
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, peer.m_getdata_requests_mutex) LOCKS_EXCLUDED(::cs_main);
EXCLUSIVE_LOCKS_REQUIRED(!m_most_recent_block_mutex, peer.m_getdata_requests_mutex, g_msgproc_mutex) LOCKS_EXCLUDED(::cs_main);

/** Process a new block. Perform any post-processing housekeeping */
void ProcessBlock(CNode& node, const std::shared_ptr<const CBlock>& block, bool force_processing, bool min_pow_checked);
Expand Down Expand Up @@ -1008,6 +1012,7 @@ class PeerManagerImpl final : public PeerManager

void AddAddressKnown(Peer& peer, const CAddress& addr) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
void PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& insecure_rand) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
void PushNotFound(Peer& peer, CNode& node, const CInv& inv) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
};

const CNodeState* PeerManagerImpl::State(NodeId pnode) const EXCLUSIVE_LOCKS_REQUIRED(cs_main)
Expand Down Expand Up @@ -1039,6 +1044,16 @@ void PeerManagerImpl::AddAddressKnown(Peer& peer, const CAddress& addr)
peer.m_addr_known->insert(addr.GetKey());
}

void PeerManagerImpl::PushNotFound(Peer& peer, CNode& node, const CInv& inv)
{
if (peer.m_notfounds_to_send.size() >= MAX_GETDATA_SZ) {
const CNetMsgMaker msg_maker(node.GetCommonVersion());
m_connman.PushMessage(&node, msg_maker.Make(NetMsgType::NOTFOUND, peer.m_notfounds_to_send));
peer.m_notfounds_to_send.clear();
}
peer.m_notfounds_to_send.push_back(inv);
}

void PeerManagerImpl::PushAddress(Peer& peer, const CAddress& addr, FastRandomContext& insecure_rand)
{
// Known checking here is only to save space from duplicates.
Expand Down Expand Up @@ -2250,7 +2265,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
auto tx_relay = peer.GetTxRelay();

std::deque<CInv>::iterator it = peer.m_getdata_requests.begin();
std::vector<CInv> vNotFound;
const CNetMsgMaker msgMaker(pfrom.GetCommonVersion());

const auto now{GetTime<std::chrono::seconds>()};
Expand Down Expand Up @@ -2303,7 +2317,21 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
}
}
} else {
vNotFound.push_back(inv);
// Let the peer know that we didn't find what it asked for, so it doesn't
// have to wait around forever.
// SPV clients care about this message: it's needed when they are
// recursively walking the dependencies of relevant unconfirmed
// transactions. SPV clients want to do that because they want to know
// about (and store and rebroadcast and risk analyze) the dependencies
// of transactions relevant to them, without having to download the
// entire memory pool.
// Also, other nodes can use these messages to automatically request a
// transaction from some other peer that annnounced it, and stop
// waiting for us to respond.
// In normal operation, we often send NOTFOUND messages for parents of
// transactions that we relay; if a peer is missing a parent, they may
// assume we have them and request the parents from us.
PushNotFound(peer, pfrom, inv);
}
}

Expand All @@ -2319,24 +2347,6 @@ void PeerManagerImpl::ProcessGetData(CNode& pfrom, Peer& peer, const std::atomic
}

peer.m_getdata_requests.erase(peer.m_getdata_requests.begin(), it);

if (!vNotFound.empty()) {
// Let the peer know that we didn't find what it asked for, so it doesn't
// have to wait around forever.
// SPV clients care about this message: it's needed when they are
// recursively walking the dependencies of relevant unconfirmed
// transactions. SPV clients want to do that because they want to know
// about (and store and rebroadcast and risk analyze) the dependencies
// of transactions relevant to them, without having to download the
// entire memory pool.
// Also, other nodes can use these messages to automatically request a
// transaction from some other peer that annnounced it, and stop
// waiting for us to respond.
// In normal operation, we often send NOTFOUND messages for parents of
// transactions that we relay; if a peer is missing a parent, they may
// assume we have them and request the parents from us.
m_connman.PushMessage(&pfrom, msgMaker.Make(NetMsgType::NOTFOUND, vNotFound));
}
}

uint32_t PeerManagerImpl::GetFetchFlags(const Peer& peer) const
Expand Down Expand Up @@ -5820,10 +5830,14 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
m_txrequest.ForgetTxHash(gtxid.GetHash());
}
}


if (!vGetData.empty())
m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::GETDATA, vGetData));

// Message: notfound
if (!peer->m_notfounds_to_send.empty()) {
m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::NOTFOUND, peer->m_notfounds_to_send));
peer->m_notfounds_to_send.clear();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain why it is safe to change the order of responses? From a first impression, I think it is possible that a peer sends us a getdata [1], then a ping, then they would receive the pong first, and then the notfound, no? Previously they should be in order.

If this is safe to change, it might be good to explain. Also, then it would be better to move this out of the cs_main scope here, because the lock is not needed.

If it is not safe to change, I guess you can just move the send to the process function?

[1] This should happen intermittently whenever there is fPauseSend backpressure, or you can force it deterministically by sending an unknown getdata type.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We already have out of order notfounds: if you request getdata [a,b,c] and b is known but a and c aren't; we'll send back tx b; notfound [a,c]. Having a pong interspersed as well doesn't seem like it makes a big difference?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • agree that this code can be moved out of cs_main
  • I spent some time, and can't think of any specific circumstances that would cause this change in ordering to be problematic in normal node operations (which is not to say its necessarily fine)
  • however, one question that came to mind is if this could negatively impact timing in the tests because of the expectations from the sync_with_ping function. however, with AJ's patch, any test covering NOTFOUND behavior would require some time delay. so this shouldn’t be an issue.

} // release cs_main
MaybeSendFeefilter(*pto, *peer, current_time);
return true;
Expand Down