Decrypt completed sync transactions outside mNxsMutex

[jolavillette]

processTransactions() ran processTransactionForDecryption() — per-item circle RSA decryption of completed incoming transactions — while holding mNxsMutex. On a node subscribed to circle-restricted channels this was measured at 1–3 s of RSA per sync cycle (about 225 s accumulated over a 73-minute session), blocking every other user of the service (tick, item reception, API) for the duration.

Completed transactions now leave the active map under the mutex, are decrypted after it is released — the thread is their sole owner at that point — and are queued into mComplTransactions under a brief relock. Behavior is otherwise unchanged: items whose keys are unavailable stay encrypted and are dropped at validation, as before.

🤖 Generated with Claude Code

[csoler]

I think the logic here has a flaw: the list of uncompleted transactions is also used as a waiting list because the decryption of transactions needs the private part of GxsId keys (requested through decryptSingleNxsItem()) and these are loaded asynchronously (see p3IdService::getPrivateKey()). These are requested to friends in parallel when needed and therefore processTransactionForDecryption() does not always return true nor succeed. Therefore the outcome of this function needs to be accounted for and the items (or the transaction) should remain in the list if decryption doesn't work.

What could be done: if processTransactionForDecryption() fails, put back the transaction in mTransactions after locking mNxsMtx.

or...

just keep the code as it is. It all depends on how much the change is actually improving the software.

[jolavillette]

The else branch this PR removes was already unreachable. processTransactionForDecryption() contains a single return — an unconditional return true — since 18dbfd1e9 (2016-04-06), which moved the key-loading check out into decryptSingleNxsItem() and lost the early return false on the way. In 549002d14 the check was still inline and the retry did work. The header comment ("return false when the keys are not loaded => need retry later") has been describing the 2016 intent rather than the code ever since.

Putting the transaction back wouldn't restore it either: in the item loop the encrypted item is erase()d from tr->mItems and deleted whatever the outcome, and nothing is inserted back on failure. A requeued transaction would be retried against a list its items have already left, so those items are lost today, before this patch.

One detail on the async keys: decryptSingleNxsItem() calls mGixs->getOwnIds() then getPrivateKey() for each — our own identities, served from the local key cache via cache_request_load(). Friends supply the public keys of other identities, not these.

So the patch removes dead code, and the decryption outcome is no more accounted for after it than before.

On whether it is worth it — instrumented build with the decryption timed separately, GXS channels (service 0x217), one continuous 30.6 h session: 2976 passes of processTransactions() above 1 s, mean 1.73 s, max 5.6 s, 5159 s total spent in transaction decryption. On master every bit of that is inside mNxsMutex — about 4.7% of wall-clock with the service blocked for tick, item reception and API, and that only counts the passes above the 1 s reporting threshold. processCompletedTransactions() runs immediately after in the same threadTick(), so deferring the queueing changes no ordering.

Correcting my own PR description while I am here: it says items whose keys are unavailable "stay encrypted and are dropped at validation". They don't — they are deleted right there in processTransactionForDecryption(). The "as before" holds, the mechanism I described doesn't.

If you want the waiting list to actually exist, I'm happy to do it as a follow-up: have decryptSingleNxsItem() distinguish "key not loaded yet" from "cannot decrypt", keep the encrypted item in place in the first case, return false from processTransactionForDecryption(), and put the transaction back under the mutex. That fix wants this PR underneath it — retrying is only cheap once the decryption isn't holding mNxsMutex for seconds at a time.