Skip to content
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

Closed
wants to merge 8 commits into from
Closed

Conversation

atoulme
Copy link
Contributor

@atoulme atoulme commented May 1, 2021

This PR adds keccak mining to Core-Geth.

Keccak mining is tested with test vectors.

@meowsbits
Copy link
Member

@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.

@atoulme
Copy link
Contributor Author

atoulme commented May 8, 2021

Thanks @meowsbits.

I was trying to run acceptance tests and had issues syncing with Astor with my modifications.
The astor genesis file is here: https://github.com/hyperledger/besu/blob/master/config/src/main/resources/astor.json

Geth raises this error:

WARN [05-01|00:32:33.211] Invalid header encountered               number=1 hash="713364…241bb3" parent="f9c534…15f5bc" err="invalid difficulty: have 1046361407488, want 1098974756864"
DEBUG[05-01|00:32:33.211] Block body download terminated           err="syncing canceled (requested)"
DEBUG[05-01|00:32:33.211] Transaction receipt download terminated  err="syncing canceled (requested)"

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?

@meowsbits
Copy link
Member

meowsbits commented May 14, 2021

@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.
I can't find any specification for the Astor testnet config (aside from the Besu implementation). Is there one?

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 0x0, so the first block will have the least possible difficulty since it will have been apparently created some 50 years after genesis.

@meowsbits
Copy link
Member

I found some FixedDifficulty calculators at Besu, which appear contemporary with Keccak256 mining. Maybe this is related?

@atoulme
Copy link
Contributor Author

atoulme commented May 15, 2021

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.

@meowsbits
Copy link
Member

meowsbits commented May 15, 2021

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.

@atoulme
Copy link
Contributor Author

atoulme commented May 16, 2021

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.

@atoulme
Copy link
Contributor Author

atoulme commented May 16, 2021

It's getting better. The difficulty calculations are still failing:

WARN [05-16|00:07:23.579] Synchronisation failed, dropping peer    peer=0cf4136259a482a1 err="retrieved hash chain is invalid: invalid difficulty: have 995780460544, want 997313216512"

You can now join Astor with a --astor command.

@atoulme
Copy link
Contributor Author

atoulme commented May 16, 2021

OK, I fixed the difficulty algorithm issues. I managed to run geth and import blocks from astor 🎉

@meowsbits
Copy link
Member

Well done! I am also now the proud owner of 95 ATH (Astor-ETC ;) at 0x67ff22551a2e2daf142bb36e0409f4ce9cb38059; mining seems to be working OK.

@atoulme
Copy link
Contributor Author

atoulme commented May 16, 2021

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.

@atoulme
Copy link
Contributor Author

atoulme commented May 18, 2021

The build doesn't pass, but the failure seems unrelated to the work here. Please advise. AFAICT, everything is ok.

@iquidus
Copy link
Contributor

iquidus commented May 18, 2021

update go.mod (missing deps [however they appear to already be in go.sum])

go get github.com/naoina/go-stringutil
go get github.com/steakknife/hamming

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 {
Copy link
Member

@ziogaschr ziogaschr May 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#335 from @iquidus might be handy to be used here.

@bobsummerwill
Copy link
Member

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.

@meowsbits
Copy link
Member

meowsbits commented Jun 28, 2021

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. keccakHasher), but these are either common and only-duplicated/reused functions which already have other unit tests (and proven use), or I'm wishing for e2e tests that don't already exist and can't be expected in this changeset.

Copy link
Member

@ziogaschr ziogaschr left a 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 {
Copy link
Member

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.

Copy link
Contributor Author

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.

@ziogaschr
Copy link
Member

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.

@atoulme any idea if Astor network is still available. As I can't finish syncing.

@atoulme
Copy link
Contributor Author

atoulme commented Jun 29, 2021

The bootnode is still up and going: http://astor.tmio.io/

@atoulme
Copy link
Contributor Author

atoulme commented Jun 29, 2021

I restarted the bootnode just in case it was stuck.

Copy link
Member

@ziogaschr ziogaschr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@meowsbits
Copy link
Member

meowsbits commented Jul 5, 2021

I just tried updating my astor sync again, got an error about invalid difficulty at the top. Off by 1.

I build from source at d8fa81e

err="retrieved hash chain is invalid: invalid difficulty: have 13603621, want 13603622"
Jul 05 17:20:59 ubp52 sh[15634]: INFO [07-05|17:20:59.739] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=113.834ms   mgasps=0.000 number=196,898 hash=5f6ea4..b59398 age=3w2h59m   dirty=97.20KiB
Jul 05 17:21:00 ubp52 sh[15634]: INFO [07-05|17:21:00.004] Looking for peers                        peercount=1 tried=55  static=0
Jul 05 17:21:00 ubp52 sh[15634]: INFO [07-05|17:21:00.074] Storing bloom trie                       section=5 head=b733669aa07a3f3b3dd473fbd45dc32278d7fe8cf20ddb0fc6ad888701e20b8b root=56e81f171bcc55a6ff8345e692c0f86e5b48e01b996cadc001622fb5e363b421 compression=0.000
Jul 05 17:21:00 ubp52 sh[15634]: INFO [07-05|17:21:00.273] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=86.556ms    mgasps=0.000 number=197,090 hash=dd6b67..16e1e0 age=3w2h12m   dirty=97.20KiB
Jul 05 17:21:00 ubp52 sh[15634]: INFO [07-05|17:21:00.346] Storing CHT                              section=5 head=b733669aa07a3f3b3dd473fbd45dc32278d7fe8cf20ddb0fc6ad888701e20b8b root=cbb9d06dd19ed2636bc0897232eb7e301bfe816c24c8d4e28f54360b4c153807
Jul 05 17:21:00 ubp52 sh[15634]: INFO [07-05|17:21:00.865] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=105.213ms   mgasps=0.000 number=197,282 hash=947aca..5ca3d6 age=3w1h30m   dirty=97.20KiB
Jul 05 17:21:01 ubp52 sh[15634]: INFO [07-05|17:21:01.461] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=87.891ms    mgasps=0.000 number=197,474 hash=8bc66d..b3f442 age=3w53m13s  dirty=97.20KiB
Jul 05 17:21:02 ubp52 sh[15634]: INFO [07-05|17:21:02.087] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=147.333ms   mgasps=0.000 number=197,666 hash=2d82a8..ee101c age=3w12m45s  dirty=97.20KiB
Jul 05 17:21:02 ubp52 sh[15634]: INFO [07-05|17:21:02.630] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=120.589ms   mgasps=0.000 number=197,858 hash=98b6c3..b7d469 age=2w6d23h   dirty=97.20KiB
Jul 05 17:21:03 ubp52 sh[15634]: INFO [07-05|17:21:03.178] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=63.441ms    mgasps=0.000 number=198,050 hash=9347b1..4b62f9 age=2w6d23h   dirty=97.20KiB
Jul 05 17:21:03 ubp52 sh[15634]: INFO [07-05|17:21:03.762] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=75.534ms    mgasps=0.000 number=198,242 hash=5e390a..da78b4 age=2w6d22h   dirty=97.20KiB
Jul 05 17:21:04 ubp52 sh[15634]: INFO [07-05|17:21:04.349] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=98.210ms    mgasps=0.000 number=198,434 hash=426e71..0828c0 age=2w6d21h   dirty=97.20KiB
Jul 05 17:21:04 ubp52 sh[15634]: INFO [07-05|17:21:04.911] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=61.200ms    mgasps=0.000 number=198,626 hash=a5dbc7..2bd200 age=2w6d20h   dirty=97.20KiB
Jul 05 17:21:05 ubp52 sh[15634]: INFO [07-05|17:21:05.466] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=52.356ms    mgasps=0.000 number=198,818 hash=424c91..a488f4 age=2w6d20h   dirty=97.20KiB
Jul 05 17:21:06 ubp52 sh[15634]: INFO [07-05|17:21:06.050] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=62.510ms    mgasps=0.000 number=199,010 hash=e41778..87173c age=2w6d19h   dirty=97.20KiB
Jul 05 17:21:06 ubp52 sh[15634]: INFO [07-05|17:21:06.637] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=52.647ms    mgasps=0.000 number=199,202 hash=ee67ef..d6378e age=2w6d18h   dirty=97.20KiB
Jul 05 17:21:07 ubp52 sh[15634]: INFO [07-05|17:21:07.241] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=82.318ms    mgasps=0.000 number=199,394 hash=0cd8f2..5a82d3 age=2w6d18h   dirty=97.20KiB
Jul 05 17:21:07 ubp52 sh[15634]: INFO [07-05|17:21:07.872] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=104.620ms   mgasps=0.000 number=199,586 hash=b8ff2d..f048ae age=2w6d17h   dirty=97.20KiB
Jul 05 17:21:08 ubp52 sh[15634]: INFO [07-05|17:21:08.378] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=54.122ms    mgasps=0.000 number=199,778 hash=f8128b..7c108c age=2w6d16h   dirty=97.20KiB
Jul 05 17:21:09 ubp52 sh[15634]: INFO [07-05|17:21:09.002] Imported new chain segment               blocks=192   txs=0 mgas=0.000 elapsed=103.560ms   mgasps=0.000 number=199,970 hash=299324..71e773 age=2w6d16h   dirty=97.20KiB
Jul 05 17:21:12 ubp52 sh[15634]: WARN [07-05|17:21:12.766] Synchronisation failed, dropping peer    peer=0cf4136259a482a1f75318eee4d063889339529b1ba699895f90dd8ffd34eef8 err="retrieved hash chain is invalid: invalid difficulty: have 13603621, want 13603622"
Jul 05 17:21:12 ubp52 sh[15634]: ERROR[07-05|17:21:12.766] Ethereum peer removal failed             peer=0cf41362 err="peer not registered"
Jul 05 17:21:12 ubp52 sh[15634]: INFO [07-05|17:21:12.766] Commit new mining work                   number=200,000 sealhash=abe203..50bf0f uncles=0 txs=0 gas=0 fees=0 elapsed="145.839µs"
Jul 05 17:21:20 ubp52 sh[15634]: INFO [07-05|17:21:20.134] Looking for peers                        peercount=0 tried=52  static=0
Jul 05 17:21:30 ubp52 sh[15634]: INFO [07-05|17:21:30.301] Looking for peers                        peercount=0 tried=108 static=0
Jul 05 17:21:34 ubp52 sh[15634]: INFO [07-05|17:21:34.392] Deep froze chain segment                 blocks=12317 elapsed=1.613s      number=109,999 hash=7dd0e9..a1514d
Jul 05 17:21:40 ubp52 sh[15634]: INFO [07-05|17:21:40.764] Looking for peers                        peercount=0 tried=72  static=0
Jul 05 17:21:51 ubp52 sh[15634]: INFO [07-05|17:21:51.875] Looking for peers                        peercount=0 tried=23  static=0
Jul 05 17:22:01 ubp52 sh[15634]: INFO [07-05|17:22:01.996] Looking for peers                        peercount=2 tried=94  static=0
xJul 05 17:22:12 ubp52 sh[15634]: INFO [07-05|17:22:12.797] Looking for peers                        peercount=0 tried=26  static=0

@meowsbits meowsbits self-requested a review July 6, 2021 12:50
Copy link
Member

@meowsbits meowsbits left a 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

@ziogaschr
Copy link
Member

ziogaschr commented Jul 7, 2021

#369 (comment)

I get exactly the same as @meowsbits.
@atoulme any clues on this one?

Here you can find the full log file

@jpcomps
Copy link

jpcomps commented Jul 14, 2021

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

http://astor.tmio.io/

image

@ziogaschr
Copy link
Member

@jpcomps I can confirm that the fix works nice for both syncing and mining. Nice find man. Can you please commit the fix?

@atoulme
Copy link
Contributor Author

atoulme commented Jul 14, 2021

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.

@jpcomps
Copy link

jpcomps commented Jul 23, 2021

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

@atoulme atoulme requested a review from meowsbits July 23, 2021 19:01

ECIP1099FBlock: big.NewInt(0), // Etchash

DisposalBlock: big.NewInt(0),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@atoulme's change in bd9667f removes code support by the Keccak engine for all Difficulty bomb related logic (including defusal), causing these configuration fields to be silently non-operative.

@meowsbits
Copy link
Member

meowsbits commented Jul 28, 2021

Removing implementation by the Keccak engine for difficulty bomb seems a little troublesome to me in a couple ways.

  • The code design was (and is) almost entirely verbatim copy-paste from the Ethash engine, with the sole difference being the sealing and seal-validation. Carving out a chunk of the mirrored code creates an asymmetry which makes sense in the context of the Astor testnet, but isn't a necessarily general case.
  • If it is does make sense for Keccak to carve this chunk out (and indeed, the difficulty bomb is a very strange chunk and (hopefully) very particular to Ethash) -- then I think we just need to follow through with that assumption and add a few associated conditions in the Configurator, so both describe and enforce this logic. Comment above points to an example of a harmless but misleading use of the configuration vs. implementation (where the chain config describes parameters relating to difficulty, which will be silently ignored).

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 EthashECIP1041Transition after all.... will follow up.

@atoulme
Copy link
Contributor Author

atoulme commented Jul 28, 2021

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.

@meowsbits
Copy link
Member

meowsbits commented Aug 4, 2021

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:

  • Rebase on master, which now includes a similar parallel idea of a Lyra2 engine.
  • Implement Keccak-specific configurator interfaces for EIP100b and ECIP1041 (re difficulty bomb)
  • Get rid of ethash.ModeFake etc. import that keccak package was re-using, and duplicate those types and logic in the keccak package itself (the idea is to make keccak actually it's own thing, and to trim out the hacky copy-pasta from ethash imports)
  • Still TODO: handle the ECIP1099 feature for keccak, which I gather the astor testnet presumes from block 0. I think this can just be removed since it should be totally irrelevant without a DAG.
  • TODO: Double check the ModeTest* logics, since I'm not sure off the top of my head what they want to do that might be Ethash-specific

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.

@bobsummerwill
Copy link
Member

How can we progress this support, @meowsbits?

What can we do to help?

@jpcomps
Copy link

jpcomps commented Nov 8, 2021

second @bobsummerwill . let us know how we can help @meowsbits

@atoulme atoulme closed this Nov 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants