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

Context overhaul tracking issue #538

Open
6 tasks
apoelstra opened this issue Nov 24, 2022 · 4 comments
Open
6 tasks

Context overhaul tracking issue #538

apoelstra opened this issue Nov 24, 2022 · 4 comments

Comments

@apoelstra
Copy link
Member

Moving discussion from #346 and #529.

On a high level our plan is to:

  • Eliminate all verification contexts from the library (internally we replace them with ffi::secp256k1_context_no_precomp)
  • Eliminate all signing contexts from the library (internally, by using TLS when compiling with std and using a global static otherwise)
  • Signing contexts will be rerandomized after every operation unless the user explicitly calls a _no_rerandomize variant...
  • ...where rerandomization, on no-std builds, will be a "best effort" basis where we do a manual lock using atomics and give up when there is contention

Specifically we need to:

  • mirror the context structure in Rust so that we can directly construct/destruct them without FFI or allocation
  • implement the TLS/static context and its best-effort rerandomize function
  • make all our context-taking functions use this structure and ignore their context arguments
  • deprecate the Secp256k1 struct and all the context traits; move their non-constructor methods to bare functions
  • deprecate all the context-taking functions on the keytypes (this is harder; if we deprecate e.g. KeyPair::negate then what should the new function name be called?)
  • (after a release) delete the deprecated functions
@Kixunil
Copy link
Collaborator

Kixunil commented Nov 24, 2022

I wouldn't be too mad if we just break the API. The change will be obvious and pretty simple (just delete the argument wherever compiler complains). The library is not stable anyway.

@apoelstra
Copy link
Member Author

@Kixunil heh, yeah, after attempting this I tend to agree. But it's the sort of thing where I'd give veto power to anybody who insisted that we avoid breaking things :).

I think we should make an effort to deprecate things where it's easy/possible. But in cases like Pubkey::negate we should just delete the argument because the alternative would make the API worse.

@apoelstra
Copy link
Member Author

Ok, I don't think that mirroring the context structure in Rust by hand is going to be feasible. It contains a ecmult_gen_context, which contains a secp256k1_scalar (which has 3 different representations depending on compile-time flags) and a secp256k1_gej (which has 3 secp256k1_fes which also have multiple representations).

What I probably can do, though, is patch the C code to provide a (non-synchronized) static pre-initialized signing context. I will investigate this, and see if maybe we want to propose this upstream.

@apoelstra
Copy link
Member Author

First two checkboxes should be covered by #539

apoelstra referenced this issue Nov 30, 2022
For raw pointers that can never be null Rust provides the
`core::ptr::NonNull` type. Our `Secp256k1` type has an inner field that
is a non-null pointer; use `NonNull` for it.

Fix: #534
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

No branches or pull requests

2 participants