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

Make RBF policies optional #7219

Closed
wants to merge 2 commits into from
Closed

Make RBF policies optional #7219

wants to merge 2 commits into from

Conversation

@luke-jr
Copy link
Member

luke-jr commented Dec 16, 2015

At least I assumed this was part of the original RBF merge

@jonasschnelli
jonasschnelli reviewed Dec 16, 2015
View changes
src/init.cpp Outdated
@@ -456,6 +456,7 @@ std::string HelpMessage(HelpMessageMode mode)
strUsage += HelpMessageOpt("-acceptnonstdtxn", strprintf("Relay and mine \"non-standard\" transactions (%sdefault: %u)", "testnet/regtest only; ", !Params(CBaseChainParams::TESTNET).RequireStandard()));
strUsage += HelpMessageOpt("-datacarrier", strprintf(_("Relay and mine data carrier transactions (default: %u)"), 1));
strUsage += HelpMessageOpt("-datacarriersize", strprintf(_("Maximum size of data in data carrier transactions we relay and mine (default: %u)"), MAX_OP_RETURN_RELAY));
strUsage += HelpMessageOpt("-rbf", strprintf(_("Allow replacement of transactions paying sufficiently higher fees (0 = never, 1 = always, 2 = opt-in, default: %s)"), nRbfPolicy));

This comment has been minimized.

@jonasschnelli

jonasschnelli Dec 16, 2015 Member

-replacebyfee?

This comment has been minimized.

@luke-jr

luke-jr Dec 16, 2015 Author Member

Hm, maybe... a bit long though? Anyone else have any opinions on what to call this?

I was originally thinking -replacementpolicy, but then started overengineering it and figured that would be better left to a general policy refactoring later...

This comment has been minimized.

@tulip0

tulip0 Dec 16, 2015

Go with the longer, more detailed one. Mystery command line flags are no fun.

This comment has been minimized.

@dcousens

dcousens Dec 16, 2015 Contributor

Both -rbf and -replacebyfee are acceptable IMHO.
I'd personally opt towards the latter in all cases (API, code, variables, etc)

This comment has been minimized.

@jonasschnelli

jonasschnelli Dec 16, 2015 Member

I think abbreviations should not be used for command line arguments or RPC commands. Our scheme looks more (not everywhere) after full length parameters/commands (we use createrawtransaction instead of createrawtx, etc.).

@dcousens
Copy link
Contributor

dcousens commented Dec 16, 2015

concept ACK, once-over utACK @ 0d0f285

@sdaftuar
Copy link
Member

sdaftuar commented Dec 16, 2015

In general I am sympathetic to the idea of trying to offer up policies that people want as much as reasonably possible, but my initial reaction is that this change isn't a good idea. I don't think we should offer policies that don't make sense for a user to logically choose.

What would be the motivation for someone turning off opt-in RBF in favor of a no-replacement policy? If it's because they don't want to personally accept transactions that might be later replaced, then I think we should just make sure that bitcoind offers whatever tools a user would need to determine whether a transaction is opting-in, so the user can reject the payment if they choose. Not accepting opt-in transactions at all seems much more useful to the user than locally choosing to not accept replacements of such transactions. Breaking relay of opt-in RBF replacement transactions seems to offer no additional local benefits that I can see (at least, not any local benefits that can't be better achieved a different way), while it would provide some harm to part of the network (namely those users who might use it in the future).

So offering it as a policy might be confusing to people, since having the option implies there's a good reason some might want to turn it off, and I don't think there is such a reason. (If there is currently a reason, say because bitcoind doesn't currently make it easy for users to know whether a transaction is opting-in, then I think we should make that easy instead, which is not difficult to implement. If there are other reasons, I'd like to hear them.)

I can understand the motivation for offering full-RBF, as it makes sense to me that some users might want that policy, but given the existence of opt-in RBF, is full-RBF likely to offer meaningfully different outcomes for what gets into your mempool? I expect not, and I don't see the need for Bitcoin Core to support what has been a controversial policy that we expect would provide minimal actual benefit to users choosing to run it.

@luke-jr
Copy link
Member Author

luke-jr commented Dec 17, 2015

What would be the motivation for someone turning off opt-in RBF in favor of a no-replacement policy?

I'm sure there's more, but a few that come to mind:

  • "I haven't considered this enough to make a decision, so I want to maintain my status quo policy while still upgrading to 0.12."
  • "I am mining myself, and would prefer to simply include users' lower fees to encourage Bitcoin usage."
  • "I prefer a non-fee based policy, such as priority."
  • "I personally think Bitcoin would be better if only first-seen transactions were relayed or mined."

Breaking relay of opt-in RBF replacement transactions seems to offer no additional local benefits that I can see (at least, not any local benefits that can't be better achieved a different way), while it would provide some harm to part of the network (namely those users who might use it in the future).

That's not our decision to make. People running nodes de facto want to turn this off/on, and have a right to do so; there is no reason to make it gratuitously difficult - that's merely developers trying to usurp an authority that doesn't belong to us.

@dcousens
Copy link
Contributor

dcousens commented Dec 17, 2015

that's merely developers trying to usurp an authority that doesn't belong to us.

+1

I think there is a fine line between what policies we should choose to bundle by default (and hence, if they can be, they should be configurable) and what we choose to omit.
We have included RBF as a embedded policy, it is easily configurable, so why not?

@tulip0
Copy link

tulip0 commented Dec 17, 2015

Supplying options that are illogical or dangerous does have some risk, there's commonly distributed bitcoin.conf which users often copy terrible configurations. A quick google shows this one which sets maxconnections so high the node will certainly crash, turns on txindex and binds to 8.8.8.8. Others set gen, keypool=2, par=1, bind the rpc interface to 0.0.0.0, many other weird configurations.

This probably isn't up there with high potential for stabbing yourself in the foot, but it's something to be aware of.

@petertodd
Copy link
Contributor

petertodd commented Dec 17, 2015

utACK luke-jr@0d0f285

I don't care about -rbf vs -replacebyfee

@morcos
Copy link
Member

morcos commented Dec 17, 2015

I think without any reason to believe this is a desired feature, this is just another example of putting in too many knobs which complicate the code and the logic and hamstring further improvements without good reason. There should be a high bar for introducing complexity. I'm opposed to this change.

@luke-jr
Copy link
Member Author

luke-jr commented Dec 17, 2015

@morcos This isn't complicated, and is clearly much-desired by users.

@luke-jr luke-jr force-pushed the luke-jr:rbf_opts branch to 786f92d Dec 17, 2015
@luke-jr
Copy link
Member Author

luke-jr commented Dec 17, 2015

-rbf renamed to -replacebyfee

@sdaftuar
Copy link
Member

sdaftuar commented Dec 17, 2015

@luke-jr I don't think it's at all intuitive to users that setting -replacebyfee to 0 would mean that you would still accept transactions into your mempool that are signaling opt-in to RBF (just not allowing them to be replaced). I would expect many users to be confused if they were to use that setting and then see transactions in their mempool get double-spent, because the rest of the network has a (known and documented!) different policy. That strikes me as a dangerous configuration and likely to cause any non-expert user significant confusion.

@morcos
Copy link
Member

morcos commented Dec 17, 2015

and is clearly much-desired by users.

@luke-jr that would certainly influence my opinion. Can you elaborate? To @sdaftuar's point, they may want to be able to make txs non-replaceable, but that doesn't occur just by them changing their local policy. I can't believe I'm suggesting this, but perhaps a more useful feature would be the ability to flag a single tx as non-replaceable, which might be something miners wanted to do if they want to mine a particular version of a spend. Would require some changes to the RBF logic though..

@dgenr8
Copy link
Contributor

dgenr8 commented Dec 17, 2015

NACK. I want opt-in RBF to work, and this change can only help it work less well.

@btcdrak
Copy link
Contributor

btcdrak commented Dec 17, 2015

utACK 786f92d

@luke-jr
Copy link
Member Author

luke-jr commented Dec 17, 2015

@morcos The merge of opt-in RBF has been and is highly controversial among users, at least on reddit.

@jgarzik
Copy link
Contributor

jgarzik commented Dec 17, 2015

@luke-jr Strictly true, but those comments often seemed to (intentionally?) miss the opt-in aspect, and so should be discounted. A typical reddit-esque comment of "$company is trying to kill zero-conf!" is silly when in fact opt-in RBF will not lead to increased attack surface at payment processors.

opt-in RBF does not lead to ecosystem breakage and theft, unless I'm missing something.

@instagibbs
Copy link
Member

instagibbs commented Dec 18, 2015

Seems like the "always" option would upset the apple cart, if you are ostensibly caring about people's concerns about opt-in.

@morcos
Copy link
Member

morcos commented Dec 18, 2015

I won't say NACK, but I'd say I have a preference not to merge this.

@amacneil
Copy link

amacneil commented Dec 19, 2015

NACK. Allowing nodes to run full-RBF (nRbfPolicy = 1) is dangerous for existing users who are not expecting their transactions to be replaced. If only a small portion of miners adopted this setting (e.g. 10%), it would allow non-RBF transactions to be reliably double spent 10% of the time.

It would be better to release opt-in RBF without changing behavior/expectations of existing transactions, and at least get users used to this concept before allowing nodes to opt-in to full-RBF.

@luke-jr
Copy link
Member Author

luke-jr commented Dec 19, 2015

Again, that is not our decision to make.

@petertodd
Copy link
Contributor

petertodd commented Dec 19, 2015

@amacneil IMO that's a good argument to have an option to always do full-RBF: it reminds people that first-seen reliant zeroconf can be easily broken by a small % of miners.

@petertodd
Copy link
Contributor

petertodd commented Dec 19, 2015

Tested ACK 786f92d

  • Help text correct
  • -replacebyfee=0
  • -replacebyfee=1/-replace-byfee
  • -replacebyfee=2
@amacneil
Copy link

amacneil commented Dec 19, 2015

@luke-jr @petertodd well that makes the arguments for this patch the same as full-RBF then.

While I agree that long term it will be beneficial to move towards full-RBF, for now I was under the impression that the point of releasing opt-in RBF was to not change first seen relay expectations for existing users (at least for the 0.12 release).

@petertodd
Copy link
Contributor

petertodd commented Dec 19, 2015

@amacneil It's a default-opt-in. @luke-jr and I don't agree 100% here, but I'd make the argument that the default will likely lead to the vast majority of the hashing power leaving it opt-in. As for those that enable full-RBF always, other than Bitcoin XT nodes they're not going to get double-spends relayed to them anyway... which is I guess a bit ironic.

@DavidVorick
Copy link

DavidVorick commented Dec 19, 2015

concept ACK, there seem to be enough legitimate reasons for a user to want to turn off opt-in rbf, particularly if a miner. I do have concerns that people will disable opt-in RBF for the wrong reasons (they want to protect 0-conf), but I'm expecting that few enough people change the defaults in the first place that it won't be an issue.

@HostFat
Copy link
Contributor

HostFat commented Dec 21, 2015

I agree with @chaosgrid, and it's true that accepting zeroconf on face2face trades is very common.
Many are completely aware of the risks, but usually the cost of the fraud is higher of the benefit, by losing reputation or even worst.
So by knowing this they still accept zeroconf on small tx, even by judging one by one their customers.
No one is forced on accepting them.

Anyway, I think that any feature that can enable filtering broadcasting of tx compatible with the protocol needs to be avoided.

@dgenr8
Copy link
Contributor

dgenr8 commented Dec 21, 2015

having an obscure CLI option to enable full-rbf isn't such a big deal in practice

@petertodd You must have forgotten about Linux distros who can package default options easily, but are much less likely to include software patches. And what about windows/mac users who are happy to change a shortcut or conf file, but aren't going to download an untrusted binary? The upstream project, us, has some trust.

Since you also proposed a straight change to the default earlier that @btcdrak questioned and @gmaxwell closed, your position is clearly pro-RBF, and not pro-opt-in-RBF.

@amacneil
Copy link

amacneil commented Dec 21, 2015

@petertodd note for the record I am no longer affiliated with Coinbase, so information may be a little out of date and opinions here are my own.

The biggest target for 0-conf scammers are gift cards (both dedicated gift retailers, and regular merchants who sell gift cards). These are usually delivered electronically and easily be cashed out via purse.io or similar with little to no verified KYC. Generally the bitcoin payment processor eats the fraud cost once a transaction is marked as accepted via their API, so even though the retailer could simply cancel the gift card (if they were fast enough), they have no incentive to, and these systems are usually not automated anyway.

Requiring confirmations is also not even possible for some merchants (for example: legacy e-commerce systems which cannot place a hold on inventory longer than 15 or 30 minutes, when a single bitcoin confirmation can sometimes take upwards of 1 hour). So these merchants would probably opt to stop receiving bitcoin payments altogether rather than wait for confirmations. Arguably this is not a huge loss for the bitcoin ecosystem, since very few people are using bitcoin to make online purchases where credit cards are already accepted, but it would be disappointing to see nonetheless.

Generally I believe fraud costs are currently at an acceptable level (because merchants are still accepting 0-conf). However, if the % of mining power running full-RBF ever increases above % discount which gift cards can be flipped online, it would significantly alter the economics here.

@surg0r
Copy link

surg0r commented Dec 21, 2015

nack

@sandakersmann
Copy link
Contributor

sandakersmann commented Dec 21, 2015

The people advocating for full-RBF do not want bitcoin to be used in retail situations. They want to cut this use case out of the ecosystem and full-RBF is their tool to accomplish this. They also think that full-RBF will ease the pain when we run into the 1MB blocksize limit and this is why they are in such a hurry to implement. They don't wish to be open about this since the public outcry will be even bigger than it already is.

@pstratem
Copy link
Contributor

pstratem commented Dec 21, 2015

utACK 786f92d

Each node operator has the right to run whatever policy they wish.

If you oppose this pull request you oppose freedom of choice.

@sandakersmann
Copy link
Contributor

sandakersmann commented Dec 21, 2015

If you oppose this pull request you oppose freedom of choice.

Freedom of choice is great. Let's remove the blocksize limit and let the miners make as big blocks that they wish!

@JornC
Copy link

JornC commented Dec 21, 2015

It's slightly unfortunate that this would negatively alter a small fraction of businesses' faulty security assumptions while there is not yet a suitable alternative available short of waiting for confirmation or introducing any sort of trust, but considering this has been a topic of strong contention for well over a year now, and that this outcome or anything equivalent in implication (0-conf is not secure) has always been the sensible outcome, unfortunate is all it will have to be.

utACK, with preference to default full-rbf when application-equivalent secure alternatives to 0-conf are available and practical.

@sandakersmann
Copy link
Contributor

sandakersmann commented Dec 21, 2015

businesses' faulty security assumptions

Businesses have done commerce with 0-conf transactions for years now. It is not assumptions, it works. But with full-RBF it won't work any more.

@surg0r
Copy link

surg0r commented Dec 21, 2015

They know exactly what they are doing and breaking.
On 21 Dec 2015 23:23, "sandakersmann" notifications@github.com wrote:

businesses' faulty security assumptions

Businesses have done commerce with 0-conf transactions for years now. It
is not assumptions, it works. But with full-RBF it won't work any more.


Reply to this email directly or view it on GitHub
#7219 (comment).

@sipa
Copy link
Member

sipa commented Dec 21, 2015

@sandakersmann
Copy link
Contributor

sandakersmann commented Dec 21, 2015

Good to hear that Pieter. You have always been the voice of reason from Blockstream.

@luke-jr
Copy link
Member Author

luke-jr commented Dec 22, 2015

@sipa RBF does not change the implied promise of transactions, nor does setting the opt-out flag remove such a promise. I find it disappointing that even after your excellent post on how developers don't have authority to do a hardfork on our own, you seem to support developers usurping this authority from node operators. :/

@dcousens
Copy link
Contributor

dcousens commented Dec 22, 2015

@luke-jr as mentioned above, I agree with this PR personally, probably for the same reasons you do.

But, I think you are confusing @sipa's comments on consensus authority with bitcoin/bitcoin's policy authority, which, is much less important not relevant?

@jtimon
Copy link
Member

jtimon commented Dec 22, 2015

What about a -policy string argument:

-policy=optinrbf [default]: Allows opt-in RBF.
-policy=firstseen: Never allows spending conflicts replacements.
-policy=fullrbf: Just does full RBF without looking at the op-in signal.

That would make it easier to add other policies in the future, for example:

-policy=random: randomly rejects some consensus-valid txs to the mempool.
-policy=test: this would be basically equivalent to --acceptnonstdtxn=1 -policy=optinrbf
...

if (!setConflicts.count(ptxConflicting->GetHash()))
{
if (setConflicts.count(ptxConflicting->GetHash()))
continue;

This comment has been minimized.

@jtimon

jtimon Dec 22, 2015 Member

There's no need to use continue here.

This comment has been minimized.

@luke-jr

luke-jr Dec 22, 2015 Author Member

How not? Please elaborate.

This comment has been minimized.

@jtimon

jtimon Dec 22, 2015 Member

There's never a strict need for a continue statement:

while(fCondition)
    if (!fA)
       continue;
    // Do AAA
}

Is equivalent to

while(fCondition)
    if (fA) {
      // Do AAA
    }
}

which is what we had and you are changing for no good reason.
The second version (what we currently have) is not only more readable but also more flexible, for example, say we want to change the code to do the following:

while(fCondition)
    if (fA) {
      // Do AAA
    }
   // Do BBB
}

In the second example it's just adding the new code out of the if statement, in the first case we have to rewrite the code like the first case and indent first.
The only reason I can think of for using continue is reducing this PR's total diff(by avoiding the indent to the code under the new condition, or creating a new sub-function for the code under the new condition), but it doesn't make the diff (or the final resulting code) more readable, so I really don't see the point.
I would generally avoid using continue unless it's one of those rare cases where its use actually increases readability and reduces the potential for bug introduction (instead of the opposite as it usually does).
For example, many continue statements in the same loop may be more readable than excessive nesting:

while(fCondition)
    if (!fA)
       continue;
    // Do AAA
    if (!fB)
       continue;
    // Do BBB
    if (!fC)
       continue;
    // Do CCC
    if (!fD)
       continue;
    // Do DDD
    // E, F, G...
    // Do ZZZ
}

is more readable than, but equivalent to

while(fCondition)
    if (fA) {
      // Do AAA
      if (fB) {
         // Do BBB
         if (fC) {
           // Do CCC
           if (fD) {
              // Do DDD
                // E, F, G...
           } // D
         } // C
      } // B
    } // A
    // Do ZZZ
}

Of course, the readability disadvantages of excessive nesting can always be fixed with sub-functions instead:

bool DoFromA()
{
   if(fA) {
       // Do AAA
       return DoFromB();
   }
   return false
}
bool DoFromB()
{
   if(fB) {
       // Do BBB
       return DoFromC();
   }
   return false
}
// DoFromC(), etc

while(fCondition)
    if (DoFromA()) {
      // Do ZZZ
    }
* @return command-line argument (0 if invalid number) or default value
*/
int64_t GetArg(const std::string& strArg, int64_t nDefault);
int64_t GetArg(const std::string& strArg, int64_t nDefault, int64_t nNoValue = 0);

This comment has been minimized.

@jtimon

jtimon Dec 22, 2015 Member

Why not just return nDefault when the option is provided with no value?

This comment has been minimized.

@luke-jr

luke-jr Dec 22, 2015 Author Member

The use case is given in the PR...

This comment has been minimized.

@jtimon

jtimon Dec 22, 2015 Member

The exact same behavior can be achieved on init without touching this function.
I repeat the question, why not just return nDefault when the option is provided with no value?
In the concrete case of this PR, seems just a hack so that "-replacebyfee" is equivalent to "-replacebyfee=1" instead of "-replacebyfee=2" like it would be if this function received no extra parameters. In my opinion, that doesn't justify adding a third parameter to this function.
Anyway, if we switch to strings as we should, this discussion would be irrelevant (unless you want not using policy to default to one value, but using an empty -policy to default to another value).

@dcousens
Copy link
Contributor

dcousens commented Dec 22, 2015

@jtimon IMHO that is certainly the pipe dream.

@luke-jr
Copy link
Member Author

luke-jr commented Dec 22, 2015

But, I think you are confusing @sipa's comments on consensus authority with bitcoin/bitcoin's policy authority, which, is not relevant?

My point is that we don't have such an authority at all, and trying to force users to exercise their authority in specific ways by making their desired policies explicitly difficult, especially in a case like this where it is easy to provide the configuration options, is an attempt to usurp their authority, and essentially the same as an attempt to hardfork without economic consensus.

What about a -policy string argument:

This is definitely where I'd like to see us go in the future, but not something viable for 0.12, which is the target of this PR.

@jtimon
Copy link
Member

jtimon commented Dec 22, 2015

@dcousens
"pipe dream": a hope, wish, or dream that is impossible to achieve or not practical.
How is changing an integer enumeration to a string enumeration impossible?

Re mergeability discussion:

  1. Miners de facto enable full-RBF-always regardless of what Core supports

This alone justifies supporting full-RBF optionally IMO.

  • The feature does not change consensus critical code
  • The feature will have users (and @petertodd will be able to stop maintaining his fullrbf patch)
  • Maintaining the feature optionally is cheap in terms of code complexity

The fact that some people so strongly oppose to supporting -policy=fullrbf in bitcoin core, indicates that there will be demand for -policy=firstseen. And since is also trivial to maintain, the same arguments apply, even if I personally think that is probably the stupidest spend conflict replacement policy after -policy=lastseen (yes, I think -policy=randomreplace makes a lot more sense).

@jtimon
Copy link
Member

jtimon commented Dec 22, 2015

This is definitely where I'd like to see us go in the future, but not something viable for 0.12, which is the target of this PR.

Well, in the future each policy could have their own parameters and you told me those could be easily configurable in the GUI dynamically (see #6423 ), but the command-line interface can look more like that right now. In fact, if people don't like -policy=test and this is going to be implemented with specific parameters too, I will have a hard time finding the next example to introduce the -policy string argument that truly "scales" command-line-complexity-wise.

@dcousens
Copy link
Contributor

dcousens commented Dec 22, 2015

@jtimon to clarify, I meant, that is exactly what I'd love to see, but, I'm not sure if it is going to be possible [in master anyway] due to how controversial this PR seems to be.

@luke-jr
Copy link
Member Author

luke-jr commented Dec 22, 2015

Not worth arguing over this, so I'm just going to throw it in Bitcoin LJR and call it done.

@luke-jr luke-jr closed this Dec 22, 2015
@jtimon
Copy link
Member

jtimon commented Dec 22, 2015

Not worth arguing over this, so I'm just going to throw it in Bitcoin LJR and call it done.

I didn't saw anyone complaining about offering firstseen (the previous default replacement policy) optionally. That will also make branches outside of Bitcoin Core that support -policy=fulrbf slightly easier to maintain (although it's currently pretty easy as shown by this PR anyway).
If we leave

-policy=fullrbf: Just does full RBF without looking at the op-in signal.

out of the scope of this PR to avoid controversy, can we at least support:

-policy=default [default]: Allows opt-in RBF.
-policy=firstseen: Never allows spending conflicts replacements.

?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.