Skip to content

TxOrphans: Refactor the orphan mechanism to its own class + bugfix

Summary

The network relay transaction orphan mechanism was ad-hoc implemented in net_processing.cpp. This MR refactors this mechanism and its associated functions to its own class.

This refactor paves the way for future work to fine-tune the transaction orphan mechanism. By having the orphan related data and code in a single class, and attempting to encapsulate it as much as possible, it is hoped that future maintenance and modification will be made easier.

Also in this MR:

  • A bugfix:
    • Previous to this MR if -blockreconstructionextratxn=-1 (or any negative value, really) would be specified in the conf file, the node would generate an exception (or potentially even crash) when processing messages if it received an orphan tx. This looks like potential UB which could lead to a crash, and has been addressed.
    • The bug occurs because GetArgs returns int64_t but this was being assigned to a size_t (unsigned type). If -1, then this would be 2^63-1. See: !1831 (diffs)

Note: No node behavioral change is introduced by this MR (aside from the above-mentioned bugfix), this is a pure refactor MR.

Summary of Changes

  1. All net_processing internal namespace objects and functions related to the txn orphan mechanism have been moved to the new class TxOrphanage (txorphanage.h).
  2. net_processing.cpp just has a static private global txOrphans object now that is an instance of TxOrphanage.
  3. denialofservice_tests.cpp now just accesses this TxOrphanage class rather than being forced to see/modify internal data of net_processing.cpp
  4. The txn orphange also manages a concept known as "extra txns for compact blocks" which (largely) is just full of orphans but in vector form for easy use. This also now lives in class TxOrphanage
  5. In net_pricessing.cpp, removed the un-needed CNetProcessingCleanup. txOrpahns is cleaned-up anyway on app-exit without this object.
  6. BUGFIX: Due to mis-uses of type size_t in conjunction with int64_t in AddToCompactExtraTransactions, an exception could be thrown in the middle of processing messages if specifying -blockreconstructionextratxn=-1 in the conf file. The exception would occur as soon as an orphan txn is seen and would involve the node attempting to allocate a vector with 2^63 elements. Offending code was here: !1831 (diffs)

Test Plan

ninja all check-all

(Optional) Without this MR, run a node with -blockreconstructionextratxn=-1 -debug=mempool -debug=net and wait around for a long time for an orphan tx to appear. Node should log an exception of the form:

ProcessMessages(tx, 192 bytes): Exception 'vector' (St12length_error) caught
ProcessMessages(tx, 192 bytes) FAILED peer=0

With this MR node will not raise the above exception internally and the -1 is treated as if it were 0 by the node (old behavior would effectively be this anyway).

Edited by Calin Culianu

Merge request reports