Discourage fee sniping with nLockTime #2340
Conversation
|
@sipa I looked into it and I think we don't need to worry about nBestHeight decreasing on a retarget reorg. Anything already in a node's mempool stays there and will be mined once the chain height catches upl so it would be extremely rare for that to take more than an extra block or two. |
|
Coinbase got back to me and they've now fixed the issue - they're using the same IsFinal() logic as the reference client. |
|
Re-based on top of the "fix off-by-one errors" fix, which unfortunately means this has to be weakened until the network upgrades. It'll still at least shake out bugs in the meantime. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/745083a87c9e8b42e472b1d232d68bb332a86bc1 for binaries and test log. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/903fec9192e71b9734388f5d951a7163dee5b852 for binaries and test log. |
|
@petertodd Needs rebase. |
|
This seems nice to have. |
|
@jgarzik @luke-jr Updated and tested it against inputs.io, Coinbase, EasyWallet, SatoshiDice and the Android Bitcoin Wallet. It may make zero-conf tx's take a little longer to show up for SatoshiDice, but other than that possible issue I didn't have any problems. (the android wallet seems to have been updated to never show unconfirmed tx's so that's a non-issue) I couldn't test inputs.io properly because right now they aren't showing any transactions as confirmed for my account, nLockTime or not. Note that this version is still the weaker one compatible with the current off-by-one behavior of the rest of the network that #2342 fixes. What does BitPay do with nLockTime-using transactions? |
|
@luke-jr You added this patch to next-test - any related bug reports? |
|
Without getting into too much public detail: BitPay uses stock bitcoind as boundary nodes if at all possible. |
|
Well if you trust those bitcoind's 100% for what is or isn't a real transaction then this patch won't cause any problems for BitPay customers. |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/50e84e991d30ae86a40603cf8de5b7ac35734dad for binaries and test log. |
Why do you think so? That's certainly not the case. However it will display a warning if lockTime > 0, and it also checks the unconfirmed dependency chain. |
|
I've been using this as part of next-test for months now and haven't encountered any problems as a result. |
|
@schildbach Actually seems that Android Wallet has a number of issues: 8f8dee4bbd74b573c324745d9d23938a1e4d12f269f9afca022224cf740f16aa - This tx has nLockTime=1, but nSequence=int max so it is a final tx. Instead the wallet shows a big read "this transaction is non-standard and should not be trusted" until it confirmed - kinda silly. fdb100df609349802c90dee38c694f3626b6c1f62a20ba92603ad17202b09322 - nLockTime=1, and nSequence set so nLockTime is active, but the tx is locked. It eventually showed up in my wallet, but only after a confirmation. It didn't show up immediately. a4cceb4df7db3507966e57aea6d8f7b21ceabee55bac573e9b9590229fde6a3f - This one, and a few like it, are the worst though: they are time-locked transactions, and every one of them not only didn't know up prior to being confirmed, but even after being confirmed they still didn't show up in my wallet. tx 6ed945173e1455edf09931b4c7caac165c7d834ddc1ea296a24b9213a45cf24d is a particularly extreme example, having the minimum possible "lock-by-time" nLockTime. Curiously if nLockTime > the "lock-by-time" range, but all the sequence numbers are set so the tx is locked, the transaction also never shows up and doesn't display that "this transaction is non-standard" error message. For example: 8100cb9c84cf2f9c78ab2e6b488feb0a531e2ef88a1d1d28243a9e8361a433a7 Finally after re-scanning the chain all the tx's showed up in my wallet. |
|
@petertodd Thanks for your detailed tests. I'll investigate. |
|
@schildbach any results? |
|
@wtogami I fixed the UI so that the first case should not show up as timelocked any more. The other cases should not show up in their unconfirmed state. However, they will show once they're blockchain confirmed. |
|
@schildbach What's blocking showing those tx's in their unconfirmed, but final, state? |
|
I think the rationale is those transactions currently do not constitute a usecase that is supported by bitcoinj, so for safety reasons they are not allowed into the wallet. I believe this will change in future, probably with the introduction of more complex payment types (consisting of more than one tx). Probably @mikehearn can tell more. |
|
Using the nLockTime feature is the business of the sender; the receiver has no reason to care about whether or not that feature was used if the transaction is now final and can be mined. This is just another example of the "death-spiral" of feature disablement that we keep seeing in Bitcoin where because we disable features based on nothing more than a suspicion that they might somehow be used for something nefarious, which in turn makes it impossible to develop anything useful using that feature because wallet software interacts badly with it. |
|
Hmm, that argument sounds familiar :) This came out of the conclusion that people could create time-locked transactions that people would think would confirm quickly, then wouldn't, making it easier to double spend. I think it was you that brought that up originally actually. Anyway it was a fair point so those transactions just don't get accepted into the wallet by default. And nowadays they're non standard anyway so they shouldn't even propagate to those wallets. People upgrade SPV wallets fairly fast, so we can certainly change that for a subset of cases if it's important and won't increase risk to merchants. |
|
@mikehearn The problem right now is that they don't show up in android wallet even after they're locked. Those transactions are not non-standard, they propagate fine, and other wallets (most?) display them okay too. |
|
I'll re-review that code, but I think final transactions are allowed, or are supposed to be. There was an issue with the Android UI checking if there was a time lock rather than if it was final, but I thought that was fixed. I filed bug 469 to investigate: |
|
I fixed the bitcoinj side issue. It may require a quick new API to make the UI do the right thing though. Andreas, let me know when you have time to retest this. |
|
@schildbach @mikehearn Current version of the Android wallet is rejecting all using txs with nSequence != max and/or nLockTime != 0 even once they are confirmed. |
|
@petertodd If you have a test script, can you publish that so I can reproduce? |
|
The current version of the app is not using bitcoinj 0.11-SNAPSHOT which is where I made the fixes. So that would be expected. |
|
@schildbach I don't have a test script; I used the raw tx API and just edited the hex manually. |
|
@petertodd Oops, I meant +1 to the idea. |
|
@btcdrak Your punishment: go fix that compile error for me so I can watch bad movies on my flight home instead. :) |
| // e.g. high-latency mix networks and some CoinJoin implementations, have | ||
| // better privacy. | ||
| if (GetRandInt(10) == 0) | ||
| txNew.nLockTime = std::max(0, txNew.nLockTime - GetRandInt(100)); |
There was a problem hiding this comment.
txNew.nLockTime = std::max(0U, txNew.nLockTime - GetRandInt(100));
(type mismatch)
There was a problem hiding this comment.
I think it needs to be
txNew.nLockTime = std::max(0, (int)txNew.nLockTime - GetRandInt(100))
otherwise, the second argument can't be negative as it must for the max to work.
There was a problem hiding this comment.
While both patches work, since txNew.nLockTime is already an unsigned int, it makes more sense to me to make the first parameter unsigned.
|
@petertodd done, but a single character diff doesnt seem like punishment |
petertodd
force-pushed
the
discourage-fee-sniping
branch
from
aea2903
to
c75516a
Compare
|
Fixed unsigned-vs-signed issue and did one last bit of testing. Lets just merge this as is before it drags on any further. Adding a command line option to disable it can easily be done in another pull-req. |
|
Well I can definitely say "tested ACK" it this time. Tested ACK |
petertodd
force-pushed
the
discourage-fee-sniping
branch
from
c75516a
to
ba7fcc8
Compare
|
Fixed issue brought up by @dgenr8 and added an assert() to double-check the resulting nLockTime is sane. |
|
It's unfortunate there's no unittest that actually does a CreateTransaction()... at first glance it looks like quite a bit of work to add however. :( |
|
tested ACK |
ba7fcc8 Discourage fee sniping with nLockTime (Peter Todd)
|
Bitcoin Wallet version 4.16 – just released – is now compatible to this patch in that it doesn't display an 'untrusted' warning on those tx any more. |
|
@schildbach Thanks! Out of curiosity, how long for the Android store to update? |
|
@petertodd It's already on Google Play. It usually takes an hour or so to appear. |
|
@schildbach Thanks! Confirmed working here too. |
ba7fcc8 Discourage fee sniping with nLockTime (Peter Todd) (cherry picked from commit 811c71d)
Set nLockTime on wallet transactions (only, no RPC changes) such that they can only be mined by the next block, rather than a block orphaning the current best block. There are two reasons to do this, the first is the minor benefit that using nLockTime ensures related bugs get caught immediately, so protocols that need that feature don't become "unusual" transactions with flaky behavior.
The more important reason is to discourage "fee sniping" by deliberately mining blocks that orphan the current best block. Basically for a large miner the value of the transactions in the best block and the mempool can exceed the cost of deliberately attempting to mine two blocks to orphan the best block. However with nLockTime you'll soon run out of transactions you can put in the first block, which means they now need to go in the second. With limited block sizes you're run out of room, and additionally another miner now only needs to orphan one block to in-turn snipe the high-fee transactions you had to place in the second block, wrecking all your hard work.
Of course, the subsidy is high enough, and transaction volume low enough, that fee sniping isn't a problem yet, but by implementing a fix now we ensure code won't be written that makes assumptions about nLockTime that preclude a fix later. Transaction propagation is not impacted; even with non-final is non-standard the best block height implies we have at least one peer, and very soon more peers, that will accept and rebroadcast the transaction immediately.
Testing
Unit tests
Pass
Propagation
No issues. Used -blocknotify='bitcoind sendtoaddress' to send transactions as soon as a few block is found with worst-case of a node connected to only two 0.8 peers. Enabled -logtimestamps w/ ntp on that node and another node, and every transaction got to the second node within 5 seconds.
Services
No problems:
Easywallet, Instawallet, Coinbase Wallet, Coinbase Merchant Services, Blockchain.info, BitPay, bitfetch, localbitcoins
Won't accept until 1 confirmation:
Satoshidice
Most likely SatoshiDice implemented nLockTime == 0 rather than IsFinal() as their never-confirm nLockTime fix. I think there is an argument to be made that forcing them to make a minor change like this one would be a good way to test the waters to see if they'll make a more drastic change, as would be required if we make dust outputs non-standard.