Refuse to retransmit transactions without vins

laanwj commented at 2:45 pm on December 4, 2013: member

There is a bug that inserted empty transactions into the vtxPrev in the wallet (see #3356 for the fix), which will cause the node to be banned when retransmitted.

As wallets with these invalid vtxprevs will be around for a while to come, add a check for !tx.vin.empty() before RelayTransaction.

Part two of the solution for #3190. This will need to go into 0.8.6 as well.

Versions of bitcoin before 0.8.6 have a bug that inserted
empty transactions into the vtxPrev in the wallet, which will cause the node to be
banned when retransmitted, hence add a check for !tx.vin.empty()
before RelayTransaction.

4ef92a9067

BitcoinPullTester commented at 3:02 pm on December 4, 2013: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/4ef92a9067a51c962d662b857704f63d936effa0 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.

gmaxwell commented at 3:06 pm on December 4, 2013: contributor

Ack, also for 0.8.x (after it’s merged and run for a few days in git). This appears intuitively safe. I don’t have a reproduction handy, so I don’t have a good way to test it.

laanwj commented at 3:54 pm on December 4, 2013: member

I’ve tested with @wtogami’s litecoin testnnet wallet, and with some additional debugging code added:

02013-12-04 15:50:31 RelayWalletTransaction(1ce01b9bf5776646fbff0a3d0fd0905379490e5b7b296fead5ca5e775b14f7e6)
12013-12-04 15:50:31   Not relaying wtx prev d21633ba23f70118185227be58a63527675641ad37967e2aa461559f577aec43
22013-12-04 15:50:31   Not relaying wtx prev d21633ba23f70118185227be58a63527675641ad37967e2aa461559f577aec43
32013-12-04 15:50:31   Relaying wtx prev be0badd502f113001d0e05ab4f2d1790f1461a749cd88511716744c82537cf41
42013-12-04 15:50:31   Relaying wtx prev c873a065f74f37706b6196e74f5d085d55f06d29c98000fb99327f3fe3c11d8b

The offending transaction is no longer being relayed. A point could be made that the other two transactions should also not be retransmitted, but you won’t get banned for being noisy. This could be improved at some later time (it’s not needed to solve the immediate issue in 0.8.x).

gmaxwell commented at 3:57 pm on December 4, 2013: contributor

The excessive relays aren’t just noisy, they’re a privacy problem: You’re not supposed to be able to tell if someone relaying you a txn is the original source, or just someone they’re relaying through, but the noise is something other peers wouldn’t produce. But no need to fix it all in one commit.

sipa commented at 5:03 pm on December 4, 2013: member

ACK code change, though I wonder if you couldn’t just call CheckTransaction. That would make it much more obvious that we won’t send out anything that we couldn’t have relayed from someone else.

laanwj commented at 5:12 pm on December 4, 2013: member

@gmaxwell Yes, there are certainly privacy issues as well with transaction retransmission, especially in retransmitting received transactions. @sipa That would be a more general solution. Maybe also place the check in RelayTransaction itself instead of specific callers?

However, in this pull I tried to do the minimum to prevent the immediate issue, as this has the least impact and is easiest to test.

gmaxwell commented at 5:24 pm on December 4, 2013: contributor

I think we should pull this first, test it, and ship it in 0.8. Then in git call CheckTransaction.

laanwj referenced this in commit 80ca273b7f on Dec 4, 2013

laanwj merged this on Dec 4, 2013

laanwj closed this on Dec 4, 2013

laanwj commented at 5:30 pm on December 4, 2013: member

Ok, will merge this to master first. What about #3356? I think it important to have that one too, to prevent new empty transactions from being added to vtxPrev for new wallet transactions.

laanwj deleted the branch on Apr 9, 2014

Bushstar referenced this in commit d52020926c on Apr 8, 2020

DrahtBot locked this on Sep 8, 2021

Refuse to retransmit transactions without vins #3357