Unneeded manual alignment in *_context_create

[Kixunil]

There seems to be unneeded code for manual alignment in the *_context_create function. alloc already returns memory aligned to ALIGN_TO. Am I missing something?

[elichai]

The code just writes the size of the allocated buffer before the actual buffer. So we add ALIGN_TO to get the actual aligned buffer after we wrote a usize describing how many bytes we've allocated.

[elichai]

I do see now that the code doesn't handle failed allocation, we should add:

if ptr.is_null() { handle_alloc_error(layout); }

I can PR this in the weekend if no one else gets to it before

[Kixunil]

OK, that explains it but why do we allocate more rather than calling secp256k1_context_preallocated_size to compute the layout again?

[elichai]

OK, that explains it but why do we allocate more rather than calling secp256k1_context_preallocated_size to compute the layout again?

That would require us to keep the flags around, so requires bigger allocation anyway

[Kixunil]

There's also rustsecp256k1_v0_6_1_context_preallocated_clone_size which doesn't need flags and "reads the size off existing context".

Previous idea that I realized doesn't work

This doesn't work for dynamic linking since the dynamically linked library might have a context of different size.

But I checked the function and it actually returns sizeof(rustsecp256k1_v0_6_1_context) regardless of the flags. The flags passed are only checked. So we could just pass any valid flags to it. (But of course then updating the version will always involve having to check if the behavior changed.)

If we're willing to incur the cost of checking the code on each update maybe consider this approach:

• Make a new exported symbol in the C library static size_t rustsecp256k1_v0_6_1_rust_private_context_size = sizeof(rustsecp256k1_v0_6_1_context)'
• Have the build script read off the value after the C code is built and generate size.rs in OUT_DIR containing const SECP256K1_CONTEXT_SIZE: usize = {value_here};
• Include size.rs and define: #[repr(align(16))] struct Secp256k1<C: Context>([u8; SECP256K1_CONTEXT_SIZE], PhantomData<C>);
• Create and destroy functions will work same as now except not storing the size

If the code ever becomes dependent on flags we can probably reimplement it in Rust as const fn (I don't expect it to become crazy-complicated).

This gets rid of the additional allocation. People can store Secp256k1 in Box or Arc if they want or use a global. We won't need to manage allocations, just creation and destruction. For cases when rustc can't remove copying whole context we can provide fn init_in_place(memory: &mut MaybeUninit<Secp256k1>) -> &mut Secp256k1. This approach also supersedes #534.

Also note that we could use this technique to get the actual alignment and not hard-code 16 (which is probably overkill and we likely only need 8 on current platforms).

@apoelstra I'm curious what do you think about this since it's somewhat big change.

[apoelstra]

If the code ever becomes dependent on flags we can probably reimplement it in Rust as const fn (I don't expect it to become crazy-complicated).

All these functions did depend on the size until quite recently, and when they did, context_create involved multiple novel algorithms from multiple people across 100+ lines of C code. It was indeed "crazy complicated". As far as "reimplementing in Rust", that is exactly what we did, as far as we could -- we reimplemented all the memory management while leaving the hard crypto alone.

The size of these objects was also multiple megabytes, too much to put on the stack.

As you note, the flags are not really used now ... so what purpose do they serve? That is part of a larger issue, #346.

Having said all this I am very doubtful that we'll return to a "massive context object whose size is runtime-dependent" model in the next several years. So given that, I am really tempted by your idea and I think I will try to explore it.

[Kixunil]

Oh, interesting, I'm curious what made upstream change from those complex things to the simple implementation.

I am really tempted by your idea and I think I will try to explore it.

If we're going to do this we must make sure dynamically linking a library with different context layout is impossible or fall back to the current version if dynamic linking is enabled.

I was also thinking about providing const constructor but I doubt it's a good idea.

[apoelstra]

Oh, interesting, I'm curious what made upstream change from those complex things to the simple implementation.

They moved to a model where they precompute tables at compile-time rather than run-time. (And you can configure their size at compile-time; I think the lowmemory feature in this lib does that.)

This does eliminate the case where somebody might have limited flash but lots of RAM, but it dramatically simplifies the context object management (and in fact, eliminates the need for any context object at all, in much of the library, strictly speaking).

[apoelstra]

If we're going to do this we must make sure dynamically linking a library with different context layout is impossible or fall back to the current version if dynamic linking is enabled.

Because upstream libsecp256k1 has no released versions this is already pretty-much guaranteed UB to try to dynamically link whatever random shit somebody might have in their /usr/lib. I don't know if we can make it impossible (we should explore that) but we're not making it any less safe by changing our allocation strategy.

I was also thinking about providing const constructor but I doubt it's a good idea.

The context object now only contains aux data for rerandomization, which is only used by signing code. I think a const constructor would be fine.

[Kixunil]

and in fact, eliminates the need for any context object at all, in much of the library, strictly speaking

There are still places I find surprising that they need it. For instance tweak_add_assign is annoying me lately. I suspect tweaks are sometimes private but then maybe it'd make sense to have a non-randomized version for non-private tweaks with all the appropriate warnings?

[apoelstra]

There are still places I find surprising that they need it.

All the API functions still "need" it, as a form of future-proofing. tweak_add_assign definitely does not assume the tweak is secret -- it is not even constant-time -- and does not actually require a context object anymore. (You can see in our docs that we require a C: Verification context. Only things with the C: Signing bound actually need a context object.)

This library is in an awkward state right now because upstream has de-facto eliminated the need for context objects in all non-signing contexts, but kept context arguments in all the APIs in case they want to use them one day in the future. We have not decided if we want to follow, or if we should just eliminate the context objects from our APIs. Separately but relatedly, we haven't decided what the best approach to signing contexts is, which is the subject of #346 .

[Kixunil]

Ah, so theoretically we could pass null or dangling pointers into those APIs? If yes that'd make const for Verification much easier.

we haven't decided what the best approach to signing contexts is

Didn't we conclude to use TLS with a fallback to a "lock" that doesn't re-randomize on contention?

[apoelstra]

No, the code does check that they're non-null (and may even dereference them). But we can pass a static context or even generate one on-the-fly with no capabilities (which will be essentially free, especially if we do it on the stack).

Didn't we conclude to use TLS with a fallback to a "lock" that doesn't re-randomize on contention?

Yeah, I guess so :) so maybe we can start moving forward.

[Kixunil]

with no capabilities

Wouldn't it break if the library changes to require verification capability?

FTR, the context size is now 208B on x86_64 which shouldn't cause problems.

[apoelstra]

Wouldn't it break if the library changes to require verification capability?

Yeah, but in that case we'll figure something out. It will only break when we upgrade the copy of the library that we vendor.

FTR, the context size is now 208B on x86_64 which shouldn't cause problems.

Nice. Though the actual contents include function pointers so I'm doubtful we'll be able to make a const contsructor for it.

[Kixunil]

With all these changes I wonder if we should fail compilation if someone tries to link /usr/lib/({architecture}/)?secp256k1.so since it's becoming even more dangerous.

the actual contents include function pointers

Sucks but since we already make a bunch of assumptions maybe it wouldn't be too bad to just mirror the C struct and fill it from Rust?

[apoelstra]

it's becoming even more dangerous

I don't think anything we're discussing makes this more dangerous, but regardless, I think we might actually be safe because we rename all the symbols that we use.

Sucks but since we already make a bunch of assumptions maybe it wouldn't be too bad to just mirror the C struct and fill it from Rust?

If we can mirror function pointers (I'm never sure what the FFI supports) then yeah, for sure we can do this.

[Kixunil]

we rename all the symbols that we use.

We have a feature to not rename them 🤷‍♂️ Maybe it should be renamed to say dangerous_no_rename_symbols?

If we can mirror function pointers (I'm never sure what the FFI supports)

We can, if we couldn't, the library would already not work. :)

[apoelstra]

We have a feature to not rename them  Maybe it should be renamed to say dangerous_no_rename_symbols?

IMO this is way overkill. It's not even a feature, it's a cfg flag you need to modify RUSTFLAGS to set, whose documented behavior is to fuck with the linker in ways that affect the FFI boundary. I think it's already high-friction and obviously dangerous.

We can, if we couldn't, the library would already not work. :)

Hah, good point.

[apoelstra]

BTW I opened #538 so we can move discussion to there (or perhaps we should have yet another issue specifically about trying to prevent people from replacing our vendored library with a different incompatible one)

[Kixunil]

obviously dangerous

Honestly, it wasn't to me. I didn't even know libsecp256k1 is unstable. And I don't think I'm particularly dumb when it comes to these things. :) It needs at least a visible warning somewhere where people will see it.

[apoelstra]

For sure -- wherever we document that cfg flag we should add a paragraph explaining why somebody might do this and why it's probably a bad idea, and what compatibility requirements they need ta uphold.