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

Progpow integration #17731

Closed
wants to merge 6 commits into from
Closed

Progpow integration #17731

wants to merge 6 commits into from

Conversation

holiman
Copy link
Contributor

@holiman holiman commented Sep 21, 2018

This PR is a test to see how easy it would be to integrate progpow into go-ethereum. It adds a progpowBlock to the chain config, making it easy to spin up/join a network with hashimoto->progouw transition, to see how well it plays with other implementations.

For example, it would be interesting to see if this

  • works well on both big-endian and little-endian systems, and
  • if there are any quirks around fast or light sync
  • works with remote miners

Progpow turned out to be extremely easy to add, since it was not a new consensus engine per se, but rather an internal switchover from hashimoto to progpow inside ethash.

cc @OhGodAGirl

@holiman holiman changed the title Implement Progpow Poc integration of progpow Sep 21, 2018
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Looks awesome, will launch a public testnet with progpow maybe tomorrow

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Compile fails

# github.com/ethereum/go-ethereum/tests
tests/block_test_util.go:114:68: not enough arguments in call to ethash.NewShared
	have ()
	want (*big.Int)

I guess you missed something 😄

@holiman
Copy link
Contributor Author

holiman commented Sep 21, 2018

@eosclassicteam yup, fixed

@holiman
Copy link
Contributor Author

holiman commented Sep 21, 2018

will launch a public testnet with progpow maybe tomorrow

If you do, please make it use hashimoto for a while, so there's a proper switchover after the difficulty has set, and blocktimes are ~14s. Then we would also see how the switch affects blocktimes (though, it's a pretty moot datapoint, since it's CPU-mining and not necessarily reflects performance differences for GPU mining)

@karalabe
Copy link
Member

It would probably make sense to integrate it into puppeth, because then we have everything for a testnet and don't have to hack all the pieces together individually.

@ghost
Copy link

ghost commented Sep 21, 2018

@holiman Yes I would like to configure a new testnet like this one

TestnetChainConfig = &ChainConfig{
		ChainID:             big.NewInt(3),
		HomesteadBlock:      big.NewInt(0),
		DAOForkBlock:        nil,
		DAOForkSupport:      true,
		EIP150Block:         big.NewInt(0),
		EIP150Hash:          common.HexToHash("0x41941023680923e0fe4d74a34bdac8141f2540e3ae90623718e47d66d1ca4a2d"),
		EIP155Block:         big.NewInt(0),
		EIP158Block:         big.NewInt(0),
		ByzantiumBlock:      big.NewInt(0),
		ConstantinopleBlock: nil,
                ProgpowBlock:      big.NewInt(10000),
		Ethash:              new(EthashConfig),
	}

@ghost
Copy link

ghost commented Sep 21, 2018

@holiman @karalabe If we can add a wrapper for puppeth it is possible to launch a ProgPOW testnet with it

@holiman
Copy link
Contributor Author

holiman commented Sep 21, 2018

done! I also added Constantinople to puppeth, seems we hadn't done so yet

@karalabe
Copy link
Member

Do we have any benchmark numbers? I'm curious how much work we're putting on ourselves wrt progpow verification.

@holiman
Copy link
Contributor Author

holiman commented Sep 21, 2018

[user@work ethash]$ go test -run - -bench Benchmar.*Light
goos: linux
goarch: amd64
pkg: github.com/ethereum/go-ethereum/consensus/ethash
BenchmarkHashimotoLight-2   	     500	   3930131 ns/op
BenchmarkProgpowLight-2     	      20	  79983262 ns/op
PASS
ok  	github.com/ethereum/go-ethereum/consensus/ethash	7.281s

Quite a big diff

@holiman
Copy link
Contributor Author

holiman commented Sep 21, 2018

Same for full:

[user@work ethash]$ go test -run - -bench Benchmar.*FullSmall
goos: linux
goarch: amd64
pkg: github.com/ethereum/go-ethereum/consensus/ethash
BenchmarkHashimotoFullSmall-2   	  300000	      5684 ns/op
BenchmarkProgpowFullSmall-2     	    2000	    711026 ns/op
PASS
ok  	github.com/ethereum/go-ethereum/consensus/ethash	4.171s

@karalabe
Copy link
Member

The full is not too relevant because (1. geth doesn't cpu mine; 2. testnet cpu mining is fine independent of the speed; 3. it's up to gpus to max the speed). The light is more problematic because that's used during sync.

Ethash was 4ms according to your benchmark, so that's 6372574 * 4ms = ~7 hours to fully verify mainnet on a single core.

ProgPoW at 80ms would mean 6372574 * 80ms = ~6 days. That's quite a bump.

@AlexeyAkhunov
Copy link
Contributor

Going from 4ms to 80ms will also affect block propagation quite a lot. It will increase uncle rate, therefore, ostensibly, reduce miner's profitability, and increase the variability of their income.

@holiman
Copy link
Contributor Author

holiman commented Sep 21, 2018

Yup. I'm wondering, though, if some low level optimizations could bring it down an order of magnitude, or if it's inherent in the algo.

The code contains e.g. zeroing a newly alloc:ed byte array. That in itself is not a big timesink, but indicates a lack of familiarity with golang.

@holiman
Copy link
Contributor Author

holiman commented Sep 21, 2018

Would be interesting to see benchmarks comparing hashimoto vs progpow on cpp

@AlexeyAkhunov
Copy link
Contributor

@holiman I agree - some optimisation is possible (for example, implementing "clz" and "popcount" functions more efficiently, with tricks from "Hacker's Delight" or Knuth's Volume 4 fascicle 1 "Bitwise tricks"). However, we need to first figure out why it went to 80ms, what is the main contributor

@AlexeyAkhunov
Copy link
Contributor

I tried some optimisations (reducing allocations, passing arrays in/out functions, pop count, leading zeros) https://gist.github.com/AlexeyAkhunov/b3a5046fd2fa062b7d4c3fadc9194607, but it is still stays at 80ms, unfortunately

@AlexeyAkhunov
Copy link
Contributor

AlexeyAkhunov commented Sep 22, 2018

And, most of the time is spent in the calls to the generateDatasetItem function. Each invocation of progpowLoop calls it 2 * 32 = 64 times. The function progpowLoop itself is called 64 times inside the progpow function, so generateDatasetItem is called 64 * 64 = 4096 times this way. Apart from that, the progpow function calls generateDatasetItem directly 2 * 4096 = 8192 times. In total, progpow calls generateDatasetItem 4096 + 8192 = 12288 times. Hashimoto, on the other hand, cals generateDatasetItem only 64 * 2 = 128 times. My calculations might be off by a factor, but the general idea should be clear - it simply does too much access. Perhaps that was a mistake?

@ghost
Copy link

ghost commented Sep 22, 2018

@AlexeyAkhunov Perhaps ProgPOW's design might be poor for verification, I hope it's not, we can still find a way to implement it 😂

@AlexeyAkhunov
Copy link
Contributor

@eosclassicteam Efficient verification is one of the important design requirement for a good PoW algorithm. Perhaps it has not been considered. But if the verification performance is so poor, ProgPow hinders too many things - light clients, block propagation (block gas limit will have to be reduced to support that), sync performance - it will take much longer to sync. Everything will get much slower

@ifdefelse
Copy link

ifdefelse commented Sep 23, 2018

Hi all.

The ProgPoW team is looking into this- the CPU verification side is being copied from the whitepaper code, which was required for our own internal testing with ethminer, but obviously not required for proper client implementation. We'll look into this.

Also, yes, Go isn't our strongest language- we're hoping that the geth team will be able to help with actual optimizations on that side.

@MoneroCrusher
Copy link

@AlexeyAkhunov @karalabe @holiman according to ProgPOW devs it's a bug that is being addressed. Real verification time will be 8ms, not 80ms. Therefore not a problem.

@Arachnid
Copy link
Contributor

@AlexeyAkhunov @karalabe @holiman according to ProgPOW devs it's a bug that is being addressed. Real verification time will be 8ms, not 80ms. Therefore not a problem.

Please let them speak for themselves. They're here on the thread; trying to talk for them just confuses matters.

@MoneroCrusher
Copy link

@AlexeyAkhunov @karalabe @holiman according to ProgPOW devs it's a bug that is being addressed. Real verification time will be 8ms, not 80ms. Therefore not a problem.

Please let them speak for themselves. They're here on the thread; trying to talk for them just confuses matters.

I mainly made that post so it is publicly posted and that the public knows that ~8ms (of course it depends on the CPU and how many cores are utilized) is the real number after the bug is eliminated, not 80ms (I did not know this until 1 hour ago). ProgPOW devs have said they'd rather not engage in too much public discussions anymore. I expect that the people I tagged above know this already, but tagged them anyway in case they didn't. It's important that the public stays up to date too on latest developments as not everyone is part of the discord group(s).
Of course I'm not stopping the ProgPOW devs from propagating that info themselves.

@ifdefelse
Copy link

There is a slight performance improvement at ifdefelse/go-ethereum mainly reducing the number of generateDatasetItem calls (lookups), producing the following results:

$ go test -run - -bench Benchmark.*Light
goos: linux
goarch: amd64
pkg: github.com/ethereum/go-ethereum/consensus/ethash
BenchmarkHashimotoLight-8                   1000           1565141 ns/op
BenchmarkProgpowLight-8                      100          16306560 ns/op
BenchmarkProgpowOptimalLight-8               300           5343173 ns/op
PASS
ok      github.com/ethereum/go-ethereum/consensus/ethash        10.158s

BenchmarkProgpowLight tests the drop-in replacement, while BenchmarkProgpowOptimalLight pregenerates some data which is only implemented in the test right now.

There is still room for improvement, for example all the math can be done as SSE vectors instead of iterative loops. A well-optimized implementation should end up being about just 2x slower than the ethash verification as there is twice as much data read per iteration.

@ghost
Copy link

ghost commented Sep 26, 2018

@ifdefelse Yes I think this is a remarkable change well done 👍

@holiman
Copy link
Contributor Author

holiman commented Mar 2, 2020

Rebased on master, squashed most commits

@holiman
Copy link
Contributor Author

holiman commented Sep 23, 2020

Rebased, and afaik up to date with https://github.com/ethereum/EIPs/blob/master/EIPS/eip-1057.md at 0.9.3. cc @gcolvin

ifdefelse and others added 6 commits October 5, 2020 13:45
…ntation.

Do note that cDag should be generated once per epoch which results in additional performance gains as shown by BenchmarkProgpowOptimalLight over BenchmarkProgpowLight.
+ make use of pregenerated cdag for progpow, add testcases
+ update keccakf800 output, fix full verification, fix order of mixhash/finalhash
}

elapsed := time.Since(start)
log.Info("Generated progpow cDag", "elapsed", common.PrettyDuration(elapsed), "epoch", epoch)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
log.Info("Generated progpow cDag", "elapsed", common.PrettyDuration(elapsed), "epoch", epoch)
log.Debug("Generated progpow cDag", "elapsed", common.PrettyDuration(elapsed), "epoch", epoch)

@MariusVanDerWijden
Copy link
Member

This PR became quite stale. With all the work on the merge, I think it is time to retire this PR cc @holiman

@holiman holiman closed this Oct 29, 2021
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.