Skip to content
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.

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

[p2p] Request NOTFOUND transactions immediately from other outbound peers, when possible #15505

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@sdaftuar
Copy link
Member

commented Feb 28, 2019

Currently when a peer asks for a transaction that we cannot (or will not) deliver, we send a NOTFOUND message for the given txid. However, our software currently ignores the NOTFOUND messages we receive from others -- if other peers have announced a transaction, we wait until the transaction request times out before requesting from our next peer.

Improve this by immediately requesting the transaction from one of the outbound peers who have recently announced it (if any).

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Feb 28, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16279 (Return the AcceptBlock CValidationState directly in ProcessNewBlock by TheBlueMatt)
  • #16197 (net: Use mockable time for tx download by MarcoFalke)
  • #14032 (Add p2p layer encryption with ECDH/ChaCha20Poly1305 by jonasschnelli)

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.

g_outbound_peers[peer_loc] = g_outbound_peers.back();
}
g_outbound_peers.pop_back();
}

This comment has been minimized.

Copy link
@jamesob

jamesob Feb 28, 2019

Member

Seems like this could be a lot more succinct with vector::erase - is there some reason to do it this way?

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Mar 1, 2019

Author Member

I guess I have an aversion to vector::erase because it requires moving all the other entries. Possibly that concern doesn't make sense for such a small vector?

But the bigger issue here is why this is a vector at all -- originally I had this as a set, but then I changed it to be a vector in order to make the logic around when to drain the notfound queue for each peer make sense. In short, I wanted to uniformly at random assign a notfound transaction to an outbound peer that announced it, but I didn't have a good way to do that, so instead I assign it to all peers, but then have a (hacky) counter in SendMessages and each outbound peer will check that counter to decide when to drain its notfound queue.

(I'm hoping that reviewers will give me a better idea of how to implement that logic than what I've got so far...)

Show resolved Hide resolved src/net_processing.cpp Outdated

@fanquake fanquake added the P2P label Feb 28, 2019

Show resolved Hide resolved src/net_processing.cpp Outdated
Show resolved Hide resolved src/net_processing.cpp Outdated
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

Thanks for the nits; I'll address them later, but in the meantime I'm open to suggestions on how to better implement the random assignment of notfound transactions to the outbound peers that have announced them (I think the logic I have here should work, but it's a bit gross).

@jamesob
Copy link
Member

left a comment

Feel free to ignore the nits, but curious what you think about the use of GetRandInt in lieu of the vector hackery.

Show resolved Hide resolved src/net_processing.cpp Outdated
vRecv >> vInv;
if (vInv.size() <= MAX_INV_SZ) {
for (CInv &inv : vInv) {
if (inv.type == MSG_TX || inv.type == MSG_WITNESS_TX) {

This comment has been minimized.

Copy link
@jamesob

jamesob Mar 1, 2019

Member

This is just a style/readability thing, but you can avoid indenting the next ~23 lines by changing this to

if (inv.type != MSG_TX && inv.type != MSG_WITNESS_TX) {
    continue;
}

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Mar 1, 2019

Author Member

Will leave as-is -- in this case I like being able to tell that the code block exactly matches the if condition, rather than matching the opposite of an if condition above it that would have caused the loop iteration to be skipped.

@@ -3081,8 +3156,46 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
}

if (strCommand == NetMsgType::NOTFOUND) {

This comment has been minimized.

Copy link
@jamesob

jamesob Mar 1, 2019

Member

Aid for fellow reviewers: NOTFOUND messages are assembled here and their structure exactly resembles INVs, which is why we can deserialize using cInv below.

Show resolved Hide resolved src/net_processing.cpp Outdated
Show resolved Hide resolved src/net_processing.cpp Outdated

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-02-notfound-requests branch from 0f4ceef to 09d0750 Mar 1, 2019

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

@jamesob Thanks for the helpful suggestions. I rewrote this with g_outbound_peers being a set, and moving the global sequence number needed for peer selection to be in the interface between net and net_processing. (It seems to me like the idea of having synchronization between loop iterations in ThreadMessageHandler is something that is occasionally useful in net_processing, so I hope the interface change there is acceptable.)

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-02-notfound-requests branch from 09d0750 to f5dbb49 Mar 1, 2019

@jamesob
Copy link
Member

left a comment

Looks good! I'll write up a related functional test this weekend unless you have a burning desire to do so.

@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

I must be misunderstanding something.

Imagine we have 2 peers:
[p1, p2]
Start with seq_num = 1

SendMessages(p1, 1 % 2 = 1): false
SendMessages(p2, 1 % 2 = 1): false

SendMessages(p1, 2 % 2 = 0): true
SendMessages(p2, 2 % 2 = 0): true

So, p1 will be always called before p2 (and much likely to request something from p1)?

@jamesob

This comment has been minimized.

Copy link
Member

commented Mar 1, 2019

@naumenkogs it's not the result of the modulus that is evaluated to determine whether to notfound flush for a given peer; sequence_number % num_outbounds is used as an index into the set of outbounds which then determines the peer whose turn it is to flush.

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Mar 1, 2019

Let's say we have 2 peers, p1 and p2 in g_outbound_peers, and let's assume the std::set comparator puts them in that order when iterating through g_outbound_peers. Let's also say both of them have something in their respective notfound queues.

When the sequence number is 1, then 1 % 2 == 1, so we look at g_outbound_peers.begin() + 1, and we get p2's nodeid. When we call SendMessage on p1, the nodeid will not match, so its queue will not be drained. When we call SendMessages on p2, the condition will be true, and we'll drain the notfound queue. On the next loop iteration, the conditions flip.

(I'm open to simpler logic that accomplishes the same thing!)

@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Mar 1, 2019

@sdaftuar @jamesob I see, you're right, thanks.

I was not a big fan of extra sequence_numbers, but based on the discussion above and some thinking I don't see a cleaner way to select a peer in a fair way. This sequence_number is also likely to be useful somewhere else in future.

utACK

@jamesob

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

utACK f5dbb49

I was going to write a functional test for this, but I'm not sure there's a sensible way to do that given that it looks like we can't force P2PDataStore clients to be outbound connections for nodes under test. (see also: #14210)

If anyone can think of a good manual test plan, I'm happy to carry it out.

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Closing for now.

@sdaftuar sdaftuar closed this Jun 5, 2019

@sdaftuar sdaftuar reopened this Jun 12, 2019

sdaftuar added some commits Mar 1, 2019

[net] Add a global sequence number to SendMessages()
Keep an incrementing counter between loop iterations in ThreadMessageHandler.

This allows for synchronization between loop iterations in SendMessages.
Request NOTFOUND transactions immediately
Thanks to James O'Beirne for some of the code in this patch.

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-02-notfound-requests branch from f5dbb49 to 7538835 Jun 16, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jun 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.