highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-01-21 15:50:07 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#22675: RBF move 2/3: extract RBF logic into policy/rbf

Browse Source 
32748da0f4 whitespace fixups after move and scripted-diff (glozow)
fa47622e8d scripted-diff: rename variables in policy/rbf (glozow)
ac761f0a23 MOVEONLY: fee checks (Rules 3 and 4) to policy/rbf (glozow)
9c2f9f8984 MOVEONLY: check that fees > direct conflicts to policy/rbf (glozow)
3f033f01a6 MOVEONLY: check for disjoint conflicts and ancestors to policy/rbf (glozow)
7b60c02b7d MOVEONLY: BIP125 Rule 2 to policy/rbf (glozow)
f8ad2a57c6 Make GetEntriesForConflicts return std::optional (glozow)

Pull request description:

  This PR does not change behavior. It extracts the BIP125 logic into helper functions (and puts them in the policy/rbf* files). This enables three things - I think each one individually is pretty good:

  - Implementation of package RBF (see #22290). I want it to be as close to BIP125 as possible so that it doesn't become a distinct fee-bumping mechanism. Doing these move-only commits first means the diff is mostly mechanical to review, and I just need to create a function that mirrors the single transaction validation.
  - We will be able to isolate and test our RBF logic alone. Recently, there have been some discussions on discrepancies between our code and BIP125, as well as proposals for improving it. Generally, I think making this code more modular and de-bloating validation.cpp is probably a good idea.
  - Witness Replacement (replacing same-txid-different-wtxid when the witness is significantly smaller and therefore higher feerate) in a BIP125-similar way. Hopefully it can just be implemented with calls to the rbf functions (i.e. `PaysForRBF`) and an edit to the relevant mempool entries.

ACKs for top commit:
  mjdietzx:
    ACK 32748da0f4
  theStack:
    Code-review ACK 32748da0f4 📐
  MarcoFalke:
    review ACK 32748da0f4 🦇

Tree-SHA512: d89985c8b4b42b54861018deb89468e04968c85a3fb1113bbcb2eb2609577bc4fd9bf254593b5bd0e7ab059a0fa8192d1a903b00f77e6f120c7a80488ffcbfc0
This commit is contained in:
fanquake
2021-09-10 14:29:52 +08:00
parent 5446070418 32748da0f4
commit b8336b22d3
4 changed files with 189 additions and 131 deletions
Download Patch File Download Diff File Expand all files Collapse all files

106
src/validation.cpp
View File

@@ -770,16 +770,8 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
// that we have the set of all ancestors we can detect this
// pathological case by making sure setConflicts and setAncestors don't
// intersect.
for (CTxMemPool::txiter ancestorIt : setAncestors)
{
const uint256 &hashAncestor = ancestorIt->GetTx().GetHash();
if (setConflicts.count(hashAncestor))
{
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx",
strprintf("%s spends conflicting transaction %s",
hash.ToString(),
hashAncestor.ToString()));
}
if (const auto err_string{EntriesAndTxidsDisjoint(setAncestors, setConflicts, hash)}) {
return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-spends-conflicting-tx", *err_string);
}
@@ -789,98 +781,30 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
fReplacementTransaction = setConflicts.size();
if (fReplacementTransaction)
{
std::string err_string;
CFeeRate newFeeRate(nModifiedFees, nSize);
for (const auto& mi : setIterConflicting) {
// Don't allow the replacement to reduce the feerate of the
// mempool.
//
// We usually don't want to accept replacements with lower
// feerates than what they replaced as that would lower the
// feerate of the next block. Requiring that the feerate always
// be increased is also an easy-to-reason about way to prevent
// DoS attacks via replacements.
//
// We only consider the feerates of transactions being directly
// replaced, not their indirect descendants. While that does
// mean high feerate children are ignored when deciding whether
// or not to replace, we do require the replacement to pay more
// overall fees too, mitigating most cases.
CFeeRate oldFeeRate(mi->GetModifiedFee(), mi->GetTxSize());
if (newFeeRate <= oldFeeRate)
{
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
strprintf("rejecting replacement %s; new feerate %s <= old feerate %s",
hash.ToString(),
newFeeRate.ToString(),
oldFeeRate.ToString()));
}
if (const auto err_string{PaysMoreThanConflicts(setIterConflicting, newFeeRate, hash)}) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
}
// Calculate all conflicting entries and enforce Rule #5.
if (!GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting, err_string)) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too many potential replacements", err_string);
if (const auto err_string{GetEntriesForConflicts(tx, m_pool, setIterConflicting, allConflicting)}) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
"too many potential replacements", *err_string);
}
// Enforce Rule #2.
if (const auto err_string{HasNoNewUnconfirmed(tx, m_pool, setIterConflicting)}) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY,
"replacement-adds-unconfirmed", *err_string);
}
// Check if it's economically rational to mine this transaction rather
// than the ones it replaces.
// than the ones it replaces. Enforce Rules #3 and #4.
for (CTxMemPool::txiter it : allConflicting) {
nConflictingFees += it->GetModifiedFee();
nConflictingSize += it->GetTxSize();
}
std::set<uint256> setConflictsParents;
for (const auto& mi : setIterConflicting) {
for (const CTxIn &txin : mi->GetTx().vin)
{
setConflictsParents.insert(txin.prevout.hash);
}
}
for (unsigned int j = 0; j < tx.vin.size(); j++)
{
// We don't want to accept replacements that require low
// feerate junk to be mined first. Ideally we'd keep track of
// the ancestor feerates and make the decision based on that,
// but for now requiring all new inputs to be confirmed works.
//
// Note that if you relax this to make RBF a little more useful,
// this may break the CalculateMempoolAncestors RBF relaxation,
// above. See the comment above the first CalculateMempoolAncestors
// call for more info.
if (!setConflictsParents.count(tx.vin[j].prevout.hash))
{
// Rather than check the UTXO set - potentially expensive -
// it's cheaper to just check if the new input refers to a
// tx that's in the mempool.
if (m_pool.exists(tx.vin[j].prevout.hash)) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "replacement-adds-unconfirmed",
strprintf("replacement %s adds unconfirmed input, idx %d",
hash.ToString(), j));
}
}
}
// The replacement must pay greater fees than the transactions it
// replaces - if we did the bandwidth used by those conflicting
// transactions would not be paid for.
if (nModifiedFees < nConflictingFees)
{
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
strprintf("rejecting replacement %s, less fees than conflicting txs; %s < %s",
hash.ToString(), FormatMoney(nModifiedFees), FormatMoney(nConflictingFees)));
}
// Finally in addition to paying more fees than the conflicts the
// new transaction must pay for its own bandwidth.
CAmount nDeltaFees = nModifiedFees - nConflictingFees;
if (nDeltaFees < ::incrementalRelayFee.GetFee(nSize))
{
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee",
strprintf("rejecting replacement %s, not enough additional fees to relay; %s < %s",
hash.ToString(),
FormatMoney(nDeltaFees),
FormatMoney(::incrementalRelayFee.GetFee(nSize))));
if (const auto err_string{PaysForRBF(nConflictingFees, nModifiedFees, nSize, hash)}) {
return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
}
}
return true;