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

Add a static immutable zero aligned type #345

Merged
merged 2 commits into from Jan 2, 2022

Conversation

junderw
Copy link
Contributor

@junderw junderw commented Dec 4, 2021

The zeroed fn can not be used in static assignments.

In environments where it is no_std and no allocator are present, the only way to get a slice of AlignedTypes is dynamically, so preallocated_gen_new can't be used.

By offering this as a static, it can be used in static assignments as such:

#[cfg(target_pointer_width = "32")]
static mut CONTEXT_BUFFER: [AlignedType; 69645] = [ZERO_ALIGNED; 69645];
#[cfg(target_pointer_width = "64")]
static mut CONTEXT_BUFFER: [AlignedType; 69646] = [ZERO_ALIGNED; 69646];
static mut SECP256K1: Option<Secp256k1<AllPreallocated>> = None;

pub fn get_context(seed: Option<&[u8; 32]>) -> &'static Secp256k1<AllPreallocated<'static>> {
    unsafe {
        if SECP256K1.is_none() {
            SECP256K1 = Some(
                Secp256k1::preallocated_gen_new(&mut CONTEXT_BUFFER)
                    .expect("CONTEXT_BUFFER size is wrong"),
            );
        }
        if let Some(seed) = seed {
            SECP256K1.as_mut().unwrap().seeded_randomize(seed);
        }
        SECP256K1.as_ref().unwrap()
    }
}

@junderw
Copy link
Contributor Author

junderw commented Dec 4, 2021

Alternatively, adding more options to the global context so that it can be obtained with no_std and without an allocator by offering CONTEXT_BUFFER sizes for each alignment like I've shown above.

But seeing as the handling of context in this library looks to change in the near future, this is a minimal change that would simplify things a bit.

@elichai
Copy link
Member

elichai commented Dec 5, 2021

After #331 we can just make zeroed() a const fn and then you could use it in a static initialization

@junderw
Copy link
Contributor Author

junderw commented Dec 6, 2021

For now, this isn't very pretty, but it seems to be working.

I am looking forward to #331

// These would be marked for various pointer widths. removed for brevity
const ALIGN_SIZE: usize = 69_646;
static mut CONTEXT_BUFFER: [u8; ALIGN_SIZE * 16] = [0_u8; ALIGN_SIZE * 16];

let aligned = CONTEXT_BUFFER
    .as_mut_ptr()
    .cast::<[AlignedType; ALIGN_SIZE]>()
    .as_mut()
    .unwrap();

@elichai
Copy link
Member

elichai commented Dec 14, 2021

For now, this isn't very pretty, but it seems to be working.

I am looking forward to #331

// These would be marked for various pointer widths. removed for brevity
const ALIGN_SIZE: usize = 69_646;
static mut CONTEXT_BUFFER: [u8; ALIGN_SIZE * 16] = [0_u8; ALIGN_SIZE * 16];

let aligned = CONTEXT_BUFFER
    .as_mut_ptr()
    .cast::<[AlignedType; ALIGN_SIZE]>()
    .as_mut()
    .unwrap();

That's actually wrong AFAICT, as a pointer to CONTEXT_BUFFER doesn't have to be as aligned as AlignedType and casting it to a mutable reference to AlignedType is UB.

there are a few things you can do, assuming you need this to be static memory and you use a recent compiler version then this should be safe:

const ALIGN_SIZE: usize = 69_646;
static mut CONTEXT_BUFFER: [AlignedType; ALIGN_SIZE] = unsafe {std::mem::transmute([0u8; ALIGN_SIZE*16])};

the transmute is a "copy" so it doesn't have alignment requirements.

If you're using a somewhat older compiler then you can do this:

const ALIGN_SIZE: usize = 69_646;
static mut CONTEXT_BUFFER: [u8; ALIGN_SIZE * 16 + 16] = [0_u8; ALIGN_SIZE * 16 + 16];

let (_, aligned, _) = CONTEXT_BUFFER.align_to::<AlignedType>();
assert!(aligned.len() >= ALIGN_SIZE);

and if even older you might have to check alignment yourself by casting to usize etc.

@junderw
Copy link
Contributor Author

junderw commented Dec 14, 2021

const ALIGN_SIZE: usize = 69_646;
static mut CONTEXT_BUFFER: [AlignedType; ALIGN_SIZE] = unsafe {std::mem::transmute([0u8; ALIGN_SIZE*16])};

This worked fine. Thanks!

@apoelstra
Copy link
Member

Where do we stand with this?

@junderw
Copy link
Contributor Author

junderw commented Dec 23, 2021

@elichai 's comment about making zeroed a const fn is probably the best course of action.

Though depending on the timeframe for the updates required to allow that, I would appreciate this being added, as it would mean it's available quicker.

@apoelstra
Copy link
Member

Ok, perfect. I think a new MSRV is probably a month or more away since it's all blocked on getting the Taproot stuff out first. So let's move forward with this.

@elichai what do you think? Good to merge this?

apoelstra
apoelstra previously approved these changes Dec 23, 2021
Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 9cf552e

@elichai
Copy link
Member

elichai commented Dec 23, 2021

@junderw can you make this an associated constant? ( so you could do AlignedType::ZERO )

@junderw
Copy link
Contributor Author

junderw commented Dec 23, 2021

done

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 5e6d0f1

@apoelstra apoelstra merged commit 6a89320 into rust-bitcoin:master Jan 2, 2022
@elichai
Copy link
Member

elichai commented Jan 3, 2022

Missed this notification, post-merge ACK 5e6d0f1

apoelstra added a commit that referenced this pull request Jan 5, 2022
…lly accessible

c50411f release secp256k1-sys 0.4.2; make new `ZERO` type publically accessible (Andrew Poelstra)

Pull request description:

  Exposes the new const object provided by #345

ACKs for top commit:
  elichai:
    ACK c50411f

Tree-SHA512: 42fce191b68a88811c339ff267dafbb616e765108f5b2e70514b5153f64ef5152f5704982ddc0b20ece5ad15da23927e18f9c78af2763ef971c0e3b9bbf490a5
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

Successfully merging this pull request may close these issues.

None yet

3 participants