Skip to content

Conversation

@dgpv
Copy link
Contributor

@dgpv dgpv commented Apr 21, 2019

Relevant issue: #36

This is working, but not yet finished. I am asking for review and comments.

In particular, I am not sure about naming for the functions.

naming allocation function secp256k1_surjectionproof_create, to be in line with the secp256k1_scratch_space_create, have a problem that this might get confused with secp256k1_surjectionproof_generate - completely different function, but the meanings for the words 'create' and 'generate' are close.

I decided to use the name secp256k1_surjectionproof_allocate_initialized - literally what the function would do.

At the same time, for the deallocation function, I used 'destroy' (secp256k1_surjectionproof_destroy), because it is used for this meaning in other places.

Other concern what I want to request comments on, is that secp256k1_surjectionproof_destroy can potentially be called on stack-allocated struct, with bad consequences. It may be good idea to use some sort of marker bytes to distinguish stack/heap allocated structs, and if there is a mixup, then die cleanly, to prevent possible abuse. The marker bytes could be set within _initialize(), and then _allocate_initialized() can adjust these markers slightly, so other checking code would treat them as valid, but _destroy() would be able to easily distinguish between stack and heap allocated struct. But this change would mean the scope of this PR would extend to _initialize() function. Maybe the concern is not that big to warrant changes in _initialize().

@dgpv
Copy link
Contributor Author

dgpv commented Apr 25, 2019

Ping @apoelstra

Copy link
Contributor

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

Sorry for the late response.

@apoelstra apoelstra merged commit 386cd31 into BlockstreamResearch:secp256k1-zkp May 26, 2019
@apoelstra
Copy link
Contributor

Regarding your concerns

  1. I'm fine with the names. I expect these functions will only be used by FFI-binding authors anyway.
  2. Similarly I'm ok with not having marker bytes .. mainly because we don't use these elsewhere or upstream (e.g. in the scratch space API) where it's also possible to free things that were allocated on the stack. My feeling is that this is a programmer error that'd be noticed and fixed pretty quickly.

@dgpv
Copy link
Contributor Author

dgpv commented May 26, 2019

Eh, I did not expected this to be merged this quick. I thought that I would have the chance to remove XXX comments.. They are supposed to be temporary (as indicated "by working.. but is not yet finished")

Shall I add another PR to remove them ?

@dgpv
Copy link
Contributor Author

dgpv commented May 26, 2019

OK maybe that is not that important. Just that 'XXX' is historically is a sign that something is potentially wrong, and if there's nothing wrong, I think it should be removed or replaced with TODO or NOTE...

@dgpv
Copy link
Contributor Author

dgpv commented May 26, 2019

Added #65 to remove the XXX comments

tomtau pushed a commit to crypto-com/secp256k1-zkp that referenced this pull request Jul 9, 2020
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.

2 participants