This PR adds package relay for the one-parent-one-child (1p1c) happy case, that is, when an attacker is not trying to mess around with our orphanage 1️⃣.
The 1p1c case requires that the package that is trying to be relayed consist of, simply (and logically) a one parent and one child. This could be the case in a CPFP scenario in where the child is trying to bump the fee of the parent, but the parent is bellow the fee filter of the peer. Under the current logic, the child will be accepted and placed in the orphanage, but the parent won’t be accepted, preventing the package from confirming.
Notice this does not only apply to children with a single parent, but to children missing a single parent (as in, the CPFP transaction can have multiple parents that are already in mempool, and one that is bellow the mempool acceptance fee).
This PR, therefore, covers a subset of the currently rejected CPFP cases without needing to introduce any new P2P messages, which is a requirement for a more general approach to package relay: BIP331
1️⃣ Currently, the orphanage is a collection of transactions that is kept, in a best-effort way, to try to resolve conflicts in transaction propagation (e.g. receiving a children before its parent). The orphanage is rather small, and there is no attempt to protect the data inside it, meaning that an attacker trying to trigger transaction eviction from the orphanage will succeed without much effort. Conflict resolution is also currently performed in a best-effort way, that is, if a child is put in the orphanage the peer that sent us will be asked for the missing parents, but it it fails to provide the data we won’t request it from any other peers.
cpp Q: in https://github.com/bitcoin/bitcoin/pull/28970/commits/9dc967195c4965973be0174ae6041be70a886c7a, we are first building a set (to get rid of potential duplication of children iterators) and then mapping it to a vector, which is the expected return type.
// First construct a set of iterators to ensure we do not return duplicates of the same tx.
std::set<OrphanMap::iterator, IteratorComparator> set_child_iterators;
// For each output, get all entries spending this prevout.
for (unsigned int i = 0; i < parent->vout.size(); i++) {
const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(parent->GetHash(), i));
if (it_by_prev != m_outpoint_to_orphan_it.end()) {
for (const auto& elem : it_by_prev->second) {
set_child_iterators.insert(elem);
}
}
}
std::vector<std::pair<CTransactionRef, NodeId>> children_found;
children_found.reserve(set_child_iterators.size());
for (const auto child_iter : set_child_iterators) {
children_found.emplace_back(child_iter->second.tx, child_iter->second.fromPeer);
}
return children_found;
However, it feels like we should be able to simplify that last code block by constructing the vector while iterating the set and selecting the bits that we care about (in a similar fashion as we would do in rustlang by mapping the set). However, the compiler is really screaming at me in tongues (I really miss you rust compiler 😭)
std::vector<std::pair<CTransactionRef, NodeId>> children_found(set_child_iterators.begin(), set_child_iterators.end(),
[](const auto child_iter) { return std::make_pair(child_iter->second.tx, child_iter->second.fromPeer); });
The closest I’ve been able to come up with (using std::transform) is:
std::vector<std::pair<CTransactionRef, NodeId>> children_found;
children_found.reserve(set_child_iterators.size());
std::transform(set_child_iterators.begin(), set_child_iterators.end(), std::back_insert_iterator(children_found),
[](const auto child_iter) { return std::make_pair(child_iter->second.tx, child_iter->second.fromPeer); });
Which requires defining + reserving, no on-the-fly building (muh verbose 🦀)
After the first pass, GetCombinedHash has been merged with GetPackageHash, since the latter was not being called standalone. I found interesting that the way the items were hashes changed:
// From
return (HashWriter() << wtxids_copy).GetHash();
// To
HashWriter hashwriter;
for (const auto& wtxid : wtxids_copy) {
hashwriter << wtxid;
}
return hashwriter.GetSHA256();
Apart from the change of hashing algorithm sha256d → sha256 (i.e. GetHash() → GetSHA256()), looks like hashing the wtxids_copy as a collection doesn’t yield the same result as accumulating all wtxids one by one and hashing. I’m guessing there is some reordering going on if a collection is passed.
Something else worth pointing out. TxOrphanageGetChildren ****has been split in two now: TxOrphanage::GetChildrenFromSamePeer and TxOrphanage::GetChildrenFromDifferentPeer. As the names suggest, to be able to query transactions from the orphanage based on what peer sent it to us.