Musicoin / go-musicoin

:link: Go-version of Musicoin blockchain wallet and consensus
GNU Lesser General Public License v3.0
161 stars 57 forks source link

Replay attack protection - EIP155, BIP44 #44

Closed immartian closed 6 years ago

immartian commented 7 years ago

Issue by Varunram Saturday Aug 26, 2017 at 14:00 GMT Originally opened as https://github.com/Musicoin/PM/issues/117


We have to add Replay Attack Prevention (EIP 155) which has been referenced over at the go-musicoin repository. I've added the steps t doing this below.

On the way to adding EIP 155 support there exists<a href="https://github.com/bitcoin/bips/blob/master/bip-0044.mediawiki'> this issue with BIP 44 that we must comply with. Once we comply with BIP 44, we can get back on MEW and once we somehow implement EIP 155, we can shut the haters over at the MEW repo.

Let's set a timeline for this before/after UBI,

immartian commented 7 years ago

Comment by Varunram Saturday Aug 26, 2017 at 14:20 GMT


We should set CHAIN_ID to a unique value to prevent these kinds of attacks as detailed over at EIP 155. @immartian for your reference :)

immartian commented 7 years ago

Comment by immartian Saturday Aug 26, 2017 at 14:27 GMT


We know how to do it but it takes a lot of test with risking the timeline of UBI. It's not a security issue, just confined to MEW and Trezor. It'll be in timeline after UBI's deployment, together with other EIPs Ethereum brought later. Let's focus on many delayed works first.

immartian commented 7 years ago

Comment by Varunram Saturday Aug 26, 2017 at 14:28 GMT


Okay, so here are the things to do with BIP44.

As you said, not really on the fore front with UBI incoming, but we need to keep this in mind since many people are commenting on the lack of MEW support in general. We can see this after UBI :)

trustfarm-dev commented 7 years ago

@immartian @Varunram Yes it's good approach,

  1. other altcoin has changed BIP44 , at Feb 2017. So, I've looking at that time of alt coin's codes.
  2. Fork Id and Chain Id different storage , 1byte vs (4byte or 8byte). <as I searched previous 4bytes , recent days, it has changing. now musicoin chainid is 4bytes full. Let's good job!
immartian commented 7 years ago

@trustfarm-dev did you try the most recent Parity version of $MUSIC ? https://github.com/Musicoin/go-musicoin/issues/47 I can build one if you want to try.

trustfarm-dev commented 7 years ago

@immartian not yet and little time , even though holiday 👍 -) But, I'm briefly surveying the code of parity patch , just json patch. I'm more concern on geth based node - gmc. And, If you built parity-ubi-v2 and share this github, many musicoin community member will trying the parity node's verification I think.

And, I think more emergent things are review the contract code of DEV fund/Foundation fund security, before UBI 2.0 fork.

immartian commented 7 years ago

Yes, we are doing, on security part.

The reason to suggest Parity is we have adopted upstream codebase of Ethereum there. So the replay attack should be solved. GMC will follow up, soon.

trustfarm-dev commented 7 years ago

Yes I think so, latest parity code will have Easily adapt EIP-155 and BIP for chainid. Above is good news. :-)

5chdn commented 7 years ago

enabling EIP-155 in parity is just a change in the chain spec. :+1:

5chdn commented 7 years ago

https://github.com/Musicoin/go-musicoin/blob/master/params/config.go#L41

immartian commented 7 years ago

@trustfarm-dev love to test?

trustfarm-dev commented 7 years ago

@immartian I've already built parity 1.8 music as you know. recent day, I've no time to check. I tried to binding pmc-v18 , nginx and MEW. enabled with EIP155 and disabled with EIP155. But, Nginx and pmc-v18 , with MEW , there's communication problem and cross-origin problem may be. block is increasing well. I'm checking the pmc-v18 and MEW interface. But, in orbiter looks well binded pmc-v18. is it correct? Time is problem , but it may be work.. I guess.

Varunram commented 6 years ago

Ref. #74 for PR