Replace global g_cs_orphans lock with local #26295

Is it intentional to split the annotations on two different lines?

It wasn't, I just keep not noticing it. Fixed.

89e2e0da0bcd0b0757d7b42907e2d2214da9f68b

Add AssertLockHeld(g_msgproc_mutex); into implementation code?

    std::optional<TxToReconsider> GetTxToReconsider(NodeId peer) EXCLUSIVE_LOCKS_REQUIRED(!m_mutex);

With TxToReconsider being:

struct TxToReconsider {
    const CTransactionRef tx;
    const NodeId originator;
    const bool more;
};

That would seem a little cleaner to me because originator and more only have meaning if the returned transaction is not nullptr, so bundling the three would make sense imo.

Was going to comment the same thing, this seems like a better interface given that we only want originator/more if there is a tx to work on, and generally avoiding in-out params is cleaner. (non-blocking)

There was some other followup ideas in the previous PR, particularly #21527 (review) -- the idea there is that if Alice sends orphan tx Y which has a missing parent X, instead of immediately processing Y when X arrives (which might have arrived from Bob), just put it in Alice's workset, and delay processing of the orphan until it's Alice's turn. Then you'd have peer == originator and not need it to be returned as a result. At least last time I looked at this (the 202104-whohandlesorphans branch), it ended up simplifying the function to just CTransactionRef GetTxToReconsider(NodeId peer) (using nullptr as the "there's nothing to consider" indicator).

Opened this as a draft at #26551

Just wanted to note that you moved this check above the m_getdata_requests.empty() check. Any reason behind that? (Don't see how this changes anything, so feel free to just mark as resolved if there was no reason.)

It avoids taking the m_getdata_requests_mutex without changing the behaviour otherwise.

    // Get this peer's work set, emplacing an empty set if it didn't exist

Comment updated in #26551

After the work set is moved to the orphanage, ProcessOrphanTx really only needs the id of the peer not the entire Peer.

Maybe pass NodeId to ProcessOrphanTx instead of Peer in 9910ed755c3dfd7669707b44d993a20030dd7477 and have it look up the needed Peer.

If you already have a Peer, I don't think there's really any reason not to pass in.

I used to be confused about the different lock granularity for these methods. Thanks for moving all the locks inside the function. But can we use LOCKS_EXCLUDED(m_mutex) as this line used to be instead of EXCLUSIVE_LOCKS_REQUIRED(!m_mutex) for these methods? see https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#excludes

Using a negative annotation (EXCLUSIVE_LOCKS_REQUIRED(!foo)) is preferable in order to avoid "false negatives in some cases" that that link warns about (ie, the compiler not giving warnings about code that could result in double locking occurring), and Mutex is set up to require the use of negative annotations. That means making the change you suggest results in the error:

txorphanage.cpp:167:5: error: calling function 'MaybeCheckNotHeld' requires negative capability '!m_mutex' [-Werror,-Wthread-safety-analysis]
    LOCK(m_mutex);

nit in 0027174b396cacbaac5fd52f13be3dca9fcbbfb8:

It's a bit confusing what "This set" refers to.

     *                     orphan will be reconsidered on each call of this function. The peer's orphan work set

Comment updated in #26551

in 733d85f79cde353d8c9b54370f296b1031fa33d9 Note to self, my understanding here is:

• The two threads that may access orphanage are (1) msg proc and (2) scheduler for calling m_orphanage.EraseForBlock() in BlockConnected().
• Previously, we held g_cs_orphans through the entirety of ProcessOrphanTx, and now we are grabbing the lock for each call here (AddChildrenToWorkSet(*porphanTx, peer.m_id), EraseTx(orphanHash)).

And this is fine. The "problem" would be if we called BlockConnected and e.g. evicted an orphan that was just confirmed. It's fine since EraseTx() handles if a tx is already gone, and AddChildrenToworkSet() will wait on g_cs_orphans for EraseForBlock() to be done updating m_outpoint_to_orphan_it.

Since you're touching this, I think it's quite strange that we do this in a while(!work_set.empty()) loop when we're trying to make sure that we only ever process 1 orphan at a time. Perhaps a remnant of pre-#15644 code? My suggestion is to take this out of the while loop and replace the breaks with returns... but non-blocking of course, can do in another PR.

There's three "loops" I guess:

• ThreadMessageHandler repeatedly calls ProcessMessages calls ProcessMessage calls ProcessOrphanTx; if ProcessOrphanTx returns true, ProcessMessage exits early so the next run of the ThreadMessageHandler loop for this peer immediately goes back to ProcessOrphanTx
• while (GetTxToReconsider) here loops until it actually does some work (ProcessTransaction returns VALID or something other than TX_MISSING_INPUTS, returning true if the while loop could have continued
• GetTxToReconsider loops over the peer's workset, returning the first entry in the workset that still exists as an orphan

Removing the while (GetTxToReconsider) loop would mean that we'd immediately switch to the next peer anytime we came across an orphan that needed two parents, rather than just one, despite that not taking much time to determine (it's part of MemPoolAccept::PreChecks). I don't think that would be wrong, but the way it is seems better to me.

Removing the internal loop for GetTxToReconsider would mean you'd need to return a new result: "there's more work to do, but the first bit of work was a no-op", and you'd be re-querying m_peer_work_set.find(peer) repeatedly.