Accepting non-standard transactions has already been NAK'd. Putting this into a new pull req doesn't change that.

This has not already been NAK'd. Please reopen so I don't need to keep resubmitting, and consider reading the change to review (this doesn't accept any old non-standard transactions, only ones you make yourself).

The following is an automatic comment from the Bitcoin Pull Tester.
If you believe it is in error, please contact [email protected]

This pull passed automatic sanity-tests!
This means it merges cleanly onto current master, builds and unit-tests pass
You can find the test log and build output at http://jenkins.bluematt.me/pull-tester/ed6b70809b01da9bd01e9c4f81d1b9df8ac74662

This change is partially, if not totally, flawed.
This change gives a tool for an attacker to increase considerably the attack surface!

Any user can send a peer a transaction that returns IsMine() == true or IsFromMe() == true if he knows who the peer is (the peer's public key).

Creating transactions that return IsFromMe()=true the attacker can bypass penny flooding prevention and IsStandard() and go directly to the CPU-expensive ConnectInputs() code. This creates a perfect DoS attack. He can also skip IsStandard() and put non-standard txs into mapOrphanTransactions. Afterwards he can send a single tx that triggers the verification of thousands non-standard transactions at once, and I'm almost sure this can be used to mount a DoS.

Creating transactions with IsMine() = true is also dangerous. The attacker can create a non-standard tx and send 0.0000001 to the victim's address and bypass penny flooding prevention and isStandard(). The peer would blindly relay that transactions to remaining peers. This could be used (although I didn't explore how to do it) to force the remaining nodes to block the victim's node.

I find this change very disturbing, from the security point of view.

An attacker can't create transactions that return IsFromMe()==true unless they have already compromised your wallet.

No anti-DoS rule should ever be made based on relaying valid transactions, though perhaps concern should be given to potentially filling the victim's memory/wallet with spam.

Why the attacker can't create transactions that return IsFromMe()==true ?

The code IsFromMe(Tx) calls (GetDebit(tx) > 0) that calls GetDebit() from each txin, that finds the previous transaction and calls IsMine(prevout) which only checks that the recipient address is a specific key (only works for standard transaction templates). It does NOT check the signatures, just the template.

Some time ago there was a discussion on https://bitcointalk.org/index.php?topic=91915.0 about the purpose of IsStandard() check and its benefits

If the signatures don't match, the transaction is invalid and won't even get this far.

TRUE but you can create a VALID tx that sends 0 BTC to the victim and use the REMAINING previns/txouts to do nasty things.
I can't find any way to attack now, but I don't like the idea of allowing the attacker to skip a check only by adding a few bytes to the transaction that sends 0 BTC to an specific address.

I think @luke-jr is right here, though I haven't looked closely enough to be sure, so thanks for raising the point.

Anyway, this does raise another point: maybe IsFromMe() transactions should not be retransmitted unless they are really created by this instance (and not received from the network).

If that really works, it's a bug (in the existing code).

Nevertheless IsFromMe()=true is just enough of a problem.

If you want your client to allow a "fast track" for your transactions, then you can add some hidden message to the script of the transaction. The hidden message must be a MAC (Message Authentication Code) of your transaction using a secret key. Then you can check very fast that you're the sender of a transaction

No need for something that complex. Wallet transactions already have a fFromMe field which can easily be used to track whether it is not only "from me", but also "created by this instance".

@sipa That sounds like a good idea. How does it work with raw transactions (which are likely the common use case for this)?

Great!

(but maybe the luke-jr runs multiple instances in different computers, so fFromMe is not available)

If sendrawtransaction can be made to cause fFromMe to be set then it even covers the multiple instance case, so long as you do the work to trigger it from each instance. (Arguably better than the wallet duplication required to make input sniffing work)

I found two new attacks that breaks pseudonimity and allows a node to discover the peer's public address. With this information in hand (IP and Bitcoin pub address), a hacker may get the real identity and track the users payments.

a) Wait to see if the peer relays a non-standard transaction, it it does, then the public key must be the one of the txins or txouts.

b)

1. Send money to 60 different txouts (all owned by you, each one containing 0.000001 BTC) my( i )
2. List the 600K current Bitcoin addresses in use.
3. Separate the addresses in 60 groups. For each group, create a non-standard tx with 1 input an 10000 outputs. Each tx input is one of your txouts [ my(i) ]
4. One at the time, send the 60 transactions to the peer.
5. Only the one with the victim's public address in it will be relayed. (you can check that using the command "mempool"). This is because the victim's node won't check the tx.GetMinFee(1000, true, GMF_RELAY) protection for that tx since IsMine() =true.
6. Then proceed recursively (binary search) to discover which of the 10K address is the victim's pubkey.

This is a classical side-channel attack.

My opinion: Never branch the code checking IsFromMe() / IsMine() in CTxMemPool::accept()

Note that the use of fFromMe does not posses any risks .

@SergioDemianLerner "A" is not a real risk, since relaying the non-standard transaction is the only way to actually use it (so the only way to avoid this risk is to not use non-standard transactions at all). "B" seems like it could have some serious privacy implications, though - do you think it would work better if the client secretly held onto the transaction and only relayed it again later?

"A" could be achieved by sending the transaction directly to the miners (I don't know how to achieve that) . Then only the miners will know who send it.

Your idea to hide the transaction is interesting...The node should act exactly as if the transaction had been rejected. Also there should be no timing differences between rejecting and hiding.

The only problem I see is that if the attacker has two connections to the victim, then he can monitor if the transaction goes out from the other end, so the attack it still possible...

Anyway, why do you want to force to relay transactions that go to you?

Your decision to relay/not-relay as a single node in the network has very little probability to influence the fact that the transaction will or won't be included in a block by a miner. It is only relevant if you own a high percentage of the nodes of the network, so you can chain transmissions until a miners node.

Maybe what you want is to put the transaction into a special pool to monitor for 0-confirmations, but not for re-transmit.
Could you explain what is the real benefit for you of this change? What's the use case?

After thinking about it more, this gains from this seem not worth the risks at this time.