-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Rework or get rid of scratch space #1302
Comments
In secp256k1-zkp the scratch space gets actual usage, and I find it has a much nicer API than malloc/free. The API lets us:
So I think it has more benefit than just replacing |
@apoelstra We've discussed this a bit IRL, and it seems to me there are just a whole lot of only vaguely-overlapping concerns:
My thinking is that the interface at least needs to be dropped from the API today, because it serves no purpose, and it seems to be guiding our thinking for future APIs. We can keep the internal logic for now - and possibly bring it back if it makes sense for a particular use case - but it's easier to discuss all of that without being pre-biased by the scratch API. |
@sipa these are all good points. concept ACK from me on removing the API from this library. |
In the case of The predictability of A downside of this approach is that adding more functions that dynamically allocate memory besides |
Our confidence in the scratch space code isn't particularly high. It reinvents bump allocation, but it had a few issues in the past. All the code that uses scratch space is currently unreachable from the public API (except that we have
secp256k1_scratch_create
andsecp256k1_scratch_destroy
themselves in the public API.A much simpler alternative is to get rid of scratch spaces and just assume the existence of
malloc
/free
and use these directly. The disadvantage of this is that it's a bit harder for platforms that don't havemalloc
.Another alternative is to rework the scratch space code. It may be possible to simply it and improve its usability.
I think our future directions on this should be guided by whatever we feel is best for our cases:
The text was updated successfully, but these errors were encountered: