Skip to content

Add package acceptance logic to mempool #16401

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 2 commits into from
Closed

Add package acceptance logic to mempool #16401

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c067d85
Add package-acceptance logic to mempool
sdaftuar Jul 12, 2019
6a3bdba
Implement package relay for 2-tx packages from the orphan pool
sdaftuar Jul 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/consensus/validation.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ enum class TxValidationResult {
* Currently this is only used if the transaction already exists in the mempool or on chain.
*/
TX_CONFLICT,
TX_MEMPOOL_POLICY, //!< violated mempool's fee/size/descendant/RBF/etc limits
TX_MEMPOOL_POLICY, //!< violated mempool's size/descendant/RBF/etc limits
TX_MEMPOOL_INSUFFICIENT_FEE, //!< violated mempool's feerate requirements
};

/** A "reason" why a block was invalid, suitable for determining whether the
Expand Down
120 changes: 87 additions & 33 deletions src/net_processing.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,20 @@ void RequestTx(CNodeState* state, const uint256& txid, std::chrono::microseconds
peer_download_state.m_tx_process_time.emplace(process_time, txid);
}

// Add to a peer's orphan_work_set after processing a given transaction.
void UpdateOrphanWorkSet(const CTransaction& tx, CNode *peer) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can be reused too in ProcessOrphanTx

{
const uint256& hash = tx.GetHash();
for (unsigned int i=0; i < tx.vout.size(); i++) {
auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(hash, i));
if (it_by_prev != mapOrphanTransactionsByPrev.end()) {
for (const auto& elem : it_by_prev->second) {
peer->orphan_work_set.insert(elem->first);
}
}
}
}

} // namespace

// This function is used for testing the stale tip eviction logic, see
Expand Down Expand Up @@ -1078,6 +1092,7 @@ static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state,
case TxValidationResult::TX_WITNESS_MUTATED:
case TxValidationResult::TX_CONFLICT:
case TxValidationResult::TX_MEMPOOL_POLICY:
case TxValidationResult::TX_MEMPOOL_INSUFFICIENT_FEE:
break;
}
if (message != "") {
Expand Down Expand Up @@ -2504,15 +2519,8 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
if (!AlreadyHave(inv) &&
AcceptToMemoryPool(mempool, state, ptx, &lRemovedTxn, false /* bypass_limits */, 0 /* nAbsurdFee */)) {
mempool.check(&::ChainstateActive().CoinsTip());
RelayTransaction(tx.GetHash(), *connman);
for (unsigned int i = 0; i < tx.vout.size(); i++) {
auto it_by_prev = mapOrphanTransactionsByPrev.find(COutPoint(inv.hash, i));
if (it_by_prev != mapOrphanTransactionsByPrev.end()) {
for (const auto& elem : it_by_prev->second) {
pfrom->orphan_work_set.insert(elem->first);
}
}
}
RelayTransaction(ptx->GetHash(), *connman);
UpdateOrphanWorkSet(tx, pfrom);

pfrom->nLastTXTime = GetTime();

Expand Down Expand Up @@ -2557,32 +2565,78 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
recentRejects->insert(tx.GetHash());
}
} else {
if (!tx.HasWitness() && state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) {
// Do not use rejection cache for witness transactions or
// witness-stripped transactions, as they can have been malleated.
// See https://github.com/bitcoin/bitcoin/issues/8279 for details.
assert(recentRejects);
recentRejects->insert(tx.GetHash());
if (RecursiveDynamicUsage(*ptx) < 100000) {
// If this tx didn't make it in due to feerate, and there is a tx
// in the orphan pool -- then maybe that tx is only missing this
// one parent.
// Try to process the pair as a package.
bool added_as_package = false;
if (state.GetResult() == TxValidationResult::TX_MEMPOOL_INSUFFICIENT_FEE) {
std::list<std::map<uint256, COrphanTx>::iterator> orphans_missing_this_tx;
for (size_t i=0; i<tx.vout.size(); ++i) {
auto it = mapOrphanTransactionsByPrev.find(COutPoint(tx.GetHash(), i));
if (it != mapOrphanTransactionsByPrev.end()) {
for (auto orphan_iter : it->second) orphans_missing_this_tx.push_back(orphan_iter);
}
}
if (!orphans_missing_this_tx.empty()) {
const COrphanTx &orphan_tx = orphans_missing_this_tx.front()->second;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I broadcast a second CPFP due to to the first one being still too low but this one still being in the orphan pool, you need to iter and try with the whole set not only picking up the first transaction ? That would be a DoS vector bounded by the size of the orphan pool, and maybe it can be mitigate by caching the package already-tried.

// Pick the first transaction, and process the pair. If it's
// missing other inputs, this will of course fail.
std::list<CTransactionRef> package;
package.push_back(ptx);
package.push_back(orphan_tx.tx);
TxValidationState package_state;
if (AcceptPackageToMemoryPool(mempool, state, package, &lRemovedTxn, 0 /* nAbsurdFee */, false /* test_accept */)) {
LogPrintf("package accepted!!\n"); // XXX: improve logging
added_as_package = true;
mempool.check(&::ChainstateActive().CoinsTip());
EraseOrphanTx(orphan_tx.tx->GetHash());
for (auto package_tx : package) RelayTransaction(package_tx->GetHash(), *connman);
for (auto package_tx : package) UpdateOrphanWorkSet(*package_tx, pfrom);

pfrom->nLastTXTime = GetTime();

for (auto package_tx : package) {
LogPrint(BCLog::MEMPOOL, "AcceptToMemoryPool: peer=%d: accepted %s (poolsz %u txn, %u kB)\n",
pfrom->GetId(),
package_tx->GetHash().ToString(),
mempool.size(), mempool.DynamicMemoryUsage() / 1000);
}

// Recursively process any orphan transactions that depended on these
ProcessOrphanTx(connman, pfrom->orphan_work_set, lRemovedTxn);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If package wasn't accepted (like a high-fee invalid orphan tx), and if inputs are not missing, if orphan_tx is invalid we may punish peer, or at least erase it for not being standard or insufficient fee?

}
}

if (!added_as_package) {
if (!tx.HasWitness() && state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) {
// Do not use rejection cache for witness transactions or
// witness-stripped transactions, as they can have been malleated.
// See https://github.com/bitcoin/bitcoin/issues/8279 for details.
assert(recentRejects);
recentRejects->insert(tx.GetHash());
if (RecursiveDynamicUsage(*ptx) < 100000) {
AddToCompactExtraTransactions(ptx);
}
} else if (tx.HasWitness() && RecursiveDynamicUsage(*ptx) < 100000) {
AddToCompactExtraTransactions(ptx);
}
} else if (tx.HasWitness() && RecursiveDynamicUsage(*ptx) < 100000) {
AddToCompactExtraTransactions(ptx);
}

if (pfrom->HasPermission(PF_FORCERELAY)) {
// Always relay transactions received from whitelisted peers, even
// if they were already in the mempool or rejected from it due
// to policy, allowing the node to function as a gateway for
// nodes hidden behind it.
//
// Never relay transactions that might result in being
// disconnected (or banned).
if (state.IsInvalid() && TxRelayMayResultInDisconnect(state)) {
LogPrintf("Not relaying invalid transaction %s from whitelisted peer=%d (%s)\n", tx.GetHash().ToString(), pfrom->GetId(), FormatStateMessage(state));
} else {
LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId());
RelayTransaction(tx.GetHash(), *connman);

if (pfrom->HasPermission(PF_FORCERELAY)) {
// Always relay transactions received from whitelisted peers, even
// if they were already in the mempool or rejected from it due
// to policy, allowing the node to function as a gateway for
// nodes hidden behind it.
//
// Never relay transactions that might result in being
// disconnected (or banned).
if (state.IsInvalid() && TxRelayMayResultInDisconnect(state)) {
LogPrintf("Not relaying invalid transaction %s from whitelisted peer=%d (%s)\n", tx.GetHash().ToString(), pfrom->GetId(), FormatStateMessage(state));
} else {
LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId());
RelayTransaction(tx.GetHash(), *connman);
}
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions src/txmempool.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -899,6 +899,12 @@ bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const {
// conflict with the underlying cache, and it cannot have pruned entries (as it contains full)
// transactions. First checking the underlying cache risks returning a pruned entry instead.
CTransactionRef ptx = mempool.get(outpoint.hash);
if (!ptx) {
// If a coin is missing from the mempool, check to see if it's part of
// a candidate package
auto it = package_tx.find(outpoint.hash);
if (it != package_tx.end()) ptx = it->second;
}
if (ptx) {
if (outpoint.n < ptx->vout.size()) {
coin = Coin(ptx->vout[outpoint.n], MEMPOOL_HEIGHT, false);
Expand Down
4 changes: 4 additions & 0 deletions src/txmempool.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -752,8 +752,12 @@ class CTxMemPool
*/
class CCoinsViewMemPool : public CCoinsViewBacked
{
public:
void AddPotentialTransaction(const CTransactionRef& ptx) { package_tx.emplace(ptx->GetHash(), ptx); }
Copy link
Contributor

@NicolasDorier NicolasDorier Sep 30, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I see, in the current code code paths, the max number of elements in package_tx is two.
But maybe there should be a hard limit, even if high, at this level just in case?


protected:
const CTxMemPool& mempool;
std::map<uint256, const CTransactionRef> package_tx;
Copy link

@ariard ariard Oct 31, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could have a better name like package_pool just to underscore we have multiple pending txn. I didn't get the rational at first read, I think you can explain given we have mempool locked and can't write until we have the whole set of tx, we need this temporary buffer to find outpoints of package txn after first row of parents. If this to work assuming topological ordering it should be documented?


public:
CCoinsViewMemPool(CCoinsView* baseIn, const CTxMemPool& mempoolIn);
Expand Down
Loading