Skip to content
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.

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

Conservatively accept RBF bumps bumping one tx at the package limits #16421

Merged
merged 1 commit into from Sep 7, 2019

Conversation

@TheBlueMatt
Copy link
Contributor

commented Jul 18, 2019

Based on #15681, this adds support for some simple cases of RBF inside of large packages. Issue pointed out by sdaftuar in #15681, and this fix (or a broader one) is required to make #15681 fully useful.

Accept RBF bumps of single transactions (ie which evict exactly one
transaction) even when that transaction is a member of a package
which is currently at the package limit if the new transaction
does not add any additional mempool dependencies from the original.

This could be made a bit looser in the future and still be safe,
but for now this fixes the case that a transaction which was
accepted by the carve-out rule will not be directly RBF'able

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16400 (refactor: Rewrite AcceptToMemoryPoolWorker() using smaller parts by sdaftuar)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch from 5a64f35 to db4064e Jul 18, 2019
@fanquake fanquake requested a review from sdaftuar Jul 21, 2019
@jnewbery

This comment has been minimized.

Copy link
Member

commented Jul 22, 2019

Please rebase so this only contains the single commit for this PR.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 23, 2019

Rebased. Now just one commit + master.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch from db4064e to d3db3d7 Jul 23, 2019
@laanwj laanwj added this to Blockers in High-priority for review Jul 25, 2019
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch from d3db3d7 to e8bd0c5 Jul 29, 2019
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 29, 2019

Rebased after #16471. Also, @sdaftuar noted that I (a) misunderstood the meaning of setConflicts so the description in the comment was wrong in a few ways, and also we were allowing an RBF transaction to get in via the carve-out even when it shouldn't be allowed to (ie when it has more than one mempool ancestor). Sadly, after talking through it it doesn't seem like we'll be able to get away with no extra calls to CalculateMemPoolAncestors, but at least its a bit easier to reason about now (IMO).

Copy link
Member

left a comment

Looks on the right track to me, though I still need to think about the logic from scratch again and do some testing.

if (mempool.exists(input.prevout.hash)) {
conflict_existing_mempool_inputs.insert(input.prevout.hash);
}
}

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 29, 2019

Member

Can we just use GetMemPoolParents(conflict) and grab the hashes from there, instead of iterating all the inputs?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 30, 2019

Author Contributor

No, GetMemPoolParents just returns the parents set, which isn't actually filled until the very last step in addUnchecked.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 30, 2019

Member

The conflict tx is already in the mempool, so shouldn’t that work fine?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 31, 2019

Author Contributor

Oh right, was looking at the wrong line and thought you meant the new entry. Will fix.

return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", dummy_err_string);
}
}
}

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 29, 2019

Member

Similarly, if you hadn't cleared out setAncestors() a few lines up, I think you could skip having to walk all the inputs again here. Perhaps save that and then compare setAncestors to the mempool parents of conflictTx?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 30, 2019

Author Contributor

Right, but doesn't CalculateMemPoolAncestors return early if we hit a limit? I didn't want to start introducing some "if we return early, X is initialized, but Y is not" kind of invariants in CalculateMemPoolAncestors. If you think its worth it, though...

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 30, 2019

Member

Oh right, never mind.

}
}
}
if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants + 1,

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 29, 2019

Member

I think a comment would be helpful here to explain exactly what we're doing:

  • No need to bump ancestor sizes/limits (since those are only tested on the transaction itself and are unaffected by the presence of conflicting transactions)
  • Descendant size / count are bumped by conflict tx size and 1, to account for the removal of that transaction from all the ancestors of the new transaction. Since we require that the new transaction not introduce any new mempool-ancestors (compared with the conflict tx), this is sufficient to enforce our existing package limits.
}
if (!pool.CalculateMemPoolAncestors(entry, setAncestors, nLimitAncestors, nLimitAncestorSize, nLimitDescendants + 1,
nLimitDescendantSize + conflict->GetTxSize(), dummy_err_string)) {
if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 29, 2019

Member

Another comment would be helpful here, to explain that if we failed to RBF due to the package limit, we'll give it one more try using the carve-out rules. So we use the same ancestor limits that the carve-out provision requires, but bump the descendant limits to account for the to-be-removed transaction.

src/validation.cpp Outdated Show resolved Hide resolved
if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
!pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) {
return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
if (setConflicts.size() == 1) {

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 29, 2019

Member

This whole section could probably use a clearer comment that lays out the problem we have with evaluating package limits in the presence of RBF transactions, to help future readers of this code.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 19, 2019

Member

for readers of the comments, this has (attempted) to be addressed

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 30, 2019

Rewrote the comment at the top of the new block to now be a Mega Comment (tm). Hopefully its sufficient, though I can break it up and move it to corresponding code if you really want @sdaftuar, I just didn't bother cause its easy to rewrite without.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch from e8bd0c5 to ee401b3 Jul 30, 2019
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Concept ACK, though I'd prefer if this was solved for all mempool txs and not only for a special case of lightning txs.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch from ee401b3 to ab60396 Jul 31, 2019
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 31, 2019

@MarcoFalke This isn't completely targeted only at lightning/carve-out/contract applications, though it obviously ensures that carve-out transactions are a supported use-case. Mostly trying to implement this in the general case is Hard (tm), but this does work for any only-directly-conflicts-with-1-transaction tx.

@MarcoFalke MarcoFalke removed the Tests label Aug 1, 2019
Copy link
Contributor

left a comment

ACK ab60396 ; code review, check test failed before patch and succeeds afterwards.

I think the "RBFs a single tx without adding additional unconfirmed inputs" makes sense as a reasonably general rule, and doesn't seem especially lightning specific.

!pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) {
return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
if (setConflicts.size() == 1) {
// In general, when we receive an RBF transaction with mempool conflicts, we want to know whether we

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 15, 2019

Contributor

I wonder if extensive comments like this wouldn't be better as an informative BIP? Also it's probably a lost cause, but wrapping the comments at 80 columns or less rather than ~120 would be nice :)

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Aug 19, 2019

Author Contributor

80 chars is....very antiquated. I dunno anyone who is left coding on a 80x60 terminal :p.

Still, I don't think this deserves a BIP if only cause we hopefully can extend it further in the future.

src/validation.cpp Outdated Show resolved Hide resolved
if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
!pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, dummy_err_string)) {
return state.Invalid(ValidationInvalidReason::TX_MEMPOOL_POLICY, false, REJECT_NONSTANDARD, "too-long-mempool-chain", errString);
if (setConflicts.size() == 1) {

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 19, 2019

Member

for readers of the comments, this has (attempted) to be addressed

src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
src/validation.cpp Outdated Show resolved Hide resolved
// ensuring that (a) no package is growing past the package size (not count) limits and (b) we are
// not allowing something to effectively use the (below) carve-out spot when it shouldn't be allowed
// to.
// To check these we re-run CalculateMemPoolAncestors up to twice, once corresponding to the above,

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 19, 2019

Member

Having real trouble parsing this last paragraph.

Lots of different things being referenced, "above", "below", "corresponding", unsure exactly what it's referring to.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Aug 19, 2019

Author Contributor

Is this a bit more readable given the changes made for #16421 (review) ?

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch 2 times, most recently from 47552a1 to 8b1884f Aug 19, 2019
Copy link
Contributor

left a comment

ACK 8b1884f - rechecked code, compiled and reran some tests, compared to previously acked commit.

// being added is a new dependant, with no removals, of each parents' existing dependant set).
// The ancestor count limits are unmodified (as the ancestor limits should be the same for both our
// new transaction and any conflicts).
assert(setIterConflicting.size() == 1);

This comment has been minimized.

Copy link
@ajtowns

ajtowns Aug 20, 2019

Contributor

setIterConflicting.size() could theoretically be zero if the conflicting hash from setConflicts isn't in the mempool somehow. That shouldn't be possible because we've held a lock on cs_main and pool.cs since we populated setConflicts, so this assertion should be correct.

src/validation.cpp Outdated Show resolved Hide resolved
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch 2 times, most recently from 410db33 to b9dc087 Aug 20, 2019
Copy link
Member

left a comment

ACK b9dc087

The new test also fails without the logic fix.

  File "./test/functional/mempool_package_onemore.py", line 86, in run_test
    self.nodes[0].sendrawtransaction(signed_second_tx['hex'])
  File "/home/instagibbs/elements-dev/bitcoin/test/functional/test_framework/coverage.py", line 47, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
  File "/home/instagibbs/elements-dev/bitcoin/test/functional/test_framework/authproxy.py", line 141, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: too-long-mempool-chain, too many descendants for tx 6a9a7075b6bfa6f2e13b064d9ace83cc2a941598a05546b4aa68d02b9f55db90 [limit: 25] (code 64) (-26)
@@ -33,7 +33,7 @@ def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):
outputs = {}
for i in range(num_outputs):
outputs[node.getnewaddress()] = send_value
rawtx = node.createrawtransaction(inputs, outputs)
rawtx = node.createrawtransaction(inputs, outputs, 0, True)

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 30, 2019

Member

named args would make this more obvious what's going on without a comment

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

ACK b9dc087 ; changes since last ack are comments and dropping the "no new ancestors" check for this carve out; code review, checked compiled and tests pass.

Dropping the checks seems correct -- the original behaviour when conflicting with one tx should have been:

if (!has_new_ancestors && desc > LimitDesc+1) Invalid();
if (has_new_ancestors && desc > LimitDesc) Invalid();
if (has_new_ancestors) Invalid();

and it should now be if (desc > LimitDesc+1 || has_new_ancestors) Invalid(); which looks correct to me. And there's comments to document the dependency between the two bits of code, to help avoid introducing bugs in future.

Copy link
Member

left a comment

I think this works, but we can make it slightly better. I was trying to think through how we'd describe the change here, and it seemed to me the explanation is "fee bumping a single transaction according to BIP 125 should work". However, that is not quite the right explanation unless we factor in descendant size of the conflicting transaction, so I think we should go ahead and make that change.

src/validation.cpp Outdated Show resolved Hide resolved
CTxMemPool::txiter conflict = *setIterConflicting.begin();

nLimitDescendants += 1;
nLimitDescendantSize += conflict->GetTxSize();

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Sep 4, 2019

Member

I believe we can make this conflict->GetSizeWithDescendants() and still respect our mempool limits.

Without the size of the descendants, it's possible that an attempt to RBF a single transaction could fail if there are descendants of that transaction in the mempool, due to the mempool limits (even though those child transactions would be replaced as well). By accounting for the size of those transactions, we should be able to accommodate any RBF attempt that conflicts with a single transaction and introduces no new mempool ancestors.

This comment has been minimized.

Copy link
@instagibbs

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Sep 4, 2019

Author Contributor

Done! good idea.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Sep 5, 2019

Contributor

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;
}

Maybe I'm missing something though?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Sep 5, 2019

Author Contributor

I agree there's lots we can do in the future to improve things (not actually 100% sure on this one, these things are complicated...) lets try to land this as-is first, though, cause I do really want it for 0.19 still.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch from b9dc087 to c82c770 Sep 4, 2019
Accept RBF bumps of single transactions (ie which conflict with one
transaction) even when that transaction is a member of a package
which is currently at the package limit iff the new transaction
does not add any additional mempool dependencies from the original.

This could be made a bit looser in the future and still be safe,
but for now this fixes the case that a transaction which was
accepted by the carve-out rule will not be directly RBF'able.
@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-07-lightning-policy-bump branch from c82c770 to 5ce822e Sep 4, 2019
@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2019

Tweaked the increment to use GetSizeWithDescendants() instead and updated the comment (with some slight readability tweaks @instagibbs may appreciate).

@instagibbs

This comment has been minimized.

Copy link
Member

commented Sep 4, 2019

re-ACK 5ce822e

comment clarification and GetSizeWithDescendants change only.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2019

ACK 5ce822e ; GetSizeWithDescendants is only change and makes sense

@sipa

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

Code review ACK 5ce822e. I haven't thought hard about the effect on potential DoS issues this policy change may have.

fanquake added a commit that referenced this pull request Sep 7, 2019
…ackage limits

5ce822e Conservatively accept RBF bumps bumping one tx at the package limits (Matt Corallo)

Pull request description:

  Based on #15681, this adds support for some simple cases of RBF inside of large packages. Issue pointed out by sdaftuar in #15681, and this fix (or a broader one) is required ot make #15681 fully useful.

  Accept RBF bumps of single transactions (ie which evict exactly one
  transaction) even when that transaction is a member of a package
  which is currently at the package limit iff the new transaction
  does not add any additional mempool dependencies from the original.

  This could be made a bit looser in the future and still be safe,
  but for now this fixes the case that a transaction which was
  accepted by the carve-out rule will not be directly RBF'able

ACKs for top commit:
  instagibbs:
    re-ACK 5ce822e
  ajtowns:
    ACK 5ce822e ; GetSizeWithDescendants is only change and makes sense
  sipa:
    Code review ACK 5ce822e. I haven't thought hard about the effect on potential DoS issues this policy change may have.

Tree-SHA512: 1cee3bc57393940a30206679eb60c3ec8cb4f4825d27d40d1f062c86bd22542dd5944fa5567601c74c8d9fd425333ed3e686195170925cfc68777e861844bd55
@fanquake fanquake merged commit 5ce822e into bitcoin:master Sep 7, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fanquake fanquake removed this from Blockers in High-priority for review Sep 7, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

post-merge utACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.