search

clifordsymack / Electron-Cash

Electrum; Bitcoin thin client
MIT License
6 stars 3 forks source link

Last player advantage #70

Closed kobejohn closed 5 years ago

kobejohn commented 5 years ago

I had a situation that Calin investigated.

Running from commit: 573d02f5

Symptoms:

Cause:

Impact:

cculianu commented 5 years ago

I can't fix impact (1) as that would require rethinking the entire protocol. I don't think it's a huge deal since you "agreed" to the terms of the shuffle anyway. Best a bad actor can do is annoy you with that one. You are free to shuffle again and if you get your shuffle in -- that old tx is invalid now anyway.

But impact (2) is annoying from a UX perspective. I can fix it.

I'm attaching the redacted log to illustrate what happened for future me:

|28684.096| [BackgroundShufflingThread (1042176) <electron-shuffle>] Scale: 10000000 Message: 'Player 4 reaches phase 5'
|28685.048| [BackgroundShufflingThread (1042176) <electron-shuffle>] Scale: 10000000 Message: 'Player 4 send transction signatures'
|28685.372| [profiler] Round.check_for_signatures 0.0026
|28685.788| [profiler] Round.check_for_signatures 0.0027
|28685.803| [profiler] Round.check_for_signatures 0.0028
|28686.119| [profiler] Round.check_for_signatures 0.0032
|28727.679| [BackgroundShufflingThread (1046272) <electron-shuffle>] Server shuffle.servo.cash:8080 told us that it has shufflePort=1337 poolSize=5 connections=18
|28746.120| [Comm (1042176) <shuffle.servo.cash:1337> <Scale: 10000000>] Socket timeout (60.0): The read operation timed out
|28746.120| [Round (1042176)] The read operation timed out
|28746.121| [BackgroundShufflingThread (1042176) <electron-shuffle>] Scale: 10000000 Message: 'Error: Socket closed or timed out'
|28746.121| [BackgroundShufflingThread (1042176) <electron-shuffle>] Signalling stop for scale: 10000000
|28746.121| [BackgroundShufflingThread (1042176) <electron-shuffle>] Scale: 10000000 Message: 'Exit: Scale '10000000' Coin 'REDACTED-COIN-A''
|28746.121| [BackgroundShufflingThread (1046272) <electron-shuffle>] Stop protocol thread for scale: 10000000
|28746.121| [BackgroundShufflingThread (1042176) <electron-shuffle>] Signalling stop for scale: 10000000
|28746.122| [Comm (1042176) <shuffle.servo.cash:1337> <Scale: 10000000>] Closing comm (subsequent socket errors are to be expected)
|28746.126| [profiler] get_shuffled_and_unshuffled_coins 0.0038
|28746.127| [BackgroundShufflingThread (1046272) <electron-shuffle>] Stop protocol thread for scale: 10000000
|28746.129| [BackgroundShufflingThread (1046272) <electron-shuffle>] Thread already exited; cleaned up.
|28746.370| [profiler] get_shuffled_and_unshuffled_coins 0.0062
|28747.697| [BackgroundShufflingThread (1046272) <electron-shuffle>] Scale 10000000 Coin REDACTED-COIN-A OutAddr ADDR-1 Change ADDR-2 make_protocol_thread
|28747.700| [profiler] get_shuffled_and_unshuffled_coins 0.0020
|28748.681| [shuffle.servo.cash] SSL certificate signed by CA
|28748.682| [shuffle.servo.cash] connected
|28749.913| [BackgroundShufflingThread (1042176) <electron-shuffle>] Scale: 10000000 Message: 'Player 2 get session number.'
|28749.915| [BackgroundShufflingThread (1042176) <electron-shuffle>] Scale: 10000000 Message: 'Player 2 joined the pool!'
|28752.576| [BackgroundShufflingThread (1042176) <electron-shuffle>] Scale: 10000000 Message: 'Player 3 joined the pool!'
|28753.205| [BackgroundShufflingThread (1042176) <electron-shuffle>] Scale: 10000000 Message: 'Player 4 joined the pool!'
|28783.965| [Synchronizer] receiving history ADDR-3 1
|28783.966| [Synchronizer] receiving history ADDR-4 1
|28783.966| [Synchronizer] receiving history ADDR-5 2
|28783.966| [bitcoincash.quangld.com] serving improperly sorted address histories
|28784.045| [profiler] HistoryList.on_update 0.0770
|28784.073| [profiler] get_shuffled_and_unshuffled_coins 0.0017
|28784.074| [Synchronizer] received tx SURPRISING-TXID-HERE-REDACTED height: 0 bytes: 2242
|
cculianu commented 5 years ago

^^ The timeout happens at |28746.120|, and your client thought the shuffle died.

But the lagger eventually broadcasted the tx.

At time |28784.074| you got notified of the tx through normal Electron Cash SPV means. And it just looked like a 'spurious' tx to EC so it ended up not having a shuffle tx label.

A fix would be to 'remember' all the tentative txid's even if they fail due to timeouts for a time -- and if we see that tx show up in the wallet in the near future, we can apply the label to them.

imaginaryusername commented 5 years ago

This is actually an advantage of identifying shuffled coins via the shape of their tx, no? Even if the attacker successfully broadcasted his "attack" it'll still be marked as "shuffled" in all participant wallets.

cculianu commented 5 years ago

Yes. Also, restore from seed works as you'd expect, etc. :)

kobejohn commented 5 years ago

Thanks a lot for describing everything in detail. And you actually fixed it already!

cculianu commented 5 years ago

Actually when I was reviewing and fixing this code and adapting it to work better under EC -- I realized this could happen and was like "hmm. I wonder what the probability is? Will it happen 1% of the time? 3%? 0.00001%?"

The fact that it happened to both Sploit and you made me realize it's fairly likely it would happen at least once a day to the average user that leaves their wallet open -- thus reducing the perception of quality of the software.

So I had to fix it. :)