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

[mempool] Allow one extra single-ancestor transaction per package #15681

Merged
merged 1 commit into from Jul 19, 2019

Conversation

@TheBlueMatt
Copy link
Contributor

commented Mar 28, 2019

This implements the proposed policy change from [1], which allows
certain classes of contract protocols involving revocation
punishments to use CPFP. Note that some such use-cases may still
want some form of one-deep package relay, though even this alone
may greatly simplify some lightning fee negotiation.

[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html

@fanquake fanquake added the Mempool label Mar 28, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 28, 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:

  • #16409 (Remove mempool expiry, treat txs as replaceable instead by MarcoFalke)
  • #16401 (Package relay by sdaftuar)
  • #16400 ([refactor] Rewrite AcceptToMemoryPoolWorker() using smaller parts by sdaftuar)
  • #16398 (rpc: testmempoolaccept for list of transactions by MarcoFalke)
  • #15921 (Tidy up ValidationState interface by jnewbery)

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.

// and has at most one ancestor (ie ancestor limit of 2, including
// the new transaction), allow it if its parent has exactly the
// descendant limit descendants.
if (nSize <= 10000 && !pool.CalculateMemPoolAncestors(entry, setAncestors, 2, 1000000, nLimitDescendants + 1, nLimitDescendantSize + 10000, errString)) {

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 28, 2019

Member

I'm still chewing on the comment above, but doesn't this line let transactions >40k weight through no matter what?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Mar 28, 2019

Author Contributor

Lol, oops, one of these days I'll learn to code.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 28, 2019

Member

did the tests work? ;D

This comment has been minimized.

Copy link
@jachiang

jachiang Jul 1, 2019

@TheBlueMatt I have a spill-over question from the PR review club which covered this PR last week. I am unsure of why the LimitAncestorSize for the carve-out tx has been increased to the block-size.

EDIT: @harding has helped tidy up my understanding of this:

In the case of a (to-be-fee-bumped) LN parent TX with two hooks (A and B), which is close or equal to the descendant-size-limit, only a single (carve-out) child can be accepted, since the initial package size limit has been reached with the parent TX alone. This would seem like a race between A and B for the carve-out.

In regards to the 1M ancestor-size-limit for the carve-out, is it increased to 1M to prevent the ancestor-size-limit from interfering with the carve-out? I suppose this prevents a custom ancestor-size-limit setting from blocking the carve-out and subsequent network propagation.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 2, 2019

Author Contributor

Indeed, we don't really care about the size of the ancestor transaction here (its limited to the standard single-tx size limit, or 100k today), so I just set it to 1M, we already check that there is only, at max, one unconfirmed parent.

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-03-lightning-policy branch from e7e46b3 to a3053fd Mar 28, 2019

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-03-lightning-policy branch from a3053fd to 87c902d Mar 28, 2019

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Mar 28, 2019

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-03-lightning-policy branch from 87c902d to 891ae7b Mar 28, 2019


# Build a transaction that spends parent_txid:vout
# Return amount sent
def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):

This comment has been minimized.

Copy link
@practicalswift

practicalswift Apr 2, 2019

Member

Nit: This method doesn't use self: could be made a function?

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

agree that this would be better as a function than a method

src/validation.cpp Show resolved Hide resolved

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-03-lightning-policy branch from 891ae7b to 69959a5 Jun 5, 2019

@DrahtBot DrahtBot removed the Needs rebase label Jun 5, 2019

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-03-lightning-policy branch from 69959a5 to f1facdd Jul 2, 2019

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 2, 2019

Resolved comments.

@rustyrussell
Copy link
Contributor

left a comment

Concept Ack.

After much discussion, this does seem to be the minimal-intervention fix. Thanks Matt.

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Jul 3, 2019

Concept ACK.

I did some review and light testing, and I noticed that there seems to be a general problem with our RBF logic: if we have some parent transaction whose descendant count is maxed out in the mempool, then we're unable to rbf any child transaction, because we evaluate the descendant limits of a new transactions ancestors prior to looking at what might be evicted by that transaction. (However, we can generally RBF the parent itself!)

So in particular, that means that the new transaction that would be carved out by this policy would not be able to be RBF'ed in the situation that the parent transactions' descendant limit has been reached.

I'm guessing we may want to fix that to provide additional footgun protection to users that would take advantage of this new behavior, though I'll leave it to the PR author to decide if it is worth doing in this PR or separately.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jul 8, 2019

Concept ACK. Might be worth adding an explicit test case for RBF case @sdaftuar points out:

         # But not if it chains directly off the first transaction
         self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1)
+
+        # Sadly, RBFing that should fail, however
+        assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, 0, [chain[0][0]], [1], chain[0][1], fee*2, 1)
+
         # and the second chain should work just fine
         self.chain_transaction(self.nodes[0], [second_chain], [0], second_chain_value, fee, 1)

(means createrawtransaction needs to be called with replaceable=True as well)

Could change chain_transaction(self, node, ...) to always use node=self.nodes[0] which would simplify calls, and deal with @practicalswift's nit.

Could define a const for the 10k magic number. Is there any reason to have 1M instead of reusing nLimitAncestorSize? (Could not allow the carve out at all if nLimitAncestors <= 1 though that's probably getting a bit ridiculous)

I think this should work for two-party lightning channels, because the outputs will be a bunch of CSV-limited ones that won't be in the mempool, and one CPFP output for each party. So at most one output can be spammed, and that only up to the descendent limit, so the plus-one here should work fine. If there were three or more immediately-spendable outputs (for channel factories and multiparty eltoo eg), I don't think this would be enough. But that's fine today.

I guess it's not very useful to log when this carve out triggers -- it's meant to prevent attacks from being effective, so while it's there there won't be any attacks so it'll hardly ever be used, but if it weren't there there would be attacks and it would be useful.

Should this carve out be limited to only apply when it's actually raising the parent's effective fee rate? I think you'd have to loop over all the descendents of your parent, work out the best ancestor fee rate of any of them, then see if the new tx and the parent have a better combined fee rate than that. That seems like it would add a chunk of complexity though, so probably isn't worth it.

This seems like it's a sufficient workaround for current problems, and is a pretty minimal change, while doing anything much better would be lots more complicated. So: Approach ACK.

@ryanofsky
Copy link
Contributor

left a comment

ACK f1facdd with minimal testing. I verified new test fails without policy change and succeeds with it. I left only minor review comments which can be ignored.

I do like ajtowns's RBF test from #15681 (comment), and think it'd be nice if that were added.

src/validation.cpp Outdated Show resolved Hide resolved

# Build a transaction that spends parent_txid:vout
# Return amount sent
def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jul 8, 2019

Contributor

Not great how this function and the test setup here is mostly duplicated from mempool_packages.py. Someone could clean it up later, though.

vout = 0
value = sent_value
chain.append([txid, value])
for _ in range(MAX_ANCESTORS - 4):

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jul 8, 2019

Contributor

Having two basically identical loops just to add an extra output to the first four transactions seems like an odd choice. Maybe prefer

for depth in range(MAX_ANCESTORS):
    num_outputs = 2 if depth < 4 else 1
    ...

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 9, 2019

Author Contributor

I find this more readable.

chain.append([txid, value])
(second_chain, second_chain_value) = self.chain_transaction(self.nodes[0], [utxo[1]['txid']], [utxo[1]['vout']], utxo[1]['amount'], fee, 1)

# Check mempool has MAX_ANCESTORS transactions in it, and descendant and ancestor

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jul 8, 2019

Contributor

Stale comment from the old test, now MAX_ANCESTORS+1

# Adding one more transaction on to the chain should fail.
assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [txid], [0], value, fee, 1)
# ...even if it chains on from some point in the middle of the chain.
assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[2][0]], [1], chain[2][1], fee, 1)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jul 8, 2019

Contributor

These tests would be more readable, and zip() above could be dropped if chain_transaction just took a list of txid/nout tuples

This comment has been minimized.

Copy link
@ajtowns

ajtowns Jul 9, 2019

Contributor

FWIW, I thought the way it is seemed more readable. YMMV obviously :)

for _ in range(MAX_ANCESTORS - 4):
(txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [0], value, fee, 1)
value = sent_value
chain.append([txid, value])

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Jul 8, 2019

Contributor

Imo, tests below would be more reable if this were

chain.append(txid)
values.append(value)

to get rid of all the 0/1 indexing

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

+1, or used a namedtuple

[mempool] Allow one extra single-ancestor transaction per package
This implements the proposed policy change from [1], which allows
certain classes of contract protocols involving revocation
punishments to use CPFP. Note that some such use-cases may still
want some form of one-deep package relay, though even this alone
may greatly simplify some lightning fee negotiation.

[1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html

@TheBlueMatt TheBlueMatt force-pushed the TheBlueMatt:2019-03-lightning-policy branch from f1facdd to 50cede3 Jul 9, 2019

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor Author

commented Jul 9, 2019

@ajtowns I'd prefer to not add the proposed test change, as @sdaftuar's comment points out a critical limitation, and one I'd like to get resolved before this gets released, though not necessarily in this PR. I'd rather just fix the bug than add a test to ensure that the bug exists.

As for general policy questions: this doesn't result in any new transactions being accepted which violate min fee, nor does it change eviction criteria when mempool fills. What this does do, is tweak our already-somewhat-arbitrary-but-sadly-neccessary anti-DoS policies to allow one more (potentially-in-the-future) common case so that we can maximize fee revenue in the case this does get used. Thus, I don't see any reason to need to bend over backwards to make it more restrictive, if anything, less restrictive is better.

@sdaftuar

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

ACK f1facdd

@ryanofsky
Copy link
Contributor

left a comment

utACK 50cede3. Changes since last review: adding EXTRA_DESCENDANT_TX_SIZE_LIMIT constant, changing max ancestor size from 1,000,000 to nLimitAncestorSize constant (101,000), fixing test comment and getting rid of unused test node.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

ACK 50cede3 -- looked over code again, compared with previous commit, compiles, etc.

I think this is a sufficient improvement even with the problem @sdaftuar points out:

  • if there's no adversarial situation and you hit the descendent limit with low fee children just due to carelessness, this carveout can be used to RBF the first child (paying 25 times the feerate to account for the descendants' fees) and make CPFP actually work in ways where you can't do anything now
  • in the event of an adversarial situation with only two immediately spendable outputs, if you do the CPFP first, they can't block you from RBF'ing when you need to (they can't make use of the onemore carve out if they only have one output available -- they need to use it to hit the descant limit, at which point it's not available for onemore).
  • to DoS their channel partner, an adversary would need to be confident their partner will CPFP by too small an amount, and do their spam low-fee tx spamming before they do that. If the CPFP is a high enough amount, they'll likely lose the fees for their spam once the mempool clears out, making that not a free attack, as well.

It would obviously still be better to find a more general fix, but this seems to me a worthwhile improvement without introducing much of a maintenance cost.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Jul 16, 2019

Patch to mempool_package_onemore.py for checking the "can RBF if I've added too many descendants myself" case. Maybe useful for reviewers, not suitable for inclusion since it kills the checks for the intended usecase:

+++ b/test/functional/mempool_package_onemore.py
@@ -33,7 +33,7 @@ class MempoolPackagesTest(BitcoinTestFramework):
         outputs = {}
         for i in range(num_outputs):
             outputs[node.getnewaddress()] = send_value
-        rawtx = node.createrawtransaction(inputs, outputs)
+        rawtx = node.createrawtransaction(inputs, outputs, 0, True)
         signedtx = node.signrawtransactionwithwallet(rawtx)
         txid = node.sendrawtransaction(signedtx['hex'])
         fulltx = node.getrawtransaction(txid, 1)
@@ -74,6 +74,12 @@ class MempoolPackagesTest(BitcoinTestFramework):
         assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0], second_chain], [1, 0], chain[0][1] + second_chain_value, fee, 1)
         # ...especially if its > 40k weight
         assert_raises_rpc_error(-26, "too-long-mempool-chain", self.chain_transaction, self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 350)
+
+        # can even RBF the original tx
+        self.chain_transaction(self.nodes[0], [chain[0][0]], [0], chain[0][1], fee*25, 10)
+        assert_equal(len(self.nodes[0].getrawmempool(True)), 3) # parent, second_chain, RBF'd original
+        return
+
         # But not if it chains directly off the first transaction
         self.chain_transaction(self.nodes[0], [chain[0][0]], [1], chain[0][1], fee, 1)
         # and the second chain should work just fine
@sdaftuar

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

ACK 50cede3

@laanwj laanwj merged commit 50cede3 into bitcoin:master Jul 19, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 19, 2019

Merge #15681: [mempool] Allow one extra single-ancestor transaction p…
…er package

50cede3 [mempool] Allow one extra single-ancestor transaction per package (Matt Corallo)

Pull request description:

  This implements the proposed policy change from [1], which allows
  certain classes of contract protocols involving revocation
  punishments to use CPFP. Note that some such use-cases may still
  want some form of one-deep package relay, though even this alone
  may greatly simplify some lightning fee negotiation.

  [1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html

ACKs for top commit:
  ajtowns:
    ACK 50cede3 -- looked over code again, compared with previous commit, compiles, etc.
  sdaftuar:
    ACK 50cede3
  ryanofsky:
    utACK 50cede3. Changes since last review: adding EXTRA_DESCENDANT_TX_SIZE_LIMIT constant, changing max ancestor size from 1,000,000 to nLimitAncestorSize constant (101,000), fixing test comment and getting rid of unused test node.

Tree-SHA512: b052c2a0f384855572b4579310131897b612201214b5abbb225167224e4f550049e300b471dbf320928652571e92ca2d650050b7cf39ac92b3bc1d2bcd386c1c
@jnewbery
Copy link
Member

left a comment

Quick code-review ACK 50cede3.

There's one logging bug that I think we should fix. I've added a bunch of nits to the test, but I agree with other reviewers that we could probably avoid a bunch of test code duplication by merging mempool_package_onemore.py with mempool_packages.py.

// outputs - one for each counterparty. For more info on the uses for
// this, see https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html
if (nSize > EXTRA_DESCENDANT_TX_SIZE_LIMIT ||
!pool.CalculateMemPoolAncestors(entry, setAncestors, 2, nLimitAncestorSize, nLimitDescendants + 1, nLimitDescendantSize + EXTRA_DESCENDANT_TX_SIZE_LIMIT, errString)) {

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

Passing errString through to the second call of CalculateMemPoolAncestors() means that the error string in the CValidationState object will be incorrect because of the changed ancestor/descendant size/count parameters. Really we should declare a dummy_error_string to pass into the second call, and use the original errString in the call to state.Invalid() below.

# file COPYING or http://www.opensource.org/licenses/mit-license.php.
"""Test descendant package tracking carve-out allowing one final transaction in
an otherwise-full package as long as it has only one parent and is <= 10k in
size.

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

nit: virtual size (or define this in terms or transaction weight)

def run_test(self):
# Mine some blocks and have them mature.
self.nodes[0].generate(101)
utxo = self.nodes[0].listunspent(10)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

nit: it's preferable to use named transactions to make the test more readable.

MAX_ANCESTORS = 25
MAX_DESCENDANTS = 25

class MempoolPackagesTest(BitcoinTestFramework):

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

nit: name this class MempoolPackageOnemoreTest


# Build a transaction that spends parent_txid:vout
# Return amount sent
def chain_transaction(self, node, parent_txids, vouts, value, fee, num_outputs):

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

agree that this would be better as a function than a method

def run_test(self):
# Mine some blocks and have them mature.
self.nodes[0].generate(101)
utxo = self.nodes[0].listunspent(10)

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

nit: name this variable utxos or define it as self.nodes[0].listunspent(10)[0]

for _ in range(MAX_ANCESTORS - 4):
(txid, sent_value) = self.chain_transaction(self.nodes[0], [txid], [0], value, fee, 1)
value = sent_value
chain.append([txid, value])

This comment has been minimized.

Copy link
@jnewbery

jnewbery Jul 22, 2019

Member

+1, or used a namedtuple

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 29, 2019

Merge bitcoin#15681: [mempool] Allow one extra single-ancestor transa…
…ction per package

50cede3 [mempool] Allow one extra single-ancestor transaction per package (Matt Corallo)

Pull request description:

  This implements the proposed policy change from [1], which allows
  certain classes of contract protocols involving revocation
  punishments to use CPFP. Note that some such use-cases may still
  want some form of one-deep package relay, though even this alone
  may greatly simplify some lightning fee negotiation.

  [1] https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2018-November/016518.html

ACKs for top commit:
  ajtowns:
    ACK 50cede3 -- looked over code again, compared with previous commit, compiles, etc.
  sdaftuar:
    ACK 50cede3
  ryanofsky:
    utACK 50cede3. Changes since last review: adding EXTRA_DESCENDANT_TX_SIZE_LIMIT constant, changing max ancestor size from 1,000,000 to nLimitAncestorSize constant (101,000), fixing test comment and getting rid of unused test node.

Tree-SHA512: b052c2a0f384855572b4579310131897b612201214b5abbb225167224e4f550049e300b471dbf320928652571e92ca2d650050b7cf39ac92b3bc1d2bcd386c1c

laanwj added a commit that referenced this pull request Jul 29, 2019

Merge #16471: [mempool] log correct messages when CPFP fails
42a5e91 [mempool] log correct messages when CPFP fails (John Newbery)

Pull request description:

  Fixes a logging issue introduced in #15681

ACKs for top commit:
  laanwj:
    ACK 42a5e91 (+utACK from bluematt that isn't registered because it has no commit id)

Tree-SHA512: ff5f423cc4d22838eea00c5b1d39ceda89cd61474c72f256a97c698eb0ec3f2156a97139f537669376132902c1e3943bf84c356a4b98a9a306b4ec57302c2761
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.