Treat non-final transactions as non-standard #2223

gavinandresen commented at 7:51 PM on January 26, 2013: contributor

At least one service that accepted zero-confirmation transactions was vulnerable because an attacker could send a transaction with a lock time far in the future, and then have plenty of time in which to get a double-spend mined (perhaps from a miner who wasn't on the network when the first transaction was broadcast).

That is a variation on the "Finney attack". We still don't recommend anybody accept 0-confirmation transactions as final payment for anything. This change keeps non-final transactions from appearing in the wallet, and, assuming most of the network accepts this change, will eventually prevent them from being relayed until they are final.

Treat non-final transactions as non-standard

At least one service that accepted zero-confirmation transactions
was vulnerable because an attacker could send a transaction
with a lock time far in the future, and then have plenty of time in
which to get a double-spend mined (perhaps from a miner who wasn't
on the network when the first transaction was broadcast).

That is a variation on the "Finney attack". We still don't
recommend anybody accept 0-confirmation transactions as final
payment for anything. This change keeps non-final transactions
from appearing in the wallet, and, assuming most of the network
accepts this change, will prevent them from being relayed until
they are final.

6f8730752c

gavinandresen commented at 7:54 PM on January 26, 2013: contributor

Tested with transaction id a0d85e45c4b54e98bb7c67044a88e20b434db58f75fad9dd4dac6d5ba215296f

Created using createrawtransaction, then modified the input sequence number and lock time by tweaking the hex

Attempted to send before the locktime using sendrawtransaction. RESULT: TX rejected

Waited until after the locktime, sendrawtransaction again: Success

Note: since this is an IsStandard() check, you can't test on -testnet...

jgarzik commented at 8:11 PM on January 26, 2013: contributor

ACK

BitcoinPullTester commented at 8:15 PM on January 26, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/6f8730752cf92ff8269812c01a6d9d35fff82e75 for binaries and test log.

luke-jr commented at 9:06 PM on January 26, 2013: member

Doesn't this defeat the whole point of non-final transactions? Feels more like we're disabling a possibly-useful feature so that people can goof up and not be affected unless someone is trying (nothing stops them from relaying a non-final direct to the vulnerable service...).

In any case, as long as bitcoind and Bitcoin-Qt clearly expose the non-final status, I don't think this is a vulnerability in this code...

gmaxwell commented at 9:24 PM on January 26, 2013: contributor

@luke-jr Without TX replacement the purpose non-final transactions have is very limit— the only difference this will make is that you have to transmit them out of band until they lock... which you already had to do if you wanted the ability to replace.

What this fixes is that right now the wallet will display transactions that have no conceivable chance of confirming. Unless you have a more detailed understanding of the system and know to look for the impossibly far locktime you'll think that it at least has reasonable odds. This is misleading and makes Bitcoin even more unsafe for people in practice. Limiting them also closes down some potential areas that DOS vectors could be found.

The obvious alternatives to outright disabling them— e.g. relaying transactions only x hours or less before they lock— turn out to be pretty complicated to implement in the wallet (e.g. what do you do about txn whos parents were above threshold but later fall below it) and don't make an obvious improvement.

schildbach commented at 9:38 PM on January 26, 2013: contributor

@gavinandresen Do you also account for the fact that an attacker can send a timeLocked tx and then a plain normal tx that builds on the first one? The second one looks normal but in fact will also be timeLocked.

gmaxwell commented at 9:42 PM on January 26, 2013: contributor

@goonie The non-final txn should not enter the memory pool, so its children should be treated as orphaned.

luke-jr commented at 10:46 PM on January 26, 2013: member

@gmaxwell I guess that makes sense... I suppose if someone comes up with other valid use cases, this can be reconsidered in the future anyway.

petertodd commented at 12:22 AM on January 27, 2013: contributor

ACK

Tested what Gavin did as well as tested various incoming transactions including chains. Also tested what happens with non-final tx's already in your wallet; an error ends up in your logs and they don't make it into the mempool. Re-transmission does occur, and provided that a conflict is not out there, they will eventually be re-transmitted when they are final.

in src/main.cpp:None in 6f8730752c

 367 | @@ -368,6 +368,9 @@ bool CTransaction::IsStandard() const
 368 |      if (nVersion > CTransaction::CURRENT_VERSION)
 369 |          return false;
 370 |  
 371 | +    if (!IsFinal())

luke-jr commented at 1:06 AM on January 27, 2013:

This doesn't pass block height/time. Shouldn't transactions be standard once their lock time has passed?

petertodd commented at 1:18 AM on January 27, 2013:

IsFinal() without arguments uses current adjusted time and best known block.

gavinandresen referenced this in commit f73abdc82d on Jan 28, 2013

gavinandresen merged this on Jan 28, 2013

gavinandresen closed this on Jan 28, 2013

laudney referenced this in commit 5198f464e1 on Mar 19, 2014

DrahtBot locked this on Sep 8, 2021