add static context object which has no capabilities #553

pull apoelstra wants to merge 2 commits into bitcoin-core:master from apoelstra:static-context changing 3 files +20 −0

apoelstra commented at 9:27 PM on August 15, 2018: contributor

No description provided.
apoelstra force-pushed on Aug 15, 2018
gmaxwell commented at 3:59 PM on September 4, 2018: contributor

Should we document that you can't call set_illegal_callback on this and perhaps add some checks on some of the disallowed calls to fail if you try?
sipa commented at 4:00 PM on September 4, 2018: contributor

@gmaxwell I believe the set of permitted functions on this object is exactly the functions which take a const context pointer.
apoelstra commented at 4:02 PM on September 4, 2018: contributor

Sure, we can do a VERIFY_CHECK on those functions that the ctx is not the static one, much like how we compare to NULL now.
apoelstra commented at 5:53 PM on September 20, 2018: contributor

Added a commit which VERIFY_CHECKs on all functions that modify the context object.

I considered using ARG_CHECK but I didn't see the point, since you can't change the callback function from its default CHECK-like behaviour. Also, since it is actually UB to call these functions on static data (without synchronization primitives anyway), I wanted to prevent it more emphatically.

Edit Changed from VERIFY_CHECK to CHECK because Greg pointed out that VERIFY is only defined during unit testing.
apoelstra force-pushed on Sep 20, 2018

in include/secp256k1.h:187 in 9461ae6f02 outdated

 178 | @@ -179,6 +179,13 @@ typedef int (*secp256k1_nonce_function)(
 179 |  #define SECP256K1_TAG_PUBKEY_HYBRID_EVEN 0x06
 180 |  #define SECP256K1_TAG_PUBKEY_HYBRID_ODD 0x07
 181 |  
 182 | +/** A simple secp256k1 context object with no precomputed tables. These are useful for
 183 | + *  type serialization/parsing functions which require a context object to maintain
 184 | + *  API consistency, but currently do not require expensive precomputations or dynamic
 185 | + *  allocations.
 186 | + */
 187 | +SECP256K1_API extern secp256k1_context *secp256k1_context_no_precomp;

sipa commented at 5:01 AM on October 4, 2018:

Can you mark this object const? If that works you don't need any of the CHECK invocations I think as they all require a non-const argument. It would also alleviate any concerns about synchronization etc.

apoelstra commented at 3:17 PM on October 4, 2018:

Done

add static context object which has no capabilities ed7c08417a
prevent attempts to modify `secp256k1_context_no_precomp` 40fde611bd
apoelstra force-pushed on Oct 4, 2018
in src/secp256k1.c:102 in 40fde611bd
```
  98 | @@ -91,6 +99,7 @@ secp256k1_context* secp256k1_context_clone(const secp256k1_context* ctx) {
  99 |  }
 100 |  
 101 |  void secp256k1_context_destroy(secp256k1_context* ctx) {
 102 | +    CHECK(ctx != secp256k1_context_no_precomp);
```
sipa commented at 7:56 PM on October 16, 2018:

Are these necessary? A non-const ctx pointer is used, which can't be secp256k1_context_no_precomp (as that one is const).

apoelstra commented at 8:23 PM on October 16, 2018:

Oh, good point. I guess they're not necessary. I can remove them if you want, though I don't mind having them there.
sipa commented at 7:17 PM on October 17, 2018: contributor

utACK 40fde611bd145bdafb78a52af6d197489101bafb
real-or-random commented at 10:33 AM on October 18, 2018: contributor

ACK 40fde611bd145bdafb78a52af6d197489101bafb
sipa merged this on Nov 6, 2018
sipa closed this on Nov 6, 2018
sipa referenced this in commit 314a61d724 on Nov 6, 2018
sipa referenced this in commit e34ceb333b on Nov 26, 2018

Contributors

Linked (view graph)

#557 Eliminate scratch memory used when generating contexts #878 Remove unused secp256k1_fe_inv_all_var