p2p: Track orphans by who provided them #26551

Should become AssertLockNotHeld? :)

Seems more reasonable to continue to allow invoking ProcessOrphanTx with cs_main held, even though we could forbid it. (Before the "only process orphans before messages" commit, ProcessOrphanTx() was called from ProcessMessage() with cs_main held)

1. I think it should say "Creates more work if", as it's less ambiguous (I actually spent time figuring the difference out...).
2. So now bool fMoreNodeWork = m_msgproc->ProcessMessages(pnode, flagInterruptMsgProc); should be renamed to may_have_more_work right?

I don't think this PR changes whether fMoreNodeWork should be renamed -- the bool should have the same properties both before and after this PR.

But... I think its current behaviour is technically not quite right? If we process a TX message in ProcessMessage now, we'll set fMoreWork = true only when m_getdata_requests has entries, but I think we should also be doing it if the peer's orphanage work queue isn't empty, as ProcessMessage may have added to it. I think this is technically a bug introduced in #15644 -- there should have been an if (!pfrom->orphan_work_set.empty()) fMoreWork = true; added after the ProcessMessage() call, as well as after the // this maintains the order of responses comment.

The only result of the bug is that when a parent comes in, processing its orphaned children will be delayed by 100ms rather than occurring essentially immediately.

Maybe it would be better to write:

while (true) {
    bool all_nodes_finished_processing = true;
    for (pnode : nodes) {
        if (m_msgproc->ProcessMessages(pnode)) all_nodes_finished_processing = false;
        m_msgproc->SendMessages(pnode);
    }
    if (all_nodes_finished_processing) wait_for(100ms);
}

The idea (as I understand it) is just that ProcessMessages() returns true if it does some substantial work and exits early, without checking everything. If that's the case for any peer, then it doesn't wait before looping through all peers again. If ProcessMessages() completed for all peers, whether it did work or not, then it is okay to sleep briefly before looping over everything again.

I think you are right. Let's try applying these changes and see whether everything makes sense once again.

Part of my confusion is that it's unclear whether has_more_work applies to a given peer, or to all peers... That variable rename could help. (peer_has_more_work vs node_has_more_work or something.)

The doc for GetTxToReconsider needs an update after both more and originator are removed.

nit for 10de619ba2bc48de6816b3940509576c575e9ff3 typo "indidicate" in commit message

This doesn't work after the other changes in this PR -- you might be creating more work for some other peer, so should be checking if any peers have txs to reconsider, and only doing this once after looping through all the peers.

So, I guess this raises the question: should we move orphan processing outside of ProcessMessages and SendMessages entirely, and just do it separately, independent of both who sent the orphan and who sent the parent?

Unless someone has a good reason not to, I think I might draft that up as an alternative PR so the approaches can be compared more directly.

should we move orphan processing outside of ProcessMessages and SendMessages entirely, and just do it separately, independent of both who sent the orphan and who sent the parent?

Had a similar thought, mainly I don't see why we should try to process all orphans before moving on to ProcessMessage()... afaict we never send a message to a peer as a result of processing an orphan (other than potentially disconnecting) so maintaining the order of responses is not justification like it is for finishing ProcessGetData() queue.

fwiw doing orphan processing independently seems reasonable to me, though I don't have a very strong grasp of all the possible implications.

I thought of it more along the lines of "if a node gives us a lot of orphans to process, and we're now processing them, we shouldn't do more work for that node until all the orphan work is done; also we shouldn't delay work from other nodes while we're processing that node's orphans".

So, I guess this raises the question: should we move orphan processing outside of ProcessMessages and SendMessages entirely,

Where do you intend to move it? Into a dedicated thread for orphan processing?

This doesn't work after the other changes in this PR -- you might be creating more work for some other peer, so should be checking if any peers have txs to reconsider, and only doing this once after looping through all the peers.

Is this situation (that the peer who sent us an orphan is different from the peer that sent us the parent) typical? Otherwise might it be acceptable to just have a unnecessary 100ms wait if it occurs very rarely?

Where do you intend to move it? Into a dedicated thread for orphan processing?

No, I was thinking a separate ProcessOrphans method like ProcessMessages and SendMessages.

Is this situation (that the peer who sent us an orphan is different from the peer that sent us the parent) typical? Otherwise might it be acceptable to just have a unnecessary 100ms wait if it occurs very rarely?

On a long running node, I'm seeing maybe a few hundred orphans being accepted during startup (ie, children whose parent txs were announced while my node was offline) but then perhaps only 20-60 accepted orphans per day. I don't have much of an idea how many of those would have a different node sending the parent compared to the child, but it doesn't seem likely to be a real problem.

should we move orphan processing outside of ProcessMessages and SendMessages entirely, and just do it separately, independent of both who sent the orphan and who sent the parent?

Unless someone has a good reason not to, I think I might draft that up as an alternative PR so the approaches can be compared more directly.

See #19364

048bfe16795e2da18c7bb5bf1137e4370079dc33 nit: does "with and whether" make any sense? Not for me, so might be a typo.

fixed

c58c249a5b694c88122589fedbef4e2f13f08bb4 nit: s/has/have

nit: I think "Generally only one orphan will be reconsidered" is misleading. Calling ProcessTransaction for a tx once more, getting TX_MISSING_INPUTS again and therefore keeping the orphan would qualify as being "reconsidered" for me, whereas what is meant is that the tx is being removed from the orphanage. But no need to address here, since this PR didn't change this.

nit: double ;