[API BREAK] Change argument order to out/outin/in

sipa commented at 11:53 pm on August 27, 2015: contributor

This introduces a strict argument order for all API functions (including modules), explained in include/secp256k1.h.

sipa force-pushed on Aug 28, 2015

sipa cross-referenced this on Aug 28, 2015 from issue [API BREAK] Remove _t in types due to being POSIX-reserved by sipa

dcousens commented at 1:24 am on August 28, 2015: contributor

concept ACK

sipa force-pushed on Aug 28, 2015

sipa commented at 3:04 am on August 28, 2015: contributor

Rebased.

DavidEGrayson commented at 3:06 am on August 28, 2015: none

Here is an excerpt from the secp256k1.h in this pull request:

 0General ordering of arguments in API calls:
 1
 2- context pointer.
 3- output-only arguments.
 4- output/input arguments.
 5- input-only arguments.
 6
 7Within each group, argument types are ordered based on their type:
 8- Counts of other arguments always go immdiately before those arguments.
 9- Serializations
10- Serialization lengths go immediately after the serialization, even if
11  that violates the output/input order.
12- Signatures
13- Public keys
14- Secret keys
15- Tweaks
16- Messages
17- Public nonces
18- Private nonces
19- Nonce functions
20- Other function pointers
21- Opaque data pointers follow the function pointer they are to be passed to.
22- Algorithm names
23- Attempt number
24- Flags

With the long list of argument types that does not come with any justification, this scheme sounds pretty arbitrary and hard to remember. I would propose a simpler scheme based on chronology:

 0These rules specify the ordering of arguments in API calls:
 1
 21. Context pointers come first, and are exempt from all other rules.
 32. Buffer/array lengths are immediately after the argument they describe,
 4   and are exempt from all other rules.
 53. Opaque data pointers are immediately after the function pointer they
 6   are passed to, and are exempt from all other rules.
 74. Arguments are ordered chronologically: data that was generated earlier
 8   comes first.
 95. When there is no clear chronological ordering, use these additional
10   arbitrary rules to break ties:
11    1. Flags, algorithm names, and function pointers come last.
12    2. For IN/OUT parameters, place the parameter as if it were just an
13       input.

I think it is natural for humans to assign chronological orders to things, and it is natural (in English documents like this API) to order things from left to right according to that chronology. This means outputs will always be last, which is roughly the opposite of what is proposed in this pull request.

Not every user story has the same chronology though. For example, with secp256k1_ec_pubkey_serialize, maybe the user planned ahead of time to always set compressed to 0 when serializing a public key, in which case they would want flags to come before the public key. Alternatively, maybe they always generate both compressed and uncompressed versions of every public key, so the decision of which value to use for compressed in the current function call really was made after the public key existed. That’s why we need some arbitrary rules like 5.1 to clear up ambiguity.

So here is what the API functions and their argument order would look like if we applied the rules from this chronological scheme:

 0secp256k1_nonce_function_type(IN key32, IN msg32, IN data, IN algo16, IN attempt, OUT nonce32)
 1secp256k1_context_set_illegal_callback(ctx, IN fun, IN data)
 2secp256k1_context_set_error_callback(ctx, IN fun, IN data)
 3secp256k1_ec_pubkey_parse(ctx, IN input+length, OUT pubkey)
 4secp256k1_ec_pubkey_serialize(ctx, IN pubkey, IN compressed, OUT output+length)
 5secp256k1_ecdsa_signature_parse_der(ctx, IN input+length, OUT sig)
 6secp256k1_ecdsa_signature_serialize_der(ctx, IN sig, OUT output+length)
 7secp256k1_ecdsa_verify(ctx, IN pubkey, IN msg, IN sig)
 8secp256k1_ecdsa_sign(ctx, IN seckey, IN msg32, IN noncefp+data, OUT sig)
 9secp256k1_ec_seckey_verify(ctx, IN seckey)
10secp256k1_ec_pubkey_create(ctx, IN seckey, OUT pubkey)
11secp256k1_ec_privkey_export(ctx, IN seckey, IN compressed, OUT privkey+length)
12secp256k1_ec_privkey_import(ctx, IN privkey+length, OUT seckey)
13secp256k1_ec_privkey_tweak_add(ctx, INOUT seckey, IN tweak)
14secp256k1_ec_pubkey_tweak_add(ctx, INOUT pubkey, IN tweak)
15secp256k1_ec_privkey_tweak_mul(ctx, INOUT seckey, IN tweak)
16secp256k1_ec_pubkey_tweak_mul(ctx, INOUT pubkey, IN tweak)
17secp256k1_context_randomize(ctx, IN seed32)
18secp256k1_ec_pubkey_combine(ctx, IN pubkeyins+length, OUT pubkey)

dcousens commented at 3:09 am on August 28, 2015: contributor

@DavidEGrayson that is exactly what I would have expected from a modern API. C conventions aside.

Chronology aside, I also think the list of argument type orders is very cumbersome and difficult to remember, and probably all round unnecessary.

DavidEGrayson commented at 3:12 am on August 28, 2015: none

Thanks @dcousens! I had to make a few arbitrary decisions that weren’t really backed up by any principles, so I’m glad it exactly matches what you had in mind.

sipa commented at 3:13 am on August 28, 2015: contributor

@DavidEGrayson That’s pretty close to what I was aiming for, only the other way around. I’m aiming for following “out = f(in)” as much as possible, and retain that argument order when earlier outs become ins (for example: signing will do sig = f(key,msg), then later verification takes f(sig,[pub]key,msg). Perhaps I should have given the principles behind that ordering.

dcousens commented at 3:16 am on August 28, 2015: contributor

IMHO, it also lends it self to a more formal type notation too. eg, using pseudo-Haskell-like type grammar

0secp256k1_ec_privkey_import(ctx, IN privkey+length, OUT seckey)   becomes
1secp256k1_ec_privkey_import :: context -> privateKey -> secret

The alternative reads more like C’s syntax, which is to specify the output types initially

0secp256k1_ec_privkey_import :: secret <- (context -> privateKey)

DavidEGrayson commented at 3:20 am on August 28, 2015: none

@sipa I thought that usually the keys (or at least an algorithm for generating keys) is created before the message, so the “sig, key, msg” ordering looks off to me even if you are going for a reverse ordering. With PGP you would probably make a key pair and attempt to prove its veracity before signing things with it. With Bitcoin, you use the public key to receive funds before you generate transactions (messages) to use those funds.

dcousens commented at 3:23 am on August 28, 2015: contributor

@DavidEGrayson

I’m aiming for following “out = f(in)” as much as possible, and retain that argument order when earlier outs become ins

That is, composability of arguments comes from the front, which makes sense if you look at how you would do it if you were following the: secp256k1_ec_privkey_import :: secret <- (context -> privateKey) grammar. @sipa though, given this reasoning, why is the context at the front?

sipa commented at 4:09 am on August 28, 2015: contributor

Because every function has a context it seems reasonable to put it first. It’s also not really an input or an output. Compare it with the object in OO on which you invoke methods.

Having the key before the message makes sense.

dcousens commented at 4:19 am on August 28, 2015: contributor

Because every function has a context it seems reasonable to put it first.

Well, I guess it depends on how you deep you want to take that rabbit hole, by no means do functions actually need a context. We just choose that for optimization purposes because instantiating it on every call would be ridiculous. State, as it were.

sipa commented at 4:25 am on August 28, 2015: contributor

Sure, it’s just a rationalization. Any order is fine, IMHO, and I think we should just aim for a consistent and easily understandable one. I like the “order of generation” one for that reason.

I mostly pick “first out, then in” because that’s what most are already doing (and it’s not reasonable, as it is common in C APIs).

dcousens commented at 4:38 am on August 28, 2015: contributor

Any order is fine, IMHO, and I think we should just aim for a consistent and easily understandable one

Of course, this was just bike shedding, what is most important is consistency. The devil isn’t in the API, its in the implementation details [for this library].

It would be nice if you could even put it under test, though I’m not sure how what C tooling is like in that regard.

easily understandable one

That is always going to be subjective, as noted above, myself and @DavidEGrayson read things in the reverse order to your proposal.

What might be an important consideration, is what background do you think most programmers reviewing / using this library will have? If it is a C background, then the out/in will cater to the crowd; however, if you think most people are going to use a modern language (but linking through the ABI say), it might be more likely an in/out will be less disruptive.

sipa commented at 4:43 am on August 28, 2015: contributor

No reason a wrapper can’t use another order.

apoelstra commented at 1:46 pm on August 28, 2015: contributor

@dcousens most people developing the software have a C background ;)

I don’t really want to participate in a bikeshedding thread but for what it’s worth my intuition matches @sipa’s. Also I do strongly prefer out-then-in; none of my other preferences are strong.

voisine commented at 7:44 pm on August 28, 2015: contributor

Out-then-in is nice because it matches the ordering of assignments. a = b

gmaxwell commented at 8:01 pm on August 28, 2015: contributor

This is the most common convention than C; indeed, because it matches assignment.

Probably consistency is more important than external convention, but the conventional ordering is good.

sipa force-pushed on Aug 28, 2015

sipa commented at 11:51 pm on August 28, 2015: contributor

Updated to a “out-then-in”, and within each group a (“later-then-earlier” for data arguments, and “complex-then-simple” for the rest) policy.

sipa commented at 11:52 pm on August 28, 2015: contributor

Also changed the order of the documentation for each function to match the actual argument order.

dcousens commented at 1:14 am on August 29, 2015: contributor

Updated to a “out-then-in”, and within each group a (“later-then-earlier” for data arguments, and “complex-then-simple” for the rest) policy.

ACK, thanks for simplifying it :+1:

@dcousens most people developing the software have a C background ;) @apoelstra and assuredly so will those writing the wrappers, but I get the feeling this library will see a lot of usage across language boundaries, and I’d be surprised if I didn’t see these particular remnants of the API translating into other languages. Food for thought, but LGTM.

DavidEGrayson commented at 1:35 am on August 29, 2015: none

The new, simpler rules are looking good. But I think it’s a bit odd that rules 2 and 3 are separate:

02. Buffer array lengths always immediately the follow the argument whose
1    length they describe, even if this violates rule 1.
23. Counts of batches of arguments always go before that batch.

Rule 3 seems to affect the ordering of arguments to secp256k1_ec_pubkey_combine:

0SECP256K1_WARN_UNUSED_RESULT int secp256k1_ec_pubkey_combine(
1    const secp256k1_context_t* ctx,
2    secp256k1_pubkey_t *out,
3    int n,
4    const secp256k1_pubkey_t * const * ins
5) SECP256K1_ARG_NONNULL(2) SECP256K1_ARG_NONNULL(4);

However, the documentation says ins is a “pointer to array”, so I would think we should apply Rule 2. This means n would come last.

Unless you are going to use C’s variadic arguments, I don’t see how Rule 3 would ever really apply, and it can probably be removed.

EDIT: I just don’t know what “batches of arguments” means. I suspect you are trying to assign some special semantics to certain arrays that you consider to be batches of arguments. I don’t see how it makes the API better to treat some arrays differently and apply different rules to them.

sipa commented at 2:16 am on August 29, 2015: contributor

I wasn’t considering ins as a buffer (rule 2) but as a list of “data pointer” arguments (rule 4), but I guess they can be unified.

gmaxwell added this to the milestone initial release on Aug 31, 2015

sipa force-pushed on Sep 4, 2015

sipa commented at 3:44 pm on September 4, 2015: contributor

@DavidEGrayson Unified the batch size and buffer size rules, and updated the code accordingly.

[API BREAK] Change argument order to out/outin/in dc0ce9fc41

sipa force-pushed on Sep 4, 2015

sipa merged this on Sep 4, 2015

sipa closed this on Sep 4, 2015

sipa referenced this in commit b2eb63b2d7 on Sep 4, 2015

apoelstra cross-referenced this on Oct 21, 2015 from issue reflect (interim?) updated secp256k1 API by thedoctor

[API BREAK] Change argument order to out/outin/in #293