Discourage fee sniping with nLockTime #2340

pull petertodd wants to merge 1 commits into bitcoin:master from petertodd:discourage-fee-sniping changing 1 files +27 −1
  1. petertodd commented at 11:09 am on March 1, 2013: contributor

    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.

  2. petertodd commented at 5:11 am on March 2, 2013: contributor
    @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.
  3. petertodd commented at 5:48 pm on March 3, 2013: contributor
    Coinbase got back to me and they’ve now fixed the issue - they’re using the same IsFinal() logic as the reference client.
  4. petertodd commented at 11:40 am on March 4, 2013: contributor
    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.
  5. BitcoinPullTester commented at 12:44 pm on March 4, 2013: none
    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/745083a87c9e8b42e472b1d232d68bb332a86bc1 for binaries and test log.
  6. BitcoinPullTester commented at 2:25 am on April 14, 2013: none
    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/903fec9192e71b9734388f5d951a7163dee5b852 for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
  7. luke-jr commented at 3:47 am on July 17, 2013: member
    @petertodd Needs rebase.
  8. jgarzik commented at 3:23 am on August 25, 2013: contributor
    This seems nice to have.
  9. petertodd commented at 7:16 pm on August 25, 2013: contributor

    @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?

  10. petertodd commented at 7:17 pm on August 25, 2013: contributor
    @luke-jr You added this patch to next-test - any related bug reports?
  11. jgarzik commented at 7:23 pm on August 25, 2013: contributor
    Without getting into too much public detail: BitPay uses stock bitcoind as boundary nodes if at all possible.
  12. petertodd commented at 7:26 pm on August 25, 2013: contributor
    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.
  13. BitcoinPullTester commented at 9:02 pm on August 25, 2013: none
    Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/50e84e991d30ae86a40603cf8de5b7ac35734dad for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.
  14. schildbach commented at 9:55 pm on August 25, 2013: contributor

    @petertodd

    the android wallet seems to have been updated to never show unconfirmed tx’s

    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.

  15. luke-jr commented at 10:38 pm on August 25, 2013: member
    I’ve been using this as part of next-test for months now and haven’t encountered any problems as a result.
  16. petertodd commented at 0:37 am on August 26, 2013: contributor

    @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.

  17. schildbach commented at 7:29 am on August 26, 2013: contributor
    @petertodd Thanks for your detailed tests. I’ll investigate.
  18. wtogami commented at 7:56 am on October 14, 2013: contributor
    @schildbach any results?
  19. schildbach commented at 8:06 am on October 14, 2013: contributor

    @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.

  20. petertodd commented at 6:50 pm on October 15, 2013: contributor
    @schildbach What’s blocking showing those tx’s in their unconfirmed, but final, state?
  21. schildbach commented at 7:54 am on October 16, 2013: contributor
    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.
  22. petertodd commented at 4:47 pm on October 17, 2013: contributor

    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.

  23. mikehearn commented at 9:17 pm on October 17, 2013: contributor

    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.

  24. gmaxwell commented at 9:19 pm on October 17, 2013: contributor
    @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.
  25. mikehearn commented at 9:25 pm on October 17, 2013: contributor

    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:

    https://code.google.com/p/bitcoinj/issues/detail?id=469

  26. mikehearn commented at 2:33 pm on November 14, 2013: contributor
    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.
  27. petertodd commented at 6:00 am on November 21, 2013: contributor
    @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.
  28. schildbach commented at 9:09 am on November 21, 2013: contributor
    @petertodd If you have a test script, can you publish that so I can reproduce?
  29. mikehearn commented at 10:04 am on November 21, 2013: contributor
    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.
  30. petertodd commented at 6:46 pm on November 21, 2013: contributor
    @schildbach I don’t have a test script; I used the raw tx API and just edited the hex manually.
  31. luke-jr commented at 0:14 am on February 21, 2014: member
    ACK. I have been using this for 4 months now, at least 2 months of that including my offline wallet with hundreds of BTC. No problems at all.
  32. jgarzik commented at 1:04 am on February 21, 2014: contributor
    ACK
  33. petertodd commented at 1:09 am on February 21, 2014: contributor

    NACK

    This has potential privacy problems by acting as yet another bit of metadata added to transactions that can distinguish wallets and thus coins. While it may be a useful thing in the future, for now I consider those privacy issues significantly more important than the fee-sniping problem.

    Closing

  34. petertodd closed this on Feb 21, 2014

  35. gmaxwell commented at 1:32 am on February 21, 2014: contributor
    Right now Bitcoin-QT created transactions are trivially distinguishable.
  36. petertodd commented at 1:40 am on February 21, 2014: contributor
    @gmaxwell Explain?
  37. luke-jr commented at 1:59 am on February 21, 2014: member
    AFAIK this is only a privacy problem as long as only a few people are using it.
  38. gmaxwell commented at 2:25 am on February 21, 2014: contributor
    @petertodd All our signatures are have S values in half the order. Once you have more than a couple signatures on your transaction it’s distinguishable with near certainty. We also do things like … not resending change to the same address which distinguishes us from multibit and bc.i, for example. Or compute fees correctly, distinguishing us from multibit (and I think anything bitcoinj based?), bc.i, mtgox, etc.
  39. mikehearn commented at 5:52 am on February 21, 2014: contributor

    Bitcoinj 0.11 wallets also use half order s values now and fees are computed correctly by bitcoinj, save for the fact that it won’t ever create free transactions regardless of priority due to the tiny fixed space available. Once floating fees come in I hope to change that as we will have some notion of how fast a free tx could confirm. And I’m working on HD wallets now so the address reuse will go away in the coming months. We’re keeping up…. mostly :)

    Anyway, Peter’s argument could apply to any upgrade. Perhaps in this case the benefit is so minor it’s really not worth it, but I’m not comfortable with arguments that could be reused in any tx format upgrade.

    BTW the issues with how finality are represented by bcj should be fixed in 0.11 On 21 Feb 2014 07:56, “Gregory Maxwell” notifications@github.com wrote:

    @petertodd https://github.com/petertodd All our signatures are have S values in half the order. Once you have more than a couple signatures on your transaction it’s distinguishable with near certainty. We also do things like … not resending change to the same address which distinguishes us from multibit and bc.i, for example. Or compute fees correctly, distinguishing us from multibit (and I think anything bitcoinj based?), bc.i, mtgox, etc.

    — Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2340#issuecomment-35693235 .

  40. petertodd commented at 5:55 am on February 21, 2014: contributor

    @gmaxwell Most of those practices are things other wallets can and should adopt, e.g. S-values and using new addresses for change. Fees will be increasingly floating in the future, which makes them less useful as a distinguisher. @luke-jr Not necessarily. For instance a CoinJoin transaction may be completed in a time long enough for multiple blocks to be created, in which case the transaction sticks out compared to transactions created otherwise. Similarly users using high-latency anonymization networks can only guess at what nLockTime to use.

    Now maybe those issues can be satisfactorily solved by randomizing the nLockTime chosen, say 90% of the time pick the default, 10% of the time pick something at random from the last 100 blocks.

    Note that there is also a propagation issue, although that’s similar to how we don’t repropagate orphan tx’s after we learn about their parents - the fix can make use of that machinery. The code has to be neutered right now by setting nLockTime back 1 from what it could be to deal with the nLockTime off-by-one bug - maybe it’d be best to change that to 10 back to be safer so that the initial rollout only triggers nLockTime-related bugs rather than propagation problems.

    In any case this is a cost-benefit problem. The problem of fee-sniping is only apparent with large mining pools comprising a significant fraction of the total mining power and fees significantly larger than the block reward. While we have the former, the latter is a long way off. At the same time we already know mining pools already have an incentive to become bigger due to orphan costs; the incentive to become larger to fee snipe is probably small in comparison.

  41. luke-jr commented at 5:56 am on February 21, 2014: member
    @mikehearn What are your thoughts about porting this change to BitcoinJ?
  42. luke-jr commented at 5:59 am on February 21, 2014: member
    @petertodd High latency users can afford to pick a future block and have it wait for relaying.
  43. mikehearn commented at 6:01 am on February 21, 2014: contributor

    I’m not sure yet. Still watching the debate. I’m not against doing that, although right now there are a lot of other changes that seem more important. I guess I’d like to see at least a BIP explaining the change and rationale in detail, so wallet authors can be pointed there to understand things. On 21 Feb 2014 11:26, “Luke-Jr” notifications@github.com wrote:

    @mikehearn https://github.com/mikehearn What are your thoughts about porting this change to BitcoinJ?

    — Reply to this email directly or view it on GitHubhttps://github.com/bitcoin/bitcoin/pull/2340#issuecomment-35701301 .

  44. petertodd commented at 6:55 am on February 21, 2014: contributor

    Well anyway this is pretty low on my priority list; I updated it so that it goes 10 blocks back, and 1/10 times the depth is randomized.

    If it pans out, it pans out, and in the meantime it’ll help get rid of whatever remaining nLockTime-related bugs people have in their custom implementations.

    ACK

  45. petertodd reopened this on Feb 21, 2014

  46. petertodd commented at 11:57 pm on March 17, 2014: contributor
    rebased
  47. laanwj added the label TX fees on May 9, 2014
  48. sipa commented at 11:30 am on June 1, 2014: member
    I think this is a good idea for normal wallet behaviour. Perhaps we should open some discussion on the mailing list, or have an informative BIP?
  49. jgarzik commented at 2:29 pm on June 1, 2014: contributor
    @sipa +1
  50. laanwj commented at 9:09 am on June 25, 2014: member
    If this is conditional on a BIP or mailing list condition I’m going to close this for now. It makes no sense to continue asking for rebases. Let me know when discussion is concluded and it can be merged.
  51. laanwj closed this on Jun 25, 2014

  52. petertodd commented at 3:37 pm on June 25, 2014: contributor
    @laanwj Quite reasonable; I suspect there’s zero interest in it frankly. It’s a solution to a very far off problem.
  53. jgarzik commented at 3:43 pm on June 25, 2014: contributor

    I continue to think this is good wallet behavior, and it is cheap to put in place now for the long term. RE privacy, that is only true if others fail to adopt this practice. bitcoind wallets can usually be detected by other means today anyway (thus in the future it should be private presuming it’s adopted, and today it’s not the biggest problem in that arena).

    Just mailed the list. I think that is sufficient. I don’t think this rises to the level of a BIP.

  54. gmaxwell commented at 4:34 pm on June 25, 2014: contributor
    It doesn’t change how anyone else must behave… I agree it would be a best practice and consistency would be good for improved privacy, but I don’t think thats a reason to defer or delay doing this.
  55. sipa commented at 4:36 pm on June 25, 2014: member
    I suggested the BIP and mailinglist discussion mostly to let other wallet implementation be aware of this possibility and suggest using it. I don’t think it’s a blocker. The only concern here seems to be privacy, but I’m pretty sure our wallet implementation is already pretty recognizable.
  56. petertodd commented at 4:39 pm on June 25, 2014: contributor

    Anyway, if it doesn’t turn out to be a good idea, we can easily revert it; if it does turn out to be a good idea, it’ll be easier to implement it more widely once someone has forced people to fix their nLockTime related bugs.

    But I’m going to catch a flight soon, so if you want to merge it, someone else needs to fix whatever is making the merge fail. :)

  57. laanwj reopened this on Jun 25, 2014

  58. schildbach commented at 12:48 pm on June 29, 2014: contributor
    I must admit that I somehow don’t understand this issue fully any more. Can someone post a tl;dr what nLockTime related bugs do other (perhaps specifically bitcoinj-based) wallets have?
  59. mikehearn commented at 12:56 pm on June 29, 2014: contributor
    I think the bugs were fixed a long time ago, at least at the library level.
  60. petertodd commented at 1:24 pm on June 29, 2014: contributor
    Oh, I didn’t mean to say I knew of any unfixed bugs right now, just that this will keep new ones from sticking around for long.
  61. petertodd commented at 2:10 pm on July 13, 2014: contributor
    Rebased. This should be ready for merging.
  62. jgarzik commented at 5:09 pm on July 29, 2014: contributor

    Current dev consensus on this is “looks good to have” It was posted to the mailing list thread “Wallet nLockTime best practices” with little to no response.

    Let’s collect ACKs and get it in: ACK

  63. btcdrak commented at 9:19 pm on July 30, 2014: contributor
    ACK
  64. laanwj commented at 6:51 am on July 31, 2014: member
    I think we’re all a bit hesitant to be the first wallet to merge this.
  65. dgenr8 commented at 5:40 pm on August 8, 2014: contributor
    ACK. Another benefit is more txes in the blockchain with information on first-eligible-block vs. actual-block, at no additional storage cost.
  66. in src/wallet.cpp: in ccc521e98e outdated
    1363+    // Secondly occasionally randomly pick a nLockTime even further back, so
    1364+    // that transactions that are delayed after signing for whatever reason,
    1365+    // e.g. high-latency mix networks and some CoinJoin implementations, have
    1366+    // better privacy.
    1367+    if (GetRandInt(10) == 0)
    1368+        txNew.nLockTime -= GetRandInt(100);
    


    dgenr8 commented at 5:49 pm on August 8, 2014:
    This can cause an underflow when current height is less than 110 (or 10 on line 1361).

    Michagogo commented at 6:08 pm on August 10, 2014:
    Correct me if I’m wrong, but I would think that it’s not a problem on line 1361 because this is (if understand correctly) when creating a transaction spending wallet coins, and if the height is less than 10 then no mature coinbases exist and so a transaction can’t be created. If I’m right about that, then the 110 issue can be fixed by dropping the number down from 100 to 89(?). (or you could just add a check for an underflow, or for a low chainActive.Height…)

    petertodd commented at 9:48 pm on August 13, 2014:
    Nice catch! I just put std::max() on both. ‘can’t test on this computer; mind telling me if that fixed that edge case?
  67. BitcoinPullTester commented at 7:57 pm on August 18, 2014: none

    Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p2340_4cb83ab0c0d4eb43a90b6b14381fb04f115bd084/ for binaries and test log.

    This could happen for one of several reasons:

    1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
    2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
    3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
    4. The test suite fails on either Linux i386 or Win32
    5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

    If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

    This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

  68. gmaxwell commented at 0:31 am on October 9, 2014: contributor
    I for some reason thought this was merged already. Functionality that doesn’t get used will rot. It’s also useful for some far out concerns. I’m in favor of this, but it should probably get a configuration option to disable it. (e.g. if it turns out to be problematic for someone even after we ship it).
  69. TheBlueMatt commented at 0:34 am on October 9, 2014: member
    Bitcoin Core is the reference wallet, and, as such, should set the best practices, ACK.
  70. jgarzik commented at 0:47 am on October 9, 2014: contributor
    +1 still want this
  71. luke-jr commented at 1:23 am on October 9, 2014: member
    If someone hasn’t complained by now, they can hack the code and recompile. I’ve been using this in production for over a year now with no issues. So I’d favour just merging it without a config option.
  72. btcdrak commented at 6:28 am on October 9, 2014: contributor

    ACK

    +1

  73. petertodd commented at 6:30 am on October 9, 2014: contributor
    @btcdrak You realize code you ACKed doesn’t actually compile right now… Lemme get this fixed and we can get this actually tested and merged.
  74. btcdrak commented at 6:34 am on October 9, 2014: contributor
    @petertodd Oops, I meant +1 to the idea.
  75. petertodd commented at 6:41 am on October 9, 2014: contributor
    @btcdrak Your punishment: go fix that compile error for me so I can watch bad movies on my flight home instead. :)
  76. in src/wallet.cpp: in 4cb83ab0c0 outdated
    1363+    // Secondly occasionally randomly pick a nLockTime even further back, so
    1364+    // that transactions that are delayed after signing for whatever reason,
    1365+    // e.g. high-latency mix networks and some CoinJoin implementations, have
    1366+    // better privacy.
    1367+    if (GetRandInt(10) == 0)
    1368+        txNew.nLockTime = std::max(0, txNew.nLockTime - GetRandInt(100));
    


    btcdrak commented at 7:16 am on October 9, 2014:
    0txNew.nLockTime = std::max(0U, txNew.nLockTime - GetRandInt(100));
    

    (type mismatch)


    dgenr8 commented at 4:23 pm on October 9, 2014:

    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.


    btcdrak commented at 6:39 pm on October 9, 2014:
    While both patches work, since txNew.nLockTime is already an unsigned int, it makes more sense to me to make the first parameter unsigned.
  77. btcdrak commented at 7:18 am on October 9, 2014: contributor
    @petertodd done, but a single character diff doesnt seem like punishment
  78. petertodd force-pushed on Oct 10, 2014
  79. petertodd commented at 6:33 am on October 10, 2014: contributor

    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.

  80. btcdrak commented at 6:39 am on October 10, 2014: contributor

    Well I can definitely say “tested ACK” it this time.

    Tested ACK

  81. Discourage fee sniping with nLockTime ba7fcc8de0
  82. petertodd force-pushed on Oct 13, 2014
  83. petertodd commented at 12:20 pm on October 13, 2014: contributor
    Fixed issue brought up by @dgenr8 and added an assert() to double-check the resulting nLockTime is sane.
  84. petertodd commented at 12:21 pm on October 13, 2014: contributor
    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. :(
  85. jgarzik commented at 3:29 pm on November 18, 2014: contributor
    tested ACK
  86. laanwj added the label Wallet on Dec 5, 2014
  87. laanwj removed the label TX fees on Dec 5, 2014
  88. laanwj merged this on Dec 19, 2014
  89. laanwj closed this on Dec 19, 2014

  90. laanwj referenced this in commit 811c71d287 on Dec 19, 2014
  91. schildbach commented at 2:38 pm on December 22, 2014: contributor
    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.
  92. petertodd commented at 2:40 pm on December 22, 2014: contributor

    @schildbach Thanks!

    Out of curiosity, how long for the Android store to update?

  93. schildbach commented at 2:46 pm on December 22, 2014: contributor
    @petertodd It’s already on Google Play. It usually takes an hour or so to appear.
  94. petertodd commented at 7:22 pm on December 23, 2014: contributor
    @schildbach Thanks! Confirmed working here too.
  95. 0xartem referenced this in commit 24acfedf88 on Jun 1, 2018
  96. DrahtBot locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-06-29 10:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me