-
Notifications
You must be signed in to change notification settings - Fork 147
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add keccak mining #369
Add keccak mining #369
Conversation
@atoulme Great to have you here, and thank you for this proposal. Given the size and complexity, this will take a bit to review, but in general it looks like quite strong. We'll start taking a look at this this week, and will keep you in the loop throughout review. Please feel welcome to solicit feedback on anything you'd like specific attention on; we're here and happy to help. |
Thanks @meowsbits. I was trying to run acceptance tests and had issues syncing with Astor with my modifications. Geth raises this error:
I was wondering if the change in difficulty calculation might be tied to a hard fork schedule that's missing from the astor genesis file. Would you please take a quick look? |
@atoulme Apologies for being tardy in responding. Somehow I missed the notification entirely. I'm starting to write a test for this -- here -- to investigate, but am coming up short on some stuff. To take a stab in the dark without any detailed information about Astor, I note that the chain configuration doesn't specify any features pertaining to difficulty. This means that core-geth will use the Frontier difficulty algorithm and bomb schedule. Probably, this doesn't make a lot of sense for a new testnet, since it would likely want to use the latest difficulty algorithm upgrades from EIP2 and EIP100, and to disable the difficulty bomb (ECIP1041). Without knowing implementation defaults or configuration options for Besu, nor the configuration Astor specifies, I can't confirm if this is relevant or not. A small side note is the genesis block uses a timestamp of |
I found some FixedDifficulty calculators at Besu, which appear contemporary with Keccak256 mining. Maybe this is related? |
OK, the astor testnet is probably best described under https://astor.host. Probably not to your satisfaction though. The testnet is using the fork block ecip1049Block set to 0, as the latest hard fork. This fork block was added after the last hard fork for etc supported by besu, which would be thanos. The definition is here: https://github.com/hyperledger/besu/blob/e230a445bb457bbac27bf4c384da449ccbd3f043/ethereum/core/src/main/java/org/hyperledger/besu/ethereum/mainnet/ClassicProtocolSpecs.java#L265 Didn't think about the block timestamp. We did manage to produce blocks now, you can see our progress at astor.tmio.io. |
Alright, so chain configuration is definitely the root problem here then (or at least an important contributor). AFAIU, Besu uses a "stacked subset features" paradigm, eg. a "Forks"-based configuration schema. This suggests to me that since Astor assumes ECIP1049 as an inheritor of the Thanos fork, that it will implicitly assume all of the preceding canonical ETC mainnet features to have been activated. CoreGeth uses a "Features, not Forks" configuration schema paradigm, so all desired features need to be explicitly articulated. You can reference the ETC config for CoreGeth here: https://github.com/etclabscore/core-geth/blob/master/params/config_classic.go#L30. |
This is great. So it looks like I could potentially use the same approach that was taken for Mordor, I'll look at the ins and outs of it. |
It's getting better. The difficulty calculations are still failing:
You can now join Astor with a |
OK, I fixed the difficulty algorithm issues. I managed to run geth and import blocks from astor 🎉 |
Well done! I am also now the proud owner of 95 ATH (Astor-ETC ;) at |
Congratulations, and welcome to the close circle of ATH 🐳 Confirmed mining with an external GPU miner works for me and for miners helping run the network. |
The build doesn't pass, but the failure seems unrelated to the work here. Please advise. AFAICT, everything is ok. |
update go.mod (missing deps [however they appear to already be in go.sum])
edit: this is likely a local issue on my end. but here for note taking purposes. |
"github.com/ethereum/go-ethereum/params/vars" | ||
) | ||
|
||
func KeccakBlockReward(c ChainConfigurator, n *big.Int) *big.Int { |
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.
Hey @meowsbits, @iquidus and @ziogaschr! Is there anything specific which is still needed for this PR to be merged? Thanks! @atoulme resolved the merge conflicts. |
Requesting another review (or two) given the scope and size of changes; otherwise LGTM. There's a couple of nitpicks I noticed for small stuff like out-of-place comments, but I'm not going to bother messing with them at this point; we can do a cleanup review+PR as desired later. The tests are rather sparse (eg. |
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.
I was able to sync and mine with Astor testnet 2-3 weeks ago.
I have tried doing the same today and it is syncing, but on the last blocks it goes too slow and it loses the peers.
> eth.syncing
{
currentBlock: 197922,
highestBlock: 200033,
knownStates: 28,
pulledStates: 28,
startingBlock: 197796
}
I will leave it on for sync to finish.
@@ -224,6 +225,27 @@ func CreateConsensusEngine(stack *node.Node, chainConfig ctypes.ChainConfigurato | |||
Epoch: chainConfig.GetCliqueEpoch(), | |||
}, db) | |||
} | |||
if chainConfig.GetConsensusEngineType().IsKeccak() { | |||
// Otherwise assume proof-of-work | |||
switch config.PowMode { |
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.
I am thinking that it might be useful to move the logic of config.PowMode
and ethash.ModeFake
out of ethash package or maybe duplicate this logic.
Reusing the logic directly from ethash seems to be error prone in future.
It will be a nice to have if this refactor can be done in this PR, but I am also fine leaving this for another PR so as to not block merging of this one.
Based on your feedback @atoulme I will either accept the PR or wait for welcoming the refactor.
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.
I’d rather have another PR.
@atoulme any idea if Astor network is still available. As I can't finish syncing. |
The bootnode is still up and going: http://astor.tmio.io/ |
I restarted the bootnode just in case it was stuck. |
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.
LGTM
I just tried updating my I build from source at d8fa81e
|
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.
I just attempted a fresh sync; still fails at block 200,000.
Jul 06 07:44:50 ubp52 sh[25705]: INFO [07-06|07:44:50.417] Imported new chain segment blocks=192 txs=0 mgas=0.000 elapsed=88.154ms mgasps=0.000 number=199,680 hash=abff04..74590d age=3w7h33m dirty=98.59KiB
Jul 06 07:44:50 ubp52 sh[25705]: INFO [07-06|07:44:50.993] Imported new chain segment blocks=192 txs=0 mgas=0.000 elapsed=88.705ms mgasps=0.000 number=199,872 hash=46b703..8ef666 age=3w6h56m dirty=98.59KiB
Jul 06 07:44:52 ubp52 sh[25705]: ERROR[07-06|07:44:52.598] Snapshot extension registration failed peer=cb5b9771 err="peer connected on snap without compatible eth support"
Jul 06 07:44:54 ubp52 sh[25705]: WARN [07-06|07:44:54.985] Synchronisation failed, dropping peer peer=0cf4136259a482a1f75318eee4d063889339529b1ba699895f90dd8ffd34eef8 err="retrieved hash chain is invalid: invalid difficulty: have 13603621, want 13603622"
Jul 06 07:44:54 ubp52 sh[25705]: ERROR[07-06|07:44:54.986] Ethereum peer removal failed peer=0cf41362 err="peer not registered"
Jul 06 07:44:54 ubp52 sh[25705]: INFO [07-06|07:44:54.986] Commit new mining work number=200,000 sealhash=2da3c6..edfc3c uncles=0 txs=0 gas=0 fees=0 elapsed="95.201µs"
Jul 06 07:45:01 ubp52 sh[25705]: INFO [07-06|07:45:01.100] Deep froze chain segment blocks=15487 elapsed=2.108s number=109,999 hash=7dd0e9..a1514d
Jul 06 07:45:02 ubp52 sh[25705]: INFO [07-06|07:45:02.446] Looking for peers peercount=1 tried=91 static=0
Jul 06 07:45:12 ubp52 sh[25705]: INFO [07-06|07:45:12.809] Looking for peers peercount=0 tried=99 static=0
Jul 06 07:45:23 ubp52 sh[25705]: INFO [07-06|07:45:23.564] Looking for peers peercount=1 tried=70 static=0
> eth.blockNumber
199999
I get exactly the same as @meowsbits. Here you can find the full log file |
Found the problem. Guard missing on the ECIP1041 Transition. With this added, node is now syncing with the boot node and mining. @atoulme @ziogaschr @meowsbits hopefully you can confirm this works for you as well |
@jpcomps I can confirm that the fix works nice for both syncing and mining. Nice find man. Can you please commit the fix? |
OK, I removed difficulty bomb calculations for keccak since they should be separate ECIPs. I can sync locally to the head of the chain now. |
Just got around to re-syncing with the new changes. Looks good! Hopefully good to merge now. Change makes more sense than my previous "hack" Thanks @atoulme |
|
||
ECIP1099FBlock: big.NewInt(0), // Etchash | ||
|
||
DisposalBlock: big.NewInt(0), |
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.
Removing implementation by the Keccak engine for difficulty bomb seems a little troublesome to me in a couple ways.
In general, I think the cited patch could probably be replaced with a small tweak to configuration only. EDIT On second thought, having doubts. It is called |
I'd look at it as this change matches exactly how Besu behaves and how the ECIP-1049 was drafted. We don't have a difficulty bomb designed for keccak mining. |
I have a branch going for development on this at https://github.com/etclabscore/core-geth/tree/keccak_mining-rebase-047f1129f3. PTAL before developing yourself, might save some time. TLDR:
In general these are sort of sticky configuration/implementation ideas, since it seems like Astor presumes a lot of stuff about network configuration as copy-paste from antecedent Ethash-based network configurations and upgrades. This is sort of ugly, since much of these specifications were/are explicitly for Ethash and the networks which used it. |
How can we progress this support, @meowsbits? What can we do to help? |
second @bobsummerwill . let us know how we can help @meowsbits |
This PR adds keccak mining to Core-Geth.
Keccak mining is tested with test vectors.