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

nSequence-based Full-RBF opt-in #6871

Merged
merged 9 commits into from Nov 27, 2015

Conversation

Projects
None yet
@petertodd
Contributor

petertodd commented Oct 22, 2015

Replaces transactions already in the mempool if a new transaction seen with a higher fee, specifically both a higher fee per KB and a higher absolute fee. Children are evaluated for replacement as well, using the mempool package tracking to calculate replaced fees/size efficiently. Transactions opt-in to transaction replacement by setting nSequence < maxint-1 on at least one input. (which no wallet currently does)

No first-seen-safe functionality, but that can easily be added as a separate pull if there's demand from wallet vendors.

If anyone feels like converting the tests to the internal python library used by the qa/rpc-tests, pull-reqs accepted.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Oct 22, 2015

Member

Feedback I heard from wallet vendors previously was that FSS was burdensome (needed extra txins, and so less useful).

Member

gmaxwell commented Oct 22, 2015

Feedback I heard from wallet vendors previously was that FSS was burdensome (needed extra txins, and so less useful).

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Oct 22, 2015

Contributor

@gmaxwell Same feedback I've heard too.

Contributor

petertodd commented Oct 22, 2015

@gmaxwell Same feedback I've heard too.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Oct 22, 2015

Contributor

Concept ACK.

Contributor

TheBlueMatt commented Oct 22, 2015

Concept ACK.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Oct 22, 2015

Member

Concept ACK.

Member

btcdrak commented Oct 22, 2015

Concept ACK.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 22, 2015

Contributor

concept ACK

Contributor

dcousens commented Oct 22, 2015

concept ACK

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 22, 2015

Contributor

How does this work with CPFP, and descendent transactions (e.g a chained transaction that could have its based replaced?)

Contributor

dcousens commented Oct 22, 2015

How does this work with CPFP, and descendent transactions (e.g a chained transaction that could have its based replaced?)

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Oct 22, 2015

Contributor

@dcousens What do you mean "how does this work" with CPFP? CPFP isn't implemented in Bitcoin Core, so there's nothing to affect.

Now, when replacing a tx with children, the fees and size of all children are taken into account before deciding to replace or not.

Contributor

petertodd commented Oct 22, 2015

@dcousens What do you mean "how does this work" with CPFP? CPFP isn't implemented in Bitcoin Core, so there's nothing to affect.

Now, when replacing a tx with children, the fees and size of all children are taken into account before deciding to replace or not.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Oct 22, 2015

Contributor

@dcousens What do you mean "how does this work" with CPFP? CPFP isn't implemented in Bitcoin Core, so there's nothing to affect.

I meant conceptually, sorry. CPFP is something that was on the roadmap AFAIK?

all children are taken into account before deciding to replace or not.

By taken into account, do you mean that you have to have a higher fee than all subsequent children to replace?

Contributor

dcousens commented Oct 22, 2015

@dcousens What do you mean "how does this work" with CPFP? CPFP isn't implemented in Bitcoin Core, so there's nothing to affect.

I meant conceptually, sorry. CPFP is something that was on the roadmap AFAIK?

all children are taken into account before deciding to replace or not.

By taken into account, do you mean that you have to have a higher fee than all subsequent children to replace?

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Oct 22, 2015

Contributor

By taken into account, do you mean that you have to have a higher fee than all subsequent children?

Yes.

Only extremely sophisticated CPFP that does relaying of whole packages of transactions based on fees paid by children is affected by RBF, and no-one has any plans to actually implement that yet.

Contributor

petertodd commented Oct 22, 2015

By taken into account, do you mean that you have to have a higher fee than all subsequent children?

Yes.

Only extremely sophisticated CPFP that does relaying of whole packages of transactions based on fees paid by children is affected by RBF, and no-one has any plans to actually implement that yet.

@petertodd petertodd changed the title from Full-RBF with opt-out to nSequence-based Full-RBF opt-in Oct 23, 2015

@greenaddress

This comment has been minimized.

Show comment
Hide comment
@greenaddress

greenaddress Oct 23, 2015

Contributor

Concept ACK

Contributor

greenaddress commented Oct 23, 2015

Concept ACK

@rubensayshi

This comment has been minimized.

Show comment
Hide comment
@rubensayshi

rubensayshi Oct 23, 2015

Contributor

concept ACK

Contributor

rubensayshi commented Oct 23, 2015

concept ACK

@laanwj laanwj added the Mempool label Oct 23, 2015

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Oct 23, 2015

Member

Concept ACK

Member

jtimon commented Oct 23, 2015

Concept ACK

Show outdated Hide outdated src/main.cpp
// the replacements.
CFeeRate oldFeeRate(nConflictingFees, nConflictingSize);
CFeeRate newFeeRate(nFees, nSize);
if (newFeeRate <= oldFeeRate)

This comment has been minimized.

@sdaftuar

sdaftuar Oct 23, 2015

Member

I'm not sure this comparison makes sense. The existence of a low-fee-rate descendant doesn't make a transaction worse for miners, but it would cause the feerate to look worse in this comparison.

@sdaftuar

sdaftuar Oct 23, 2015

Member

I'm not sure this comparison makes sense. The existence of a low-fee-rate descendant doesn't make a transaction worse for miners, but it would cause the feerate to look worse in this comparison.

This comment has been minimized.

@petertodd

petertodd Oct 23, 2015

Contributor

So, you mean the scenario where we have a high fee-rate transaction that is spent by one or more low fee-rate transactions? For instance suppose we have two transactions in our mempool: tx1a, 1KB w/ 1mBTC fee, which is spent by tx2, 10KB w/ 1mBTC fee. We get tx1b, 10KB w/ 2.1mBTC fee. Since the overall feerate of tx1b is higher than tx1a+tx2, it'll be accepted, even though a miner might have rather mined tx1a instead, ignoring tx2 (for now).

I agree that's less than optimal. If we make the assumption that there's always more demand for blockchain space than supply, it might be reasonable for the replacement logic criteria to be whether or not we're increasing the fee-rate of any subset of the mempool. (while still paying more fees than the replaced transactions)

Without taking CPFP into account, you could simply use the same kind of priority heap logic as in CreateNewBlock() on the list of transactions that would be replaced by the new transaction. You'd iterate through the heap from highest priority to lowest, stopping once you had found as many bytes worth of transactions as the candidate replacement. If the fee-rate of the replacement is higher than the fee-rate of those transactions, accept the replacement into the mempool.

To take CPFP into account... Thinking about it more the mempool package tracking is probably backwards from what we want. Right now it tracks descendants, when really what we want to know is "what's the total fee-rate if I mine this transaction, and all ancestors?" If we kept track of "packages" that way, we'd be able to do the comparison by determining if the total fee-rate of the new package created by the replacement is higher than the fee-rates of all the packages invalidated by it. I actually did some work on something along these lines a few years ago, though didn't finish it - the implementation is a lot simpler now that we have strict ancestor limits. (when limiting, just throw away the tx associated with the lowest fee-rate package, which is guaranteed to have no descendants)

For now though I'd be inclined to merge this PR as-is, as all the above options are pretty complex. I also don't see any way this code could be used in an DoS attack.

@petertodd

petertodd Oct 23, 2015

Contributor

So, you mean the scenario where we have a high fee-rate transaction that is spent by one or more low fee-rate transactions? For instance suppose we have two transactions in our mempool: tx1a, 1KB w/ 1mBTC fee, which is spent by tx2, 10KB w/ 1mBTC fee. We get tx1b, 10KB w/ 2.1mBTC fee. Since the overall feerate of tx1b is higher than tx1a+tx2, it'll be accepted, even though a miner might have rather mined tx1a instead, ignoring tx2 (for now).

I agree that's less than optimal. If we make the assumption that there's always more demand for blockchain space than supply, it might be reasonable for the replacement logic criteria to be whether or not we're increasing the fee-rate of any subset of the mempool. (while still paying more fees than the replaced transactions)

Without taking CPFP into account, you could simply use the same kind of priority heap logic as in CreateNewBlock() on the list of transactions that would be replaced by the new transaction. You'd iterate through the heap from highest priority to lowest, stopping once you had found as many bytes worth of transactions as the candidate replacement. If the fee-rate of the replacement is higher than the fee-rate of those transactions, accept the replacement into the mempool.

To take CPFP into account... Thinking about it more the mempool package tracking is probably backwards from what we want. Right now it tracks descendants, when really what we want to know is "what's the total fee-rate if I mine this transaction, and all ancestors?" If we kept track of "packages" that way, we'd be able to do the comparison by determining if the total fee-rate of the new package created by the replacement is higher than the fee-rates of all the packages invalidated by it. I actually did some work on something along these lines a few years ago, though didn't finish it - the implementation is a lot simpler now that we have strict ancestor limits. (when limiting, just throw away the tx associated with the lowest fee-rate package, which is guaranteed to have no descendants)

For now though I'd be inclined to merge this PR as-is, as all the above options are pretty complex. I also don't see any way this code could be used in an DoS attack.

This comment has been minimized.

@sdaftuar

sdaftuar Oct 26, 2015

Member

Right, the distinction between ancestor and descendant package is what I was getting at.

Descendant packages are I think the right thing to use for mempool limiting. I don't follow what you're saying about a limiting algorithm that uses fee with ancestors -- it's entirely possible that the worst thing under that sort would have descendants in the mempool.

(FYI I have a branch that implements ancestor packages. I might propose merging it at some point if it looks like we can take advantage of it in the mining code, but I'm not ready to advocate that now.)

Anyway in this code, we are comparing the feerate of the replacing transaction (with uncalculated/unknown fee rate including its ancestors) to an estimate of the feerate of the descendants of all the conflicting transactions. This strikes me as incorrect on both fronts; by not putting any bounds on the ancestor fee rate, we might be accepting a replacement transaction that is unlikely to confirm anytime soon. On the other hand, by looking at feerate with children (and overcounting those children, at least in the current implementation), we might be making it so that miners would prefer the original transactions to the replacing one.

I don't know that either of these issues constitutes an attack, but I do think it's useful to try to help users avoid shooting themselves in the foot, say by accidentally adding an input that is part of a long unconfirmed chain (causing the replacing transaction to be worse), and to give miners code that doesn't require further optimization to do the economically rational thing.

So with that in mind, how about this:

  • Require that any new inputs that show up in the replacing transaction be already confirmed. In the future, if we do merge something like ancestor package tracking and better mining code, we could update this test appropriately.
  • Require that for each entry E in setConflicts,
    feerate(replacing transaction) > max(feerate of E, feerate of E with descendants). That doesn't completely eliminate the possibility that it could be somehow worse for a miner to accept the new transaction, but it eliminates some degenerate cases (where a high fee rate transaction is dragged down by lower fee rate transactions) and is easy to calculate.
@sdaftuar

sdaftuar Oct 26, 2015

Member

Right, the distinction between ancestor and descendant package is what I was getting at.

Descendant packages are I think the right thing to use for mempool limiting. I don't follow what you're saying about a limiting algorithm that uses fee with ancestors -- it's entirely possible that the worst thing under that sort would have descendants in the mempool.

(FYI I have a branch that implements ancestor packages. I might propose merging it at some point if it looks like we can take advantage of it in the mining code, but I'm not ready to advocate that now.)

Anyway in this code, we are comparing the feerate of the replacing transaction (with uncalculated/unknown fee rate including its ancestors) to an estimate of the feerate of the descendants of all the conflicting transactions. This strikes me as incorrect on both fronts; by not putting any bounds on the ancestor fee rate, we might be accepting a replacement transaction that is unlikely to confirm anytime soon. On the other hand, by looking at feerate with children (and overcounting those children, at least in the current implementation), we might be making it so that miners would prefer the original transactions to the replacing one.

I don't know that either of these issues constitutes an attack, but I do think it's useful to try to help users avoid shooting themselves in the foot, say by accidentally adding an input that is part of a long unconfirmed chain (causing the replacing transaction to be worse), and to give miners code that doesn't require further optimization to do the economically rational thing.

So with that in mind, how about this:

  • Require that any new inputs that show up in the replacing transaction be already confirmed. In the future, if we do merge something like ancestor package tracking and better mining code, we could update this test appropriately.
  • Require that for each entry E in setConflicts,
    feerate(replacing transaction) > max(feerate of E, feerate of E with descendants). That doesn't completely eliminate the possibility that it could be somehow worse for a miner to accept the new transaction, but it eliminates some degenerate cases (where a high fee rate transaction is dragged down by lower fee rate transactions) and is easy to calculate.

This comment has been minimized.

@petertodd

petertodd Oct 30, 2015

Contributor

Fixed both these issues.

I decided not to do the max() version of this, as I think the requirement that the new fees > total-replaced-fees is sufficient; might help to get txs unstuck in some cases, and non-CPFP miners aren't evaluating that anyway.

@petertodd

petertodd Oct 30, 2015

Contributor

Fixed both these issues.

I decided not to do the max() version of this, as I think the requirement that the new fees > total-replaced-fees is sufficient; might help to get txs unstuck in some cases, and non-CPFP miners aren't evaluating that anyway.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Oct 26, 2015

Member

Concept ACK. FSS is a pain.

Member

instagibbs commented Oct 26, 2015

Concept ACK. FSS is a pain.

Show outdated Hide outdated qa/replace-by-fee/rbf-tests.py
# Make sure mining works
mempool_size = 1
while mempool_size:
cls.proxy.generate(1)

This comment has been minimized.

@sdaftuar

sdaftuar Oct 26, 2015

Member

When I check out the commit referenced in the README and run the test, I get an error:
AttributeError: 'Proxy' object has no attribute 'generate'

I think this function is not defined in the Proxy class? Adding it in the appropriate place in python-bitcoinlib seems to fix it.

@sdaftuar

sdaftuar Oct 26, 2015

Member

When I check out the commit referenced in the README and run the test, I get an error:
AttributeError: 'Proxy' object has no attribute 'generate'

I think this function is not defined in the Proxy class? Adding it in the appropriate place in python-bitcoinlib seems to fix it.

This comment has been minimized.

@instagibbs

instagibbs Oct 26, 2015

Member

It's not included, no. You can just do a "call" instead.

proxy.call("generate", 1)

@instagibbs

instagibbs Oct 26, 2015

Member

It's not included, no. You can just do a "call" instead.

proxy.call("generate", 1)

This comment has been minimized.

@petertodd

petertodd Oct 30, 2015

Contributor

Fixed.

I forgot to add generate() when python-bitcoinlib dropped support for calling RPC commands implicitly; replaced with .call() so as to continue to use the official v0.5.0 release rather than git master.

@petertodd

petertodd Oct 30, 2015

Contributor

Fixed.

I forgot to add generate() when python-bitcoinlib dropped support for calling RPC commands implicitly; replaced with .call() so as to continue to use the official v0.5.0 release rather than git master.

Show outdated Hide outdated src/main.cpp
{
LOCK(pool.cs); // protect pool.mapNextTx
for (unsigned int i = 0; i < tx.vin.size(); i++)
BOOST_FOREACH(const CTxIn txin, tx.vin)

This comment has been minimized.

@sdaftuar

sdaftuar Oct 26, 2015

Member

Any reason not to make this const CTxIn &txin, here and at line 832 below?

@sdaftuar

sdaftuar Oct 26, 2015

Member

Any reason not to make this const CTxIn &txin, here and at line 832 below?

This comment has been minimized.

@petertodd

petertodd Oct 30, 2015

Contributor

Good point! Fixed.

@petertodd

petertodd Oct 30, 2015

Contributor

Good point! Fixed.

Show outdated Hide outdated src/main.cpp
// intersect.
BOOST_FOREACH(CTxMemPool::txiter ancestorIt, setAncestors)
{
const uint256 hashAncestor = ancestorIt->GetTx().GetHash();

This comment has been minimized.

@sdaftuar

sdaftuar Oct 26, 2015

Member

This could also be const uint256 &hashAncestor.

@sdaftuar

sdaftuar Oct 26, 2015

Member

This could also be const uint256 &hashAncestor.

This comment has been minimized.

@petertodd

petertodd Oct 30, 2015

Contributor

Also fixed.

@petertodd

petertodd Oct 30, 2015

Contributor

Also fixed.

Show outdated Hide outdated src/main.cpp
// up the pre-calculated fees/size-with-descendants values from the
// mempool package tracking; this does mean the pathological case
// of diamond tx graphs will be overcounted.
BOOST_FOREACH(const uint256 hashConflicting, setConflicts)

This comment has been minimized.

@sdaftuar

sdaftuar Oct 26, 2015

Member

I think this logic needlessly overcounts the descendants. Why not just call CalculateDescendants on the entries in setConflicts, and then loop over all of them?

Assuming the replacing transaction is successful, you don't end up wasting any time, because you can pass the descendant set directly into RemoveStaged later.

If the replacing transaction were to fail, and we're worried about the amount of work an attacker might try to make us do, then we could just put a work bound here where we fail to replace if there are too many things to look at.

I think it's a good idea to address though because a simple pattern where you have two parent transactions that are spent by a single child of both couldn't be consolidated down to a single transaction without paying for that child twice, if we went with this algorithm; that seems like a needless overhead.

@sdaftuar

sdaftuar Oct 26, 2015

Member

I think this logic needlessly overcounts the descendants. Why not just call CalculateDescendants on the entries in setConflicts, and then loop over all of them?

Assuming the replacing transaction is successful, you don't end up wasting any time, because you can pass the descendant set directly into RemoveStaged later.

If the replacing transaction were to fail, and we're worried about the amount of work an attacker might try to make us do, then we could just put a work bound here where we fail to replace if there are too many things to look at.

I think it's a good idea to address though because a simple pattern where you have two parent transactions that are spent by a single child of both couldn't be consolidated down to a single transaction without paying for that child twice, if we went with this algorithm; that seems like a needless overhead.

This comment has been minimized.

@petertodd

petertodd Oct 30, 2015

Contributor

Well, I'm trying to keep this pull as simple as possible, while writing it in a way that isn't likely to lead to any DoS attacks; a previous version of this patch did do exactly what you suggest, but given that descendant tracking exists I figured I'd use it. (after all, I'm writing this patch pro bono)

As for your example where it would matter, that'd require wallets that attempted to both get txs to non-RBF miners and RBF miners simultaneously by broadcasting one then the other. It's a good idea, but doing that doesn't yet save you any money due to the rule that replacements must pay >= the fees of the transactions being replaced.

@petertodd

petertodd Oct 30, 2015

Contributor

Well, I'm trying to keep this pull as simple as possible, while writing it in a way that isn't likely to lead to any DoS attacks; a previous version of this patch did do exactly what you suggest, but given that descendant tracking exists I figured I'd use it. (after all, I'm writing this patch pro bono)

As for your example where it would matter, that'd require wallets that attempted to both get txs to non-RBF miners and RBF miners simultaneously by broadcasting one then the other. It's a good idea, but doing that doesn't yet save you any money due to the rule that replacements must pay >= the fees of the transactions being replaced.

Show outdated Hide outdated src/main.cpp
// is for the sake of multi-party protocols, where we don't
// want a single party to be able to disable replacement.
//
// The opt-out ignores decendents as anyone relying on

This comment has been minimized.

@sdaftuar

sdaftuar Oct 26, 2015

Member

nit: "descendants"

@sdaftuar

sdaftuar Oct 26, 2015

Member

nit: "descendants"

This comment has been minimized.

@petertodd

petertodd Oct 30, 2015

Contributor

Fixed

@petertodd

petertodd Oct 30, 2015

Contributor

Fixed

Show outdated Hide outdated src/main.cpp
list<CTransaction> ltxConflicted;
pool.removeConflicts(tx, ltxConflicted);
BOOST_FOREACH(const CTransaction &txConflicted, ltxConflicted)

This comment has been minimized.

@sdaftuar

sdaftuar Oct 26, 2015

Member

Just to add on to my comment above -- if you call CalculateDescendants above and use RemoveStaged here, then we don't have to copy all these transactions...

@sdaftuar

sdaftuar Oct 26, 2015

Member

Just to add on to my comment above -- if you call CalculateDescendants above and use RemoveStaged here, then we don't have to copy all these transactions...

This comment has been minimized.

@petertodd

petertodd Oct 30, 2015

Contributor

Agreed. Although again, I'm not worried about the case where the transactions do end up replaced; I'm worried about the possible DoS attack case where they aren't.

@petertodd

petertodd Oct 30, 2015

Contributor

Agreed. Although again, I'm not worried about the case where the transactions do end up replaced; I'm worried about the possible DoS attack case where they aren't.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 4, 2015

Contributor

Added @sdaftuar's changes from petertodd#4 (comment)

Contributor

petertodd commented Nov 4, 2015

Added @sdaftuar's changes from petertodd#4 (comment)

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 5, 2015

Contributor

Great work @sdaftuar on the added tests, once-over ACK

Contributor

dcousens commented Nov 5, 2015

Great work @sdaftuar on the added tests, once-over ACK

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Nov 5, 2015

Member

needs rebase

Member

btcdrak commented Nov 5, 2015

needs rebase

@gmaxwell gmaxwell added this to the 0.12.0 milestone Nov 5, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 10, 2015

Member

Concept ACK

Member

laanwj commented Nov 10, 2015

Concept ACK

@jgarzik

This comment has been minimized.

Show comment
Hide comment
@jgarzik

jgarzik Nov 10, 2015

Contributor

concept ACK

Contributor

jgarzik commented Nov 10, 2015

concept ACK

petertodd and others added some commits Oct 22, 2015

Add opt-in full-RBF to mempool
Replaces transactions already in the mempool if a new transaction seen
with a higher fee, specifically both a higher fee per KB and a higher
absolute fee. Children are evaluateed for replacement as well, using the
mempool package tracking to calculate replaced fees/size. Transactions
can opt-out of transaction replacement by setting nSequence >= maxint-1
on all inputs. (which all wallets do already)
Improve RBF replacement criteria
Fix the calculation of conflicting size/conflicting fees.
Prevent low feerate txs from (directly) replacing high feerate txs
Previously all conflicting transactions were evaluated as a whole to
determine if the feerate was being increased. This meant that low
feerate children pulled the feerate down, potentially allowing a high
transaction with a high feerate to be replaced by one with a lower
feerate.
@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 10, 2015

Contributor

Rebased

Contributor

petertodd commented Nov 10, 2015

Rebased

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 10, 2015

Contributor

Main question I have right now, is do we want to go with @sdaftuar's somewhat more complex, but more correct, code? (petertodd@20367d8) Or keep it simple?

I'm happy with either way, and the code is written and looks good to me. Just a matter of risk tolerance.

Contributor

petertodd commented Nov 10, 2015

Main question I have right now, is do we want to go with @sdaftuar's somewhat more complex, but more correct, code? (petertodd@20367d8) Or keep it simple?

I'm happy with either way, and the code is written and looks good to me. Just a matter of risk tolerance.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 10, 2015

Contributor

Also, regarding the duplicated tests, I'm inclined to leave my ones in git history for historical reference in case questions come up later, but delete them from from the codebase soon in favor of @sdaftuar's. (possibly even in this pull-req) They're relatively "battle-tested" tests. :)

Contributor

petertodd commented Nov 10, 2015

Also, regarding the duplicated tests, I'm inclined to leave my ones in git history for historical reference in case questions come up later, but delete them from from the codebase soon in favor of @sdaftuar's. (possibly even in this pull-req) They're relatively "battle-tested" tests. :)

Fix incorrect locking of mempool during RBF replacement
Previously RemoveStaged() was called without pool.cs held.
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Nov 13, 2015

Member

After further thought I think we can hold the mempool lock for a bit less time. I don't think it's possible for transactions to be added or removed from the mempool without holding cs_main, and ATMP holds cs_main the whole time. So I think all we need to do so that the mempool shows a consistent view to any RPC calls that only grab the mempool lock is acquire the lock before calling RemoveStaged(), and release the lock after calling addUnchecked(). (Hopefully someone can verify my reasoning...)

Anyway I think that's pretty minor either way.

ACK.

Member

sdaftuar commented Nov 13, 2015

After further thought I think we can hold the mempool lock for a bit less time. I don't think it's possible for transactions to be added or removed from the mempool without holding cs_main, and ATMP holds cs_main the whole time. So I think all we need to do so that the mempool shows a consistent view to any RPC calls that only grab the mempool lock is acquire the lock before calling RemoveStaged(), and release the lock after calling addUnchecked(). (Hopefully someone can verify my reasoning...)

Anyway I think that's pretty minor either way.

ACK.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 13, 2015

Contributor

@sdaftuar That's a good point, though I think changing that should be done in a separate pull-req after this is merged.

Contributor

petertodd commented Nov 13, 2015

@sdaftuar That's a good point, though I think changing that should be done in a separate pull-req after this is merged.

@btcdrak

This comment has been minimized.

Show comment
Hide comment
@btcdrak

btcdrak Nov 13, 2015

Member

Upgraded to utACK.

Member

btcdrak commented Nov 13, 2015

Upgraded to utACK.

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Nov 17, 2015

Contributor

Concept ACK, so far is this goes. It can only help to allow spenders to declare their intention not to double-spend, or to keep the option open.

It will be unfortunate if wallets silently make "replaceable" the default for new transactions. This setting should be exposed and visible, although there are different ways it could be labeled.

cc @jonasschnelli

Contributor

dgenr8 commented Nov 17, 2015

Concept ACK, so far is this goes. It can only help to allow spenders to declare their intention not to double-spend, or to keep the option open.

It will be unfortunate if wallets silently make "replaceable" the default for new transactions. This setting should be exposed and visible, although there are different ways it could be labeled.

cc @jonasschnelli

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Nov 17, 2015

Member

In response to @dgenr8 's comment, I do think we need to make it very clear that there is still no such thing as technically "declaring an intention not to double-spend". Strongly prefer adding a full RBF option, even if unwise for miners to deploy today.

Member

luke-jr commented Nov 17, 2015

In response to @dgenr8 's comment, I do think we need to make it very clear that there is still no such thing as technically "declaring an intention not to double-spend". Strongly prefer adding a full RBF option, even if unwise for miners to deploy today.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 17, 2015

Contributor

@dgenr8 The nSequence for opt-in is one that no wallets today use by default.

In any case, indicating an intention not to double-spend isn't something wallets should really be doing today, given their poor track record of not double-spending; there are many failure modes where wallets today will double-spend a transaction inadvertently, something you can easily see if you watch double-spend logs. This is why any type of doublespend punishment scheme needs to be carefully engineered, and definitely not used with standard wallets.

Contributor

petertodd commented Nov 17, 2015

@dgenr8 The nSequence for opt-in is one that no wallets today use by default.

In any case, indicating an intention not to double-spend isn't something wallets should really be doing today, given their poor track record of not double-spending; there are many failure modes where wallets today will double-spend a transaction inadvertently, something you can easily see if you watch double-spend logs. This is why any type of doublespend punishment scheme needs to be carefully engineered, and definitely not used with standard wallets.

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Nov 17, 2015

Contributor

I think I can phrase my point better. Allowing spenders to declare their intention to double-spend, if they should later so desire, is helpful to 0-conf-accepting receivers because they can be sure not to rely on the transaction before confirmation.

The 72-hour expiration that will soon be policy is also helpful because it gives a better option to those who have stuck transactions (even those without opt-in RBF set): "wait 72 hours and try again." This is a lot better than "download @petertodd's tools," "use rawtxes," etc.

Contributor

dgenr8 commented Nov 17, 2015

I think I can phrase my point better. Allowing spenders to declare their intention to double-spend, if they should later so desire, is helpful to 0-conf-accepting receivers because they can be sure not to rely on the transaction before confirmation.

The 72-hour expiration that will soon be policy is also helpful because it gives a better option to those who have stuck transactions (even those without opt-in RBF set): "wait 72 hours and try again." This is a lot better than "download @petertodd's tools," "use rawtxes," etc.

@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Nov 17, 2015

Member

Well, with this you don't have to wait anything. You can pay more fees at any point to "try again", that's kind of the whole point of RBF.

Member

jtimon commented Nov 17, 2015

Well, with this you don't have to wait anything. You can pay more fees at any point to "try again", that's kind of the whole point of RBF.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 18, 2015

Contributor

@dgenr8 Does the advice "wait 72 hours and try again" even work right now in Bitcoin Core? IIRC we don't delete transactions from the wallet itself on that timeout.

Anyway, while this pull-req doesn't change any wallet behavior, subsequent pull-reqs of course can of course add the required wallet code to unstick stuck transactions.

Contributor

petertodd commented Nov 18, 2015

@dgenr8 Does the advice "wait 72 hours and try again" even work right now in Bitcoin Core? IIRC we don't delete transactions from the wallet itself on that timeout.

Anyway, while this pull-req doesn't change any wallet behavior, subsequent pull-reqs of course can of course add the required wallet code to unstick stuck transactions.

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Nov 18, 2015

Contributor

@petertodd Opt-in and expiration both suggest new wallet functions:

On new tx:

  • Mark transaction as replaceable (receiver must wait for confirmation)

On replaceable or expired unconfirmed tx:

  • Send new version with higher fees
  • Send new version that pays myself instead (permanently cancels original)
  • Forget [release inputs]

On forgotten transactions with inputs still unspent:

  • Unforget
Contributor

dgenr8 commented Nov 18, 2015

@petertodd Opt-in and expiration both suggest new wallet functions:

On new tx:

  • Mark transaction as replaceable (receiver must wait for confirmation)

On replaceable or expired unconfirmed tx:

  • Send new version with higher fees
  • Send new version that pays myself instead (permanently cancels original)
  • Forget [release inputs]

On forgotten transactions with inputs still unspent:

  • Unforget
@jtimon

This comment has been minimized.

Show comment
Hide comment
@jtimon

jtimon Nov 18, 2015

Member

@dgenr8 That analysis seems about right. I would personally prefer that the replaceable option was always true (on non-raw transactions, obviously) for Bitcoin Core. But those are changes to the wallet that as explained by @petertodd are out of the scope of this PR.

Member

jtimon commented Nov 18, 2015

@dgenr8 That analysis seems about right. I would personally prefer that the replaceable option was always true (on non-raw transactions, obviously) for Bitcoin Core. But those are changes to the wallet that as explained by @petertodd are out of the scope of this PR.

}
}
// Check if it's economically rational to mine this transaction rather

This comment has been minimized.

@jtimon

jtimon Nov 18, 2015

Member

Can't all this new code (1003 to 1140) be a new function?
EDIT: since it uses the mempool, it's probably better that this doesn't move to policy.cpp yet. In main.cpp is fine, just apart from AcceptToMemoryPool().

@jtimon

jtimon Nov 18, 2015

Member

Can't all this new code (1003 to 1140) be a new function?
EDIT: since it uses the mempool, it's probably better that this doesn't move to policy.cpp yet. In main.cpp is fine, just apart from AcceptToMemoryPool().

This comment has been minimized.

@MarcoFalke

MarcoFalke Nov 20, 2015

Member

Agree with @jtimon . It's hard to read a function spanning over 400 lines not knowing which parts belong together.

@MarcoFalke

MarcoFalke Nov 20, 2015

Member

Agree with @jtimon . It's hard to read a function spanning over 400 lines not knowing which parts belong together.

This comment has been minimized.

@petertodd

petertodd Nov 20, 2015

Contributor

@jtimon Yeah, it's tricky though, because we reuse allConflicting later in the removal code for performance, and that in turn requires us to hold a lock; the way the code works isn't really modularized yet. That said compared to my previous patch we do at least take advantage of other mempool-related code, CalculateDescendants(), so we're a bit ahead.

I'd be inclined to leave it as-is right now, and do the refactor later when we add RBF-related code to the wallet and/or RPC - that's when we'll have a better idea of what the refactor should look like and how the refactored code should interact with both uses. For instance, the ability to calculate on demand how expensive it would be to replace a specific transaction would be useful.

@petertodd

petertodd Nov 20, 2015

Contributor

@jtimon Yeah, it's tricky though, because we reuse allConflicting later in the removal code for performance, and that in turn requires us to hold a lock; the way the code works isn't really modularized yet. That said compared to my previous patch we do at least take advantage of other mempool-related code, CalculateDescendants(), so we're a bit ahead.

I'd be inclined to leave it as-is right now, and do the refactor later when we add RBF-related code to the wallet and/or RPC - that's when we'll have a better idea of what the refactor should look like and how the refactored code should interact with both uses. For instance, the ability to calculate on demand how expensive it would be to replace a specific transaction would be useful.

This comment has been minimized.

@jtimon

jtimon Nov 23, 2015

Member

I don't see how is tricky, if you need something inside the new function and in the caller, just pass it as reference. If you were holding the lock, well keep holding it, etc...It should be basically cut the code I said and paste it out as a (for now static) function, and adapt the parameters until it compiles, I think.
Of course we can always leave refactors for later, but they often become harder with time (because the code keeps evolving from its current form, not from "what we plan to do with it in a later refactor".
I'm fine with not doing it within this PR (although it would be less disruptive to squash that with the commit that introduces the new code), but I highly dislike when people insist to "postpone refactor X until after feature Y". If X is ready 6months before Y starts being coded...should we wait there too?
That kind of thinking is what has led us to, for example, wait more than 1 year to introduce a CPolicy class (and I'm still waiting because apparently #6068 would be too disruptive for this and other mempool-related changes [I disagree but was tired of rebasing]).
If we leave this PR as it is, fine. But please don't postpone or restrict any kind of development unnecessarily. Complain in the PRs when they exist, but don't plan how to serialize refactors between features or I predict: new important features will be proposed and the refactors will never happen (and encapsulations will become increasingly hard).

@jtimon

jtimon Nov 23, 2015

Member

I don't see how is tricky, if you need something inside the new function and in the caller, just pass it as reference. If you were holding the lock, well keep holding it, etc...It should be basically cut the code I said and paste it out as a (for now static) function, and adapt the parameters until it compiles, I think.
Of course we can always leave refactors for later, but they often become harder with time (because the code keeps evolving from its current form, not from "what we plan to do with it in a later refactor".
I'm fine with not doing it within this PR (although it would be less disruptive to squash that with the commit that introduces the new code), but I highly dislike when people insist to "postpone refactor X until after feature Y". If X is ready 6months before Y starts being coded...should we wait there too?
That kind of thinking is what has led us to, for example, wait more than 1 year to introduce a CPolicy class (and I'm still waiting because apparently #6068 would be too disruptive for this and other mempool-related changes [I disagree but was tired of rebasing]).
If we leave this PR as it is, fine. But please don't postpone or restrict any kind of development unnecessarily. Complain in the PRs when they exist, but don't plan how to serialize refactors between features or I predict: new important features will be proposed and the refactors will never happen (and encapsulations will become increasingly hard).

This comment has been minimized.

@jtimon

jtimon Nov 23, 2015

Member

I was talking about something like this: jtimon@f10b531 but as said I'm completely fine with not creating the new function yet (although now it would be almost free diff-wise).

@jtimon

jtimon Nov 23, 2015

Member

I was talking about something like this: jtimon@f10b531 but as said I'm completely fine with not creating the new function yet (although now it would be almost free diff-wise).

@CodeShark

This comment has been minimized.

Show comment
Hide comment
@CodeShark

CodeShark Nov 19, 2015

Contributor

concept ACK, haven't had time for code review

Contributor

CodeShark commented Nov 19, 2015

concept ACK, haven't had time for code review

it->GetTx().GetHash().ToString(),
hash.ToString(),
FormatMoney(nFees - nConflictingFees),
(int)nSize - (int)nConflictingSize);

This comment has been minimized.

@jonasschnelli

jonasschnelli Nov 19, 2015

Member

(maybe out of scope for this PR)

Do we need a new signal here? How can the wallet detect removed (or replaced) transactions? Maybe the signal needs to be emitted from void CTxMemPool::RemoveStaged(setEntries &stage) (or nearby) to also include the mempool limiting behavior.

@jonasschnelli

jonasschnelli Nov 19, 2015

Member

(maybe out of scope for this PR)

Do we need a new signal here? How can the wallet detect removed (or replaced) transactions? Maybe the signal needs to be emitted from void CTxMemPool::RemoveStaged(setEntries &stage) (or nearby) to also include the mempool limiting behavior.

This comment has been minimized.

@petertodd

petertodd Nov 20, 2015

Contributor

We probably do. But yeah, I think adding that can wait for another pull-req; belongs as a wallet code change IMO, so might as well let whomever ends up writing that decide what they need.

@petertodd

petertodd Nov 20, 2015

Contributor

We probably do. But yeah, I think adding that can wait for another pull-req; belongs as a wallet code change IMO, so might as well let whomever ends up writing that decide what they need.

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 19, 2015

Member

CodeReview ACK.
(sidenote: it's always nice to review well documented PRs!)

Member

jonasschnelli commented Nov 19, 2015

CodeReview ACK.
(sidenote: it's always nice to review well documented PRs!)

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 20, 2015

Contributor

@jonasschnelli Thanks! Yeah, apparently my life long dream was actually to become a technical writer, not a programmer. :)

Contributor

petertodd commented Nov 20, 2015

@jonasschnelli Thanks! Yeah, apparently my life long dream was actually to become a technical writer, not a programmer. :)

Fix usage of local python-bitcoinlib
Previously was using the system-wide python-bitcoinlib, if it existed,
rather than the local copy that you check out in the README.
REJECT_INSUFFICIENTFEE, "insufficient fee");
}
}
// Check against previous transactions
// This is done last to help prevent CPU exhaustion denial-of-service attacks.
if (!CheckInputs(tx, state, view, true, STANDARD_SCRIPT_VERIFY_FLAGS, true))

This comment has been minimized.

@jtimon

jtimon Nov 23, 2015

Member

Wouldn't it be better to put the new code (from L987) after this check instead of before?
It would avoid doing further mempool validations for transactions that are going to be rejected by this check anyway.
I know there can be transactions that would pass this check but be rejected as a replacement, but this check already has the inputs of the transaction cached, so it seems cheaper than the RBF logic and it makes sense to me to do it first.
Note that this minor nit wouldn't change the total diff (although it can be done later with other changes too [for example, a rebased #6445 ]).

@jtimon

jtimon Nov 23, 2015

Member

Wouldn't it be better to put the new code (from L987) after this check instead of before?
It would avoid doing further mempool validations for transactions that are going to be rejected by this check anyway.
I know there can be transactions that would pass this check but be rejected as a replacement, but this check already has the inputs of the transaction cached, so it seems cheaper than the RBF logic and it makes sense to me to do it first.
Note that this minor nit wouldn't change the total diff (although it can be done later with other changes too [for example, a rebased #6445 ]).

This comment has been minimized.

@petertodd

petertodd Nov 25, 2015

Contributor

You mean after the CheckInputs() line? The RBF code is just a bunch of pointer following of in-memory data, with limited depth and breadth, so it shouldn't be expensive code to run - remember that the mempool has tx fee and size data in the CTxMemPool structs.

@petertodd

petertodd Nov 25, 2015

Contributor

You mean after the CheckInputs() line? The RBF code is just a bunch of pointer following of in-memory data, with limited depth and breadth, so it shouldn't be expensive code to run - remember that the mempool has tx fee and size data in the CTxMemPool structs.

This comment has been minimized.

@jtimon

jtimon Nov 25, 2015

Member

Never mind, I misread this line for AreInputsStandard().

@jtimon

jtimon Nov 25, 2015

Member

Never mind, I misread this line for AreInputsStandard().

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 26, 2015

Contributor

Is a higher sequence number still preferred?
In the case of a fee tie?

specifically both a higher fee per KB and a higher absolute fee

Or only the higher fee?

What is the behaviour if a transaction is broadcast with the sequence number 0xffffffff after a transaction was already found that was 'opt-in'?

Contributor

dcousens commented Nov 26, 2015

Is a higher sequence number still preferred?
In the case of a fee tie?

specifically both a higher fee per KB and a higher absolute fee

Or only the higher fee?

What is the behaviour if a transaction is broadcast with the sequence number 0xffffffff after a transaction was already found that was 'opt-in'?

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

@dcousens Just higher fee. Ties get rejected to avoid them being used as a way to waste bandwidth.

If a nSequence > maxint-2 transaction is broadcast it is subject to the same rules as any other replacement; it won't get accepted without paying a (sufficiently) higher fee. That said, if it is accepted further replacements will be rejected. The replacement behavior is stateless, acting only on what is in the mempool right now.

Contributor

petertodd commented Nov 26, 2015

@dcousens Just higher fee. Ties get rejected to avoid them being used as a way to waste bandwidth.

If a nSequence > maxint-2 transaction is broadcast it is subject to the same rules as any other replacement; it won't get accepted without paying a (sufficiently) higher fee. That said, if it is accepted further replacements will be rejected. The replacement behavior is stateless, acting only on what is in the mempool right now.

@dcousens

This comment has been minimized.

Show comment
Hide comment
@dcousens

dcousens Nov 26, 2015

Contributor

Thanks for clarifying that @petertodd 👍

@kristovatlas I wonder if the 'default' sequence number when opting into RBF should just be 0 then?
Just thinking of the privacy implications.

Contributor

dcousens commented Nov 26, 2015

Thanks for clarifying that @petertodd 👍

@kristovatlas I wonder if the 'default' sequence number when opting into RBF should just be 0 then?
Just thinking of the privacy implications.

@petertodd

This comment has been minimized.

Show comment
Hide comment
@petertodd

petertodd Nov 26, 2015

Contributor

@dcousens nSequence=0 makes sense from the perspective of https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki too.

Contributor

petertodd commented Nov 26, 2015

@dcousens nSequence=0 makes sense from the perspective of https://github.com/bitcoin/bips/blob/master/bip-0068.mediawiki too.

@laanwj laanwj merged commit 63b5840 into bitcoin:master Nov 27, 2015

1 check passed

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

laanwj added a commit that referenced this pull request Nov 27, 2015

Merge pull request #6871
63b5840 Fix usage of local python-bitcoinlib (Peter Todd)
16a2f93 Fix incorrect locking of mempool during RBF replacement (Peter Todd)
97203f5 Port test to rpc-test framework (Suhas Daftuar)
20367d8 Add test for max replacement limit (Suhas Daftuar)
73d9040 Improve RBF replacement criteria (Suhas Daftuar)
b272ecf Reject replacements that add new unconfirmed inputs (Peter Todd)
fc8c19a Prevent low feerate txs from (directly) replacing high feerate txs (Peter Todd)
0137e6f Add tests for transaction replacement (Peter Todd)
5891f87 Add opt-in full-RBF to mempool (Peter Todd)
@kristovatlas

This comment has been minimized.

Show comment
Hide comment
@kristovatlas

kristovatlas Nov 27, 2015

@dcousens defaulting to nSequence=0 for the purpose of reducing wallet client fingerprintability makes sense to me.

@dcousens defaulting to nSequence=0 for the purpose of reducing wallet client fingerprintability makes sense to me.

braydonf pushed a commit to braydonf/btcjs that referenced this pull request Dec 9, 2015

Braydon Fuller
Transaction: Added replace-by-fee (RBF) support
- Useful for bidding transactions as described in: https://bitpay.com/chaindb.pdf
- Reference: nSequence-based opt-in: bitcoin/bitcoin#6871

laanwj added a commit to laanwj/bitcoin that referenced this pull request Feb 24, 2016

mempool: Reduce ERROR logging for mempool rejects
Continues "Make logging for validation optional" from #6519.

The idea there was to remove all ERROR logging of rejected transaction,
and move it to one message in the class 'mempoolrej' which logs the
state message (and debug info). The superfluous ERRORs in the log
"terrify" users, see for example issue #5794.

Unfortunately a lot of new logging was introduced in #6871 (RBF) and
 #7287 (misc refactoring). This pull updates that new code.
// Save these to avoid repeated lookups
setIterConflicting.insert(mi);

This comment has been minimized.

@rebroad

rebroad Aug 25, 2016

Contributor

The following was later removed by #7594

@rebroad

rebroad Aug 25, 2016

Contributor

The following was later removed by #7594

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment