Skip to content

Refactor exec_jec #207

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

Merged
merged 4 commits into from
Feb 27, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 14 additions & 4 deletions src/bit_machine/frame.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ pub(super) struct Frame {
/// Current position of the cursor.
/// For read frames, this is the next bit which is to be read.
/// For write frames, this is the next bit which is to be (over)written.
pub(super) cursor: usize,
cursor: usize,
/// Start index of this frame in the referenced data.
pub(super) start: usize,
/// The total length of this frame.
pub(super) len: usize,
start: usize,
/// The total bit length of this frame.
len: usize,
}

impl Frame {
Expand All @@ -33,6 +33,16 @@ impl Frame {
}
}

/// Return the start index of the frame inside the referenced data.
pub fn start(&self) -> usize {
self.start
}

/// Return the bit width of the frame.
pub fn bit_width(&self) -> usize {
self.len
}

/// Reset the cursor to the start.
pub(super) fn reset_cursor(&mut self) {
self.cursor = self.start;
Expand Down
162 changes: 107 additions & 55 deletions src/bit_machine/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,16 @@

mod frame;

use std::fmt;
use std::sync::Arc;
use std::{cmp, error};

use crate::analysis;
use crate::dag::{DagLike, NoSharing};
use crate::jet::{Jet, JetFailed};
use crate::node::{self, RedeemNode};
use crate::{Cmr, FailEntropy, Value};
use frame::Frame;
use std::fmt;
use std::sync::Arc;
use std::{cmp, error};

/// An execution context for a Simplicity program
pub struct BitMachine {
Expand Down Expand Up @@ -85,8 +86,8 @@ impl BitMachine {
/// Drop the active read frame
fn drop_frame(&mut self) {
let active_read_frame = self.read.pop().unwrap();
self.next_frame_start -= active_read_frame.len;
assert_eq!(self.next_frame_start, active_read_frame.start);
self.next_frame_start -= active_read_frame.bit_width();
assert_eq!(self.next_frame_start, active_read_frame.start());
}

/// Write a single bit to the active write frame
Expand Down Expand Up @@ -177,6 +178,19 @@ impl BitMachine {
}
}

/// Return the bit width of the active read frame.
fn active_read_bit_width(&self) -> usize {
self.read.last().map(|frame| frame.bit_width()).unwrap_or(0)
}

/// Return the bit width of the active write frame.
fn active_write_bit_width(&self) -> usize {
self.write
.last()
.map(|frame| frame.bit_width())
.unwrap_or(0)
}

/// Add a read frame with some given value in it, as input to the
/// program
pub fn input(&mut self, input: &Value) {
Expand Down Expand Up @@ -366,66 +380,104 @@ impl BitMachine {
}

fn exec_jet<J: Jet>(&mut self, jet: J, env: &J::Environment) -> Result<(), JetFailed> {
use simplicity_sys::c_jets::frame_ffi::{c_readBit, c_writeBit, CFrameItem};
use simplicity_sys::c_jets::uword_width;
use simplicity_sys::ffi::UWORD;
use crate::ffi::c_jets::frame_ffi::{c_readBit, c_writeBit, CFrameItem};
use crate::ffi::c_jets::uword_width;
use crate::ffi::ffi::UWORD;

/// Create new C read frame that contains `bit_width` many bits from active read frame.
///
/// Return C read frame together with underlying buffer.
///
/// ## Safety
///
/// The returned frame must outlive its buffer or there is a dangling pointer.
///
/// ## Panics
///
/// Active read frame has fewer bits than `bit_width`.
unsafe fn get_input_frame(
Copy link
Collaborator

Choose a reason for hiding this comment

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

In b561354:

Ok, awesome, this is looking much cleaner.

Should add a ## Safety section to the doc simply saying that it returns a CFrameItem which points into the returned Vec and that the Vec must outlive the CFrameItem.

Alternately we could create a new opaque type struct BackedCFrameItem which holds the Vec, implements Deref<CFrameItem>, and which would then ensure that you were obeying the lifetime rules. I think that if you did this, you could eliminate unsafe from this method.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, will do. I haven't been working much with unsafe, so these comments on how to write unsafe Rust (somewhat) safely are appreciated :)

mac: &mut BitMachine,
bit_width: usize,
) -> (CFrameItem, Vec<UWORD>) {
assert!(bit_width <= mac.active_read_bit_width());
let uword_width = uword_width(bit_width);
let mut buffer = vec![0; uword_width];

// Copy bits from active read frame into input frame
let buffer_end = buffer.as_mut_ptr().add(uword_width);
let mut write_frame = CFrameItem::new_write(bit_width, buffer_end);
for _ in 0..bit_width {
let bit = mac.read_bit();
c_writeBit(&mut write_frame, bit);
}
mac.back(bit_width);

// Sanity Check: This should never really fail, but still good to do
if !simplicity_sys::c_jets::sanity_checks() {
return Err(JetFailed);
// Convert input frame into read frame
let buffer_ptr = buffer.as_mut_ptr();
let read_frame = CFrameItem::new_read(bit_width, buffer_ptr);

(read_frame, buffer)
}
let src_ty_bit_width = jet.source_ty().to_bit_width();
let target_ty_bit_width = jet.target_ty().to_bit_width();

let a_frame_size = uword_width(src_ty_bit_width);
let b_frame_size = uword_width(target_ty_bit_width);
// a_frame_size + b_frame_size must be non-zero unless it is a unit to unit jet
if a_frame_size == 0 && b_frame_size == 0 {
return Ok(());

/// Create C write frame that is as wide as `bit_width`.
///
/// Return C write frame together with underlying buffer.
///
/// ## Safety
///
/// The returned frame must outlive its buffer or there is a dangling pointer.
unsafe fn get_output_frame(bit_width: usize) -> (CFrameItem, Vec<UWORD>) {
let uword_width = uword_width(bit_width);
let mut buffer = vec![0; uword_width];

// Return output frame as write frame
let buffer_end = buffer.as_mut_ptr().add(uword_width);
let write_frame = CFrameItem::new_write(bit_width, buffer_end);

(write_frame, buffer)
}
let mut src_buf = vec![0 as UWORD; a_frame_size + b_frame_size];
let src_ptr_end = unsafe { src_buf.as_mut_ptr().add(a_frame_size) }; // A frame write
let src_ptr = src_buf.as_mut_ptr(); // A read frame at ptr start
let dst_ptr_begin = unsafe { src_buf.as_mut_ptr().add(a_frame_size) }; // B read frame at ptr begin
let dst_ptr_end = unsafe { src_buf.as_mut_ptr().add(a_frame_size + b_frame_size) }; // B write frame at ptr end

// For jet from type A -> B
// Jets execution: There is single buffer with a_frame_size + b_frame_size UWORDs
// ------[ A read frame ][ B write frame ]---
// ^ src_ptr ^src_ptr_end(dst_ptr_begin) ^ dst_ptr_end
// 1. Write into C bitmachine using A write frame(= src_ptr_end)
// Precondition satisfied: src_ptr_end is one past the end of slice of UWORDs for A.
let mut a_frame = unsafe { CFrameItem::new_write(src_ty_bit_width, src_ptr_end) };
for _ in 0..src_ty_bit_width {
let bit = self.read_bit();
unsafe {
c_writeBit(&mut a_frame, bit);

/// Write `bit_width` many bits from `buffer` into active write frame.
///
/// ## Panics
///
/// Active write frame has fewer bits than `bit_width`.
///
/// Buffer has fewer than bits than `bit_width` (converted to UWORDs).
fn update_active_write_frame(mac: &mut BitMachine, bit_width: usize, buffer: &[UWORD]) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

In b561354:

Here you still need unsafe because the passed buffer might now be wide enough for your c_readBit calls. I think you can add another assert! that buffer.len() is the right length to avoid this unsafety.

assert!(bit_width <= mac.active_write_bit_width());
assert!(uword_width(bit_width) <= buffer.len());
let buffer_ptr = buffer.as_ptr();
let mut read_frame = unsafe { CFrameItem::new_read(bit_width, buffer_ptr) };

for _ in 0..bit_width {
let bit = unsafe { c_readBit(&mut read_frame) };
mac.write_bit(bit);
}
}
self.back(src_ty_bit_width);

// 2. Execute the jet. src = A read frame, dst = B write frame
// Precondition satisfied: src_ptr is the start of slice of UWORDs of A.
let src_frame = unsafe { CFrameItem::new_read(src_ty_bit_width, src_ptr) };
// Precondition satisfied: dst_ptr_end is one past the end of slice of UWORDs of B.
let mut dst_frame = unsafe { CFrameItem::new_write(target_ty_bit_width, dst_ptr_end) };
let jet_fn = jet.c_jet_ptr();
let c_env = jet.c_jet_env(env);
let res = jet_fn(&mut dst_frame, src_frame, c_env);

if !res {
// Sanity Check: This should never really fail, but still good to do
if !simplicity_sys::c_jets::sanity_checks() {
return Err(JetFailed);
}

// 3. Read the result from B read frame
// Precondition satisfied: dst_ptr_begin is the start of slice of UWORDs of B.
let mut b_frame = unsafe { CFrameItem::new_read(target_ty_bit_width, dst_ptr_begin) };
// Read the value from b_frame
for _ in 0..target_ty_bit_width {
let bit = unsafe { c_readBit(&mut b_frame) };
self.write_bit(bit);
let input_width = jet.source_ty().to_bit_width();
let output_width = jet.target_ty().to_bit_width();
// Input buffer is implicitly referenced by input read frame!
// Same goes for output buffer
let (input_read_frame, _input_buffer) = unsafe { get_input_frame(self, input_width) };
let (mut output_write_frame, output_buffer) = unsafe { get_output_frame(output_width) };

let jet_fn = jet.c_jet_ptr();
let c_env = jet.c_jet_env(env);
let success = jet_fn(&mut output_write_frame, input_read_frame, c_env);

if !success {
Err(JetFailed)
} else {
update_active_write_frame(self, output_width, &output_buffer);
Ok(())
}
Ok(())
}
}

Expand Down
9 changes: 0 additions & 9 deletions src/jet/elements/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,15 +36,6 @@ impl From<elements::TxOut> for ElementsUtxo {
/// # Note
/// The order of `utxos` must be same as of the order of inputs in the
/// transaction.
// FIXME:
// Ideally the `Arc<elements::Transaction>` would be a generic
// `AsRef<elements::Transaction>` or something, but this is an associated type
// for the `Extension` trait, and Rust will not let us have generic parameters
// on associated types. (We could possibly add a parameter to the Extension
// trait itself, but that would be messy and layer-violating.)
//
// Similar story if we tried to use a &'a elements::Transaction rather than
// an Arc: we'd have a lifetime parameter <'a> that would cause us trouble.
#[allow(dead_code)]
pub struct ElementsEnv<T: Deref<Target = elements::Transaction>> {
/// The CTxEnv struct
Expand Down
2 changes: 1 addition & 1 deletion src/value.rs
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,7 @@ impl Value {

/// Encode value as big-endian byte string.
/// Fails if underlying bit string has length not divisible by 8
pub fn try_to_bytes(&self) -> Result<Vec<u8>, &str> {
pub fn try_to_bytes(&self) -> Result<Vec<u8>, &'static str> {
let (bytes, bit_length) = self.to_bytes_len();

if bit_length % 8 == 0 {
Expand Down