Skip to content

Commit

Permalink
Fully encapsulate DisconnectedBlockTransactions
Browse files Browse the repository at this point in the history
  • Loading branch information
theuni committed Sep 5, 2023
1 parent c19f4aa commit 98c0b86
Show file tree
Hide file tree
Showing 2 changed files with 23 additions and 11 deletions.
20 changes: 16 additions & 4 deletions src/kernel/disconnected_transactions.h
Original file line number Diff line number Diff line change
Expand Up @@ -28,11 +28,13 @@
* still-unconfirmed transactions at the end.
*/
struct DisconnectedBlockTransactions {
private:
uint64_t cachedInnerUsage = 0;
std::list<CTransactionRef> queuedTx;
using Queue = decltype(queuedTx);
std::unordered_map<uint256, Queue::iterator, SaltedTxidHasher> iters_by_txid;

public:
// It's almost certainly a logic bug if we don't clear out queuedTx before
// destruction, as we add to it while disconnecting blocks, and then we
// need to re-process remaining transactions to ensure mempool consistency.
Expand Down Expand Up @@ -75,11 +77,16 @@ struct DisconnectedBlockTransactions {
}

// Remove the first entry and update memory usage.
void remove_first()
CTransactionRef take_first()
{
cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());
iters_by_txid.erase(queuedTx.front()->GetHash());
queuedTx.pop_front();
CTransactionRef ret;
if (!queuedTx.empty()) {
ret = queuedTx.front();
cachedInnerUsage -= RecursiveDynamicUsage(queuedTx.front());

This comment has been minimized.

Copy link
@stickies-v

stickies-v Sep 6, 2023

nit: just using ret instead of queuedTx.front() seems appropriate? (+ next line)
nit: first_tx or something would probably be a better name instead of ret

iters_by_txid.erase(queuedTx.front()->GetHash());
queuedTx.pop_front();
}
return ret;
}

void clear()
Expand All @@ -88,6 +95,11 @@ struct DisconnectedBlockTransactions {
iters_by_txid.clear();
queuedTx.clear();
}
std::list<CTransactionRef> take() {
std::list<CTransactionRef> ret = queuedTx;

This comment has been minimized.

Copy link
@stickies-v

stickies-v Sep 6, 2023

I think move semantics would be appropriate here?
std::list<CTransactionRef> ret = std::move(queuedTx);

This comment has been minimized.

Copy link
@theuni

theuni Sep 6, 2023

Author Owner

Whoops, indeed!

clear();
return ret;
}
};

#endif // BITCOIN_KERNEL_DISCONNECTED_TRANSACTIONS_H
14 changes: 7 additions & 7 deletions src/validation.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,9 @@ void Chainstate::MaybeUpdateMempoolForReorg(
// Iterate disconnectpool in reverse, so that we add transactions
// back to the mempool starting with the earliest transaction that had
// been previously seen in a block.
auto it = disconnectpool.queuedTx.rbegin();
while (it != disconnectpool.queuedTx.rend()) {
std::list<CTransactionRef> queuedTx = disconnectpool.take();
auto it = queuedTx.rbegin();
while (it != queuedTx.rend()) {
// ignore errors related to fees in resurrected transactions.
if (!fAddToMempool || (*it)->IsCoinBase() ||
AcceptToMemoryPool(*this, *it, GetTime(),
Expand All @@ -317,7 +318,6 @@ void Chainstate::MaybeUpdateMempoolForReorg(
}
++it;
}
disconnectpool.queuedTx.clear();
// AcceptToMemoryPool/addUnchecked all assume that new mempool entries have
// no in-mempool children, which is generally not true when adding
// previously-confirmed transactions back to the mempool.
Expand Down Expand Up @@ -2725,11 +2725,11 @@ bool Chainstate::DisconnectTip(BlockValidationState& state, DisconnectedBlockTra
for (auto it = block.vtx.rbegin(); it != block.vtx.rend(); ++it) {
disconnectpool->addTransaction(*it);
}
while (!disconnectpool->queuedTx.empty() && disconnectpool->DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE * 1000) {
while (disconnectpool->DynamicMemoryUsage() > MAX_DISCONNECTED_TX_POOL_SIZE * 1000) {

This comment has been minimized.

Copy link
@theuni

theuni Sep 6, 2023

Author Owner

If addTransactionsForBlock() were used as suggested here, this cleanup could happen internally. Without that step, I suppose encapsulation is still not complete.

// Drop the earliest entry, and remove its children from the mempool.
auto ptx = disconnectpool->queuedTx.front();
m_mempool->removeRecursive(*ptx, MemPoolRemovalReason::REORG);
disconnectpool->remove_first();
if (auto ptx = disconnectpool->take_first()) {
m_mempool->removeRecursive(*ptx, MemPoolRemovalReason::REORG);
}
}
}

Expand Down

0 comments on commit 98c0b86

Please sign in to comment.