-
Notifications
You must be signed in to change notification settings - Fork 36.2k
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
Scheduled full-RBF deployment #6352
Conversation
Replaces transactions already in the mempool if a new transaction is 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 a fixed depth/breadth limit to prevent DoS attacks. Includes stand-alone unittests for regtest in qa/replace-by-fee/
13b65cf
to
586e3ac
Compare
NACK treating policy as a software development decision. Make it a runtime option if you want. |
@luke-jr It is a runtime option, with a default suggested setting: https://github.com/bitcoin/bitcoin/pull/6352/files#diff-c865a8939105e6350a50af02766291b7R866 |
@petertodd Not the point. |
@luke-jr As discussed on IRC, we have practical reasons to want somewhat common mempool policies for the sake of small miners who (currently) have no other way of getting transactions. In the future that may change, but for now we're stuck with defining at least some broad notion of what miner policy might be. Anyway, not adopting full-RBF is also a policy decision. |
My preference would be to support multiple policies simultaneously (ie standard policy, testing policy, standard + full-RBF policy, etc). That would require something like #5180 first. |
@jtimon Yeah, from a code point of view I also support making policy modular, but keep in mind the point of this pull-req is to set a date for planning purposes so the remaining vulnerable companies can migrate to less harmful, less dangerous, zeroconf technology in a way that minimizes losses and disruption; the situation now is such that for the vulnerable users even a small % of miners adopting full-RBF, or indeed, any change to mempool policy at all, is disruptive. For instance, even something as simple as miners increasing the minimum relay on their own nodes can be easily exploited for double-spends; a few do right now, which makes accepting zeroconf transactions with lowish fees very dangerous. In short, I think you can make a good argument that this patch is actually a prerequisite to modular mempool policy. FWIW the patch does include a way to disable full-RBF indefinitely if the user chooses too by setting -fullrbfactivationtime=2147483647 Should I document this command-line argument and provide a more ergonomic way of doing it, like setting the activation time to -1 or 0 to disable? Heck, could call it -fullrbf with =0 being disabled, =1 enabled, and = to set the switchover time manually. |
@petertodd I was going to suggest |
Credit goes to Jeff Garzik for suggesting a scheduled switchover deployment strategy.
586e3ac
to
8821579
Compare
Documented -fullrbfactivationtime option, and changed it so -fullrbfactivationtime=0 disables full-RBF entirely to match ergonomics of other options. |
As a reminder, this ridiculous and highly controversial idea has been beaten to death elsewhere over and over again. Gavin, Jeff, myself, Adam Back and Coinbase have all explicitly stated opposition to it due to it being driven by fundamentally faulty logic. The counterargument is summed up here |
@mikehearn yes, it is clear that this is controversial. There's even people who like this but think that deploying it right now would be too risky for some businesses or want to give more time to instantly secure payments (possible with some payment channels like lightning channels). Fortunately this is just node policy and not consensus rules. That means we can support multiple configurable policies simultaneously without causing consensus forks. Other implementations can of course maintain their own policies (a single one or several of them, that's up to their maintainers). @petertodd travis is not happy I don't like a switch-over date here. Policy doesn't require that kind of coordination. If full-RBF is going to be the default policy, just make it so for a given major release and that's it. In the meantime I think we can just merged disabled by default. Or my preference, wait until we can select different policies #5180 (which is easy to review after #6335 and trivial to review after #6068) to merge this. |
@jtimon "Policy doesn't require that kind of coordination." It does though! Unfortunately we have the problem that there's no way to get transactions to all but the largest, most centralized, miners other than via the p2p network. Without some pretty major changes to how that network works (e.g. hashcash) any transactions that aren't propagated by the majority of relay nodes just aren't going anywhere - why I had to define a RBF service bit and include a bunch of hacky preferential peering code in my full-RBF patch series. One way to look at full-RBF is it's a common-denominator policy that supports other policies; full-RBF provides opportunities to get a larger range of transactions to miners who in turn may have their own policies in a DoS-resistant way by the simple measure of requiring fees to increase on each replacement, and it's significantly more efficient at doing that FSS-RBF. From a mining policy point of view, because it allows transactions to propagate that previously wouldn't have, it reduces the impact of the default policy on what miners can mine and what users can generate. Of course, from a code point of view I'd be happy to port it later to a pluggable policy system, but in the meantime, the code in this pull-req is well-tested and well-reviewed. Anyway, re: a switch-over date, in many ways that's us, as Bitcoin Core maintainers, saying "by this date stop assuming everyone has the same mempool policy" - the opposite approach of first-seen-dependent zeroconf requires nodes/miners to implement uniform policies to even begin to work. You are of course free to turn full-RBF off and I've made it easy to do so. re: travis, yeah, looks like it's broken again; other pull-reqs are failing too. |
Sure, just a reminder. Similar to rebase reminders...
Right, so why not just "from this major release, stop assuming everyone else will have the same mempool policy" instead? |
I think there's an important psychological component to actually having a date for people to have in their heads - it's a clear statement that we're giving the payment processing companies affected by the change 9 months (max) to prepare, but we're not going to delay longer for their benefit to the detriment of all other Bitcoin users. (where "we're" refers to miners to choose to take the advice of the Bitcoin Core maintainers and leave the default settings as-is) An alternative would be to enable full-RBF for all relay nodes, and force miners to make a choice. (e.g. by disabling getblocktemplate until some kind of config setting was enabled) Ugly from a usability point of view, but doable. There's also a risk of DoS attack by doing that, as if very few miners support full-RBF you can fill up mempools/use-up-bandwidth with transactions that are unlikely to be mined. (same problem with double-spend relaying) |
NACK on the code changes. Asking people to pull in somebody's personal repo to test code is a bad idea (too much risk it gets hacked and pulls in malware or something). And NACK on the concept. The whole point of the Bitcoin network it to prevent double-spending; making it easier to double-spend is a bad idea. |
@petertodd , why not pursue the path that lets some version of RBF get merged. #6176 defaulted to FSS, but had an option to switch to full RBF correct? Would people be ok with that? And then if you want to separately coordinate a date where interested parties can change their command line option to run with full RBF, thats not even the subject of a PR. I agree coordination makes sense, but it doesn't HAVE to be automated. |
@gavinandresen Note how the testing instructions call out a specific version of python-bitcoinlib by SHA1 commit hash; the repo getting hacked can't cause you to get compromised. Equally, python-bitcoinlib is a widely used and reviewed library, and https://github.com/petertodd/python-bitcoinlib is the "official" repo for it. @morcos It's a good question! I'm giving people a number of options here, so we'll see what we get re: consensus; if @jgarzik's switchover date idea doesn't get consensus removing it is just a few lines of code change, leaving code that does both types of RBF robustly and in a well-tested fashion. @D9B4BEF9 It would probably be best to save that kind of broader discussion about zeroconf for the mailing list, or perhaps the BIP. |
@D9B4BEF9 @gavinandresen never said that there is 0 assumption of risk for merchants accepting zeroconf transactions. He just said that full-RBF will make the existing risk higher. The risk is not 0 and cannot be 0 regardless, this is just how bitcoin works (and this is why we need mining and proof of work security). I agree bitcoin should make everything possible to lower the double-spend risk as much as possible. It is clear that full-RBF will make zeroconf a higher risk than it already is, and without zeroconf bitcoin can become a PITA for most merchants (10-60 minutes until you know if you can process an order or not). Less merchants accepting bitcoin has direct impact over the supply and demand market and obviously on the BTC/FIAT rate. I have great appreciation for @petertodd 's work in bitcoin, and totally agree RBF is helpful, but I am against full-RBF (at least don't make it the default behavior for relay nodes/miners) and would prefer FSS-RBF. If it matters, my business accepts zeroconf (for values of max 300$, thinking to raise the limit to 600$) and the double-spend losses are currently 0. |
I just want to chime in from a payment processor (Coinbase) point of view. Full RBF is bad for Coinbase and bad for Bitcoin. If full RBF is turned on today by even a small set of nodes, Coinbase will have to stop accepting 0-conf for all orders. That means customers will need to wait for ~10 minutes for their order to confirm. This makes the checkout experience much worse than existing payment methods. It's hard enough trying to convince consumers and merchants to adopt Bitcoin as a payment method today. Without 0-conf, you can forget about Bitcoin adoption for payments getting any traction whatsoever. Accepting payments with 0-conf works ok today. It's not perfect, but it works. We do acknowledge that as block rewards go to zero, there will be more incentives for miners to implement some sort of RBF. But this will take years before that happens naturally. When it does happen, there will likely be alternative better 0-conf solutions that merchants and payment processors can switch to. There is absolutely no reason to speed that up. Seems silly that anyone would risk hurting the nascent Bitcoin payment system just to prove a point. |
@coblee You're ok with FSS-RBF though right? What if we made the default policy of full-RBF be that it only applied if a transaction had been in the mempool for at least 2 blocks. That way transactions that you estimate have enough fee to likely be confirmed in the next block or two are also "safe", but for fixing stuck transactions we can have the flexibility of full-RBF. |
@morcos Yes, FSS-RBF is fine. Why is FSS-RBF not good enough to fix stuck transactions? Having complicated rules for this in the core code leads to a lot of potential bugs. RBF seems like something miners should implement themselves and maybe charge for it. I don't think it belongs here as a default policy. |
@coblee The last time I tried to use Coinbase to pay a merchant, it required me to fund my Coinbase account first, wait for that to confirm, and only then pay in an instant transfer from my Coinbase account to the merchant. Full RBF should have no effect whatsoever on this usage pattern. (Although I would recommend changing it to at least allow automatically paying the merchant, rather than making me wait for the deposit to confirm before I can begin my attempt to pay the merchant). |
@luke-jr Coinbase has always allowed you to pay a merchant by sending btc from any wallet. There was a flow where if someone was requesting money via email, the sender had to pay from their Coinbase account, but this is different from our merchant tools. And this bug has already been fixed. You can now pay money requests with an external wallet also. |
So it seems there's no complains about making FSS-RBF the standard policy instead of FS. And again, big NACK on deployment schedules for policy code, either it becomes part of the standard policy or becomes an alternative policy (although those would require something like #6068 to get started, a factory like in #5180, and then more changes to generically encapsulate the mempool replacement). |
@coblee full-RBF is much simpler than FSS-RBF. |
@jtimon @petertodd I think it would be best for this PR to default to FSS-RBF, i.e. |
@wumpus looks like travis needs a kick |
I think this should be disabled by default with I have created #6416 as a boilerplate for implementing alternative replacement policies. |
Closing for now, as this code will need to be re-worked for whatever memory-limited mempool proposal gets merged. |
Prior to Tuesday April 5th, 2016, at 3pm UTC, this pull-req implements first-seen-safe replace-by-fee; after the switchover date the first-seen-safe restrictions automatically turn off, resulting in full-RBF behavior. On testnet/regtest full-RBF is always supported.
The implementation is from my full-RBF patch series, specifically https://github.com/petertodd/bitcoin/tree/replace-by-fee-v0.11.0rc2, with additional logic and tests from #6176
BIP: https://github.com/petertodd/bips/blob/bip-full-rbf-deadline/bip-full-rbf-deadline.mediawiki
Credit goes to @jgarzik for suggesting a scheduled-in-advance deployment strategy.