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

opencl: fix ethash light handling, fix errorf formats #83

Merged
merged 2 commits into from
Apr 11, 2016
Merged

opencl: fix ethash light handling, fix errorf formats #83

merged 2 commits into from
Apr 11, 2016

Conversation

karalabe
Copy link
Member

This PR does two things:

@@ -560,7 +560,7 @@ func (c *OpenCLMiner) Search(block pow.Block, stop <-chan struct{}, index int) (
ds := C.uint64_t(c.dagSize)
// We verify that the nonce is indeed a solution by
// executing the Ethash verification function (on the CPU).
ret := C.ethash_light_compute_internal(c.ethash.Light.current.ptr, ds, hashToH256(headerHash), cn)
ret := C.ethash_light_compute_internal(c.ethash.Light.getCache(block.NumberU64()).ptr, ds, hashToH256(headerHash), cn)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is dangerous because of the finalizer. To keep the C pointer live during the call, you need to keep a reference to it, e.g.

cache := c.ethash.Light.getCache(...)
C.ethash_light_compute_internal(cache.ptr, ...)
_ = cache // keep live

This is very subtle though, so I'd prefer a solution that shares this code with ethash.go.

Copy link
Member Author

Choose a reason for hiding this comment

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

So what should the proper solution be?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can make a method on Light that does this check and then use it here and in Light.Verify.
Basically refactor the core of Light.Verify into its own method.

@karalabe
Copy link
Member Author

@fjl Could you please check if this is enough?

@fjl
Copy link
Contributor

fjl commented Mar 22, 2016

You really do need the _ = cache. The cache needs to be live until after the call, otherwise GC may collect it and run the finalizer, which will free the C memory that ethash is operating on. I cannot explain this better. We already keep it live in exactly the same way in Light.Verify. Doing C memory management with Go finalizers might strike you as weird, but does bring the huge benefit of not having to reference-count the caches and DAGs.

What I would like to see instead of duplicating this trickery in the GPU half is a Go wrapper for ethash_light_compute_internal which can be used for both Light and OpenCLMiner.
Maybe just use this commit.

@karalabe
Copy link
Member Author

Won't this be optimized out by the compiler?

@karalabe
Copy link
Member Author

@fjl I've pulled in your commit on top of mine. This should fix the OpenCL build issue so I think we can go ahead and merge this as is, and then figure out if we need to do any changes to cover the Go SSA compilation afterwards.

@fjl
Copy link
Contributor

fjl commented Mar 29, 2016

👍

@fjl
Copy link
Contributor

fjl commented Mar 30, 2016

@obscuren please review

@karalabe
Copy link
Member Author

@obscuren pling

@obscuren
Copy link
Contributor

👍

@obscuren obscuren merged commit 25b32de into ethereum:master Apr 11, 2016
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.

ethash_opencl.go:563 c.ethash.Light.current undefined
3 participants