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

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

Merged

Conversation

@petertodd
Copy link
Contributor

petertodd commented Jun 1, 2015

Now that the off-by-one error w/nLockTime txs issue has been fixed by 87550ee (75a4d51 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 laanwj added the Wallet label Jun 3, 2015
Now that the off-by-one error w/nLockTime txs issue has been fixed by
87550ee (75a4d51 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.
@petertodd petertodd force-pushed the petertodd:take-training-wheels-off-fee-sniping branch from 08a4245 to db6047d Jun 22, 2015
@petertodd
Copy link
Contributor Author

petertodd commented Jun 22, 2015

Rebased now that Travis is fixed.

@jtimon
Copy link
Member

jtimon commented Jul 9, 2015

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

@btcdrak
Copy link
Contributor

btcdrak commented Jul 9, 2015

It is time. ACK

@jgarzik
Copy link
Contributor

jgarzik commented Jul 9, 2015

concept ACK

@sipa
Copy link
Member

sipa commented Jul 9, 2015

Untested ACK

@petertodd
Copy link
Contributor Author

petertodd commented Jul 11, 2015

@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
Copy link
Member

jtimon commented Jul 11, 2015

@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
Copy link
Contributor Author

petertodd commented Jul 17, 2015

@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
Copy link
Member

jtimon commented Jul 17, 2015

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
Copy link
Contributor Author

petertodd commented Jul 17, 2015

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

@dcousens
Copy link
Contributor

dcousens commented Aug 9, 2015

utACK

@MarcoFalke
Copy link
Member

MarcoFalke commented Aug 9, 2015

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
Copy link
Contributor Author

petertodd commented Aug 18, 2015

@MarcoFalke Thanks!

@laanwj Be good to get this merged and done with...

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Aug 24, 2015

utACK

@sdaftuar
Copy link
Member

sdaftuar commented Sep 8, 2015

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
Copy link
Contributor

dcousens commented Sep 8, 2015

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
Copy link
Contributor

TheBlueMatt commented Sep 9, 2015

Yea, I agree with @sdaftuar...#6595 kinda breaks this...it should be easy to fix though (#6595 (comment))

@petertodd
Copy link
Contributor Author

petertodd commented Sep 9, 2015

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

@TheBlueMatt
Copy link
Contributor

TheBlueMatt commented Sep 9, 2015

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
Copy link
Contributor

gmaxwell commented Nov 22, 2015

@petertodd Where are we on this?

@btcdrak
Copy link
Contributor

btcdrak commented Nov 22, 2015

looks like it's pending #6915

@jtimon
Copy link
Member

jtimon commented Dec 1, 2015

ping

@gmaxwell
Copy link
Contributor

gmaxwell commented Dec 1, 2015

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

@petertodd
Copy link
Contributor Author

petertodd commented Dec 2, 2015

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

@petertodd
Copy link
Contributor Author

petertodd commented Dec 2, 2015

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

This comment has been minimized.

Copy link

gmaxwell commented on src/wallet/wallet.cpp in db6047d Dec 2, 2015

you'll hate me, but delibrately -> deliberately

@gmaxwell
Copy link
Contributor

gmaxwell commented Dec 2, 2015

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

@laanwj
Copy link
Member

laanwj commented Dec 2, 2015

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
Copy link
Member

laanwj commented Dec 2, 2015

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

@laanwj laanwj merged commit db6047d into bitcoin:master Dec 2, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
laanwj added a commit that referenced this pull request Dec 2, 2015
db6047d Take the training wheels off anti-fee-sniping (Peter Todd)
@btcdrak
Copy link
Contributor

btcdrak commented Dec 2, 2015

ACK

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.