policy: allow RBF descendant carveout whenever conflicts exist, not just when number of conflicts == 1

[fanquake]

Comments left over from #16421.

From aj:

Can't we actually cope with merging packages this way too though? If you've got a tx that has parents A, B, C; and that conflicts with tx's X (parent A) and Y (parent B), then beforehand you had:

..., A, X, [children X]
..., B, Y, [children Y]

(maybe some tx's were descendants of both X and Y, but it's worse if there weren't any like that) and afterwards you have:

..., A, tx
..., B, tx

You don't have C in the mempool because that would fail the "replacement-adds-unconfirmed" test later.

So you know tx's ancestor checks pass, because they're actually worked out; you know A,B's ancestor checks pass because they don't change, tx's descendant check is trivial, and you know A,B and all their parent's descendant checks passed, because they did so when X and Y were there -- as far as sizes go, if they were all at their limit, then the max size for tx is the minimum of the descendant sizes of each tx that was replaced.

So I think you could replace the setConflicts.size() == 1 test with:

if (!setConflicts.empty()) {
   auto conflict = setIterConflicting.begin();
   assert(conflict != setIterConflicting.end());
   uint64_t bump = (*conflict)->GetSizeWithDescendants();
   while(++conflict != setIterConflicting.end()) {
       bump = std::min(bump, (*conflict)->GetSizeWithDescendants());
   }
   nLimitDescendants += 1;
   nLimitDescendantSize += bump;
}

From sipa

I haven't thought hard about the effect on potential DoS issues this policy change may have.

[TheBlueMatt]

Indeed, I think relaxing the rules there to allow more packages to be RBF'd would be nice, though I haven't had a chance to think too hard about aj's suggested changes. May also be pending a bit on use-cases or demand popping up for it.

[sdaftuar]

This is strictly looser than what we do right now, because it lines up with existing behavior in the case where setConflicts.size() == 1, and additionally would allow some replacements if > 1. However, it's a bit more complicated to describe the circumstances under which the replacement would succeed: using the minimum of size_with_descendants of each directly conflicting transaction should work for respecting chain limits, but it's conservative and I imagine it could be difficult for a wallet author to try to determine under what circumstances they should conflict with additional small transactions versus restricting to just replacing a single large transaction.

In particular, I think you could end up in a situation where, in order to conflict with and replace two transactions, you would have to first relay additional transactions to pump up the size_with_descendants of one of them(!). Imagine two mempool transactions A and B, where A has a large size_with_descendants and has a parent near its descendant size limit, while B is small and has no in-mempool parents. Then the minimum size_with_descendants of A and B is small, so any attempt to replace the two transactions with a new one would fail (because the bump size will be too low to pass the descendant size limits associated with A's parents) -- unless B's size_with_descendants were increased first. This is obviously counterintuitive (and wasteful), so I think we should stick with simpler behavior that is easier to explain to wallet authors...

[fanquake]

@glozow is it worth keeping this issue open? Should it be combined with another one etc?

[glozow]

Renamed the title to be more descriptive. There's definitely room to make this logic better, though I haven't heard anybody have issues with it specifically, so it's lower on my priority list. I think we can keep this open as 1 potential improvement or discussion thread for brainstorming.

If anybody is interested, my current package RBF implementation preserves the current behavior by making this carveout per-transaction (glozow@24d8536). More explanation in this comment #25038 (comment).

[glozow]

AFAIK, #27677 gets rid of RBF carveout / fixes this, as it instead calculates what the mempool would look like after removing the replacees and adding the new transaction(s)?

[instagibbs]

I think we can just close this one; we're not going to improve carveouts at this point

[glozow]

Yeah the new design stages the changes and then checks policy limits. Currently in #28676:

bitcoin/src/validation.cpp

Lines 1359 to 1363 in 61f5154

	// Check if the transaction would exceed the cluster size limit.
	if (!m_subpackage.m_changeset->CheckMemPoolPolicyLimits()) {
	ws.m_state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "too-large-cluster", "");
	return MempoolAcceptResult::Failure(ws.m_state);
	}

So the carveout becomes unnecessary since we aren't estimating cluster size, but calculating it