-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a technical point of view, I have no objections to the rust changes in this pr.
The only thing that can be improved is separating eos params from ethash params, but I see that ethash params already contains options for other chains, so this is not a blocker.
What I don't understand is why is this pr adding bootnoodes to unrelated chains?
ethcore/res/ethereum/classic.json
Outdated
"enode://83b33409349ffa25e150555f7b4f8deebc68f3d34d782129dc3c8ba07b880c209310a4191e1725f2f6bef59bce9452d821111eaa786deab08a7e6551fca41f4f@159.89.223.6:30303" | ||
"enode://83b33409349ffa25e150555f7b4f8deebc68f3d34d782129dc3c8ba07b880c209310a4191e1725f2f6bef59bce9452d821111eaa786deab08a7e6551fca41f4f@159.89.223.6:30303", | ||
"enode://5cd218959f8263bc3721d7789070806b0adff1a0ed3f95ec886fb469f9362c7507e3b32b256550b9a7964a23a938e8d42d45a0c34b332bfebc54b29081e83b93@35.187.57.94:30303", | ||
"enode://6dd3ac8147fa82e46837ec8c3223d69ac24bcdbab04b036a3705c14f3a02e968f7f1adfcdb002aacec2db46e625c04bf8b5a1f85bb2d40a479b3cc9d45a444af@104.237.131.102:30303" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this pr changing bootnode addresses of unrelated chains?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@debris Cause this is what we call open source, I can revert the commit but as you know there is no fixed maintainer of these chains, all bootnodes are being updated from parity team or volunteers like me.
I've updated the list based on these list from their chain client, you can refer them here
ELLA: https://raw.githubusercontent.com/ellaism/parity-config/master/ellaism.json
EXP: https://raw.githubusercontent.com/expanse-org/go-expanse/master/params/bootnodes.go
MUSIC: https://raw.githubusercontent.com/Musicoin/go-musicoin/master/params/bootnodes.go
And I don't see the reason for not supporting our params for now, we already have EXP, MUSIC, and ETC params for reward logic modification and there is nothing wrong for adding EOSC params between them.
If you are thinking to suggest separating them please provide a code to separate them also 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cause this is what we call open source, I can revert the commit but as you know there is no fixed maintainer of these chains, all bootnodes are being updated from parity team or volunteers like me.
I know what open-souce is. Those changes are just not related with the context of this pull request and should be checked/reviewed separately
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@debris So should I revert the commit and seperate the PR??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will allow us to easily backport bootnodes update to stable and beta
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@debris Also squashed commit for better future review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like you also need to enable ECIP1017 (https://github.com/eosclassic/go-eosclassic/commit/66bce2f5b7cf3a405ba504af6f4fc45c0656f947)? Otherwise parity and your client will split on block 2500000.
@sorpaas We use the modification of ECIP-1017 and it will take some time for parity to implement the feature of our ver ECIP-1017 ( Let's call it EOSC-1017 since it halves the Fund and POS reward also ) I will be appreciated if anyone from Parity team can implement it however we don't see in a hurry for implementing EOSC-1017 for parity since it will happen on the next year |
neweosc_fund_address: p.neweosc_fund_address.map_or_else(Address::new, Into::into), | ||
neweosc_fund_reward: p.neweosc_fund_reward.map_or_else(Default::default, Into::into), | ||
neweosc_pos_address: p.neweosc_pos_address.map_or_else(Address::new, Into::into), | ||
neweosc_pos_reward: p.neweosc_pos_reward.map_or_else(Default::default, Into::into), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is neweosc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@5chdn neweosc
is our code name of Anti-inflation Hardfork on block num 150,000 please refer here https://github.com/eosclassic/go-eosclassic/issues/5
@@ -43,6 +43,7 @@ pub enum SpecType { | |||
Ellaism, | |||
Easthub, | |||
Social, | |||
EOSC, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"EOS Classic"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@5chdn Ethereum Classic is using "Ethereum Classic" on their chain spec json, and "Classic" on parity/params.rs file.
It is same with "Ethereum Social" also so I would like to ask using "EOS Classic" on our json file and "EOSC" here 😄
@eosclassicteam My only comment is that you would need to fix ECIP1017 first. Otherwise this would be a huge maintenance burden for all of us. |
@sorpaas Well as I've already said right now we don't have enough man power to fix ECIP1017 for our implementation so if anyone is interested on our PR I will reward him as a small bounty in EOSC 😂 |
+ Added EOSC Bootnodes from go-eosclassic + Added NewEOSC Hardfork logic + Tested with rust-eosclassic
Please, reopen this when you have time to fix the consensus gap between the two clients, |
EOS Classic is a combination of ETH & EOS protocol network
We would like to request adding EOS Classic Mainnet support on parity-ethereum client
Coin info
Screenshot
Any questions about EOS Classic are welcome! ❤️