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
returnsint64_t
but this was being assigned to asize_t
(unsigned type). If-1
, then this would be 2^63-1. See: !1831 (diffs)
- Previous to this MR if
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
- All net_processing
internal
namespace objects and functions related to the txn orphan mechanism have been moved to the new classTxOrphanage
(txorphanage.h
). -
net_processing.cpp
just has a static private globaltxOrphans
object now that is an instance ofTxOrphanage
. -
denialofservice_tests.cpp
now just accesses thisTxOrphanage
class rather than being forced to see/modify internal data ofnet_processing.cpp
- 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
- In
net_pricessing.cpp
, removed the un-neededCNetProcessingCleanup
.txOrpahns
is cleaned-up anyway on app-exit without this object. - BUGFIX: Due to mis-uses of type
size_t
in conjunction withint64_t
inAddToCompactExtraTransactions
, 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).