Clean up once code by Kixunil · Pull Request #359 · rust-bitcoin/rust-secp256k1

Kixunil · 2022-01-05T17:46:37Z

This was originally intended to just change swap to compare_exchange
however other changes were needed too:

Load with Release ordering seemed wrong because if it loaded DONE
before while loop it wouldn't synchronize with the storing thread.
There's no point in repeatedly checking if the value is NONE so this
check was moved about while loop
Due to a mistake I wanted to inspect using miri but couldn't because
of FFI call. Separating the once implementation from consumer allowed
writing a test in pure Rust and inspect via miri.
The initializing code now skips loading the value again because it
already knows it's initialized.
Orderings are now explained in the comments

Closes #351

TheBlueMatt · 2022-01-05T19:48:02Z

        fn rustsecp256k1_v0_4_1_context_preallocated_clone(cx: *const Context, prealloc: *mut c_void) -> *mut Context;
    }

+    mod once {


This needs some kind of descriptive comment describing why its here, that it's only used in fuzzing, and that we don't really care too much...................all that said, maybe we should care a bit more, I realized yesterday that global-context-less-secure relies on std, even though the whole point of it is to avoid relying on std (it predates no-std so that's expected, but still). It only actually relies on std::sync::Once, so maybe we should use this there as well. Not sure how to get it across both crates, though, maybe just make a single file and symlink them across the two crates to avoid making it public?

In that case I think it should condinionaly use Once from std as suggested in #352

The whole point of the global-context-less-secure feature is to work on no-std, and sadly Once is not no-std as it requires thread-parking support.

Since cargo features are additive it's still useful to avoid performance issues if two crates turn on these features. Although maybe they should've been --cfg parameters, not features...

Oh, sorry, I misread your comment again lol. Yes, conditionally using Once if std is turned on is fine, but we still need to somehow have this once available in secp256k1 if we want to fix global-context-less-secure not building with no-std.

This was originally intended to just change `swap` to `compare_exchange` however other changes were needed too: * Load with `Release` ordering seemed wrong because if it loaded `DONE` before while loop it wouldn't synchronize with the storing thread. * There's no point in repeatedly checking if the value is `NONE` so this check was moved about while loop * Due to a mistake I wanted to inspect using `miri` but couldn't because of FFI call. Separating the once implementation from consumer allowed writing a test in pure Rust and inspect via `miri`. * The initializing code now skips loading the value again because it already knows it's initialized. * Orderings are now explained in the comments

This caches the initialization state of preallocated context in a thread local variable to avoid atomics in subsequent accesses.

Accessing `std::mem` instead of `core::mem` caused fuzzing to break without `std`.

Kixunil · 2022-01-05T21:03:59Z

I also fixed broken no_std fuzzing. Would've done in a separate PR but not worth conflict fixing I think.

This reverts commit 5cbf5ee.

TheBlueMatt · 2022-01-13T22:35:26Z

Are you planning on fixing global-context-less-secure + no-std here?

Kixunil · 2022-01-13T23:23:51Z

Oh, didn't notice it's broken. Is there an advantage of doing it here?

TheBlueMatt · 2022-01-14T06:00:10Z

I mentioned it in one of the discussions further up, but those were a mess, it's issue is use of std Once. I don't know if this PR is worth bothering unless we also bother refactoring it to make it available in both crates (I guess a symlink to a common module?) and fix global contact with it.

Kixunil · 2022-01-14T10:18:06Z

OK, will try to find the time to look into it.

tcharding

A few minor suggestions.

tcharding · 2022-03-08T03:18:13Z

+                if have_ctx == NONE {
+                    // Ordering: on success we're only signalling to other thread that work is in progress
+                    // without transferring other data, so it should be Relaxed, on failure the value may be DONE,
+                    // so we want to Acquire to safely proceed in that case.


Should this be 'so we want Acquire to safely proceed ...'?

tcharding · 2022-03-08T03:18:51Z

+                    // without transferring other data, so it should be Relaxed, on failure the value may be DONE,
+                    // so we want to Acquire to safely proceed in that case.
+                    // However compare_exchange doesn't allow failure ordering to be stronger than success
+                    // so both are Acquire.


nit, this whole comment might look better like this (includes change above):

// Ordering: on success we're only signalling to other thread that work is in // progress without transferring other data, so it should be Relaxed, on failure // the value may be DONE, so we want Acquire to safely proceed in that case. // However compare_exchange doesn't allow failure ordering to be stronger than // success so both are Acquire.

tcharding · 2022-03-08T03:20:14Z

+                    }
+                }
+                while have_ctx != DONE {
+                    // Another thread is building, just busy-loop until they're done.


This refers to the whole loop, right? Perhaps clearer if its outside the loop (above the while line).

tcharding · 2022-03-08T03:32:26Z

+            assert_eq!(rustsecp256k1_v0_4_1_context_preallocated_create(
+                    PREALLOCATED_CONTEXT[..].as_ptr() as *mut c_void,
+                    SECP256K1_START_SIGN | SECP256K1_START_VERIFY),
+                PREALLOCATED_CONTEXT[..].as_ptr() as *mut Context);


Can we assign this pointer to a local variable?

let ptr = PREALLOCATED_CONTEXT[..].as_ptr() as *mut c_void; assert_eq!(rustsecp256k1_v0_4_1_context_preallocated_create(ptr, SECP256K1_START_SIGN | SECP256K1_START_VERIFY), ptr);

They are different types. Which is strange. It'd still be shorter to cast one of them but would be better to look if we can unify them.

Oh dear, I missed the type difference :(

Kixunil · 2022-03-08T09:28:01Z

+                    // This thread will read whatever the other thread wrote so this needs to be Acquire.
+                    have_ctx = self.0.load(Ordering::Acquire);
+                    #[cfg(feature = "std")]
+                    ::std::thread::yield_now();


FTR yield_now is not great. Futex would be the best, cond var as a fallback.

Kixunil · 2022-03-08T09:31:36Z

+            assert_eq!(rustsecp256k1_v0_4_1_context_preallocated_create(
+                    PREALLOCATED_CONTEXT[..].as_ptr() as *mut c_void,
+                    SECP256K1_START_SIGN | SECP256K1_START_VERIFY),
+                PREALLOCATED_CONTEXT[..].as_ptr() as *mut Context);


They are different types. Which is strange. It'd still be shorter to cast one of them but would be better to look if we can unify them.

Kixunil · 2022-03-08T09:43:07Z

-    const HAVE_CONTEXT_WORKING: usize = 1;
-    const HAVE_CONTEXT_DONE: usize = 2;
+    static HAVE_PREALLOCATED_CONTEXT: once::Once = once::Once::INIT;
    static mut PREALLOCATED_CONTEXT: [u8; CTX_SIZE] = [0; CTX_SIZE];


Just realized this variable is very wrong due to alignment

That's what we have AlignedType for: https://docs.rs/secp256k1-sys/latest/secp256k1_sys/types/struct.AlignedType.html

Ah, so the fix should be [AlignedType; CTX_SIZE / mem::size_of::<AlignedType>()], right? Although maybe we should have a separate type for that.

Now looking at ffi::Context and it's an int. WTF?

idk what CTX_SIZE is and how was it calculated.
we can't have a type for that because we don't know the size of the context in compile time.
although everything here won't be needed when we start supporting the static contexts

we don't know the size of the context in compile time.

I think we can know the maximum and guard against it changing by calling secp256k1_context_preallocated_size

tcharding · 2022-11-21T20:11:07Z

Whats the status of this PR @Kixunil, are there more things to be done or would you like it re-reviewed as is?

Kixunil · 2022-11-21T23:25:56Z

Didn't have much time for it but meanwhile I came to conclusion that I really don't like spin locks for reasons that were mentioned elsewhere. I will probably still finish the PR to fix UB but then I want to make another one fixing the spin lock issue.

Kixunil force-pushed the once-compar-exchange branch from f32a5df to e19209d Compare January 5, 2022 18:05

TheBlueMatt reviewed Jan 5, 2022

View reviewed changes

Comment thread secp256k1-sys/src/lib.rs

TheBlueMatt reviewed Jan 5, 2022

View reviewed changes

Comment thread secp256k1-sys/src/lib.rs

TheBlueMatt reviewed Jan 5, 2022

View reviewed changes

Comment thread secp256k1-sys/src/lib.rs

Kixunil force-pushed the once-compar-exchange branch from e19209d to 36c96a9 Compare January 5, 2022 20:40

Kixunil added 2 commits January 5, 2022 21:50

Optimize loading of preallocated context

5cbf5ee

This caches the initialization state of preallocated context in a thread local variable to avoid atomics in subsequent accesses.

Fixed no_std fuzzing

beac386

Accessing `std::mem` instead of `core::mem` caused fuzzing to break without `std`.

TheBlueMatt reviewed Jan 5, 2022

View reviewed changes

Comment thread secp256k1-sys/src/lib.rs Outdated

Revert "Optimize loading of preallocated context"

51f398a

This reverts commit 5cbf5ee.

TheBlueMatt mentioned this pull request Jan 27, 2022

Enable build with global context less secure #387

Closed

tcharding mentioned this pull request Feb 1, 2022

Context randomization tracking issue #388

Closed

tcharding reviewed Mar 8, 2022

View reviewed changes

Kixunil commented Mar 8, 2022

View reviewed changes

Kixunil mentioned this pull request Jun 10, 2022

Document unsafe code #447

Closed

Conversation

Kixunil commented Jan 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kixunil commented Jan 5, 2022

Uh oh!

Uh oh!

TheBlueMatt commented Jan 13, 2022

Uh oh!

Kixunil commented Jan 13, 2022

Uh oh!

TheBlueMatt commented Jan 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kixunil commented Jan 14, 2022

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elichai Mar 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding commented Nov 21, 2022

Uh oh!

Kixunil commented Nov 21, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

TheBlueMatt commented Jan 14, 2022 •

edited

Loading

elichai Mar 8, 2022 •

edited

Loading