Take the training wheels off anti-fee-sniping

petertodd commented at 7:19 pm on June 1, 2015: contributor

Now that the off-by-one error w/nLockTime txs issue has been fixed by 87550eefc1131132e940efcaf296bb399eeb02df (75a4d512cfc9a451fa627a3487ffed102cc67cab in the 0.11 branch) we can make the anti-fee-sniping protection create transactions with nLockTime set such that they’re only valid in the next block, rather than an earlier block. This makes the protection actually effective for its intended purpose.

There was also a concern about poor propagation, however testing with transactions with nLockTime = GetAdjustedTime()+1 as a proxy for nLockTime propagation, as well as a few transactions sent the moment blocks were received, has turned up no detectable issues with propagation. If you have a block at a given height you certainly have at least one peer with that block who will accept the transaction. That peer will certainly have other peers who will accept it, and soon essentially the whole network has the transaction. In particular, if a node recives a transaction that it rejects due to the tx being non-final, it will be accepted again later as it winds its way around the network.

I do not think this should go in v0.11 Rather by including this in the devel branch for v0.12 we have an opportunity to continually remind wallet authors and the like about how the anti-fee-sniping protection is supposed to work, and remind them that nLockTime = next block transactions should be accepted. My testing turned up only blockchain.info and Bitcoin Core itself as having issues with nLockTime at the limit transactions; hopefully by the time v0.12 is released these wallets will all be fixed.

laanwj added the label Wallet on Jun 3, 2015

Take the training wheels off anti-fee-sniping

Now that the off-by-one error w/nLockTime txs issue has been fixed by
87550eef (75a4d512 in the 0.11 branch) we can make the anti-fee-sniping
protection create transactions with nLockTime set such that they're only
valid in the next block, rather than an earlier block.

There was also a concern about poor propagation, however testing with
transactions with nLockTime = GetAdjustedTime()+1 as a proxy for
nLockTime propagation, as well as a few transactions sent the moment
blocks were received, has turned up no detectable issues with
propagation. If you have a block at a given height you certainly have at
least one peer with that block who will accept the transaction. That
peer will certainly have other peers who will accept it, and soon
essentially the whole network has the transaction. In particular, if a
node recives a transaction that it rejects due to the tx being
non-final, it will be accepted again later as it winds its way around
the network.

db6047d61b

petertodd force-pushed on Jun 22, 2015

petertodd commented at 3:40 am on June 22, 2015: contributor

Rebased now that Travis is fixed.

jtimon commented at 10:07 am on July 9, 2015: contributor

Trivial-code-change ACK. I believe @maaku will like this too.

btcdrak commented at 10:18 am on July 9, 2015: contributor

It is time. ACK

jgarzik commented at 11:35 am on July 9, 2015: contributor

concept ACK

sipa commented at 1:41 pm on July 9, 2015: member

Untested ACK

petertodd commented at 1:21 pm on July 11, 2015: contributor

@jtimon FWIW this isn’t actually a trivial change - the possible relay issue with txs right when a new block is broadcast is real.

If I could get one more person confirming my logic/testing showing that it isn’t a problem that’d be great.

jtimon commented at 9:58 pm on July 11, 2015: contributor

@petertodd But the change in the code is trivial (1 line apart from the documentation) and by that I mean that I trust my review enough that I don’t think I need to test this. If I felt I need to test it I would have said ut ACK (or test it and say tested ACK). If you prefer, ut ACK.

petertodd commented at 5:46 am on July 17, 2015: contributor

@jtimon Well, to be clear, imagine if this “trivial” change had a +1 in it, or if I hadn’t gone to the trouble of fixing the (multiple!) off-by-one errors we’ve seen re: nLockTime in #6183 and #2342 - in Bitcoin it’s common for seemingly trivial changes to actually have remarkably far-reaching effects. After all, if this really was a trivial change, I wouldn’t have written a fifteen line git commit description!

That said… one good thing about merging it at the beginning of the v0.12.0 development cycle, is that it’s a change that’s highly unlikely to lead to actual loss of funds, so we’ll give the whole ecosystem plenty of time to find and fix the off-by-one type errors that are probably still out there in various wallets. (and in that sense, yeah, from just a loss-of-funds point of view I agree it’s a trivial change, but knowing that requires a surprisingly good understanding of how the whole system works!)

jtimon commented at 7:45 am on July 17, 2015: contributor

I’m not saying it is trivial to understand or make. I’m not disregarding your contribution here. All I’m saying is that even though I haven’t tested it, the change was trivial for me to review. But for the shake of not wasting each other’s time, I take it back: there’s nothing trivial about this PR but it still gets my ut ACK. Happy now? On Jul 17, 2015 7:46 AM, “Peter Todd” notifications@github.com wrote:

@jtimon https://github.com/jtimon Well, to be clear, imagine if this “trivial” change had a +1 in it, or if I hadn’t gone to the trouble of fixing the (multiple!) off-by-one errors we’ve seen re: nLockTime in #6183 #6183 and #2342 #2342 - in Bitcoin it’s common for seemingly trivial changes to actually have remarkably far-reaching effects. After all, if this really was a trivial change, I wouldn’t have written a fifteen line git commit description!

That said… one good thing about merging it at the beginning of the v0.12.0 development cycle, is that it’s a change that’s highly unlikely to lead to actual loss of funds, so we’ll give the whole ecosystem plenty of time to find and fix the off-by-one type errors that are probably still out there in various wallets. (and in that sense, yeah, from just a loss-of-funds point of view I agree it’s a trivial change, but knowing that requires a surprisingly good understanding of how the whole system works!)

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

petertodd commented at 7:56 am on July 17, 2015: contributor

@jtimon Ah! Sorry, trivial to review is quite different than what I thought you meant. :) Thanks for clearing that up.

dcousens commented at 6:11 am on August 9, 2015: contributor

utACK

MarcoFalke commented at 4:53 pm on August 9, 2015: member

ACK. I can confirm that older versions (like v0.9.5) have issues with such transactions, just as expected: My bitcoin-qt-v0.9.5 shows them in the transaction tab but they don’t appear in listunspent 0. Let’s consider this a good thing, motivating people to update their nodes.

After applying this commit, listunspent 0 shows the transaction. I did not test the propagation issue, though.

petertodd commented at 10:16 pm on August 18, 2015: contributor

@MarcoFalke Thanks! @laanwj Be good to get this merged and done with…

TheBlueMatt commented at 1:52 am on August 24, 2015: member

utACK

sdaftuar commented at 2:43 pm on September 8, 2015: member

Please see #6595 (comment); perhaps we should consider improving the way we handle transactions that are invalid in the mempool during reorgs prior to merging this?

dcousens commented at 3:01 pm on September 8, 2015: contributor

Please see #6595 (comment); perhaps we should consider improving the way we handle transactions that are invalid in the mempool during reorgs prior to merging this? @sdaftuar I don’t think that particular issue should block this from being merged. Maybe that particular issue should be resolved before a release, but, I don’t see it as a blocker to this PR IMHO.

TheBlueMatt commented at 0:54 am on September 9, 2015: member

Yea, I agree with @sdaftuar…#6595 kinda breaks this…it should be easy to fix though (https://github.com/bitcoin/bitcoin/pull/6595#issuecomment-138742155)

petertodd commented at 8:32 pm on September 9, 2015: contributor

Looks like a reasonable concern; let’s leave this unmerged until #6595 is resolved.

TheBlueMatt commented at 11:37 pm on September 9, 2015: member

Should be addressed by #6656

On 09/09/15 20:32, Peter Todd wrote:

Looks like a reasonable concern; let’s leave this unmerged until #6595 #6595 is resolved.

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

gmaxwell commented at 10:54 pm on November 22, 2015: contributor

@petertodd Where are we on this?

btcdrak commented at 10:56 pm on November 22, 2015: contributor

looks like it’s pending #6915

jtimon commented at 7:29 pm on December 1, 2015: contributor

ping

gmaxwell commented at 8:12 pm on December 1, 2015: contributor

Can someone remind me why we’re not also doing this for createrawtransaction?

petertodd commented at 9:01 am on December 2, 2015: contributor

@gmaxwell Basically to avoid breaking things. Rather start with just wallet txs until this stuff is more well-known.

petertodd commented at 10:36 am on December 2, 2015: contributor

Now that #6915 has been merged, which fixes the invalid txs in mempool issue, I feel comfortable merging this as well. Specifically, #6915 removes txs from the mempool after the reorg is finished, which means txs with nLockTime-by-height right at the edge of acceptance will no longer be removed during the reorg so long as the blockheight after the reorg doesn’t go down. (an incredibly rare event!)

gmaxwell commented at 12:10 pm on December 2, 2015: contributor

ACK with nit; (sorry found the typo when I was about to merge.)

laanwj commented at 12:39 pm on December 2, 2015: member

Can someone remind me why we’re not also doing this for createrawtransaction?

Right now, createrawtransaction is a pure utility function without any dependencies on the state of the node nor wallet. I think it’s nice to keep it that way. Since #5936 there is a locktime argument that can be passed manually, fully in the spirit with raw transaction creation.

laanwj commented at 12:56 pm on December 2, 2015: member

Going to merge this, typo be damned, can be fixed later.

laanwj merged this on Dec 2, 2015

laanwj closed this on Dec 2, 2015

laanwj referenced this in commit 83f06ca937 on Dec 2, 2015

btcdrak commented at 12:57 pm on December 2, 2015: contributor

ACK

DrahtBot locked this on Sep 8, 2021

Take the training wheels off anti-fee-sniping #6216