Skip to content

Commit

Permalink
Merge bitcoin#28948: v3 transaction policy for anti-pinning
Browse files Browse the repository at this point in the history
29029df [doc] v3 signaling in mempool-replacements.md (glozow)
e643ea7 [fuzz] v3 transactions and sigop-adjusted vsize (glozow)
1fd16b5 [functional test] v3 transaction submission (glozow)
27c8786 test framework: Add and use option for tx-version in MiniWallet methods (MarcoFalke)
9a1fea5 [policy/validation] allow v3 transactions with certain restrictions (glozow)
eb8d5a2 [policy] add v3 policy rules (glozow)
9a29d47 [rpc] return full string for package_msg and package-error (glozow)
158623b [refactor] change Workspace::m_conflicts and adjacent funcs/structs to use Txid (glozow)

Pull request description:

  See bitcoin#27463 for overall package relay tracking.

  Delving Bitcoin discussion thread: https://delvingbitcoin.org/t/v3-transaction-policy-for-anti-pinning/340
  Delving Bitcoin discussion for LN usage: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418

  Rationale:
  - There are various pinning problems with RBF and our general ancestor/descendant limits. These policies help mitigate many pinning attacks and make package RBF feasible (see bitcoin#28984 which implements package RBF on top of this). I would focus the most here on Rule 3 pinning. [1][2]
  - Switching to a cluster-based mempool (see bitcoin#27677 and bitcoin#28676) requires the removal of CPFP carve out, which applications depend on. V3 + package RBF + ephemeral anchors + 1-parent-1-child package relay provides an intermediate solution.

  V3 policy is for "Priority Transactions." [3][4] It allows users to opt in to more restrictive topological limits for shared transactions, in exchange for the more robust fee-bumping abilities that offers. Even though we don't have cluster limits, we are able to treat these transactions as having as having a maximum cluster size of 2.

  Immediate benefits:

  - You can presign a transaction with 0 fees (not just 1sat/vB!) and add a fee-bump later.
  - Rule 3 pinning is reduced by a significant amount, since the attacker can only attach a maximum of 1000vB to your shared transaction.

  This also enables some other cool things (again see bitcoin#27463 for overall roadmap):
  - Ephemeral Anchors
  - Package RBF for these 1-parent-1-child packages. That means e.g. a commitment tx + child can replace another commitment tx using the child's fees.
  - We can transition to a "single anchor" universe without worrying about package limit pinning. So current users of CPFP carve out would have something else to use.
  - We can switch to a cluster-based mempool [5] (bitcoin#27677 bitcoin#28676), which removes CPFP carve out [6].

  [1]: Original mailing list post and discussion about RBF pinning problems https://gist.github.com/glozow/25d9662c52453bd08b4b4b1d3783b9ff, https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019817.html
  [2]: A FAQ is "we need this for cluster mempool, but is this still necessary afterwards?" There are some pinning issues that are fixed here and not fully fixed in cluster mempool, so we will still want this or something similar afterward.
  [3]: Mailing list post for v3 https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-September/020937.html
  [4]: Original PR bitcoin#25038 also contains a lot of the discussion
  [5]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393/7
  [6]: https://delvingbitcoin.org/t/an-overview-of-the-cluster-mempool-proposal/393#the-cpfp-carveout-rule-can-no-longer-be-supported-12

ACKs for top commit:
  sdaftuar:
    ACK 29029df
  achow101:
    ACK 29029df
  instagibbs:
    ACK 29029df modulo that

Tree-SHA512: 9664b078890cfdca2a146439f8835c9d9ab483f43b30af8c7cd6962f09aa557fb1ce7689d5e130a2ec142235dbc8f21213881baa75241c5881660f9008d68450
  • Loading branch information
achow101 committed Feb 10, 2024
2 parents 1d334d8 + 29029df commit 7143d43
Show file tree
Hide file tree
Showing 23 changed files with 1,136 additions and 42 deletions.
3 changes: 2 additions & 1 deletion doc/policy/mempool-replacements.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@ their in-mempool descendants (together, "original transactions") if, in addition
other consensus and policy rules, each of the following conditions are met:

1. The directly conflicting transactions all signal replaceability explicitly. A transaction is
signaling replaceability if any of its inputs have an nSequence number less than (0xffffffff - 1).
signaling BIP125 replaceability if any of its inputs have an nSequence number less than (0xffffffff - 1).
A transaction also signals replaceibility if its nVersion field is set to 3.

*Rationale*: See [BIP125
explanation](https://github.com/bitcoin/bips/blob/master/bip-0125.mediawiki#motivation).
Expand Down
4 changes: 4 additions & 0 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -242,6 +242,7 @@ BITCOIN_CORE_H = \
node/validation_cache_args.h \
noui.h \
outputtype.h \
policy/v3_policy.h \
policy/feerate.h \
policy/fees.h \
policy/fees_args.h \
Expand Down Expand Up @@ -441,6 +442,7 @@ libbitcoin_node_a_SOURCES = \
node/utxo_snapshot.cpp \
node/validation_cache_args.cpp \
noui.cpp \
policy/v3_policy.cpp \
policy/fees.cpp \
policy/fees_args.cpp \
policy/packages.cpp \
Expand Down Expand Up @@ -702,6 +704,7 @@ libbitcoin_common_a_SOURCES = \
netbase.cpp \
net_permissions.cpp \
outputtype.cpp \
policy/v3_policy.cpp \
policy/feerate.cpp \
policy/policy.cpp \
protocol.cpp \
Expand Down Expand Up @@ -960,6 +963,7 @@ libbitcoinkernel_la_SOURCES = \
node/blockstorage.cpp \
node/chainstate.cpp \
node/utxo_snapshot.cpp \
policy/v3_policy.cpp \
policy/feerate.cpp \
policy/packages.cpp \
policy/policy.cpp \
Expand Down
4 changes: 2 additions & 2 deletions src/policy/rbf.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,11 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx,
}

std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
const std::set<uint256>& direct_conflicts,
const std::set<Txid>& direct_conflicts,
const uint256& txid)
{
for (CTxMemPool::txiter ancestorIt : ancestors) {
const uint256& hashAncestor = ancestorIt->GetTx().GetHash();
const Txid& hashAncestor = ancestorIt->GetTx().GetHash();
if (direct_conflicts.count(hashAncestor)) {
return strprintf("%s spends conflicting transaction %s",
txid.ToString(),
Expand Down
2 changes: 1 addition & 1 deletion src/policy/rbf.h
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ std::optional<std::string> HasNoNewUnconfirmed(const CTransaction& tx, const CTx
* @returns error message if the sets intersect, std::nullopt if they are disjoint.
*/
std::optional<std::string> EntriesAndTxidsDisjoint(const CTxMemPool::setEntries& ancestors,
const std::set<uint256>& direct_conflicts,
const std::set<Txid>& direct_conflicts,
const uint256& txid);

/** Check that the feerate of the replacement transaction(s) is higher than the feerate of each
Expand Down
220 changes: 220 additions & 0 deletions src/policy/v3_policy.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,220 @@
// Copyright (c) 2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <policy/v3_policy.h>

#include <coins.h>
#include <consensus/amount.h>
#include <logging.h>
#include <tinyformat.h>
#include <util/check.h>

#include <algorithm>
#include <numeric>
#include <vector>

/** Helper for PackageV3Checks: Returns a vector containing the indices of transactions (within
* package) that are direct parents of ptx. */
std::vector<size_t> FindInPackageParents(const Package& package, const CTransactionRef& ptx)
{
std::vector<size_t> in_package_parents;

std::set<Txid> possible_parents;
for (auto &input : ptx->vin) {
possible_parents.insert(input.prevout.hash);
}

for (size_t i{0}; i < package.size(); ++i) {
const auto& tx = package.at(i);
// We assume the package is sorted, so that we don't need to continue
// looking past the transaction itself.
if (&(*tx) == &(*ptx)) break;
if (possible_parents.count(tx->GetHash())) {
in_package_parents.push_back(i);
}
}
return in_package_parents;
}

/** Helper for PackageV3Checks, storing info for a mempool or package parent. */
struct ParentInfo {
/** Txid used to identify this parent by prevout */
const Txid& m_txid;
/** Wtxid used for debug string */
const Wtxid& m_wtxid;
/** nVersion used to check inheritance of v3 and non-v3 */
decltype(CTransaction::nVersion) m_version;
/** If parent is in mempool, whether it has any descendants in mempool. */
bool m_has_mempool_descendant;

ParentInfo() = delete;
ParentInfo(const Txid& txid, const Wtxid& wtxid, decltype(CTransaction::nVersion) version, bool has_mempool_descendant) :
m_txid{txid}, m_wtxid{wtxid}, m_version{version},
m_has_mempool_descendant{has_mempool_descendant}
{}
};

std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t vsize,
const Package& package,
const CTxMemPool::setEntries& mempool_ancestors)
{
// This function is specialized for these limits, and must be reimplemented if they ever change.
static_assert(V3_ANCESTOR_LIMIT == 2);
static_assert(V3_DESCENDANT_LIMIT == 2);

const auto in_package_parents{FindInPackageParents(package, ptx)};

// Now we have all ancestors, so we can start checking v3 rules.
if (ptx->nVersion == 3) {
if (mempool_ancestors.size() + in_package_parents.size() + 1 > V3_ANCESTOR_LIMIT) {
return strprintf("tx %s (wtxid=%s) would have too many ancestors",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString());
}

const bool has_parent{mempool_ancestors.size() + in_package_parents.size() > 0};
if (has_parent) {
// A v3 child cannot be too large.
if (vsize > V3_CHILD_MAX_VSIZE) {
return strprintf("v3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
vsize, V3_CHILD_MAX_VSIZE);
}

const auto parent_info = [&] {
if (mempool_ancestors.size() > 0) {
// There's a parent in the mempool.
auto& mempool_parent = *mempool_ancestors.begin();
Assume(mempool_parent->GetCountWithDescendants() == 1);
return ParentInfo{mempool_parent->GetTx().GetHash(),
mempool_parent->GetTx().GetWitnessHash(),
mempool_parent->GetTx().nVersion,
/*has_mempool_descendant=*/mempool_parent->GetCountWithDescendants() > 1};
} else {
// Ancestor must be in the package. Find it.
auto& parent_index = in_package_parents.front();
auto& package_parent = package.at(parent_index);
return ParentInfo{package_parent->GetHash(),
package_parent->GetWitnessHash(),
package_parent->nVersion,
/*has_mempool_descendant=*/false};
}
}();

// If there is a parent, it must have the right version.
if (parent_info.m_version != 3) {
return strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
parent_info.m_txid.ToString(), parent_info.m_wtxid.ToString());
}

for (const auto& package_tx : package) {
// Skip same tx.
if (&(*package_tx) == &(*ptx)) continue;

for (auto& input : package_tx->vin) {
// Fail if we find another tx with the same parent. We don't check whether the
// sibling is to-be-replaced (done in SingleV3Checks) because these transactions
// are within the same package.
if (input.prevout.hash == parent_info.m_txid) {
return strprintf("tx %s (wtxid=%s) would exceed descendant count limit",
parent_info.m_txid.ToString(),
parent_info.m_wtxid.ToString());
}

// This tx can't have both a parent and an in-package child.
if (input.prevout.hash == ptx->GetHash()) {
return strprintf("tx %s (wtxid=%s) would have too many ancestors",
package_tx->GetHash().ToString(), package_tx->GetWitnessHash().ToString());
}
}
}

// It shouldn't be possible to have any mempool siblings at this point. SingleV3Checks
// catches mempool siblings. Also, if the package consists of connected transactions,
// any tx having a mempool ancestor would mean the package exceeds ancestor limits.
if (!Assume(!parent_info.m_has_mempool_descendant)) {
return strprintf("tx %u would exceed descendant count limit", parent_info.m_wtxid.ToString());
}
}
} else {
// Non-v3 transactions cannot have v3 parents.
for (auto it : mempool_ancestors) {
if (it->GetTx().nVersion == 3) {
return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
it->GetSharedTx()->GetHash().ToString(), it->GetSharedTx()->GetWitnessHash().ToString());
}
}
for (const auto& index: in_package_parents) {
if (package.at(index)->nVersion == 3) {
return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
ptx->GetHash().ToString(),
ptx->GetWitnessHash().ToString(),
package.at(index)->GetHash().ToString(),
package.at(index)->GetWitnessHash().ToString());
}
}
}
return std::nullopt;
}

std::optional<std::string> SingleV3Checks(const CTransactionRef& ptx,
const CTxMemPool::setEntries& mempool_ancestors,
const std::set<Txid>& direct_conflicts,
int64_t vsize)
{
// Check v3 and non-v3 inheritance.
for (const auto& entry : mempool_ancestors) {
if (ptx->nVersion != 3 && entry->GetTx().nVersion == 3) {
return strprintf("non-v3 tx %s (wtxid=%s) cannot spend from v3 tx %s (wtxid=%s)",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString());
} else if (ptx->nVersion == 3 && entry->GetTx().nVersion != 3) {
return strprintf("v3 tx %s (wtxid=%s) cannot spend from non-v3 tx %s (wtxid=%s)",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(),
entry->GetSharedTx()->GetHash().ToString(), entry->GetSharedTx()->GetWitnessHash().ToString());
}
}

// This function is specialized for these limits, and must be reimplemented if they ever change.
static_assert(V3_ANCESTOR_LIMIT == 2);
static_assert(V3_DESCENDANT_LIMIT == 2);

// The rest of the rules only apply to transactions with nVersion=3.
if (ptx->nVersion != 3) return std::nullopt;

// Check that V3_ANCESTOR_LIMIT would not be violated, including both in-package and in-mempool.
if (mempool_ancestors.size() + 1 > V3_ANCESTOR_LIMIT) {
return strprintf("tx %s (wtxid=%s) would have too many ancestors",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString());
}

// Remaining checks only pertain to transactions with unconfirmed ancestors.
if (mempool_ancestors.size() > 0) {
// If this transaction spends V3 parents, it cannot be too large.
if (vsize > V3_CHILD_MAX_VSIZE) {
return strprintf("v3 child tx %s (wtxid=%s) is too big: %u > %u virtual bytes",
ptx->GetHash().ToString(), ptx->GetWitnessHash().ToString(), vsize, V3_CHILD_MAX_VSIZE);
}

// Check the descendant counts of in-mempool ancestors.
const auto& parent_entry = *mempool_ancestors.begin();
// If there are any ancestors, this is the only child allowed. The parent cannot have any
// other descendants. We handle the possibility of multiple children as that case is
// possible through a reorg.
const auto& children = parent_entry->GetMemPoolChildrenConst();
// Don't double-count a transaction that is going to be replaced. This logic assumes that
// any descendant of the V3 transaction is a direct child, which makes sense because a V3
// transaction can only have 1 descendant.
const bool child_will_be_replaced = !children.empty() &&
std::any_of(children.cbegin(), children.cend(),
[&direct_conflicts](const CTxMemPoolEntry& child){return direct_conflicts.count(child.GetTx().GetHash()) > 0;});
if (parent_entry->GetCountWithDescendants() + 1 > V3_DESCENDANT_LIMIT && !child_will_be_replaced) {
return strprintf("tx %u (wtxid=%s) would exceed descendant count limit",
parent_entry->GetSharedTx()->GetHash().ToString(),
parent_entry->GetSharedTx()->GetWitnessHash().ToString());
}
}
return std::nullopt;
}
83 changes: 83 additions & 0 deletions src/policy/v3_policy.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// Copyright (c) 2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#ifndef BITCOIN_POLICY_V3_POLICY_H
#define BITCOIN_POLICY_V3_POLICY_H

#include <consensus/amount.h>
#include <policy/packages.h>
#include <policy/policy.h>
#include <primitives/transaction.h>
#include <txmempool.h>
#include <util/result.h>

#include <set>
#include <string>

// This module enforces rules for transactions with nVersion=3 ("v3 transactions") which help make
// RBF abilities more robust.

// v3 only allows 1 parent and 1 child when unconfirmed.
/** Maximum number of transactions including an unconfirmed tx and its descendants. */
static constexpr unsigned int V3_DESCENDANT_LIMIT{2};
/** Maximum number of transactions including a V3 tx and all its mempool ancestors. */
static constexpr unsigned int V3_ANCESTOR_LIMIT{2};

/** Maximum sigop-adjusted virtual size of a tx which spends from an unconfirmed v3 transaction. */
static constexpr int64_t V3_CHILD_MAX_VSIZE{1000};
// These limits are within the default ancestor/descendant limits.
static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR <= DEFAULT_ANCESTOR_SIZE_LIMIT_KVB * 1000);
static_assert(V3_CHILD_MAX_VSIZE + MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR <= DEFAULT_DESCENDANT_SIZE_LIMIT_KVB * 1000);

/** Must be called for every transaction, even if not v3. Not strictly necessary for transactions
* accepted through AcceptMultipleTransactions.
*
* Checks the following rules:
* 1. A v3 tx must only have v3 unconfirmed ancestors.
* 2. A non-v3 tx must only have non-v3 unconfirmed ancestors.
* 3. A v3's ancestor set, including itself, must be within V3_ANCESTOR_LIMIT.
* 4. A v3's descendant set, including itself, must be within V3_DESCENDANT_LIMIT.
* 5. If a v3 tx has any unconfirmed ancestors, the tx's sigop-adjusted vsize must be within
* V3_CHILD_MAX_VSIZE.
*
*
* @param[in] mempool_ancestors The in-mempool ancestors of ptx.
* @param[in] direct_conflicts In-mempool transactions this tx conflicts with. These conflicts
* are used to more accurately calculate the resulting descendant
* count of in-mempool ancestors.
* @param[in] vsize The sigop-adjusted virtual size of ptx.
*
* @returns debug string if an error occurs, std::nullopt otherwise.
*/
std::optional<std::string> SingleV3Checks(const CTransactionRef& ptx,
const CTxMemPool::setEntries& mempool_ancestors,
const std::set<Txid>& direct_conflicts,
int64_t vsize);

/** Must be called for every transaction that is submitted within a package, even if not v3.
*
* For each transaction in a package:
* If it's not a v3 transaction, verify it has no direct v3 parents in the mempool or the package.
* If it is a v3 transaction, verify that any direct parents in the mempool or the package are v3.
* If such a parent exists, verify that parent has no other children in the package or the mempool,
* and that the transaction itself has no children in the package.
*
* If any v3 violations in the package exist, this test will fail for one of them:
* - if a v3 transaction T has a parent in the mempool and a child in the package, then PV3C(T) will fail
* - if a v3 transaction T has a parent in the package and a child in the package, then PV3C(T) will fail
* - if a v3 transaction T and a v3 (sibling) transaction U have some parent in the mempool,
* then PV3C(T) and PV3C(U) will fail
* - if a v3 transaction T and a v3 (sibling) transaction U have some parent in the package,
* then PV3C(T) and PV3C(U) will fail
* - if a v3 transaction T has a parent P and a grandparent G in the package, then
* PV3C(P) will fail (though PV3C(G) and PV3C(T) might succeed).
*
* @returns debug string if an error occurs, std::nullopt otherwise.
* */
std::optional<std::string> PackageV3Checks(const CTransactionRef& ptx, int64_t vsize,
const Package& package,
const CTxMemPool::setEntries& mempool_ancestors);

#endif // BITCOIN_POLICY_V3_POLICY_H
4 changes: 2 additions & 2 deletions src/rpc/mempool.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,7 @@ static RPCHelpMan testmempoolaccept()
result_inner.pushKV("txid", tx->GetHash().GetHex());
result_inner.pushKV("wtxid", tx->GetWitnessHash().GetHex());
if (package_result.m_state.GetResult() == PackageValidationResult::PCKG_POLICY) {
result_inner.pushKV("package-error", package_result.m_state.GetRejectReason());
result_inner.pushKV("package-error", package_result.m_state.ToString());
}
auto it = package_result.m_tx_results.find(tx->GetWitnessHash());
if (exit_early || it == package_result.m_tx_results.end()) {
Expand Down Expand Up @@ -907,7 +907,7 @@ static RPCHelpMan submitpackage()
case PackageValidationResult::PCKG_TX:
{
// Package-wide error we want to return, but we also want to return individual responses
package_msg = package_result.m_state.GetRejectReason();
package_msg = package_result.m_state.ToString();
CHECK_NONFATAL(package_result.m_tx_results.size() == txns.size() ||
package_result.m_tx_results.empty());
break;
Expand Down
Loading

0 comments on commit 7143d43

Please sign in to comment.