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.

Sign up for GitHub

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

Rust-based Dialect api. #109

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Rust-based Dialect api. #109

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ca503f6
First crack at `Dialect`.
richardkiss Aug 21, 2021
2fa09f5
Use `Dialect` where possible.
richardkiss Aug 23, 2021
0eed276
Refactor
richardkiss Aug 24, 2021
8fc7222
Make `pre_eval` non-optional.
richardkiss Aug 25, 2021
e3df5c1
Rollback `pub`.
richardkiss Aug 25, 2021
15e0c15
Make `RunProgramContext` private.
richardkiss Aug 26, 2021
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
95 changes: 95 additions & 0 deletions src/chia_dialect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
use std::collections::HashMap;

use crate::allocator::{Allocator, NodePtr};
use crate::cost::Cost;
use crate::dialect::Dialect;
use crate::err_utils::err;
use crate::f_table::{f_lookup_for_hashmap, FLookup};
use crate::more_ops::op_unknown;
use crate::reduction::Response;
use crate::run_program::OperatorHandler;

const QUOTE_KW: [u8; 1] = [1];
const APPLY_KW: [u8; 1] = [2];

pub struct OperatorHandlerWithMode {
Copy link
Contributor

Choose a reason for hiding this comment

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

this doesn't look like it belongs in the chia dialect. The chia dialect is known and doesn't need to be determined at run-time. It doesn't need to keep a run-time lookup-table. In fact, the run-time lookup-table seems like it belongs in the python binding, as it's a trick to allow mixing python functions with rust functions. In the rust API, there's no need for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The lowest-level run_program implementation currently expects a thing with the OperatorHandler trait, which defines the opcode-to-functionality map. Except for strict, this is pretty much the simplest implementation of that (FLookup is just an array of 256 optional function pointers).

I can move this to its own file.

Copy link
Contributor

Choose a reason for hiding this comment

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

not as simple as this, which is stateless: https://github.com/Chia-Network/clvm_rs/pull/107/files#diff-4f84c6fa87ad47416016af253147484a8eef4a052dd794d4fd144db78865a172R33

f_lookup: FLookup,
strict: bool,
}

impl OperatorHandlerWithMode {
pub fn new_with_hashmap(hashmap: HashMap<String, Vec<u8>>, strict: bool) -> Self {
let f_lookup: FLookup = f_lookup_for_hashmap(hashmap);
OperatorHandlerWithMode { f_lookup, strict }
}
}

impl OperatorHandler for OperatorHandlerWithMode {
fn op(
&self,
allocator: &mut Allocator,
o: NodePtr,
argument_list: NodePtr,
max_cost: Cost,
) -> Response {
let b = &allocator.atom(o);
if b.len() == 1 {
if let Some(f) = self.f_lookup[b[0] as usize] {
return f(allocator, argument_list, max_cost);
}
}
if self.strict {
err(o, "unimplemented operator")
} else {
op_unknown(allocator, o, argument_list, max_cost)
}
}
}

pub fn chia_opcode_mapping() -> HashMap<String, Vec<u8>> {
let mut h = HashMap::new();
let items = [
(3, "op_if"),
(4, "op_cons"),
(5, "op_first"),
(6, "op_rest"),
(7, "op_list"),
(8, "op_raise"),
(9, "op_eq"),
(10, "op_gr_bytes"),
(11, "op_sha256"),
(12, "op_substr"),
(13, "op_strlen"),
(14, "op_concat"),
(16, "op_add"),
(17, "op_subtract"),
(18, "op_multiply"),
(19, "op_divide"),
(20, "op_divmod"),
(21, "op_greater"),
(22, "op_ash"),
(23, "op_lsh"),
(24, "op_logand"),
(25, "op_logior"),
(26, "op_logxor"),
(27, "op_lognot"),
(29, "op_point_add"),
(30, "op_pubkey_for_exp"),
(32, "op_not"),
(33, "op_any"),
(34, "op_all"),
(36, "op_softfork"),
];
for (k, v) in items {
h.insert(v.to_string(), [k as u8].into());
}
h
}

pub fn chia_op_handler(strict: bool) -> OperatorHandlerWithMode {
OperatorHandlerWithMode::new_with_hashmap(chia_opcode_mapping(), strict)
}

pub fn chia_dialect(strict: bool) -> Dialect {
Copy link
Contributor

Choose a reason for hiding this comment

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

as far as I can tell, this is the only function that needs to be public in this file.

Dialect::new(&QUOTE_KW, &APPLY_KW, Box::new(chia_op_handler(strict)))
}
71 changes: 71 additions & 0 deletions src/dialect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
use crate::allocator::{Allocator, NodePtr};
use crate::cost::Cost;
use crate::reduction::Response;
use crate::run_program::{run_program, OperatorHandler, PreEval};

pub struct Dialect {
quote_kw: Vec<u8>,
apply_kw: Vec<u8>,
op_handler: Box<dyn OperatorHandler>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see why this needs to be a run-time interface instead of a compile-time trait, not why it needs to be heap allocated. In the chia dialect case this class only contains one bool of state (strict mode).

Dialect seems like a perfect candidate for being a trait rather than a concrete type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could make a dialect trait, but we'd still need at least one concrete instance.

Changing to op_handler: &dyn OperatorHandler would mean having to keep around both a Dialect and the concrete OperatorHandler.

Copy link
Contributor

Choose a reason for hiding this comment

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

what I'm suggesting is that you can just rename the OperatorHandler trait to Dialect and add the two quote_kw and apply_kw to it. Then the python binding can have a concrete implementation of this trait, which contains a run-time lookup table, and the chia_rs library could have its own concrete type that doesn't, but that has a static operator -> function mapping.

}

impl OperatorHandler for Dialect {
fn op(
&self,
allocator: &mut Allocator,
op: NodePtr,
args: NodePtr,
max_cost: Cost,
) -> Response {
self.op_handler.op(allocator, op, args, max_cost)
Copy link
Contributor

Choose a reason for hiding this comment

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

instead of forwarding this call, could the Dialect trait and OperatorHandler traits just be merged? They seem simple enough and related enough that it could just be one Dialect trait

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure that'd be possible, but I don't immediately see that it would be any more efficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

it would primarily be simpler, because you have a single interface instead of two. As far as I can tell, you'd never mix-n-match the OperatorHandler and the Dialect anyway. It would be more efficient to allow the rust API to specify a static/compile-time operator -> function mapping. It allows the optimizer to see through those calls in the LTO step.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Dialect represents the complete opcode-to-functionality map, so it really is just an OperatorHandler plus the q and a values. Making it a trait would mean we add a function to get the q and a values from the Dialect instead of just having publicly accessible fields.

I haven't benchmarked it, but I'd be surprised to see the FLookup (it's a [Option<OpFn>; 256]) implementation being slower than the let f = match b[0] { 3 => op_if, ... implementation in your PR. I'd think they both produce a look-up table, so O(1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think the LTO would be able to do anything without further changes to code here https://github.com/Chia-Network/clvm_rs/blob/main/src/run_program.rs#L299

Copy link
Contributor

Choose a reason for hiding this comment

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

Making it a trait would mean we add a function to get the q and a values from the Dialect instead of just having publicly accessible fields.

I don't think you have to do it that way. Surely a trait can declare names of constants. These opcodes is known at compile time, so they don't need to be stored in variables. Functions would allow an implementation to keep them as run-time values though, but a chia_rs library wouldn't need that.

I haven't benchmarked it, but I'd be surprised to see the FLookup (it's a [Option; 256]) implementation being slower than the let f = match b[0] { 3 => op_if, ... implementation in your PR. I'd think they both produce a look-up table, so O(1).

I would expect the our operator functions are so slow that they would dwarf the operator lookup, but once we make those faster I wouldn't be surprised this might be next. I would expect the branch prediction benefits of static calls, rather than indirect calls.

I don't see the value in requiring to keep all this state in memory. As far as I can tell, it's more code and complexity.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think the LTO would be able to do anything without further changes to code here https://github.com/Chia-Network/clvm_rs/blob/main/src/run_program.rs#L299

right, that shouldn't be a dynamic interface

}
}

impl Dialect {
pub fn new(quote_kw: &[u8], apply_kw: &[u8], op_handler: Box<dyn OperatorHandler>) -> Self {
Dialect {
quote_kw: quote_kw.to_owned(),
apply_kw: apply_kw.to_owned(),
op_handler,
}
}

pub fn run_program_with_pre_eval(
&self,
allocator: &mut Allocator,
program: NodePtr,
args: NodePtr,
max_cost: Cost,
pre_eval: PreEval,
) -> Response {
run_program(
allocator,
program,
args,
&self.quote_kw,
&self.apply_kw,
max_cost,
self,
Some(pre_eval),
)
}

pub fn run_program(
&self,
allocator: &mut Allocator,
program: NodePtr,
args: NodePtr,
max_cost: Cost,
) -> Response {
run_program(
allocator,
program,
args,
&self.quote_kw,
&self.apply_kw,
max_cost,
self,
None,
)
}
}
2 changes: 2 additions & 0 deletions src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
mod allocator;
pub mod chia_dialect;
mod core_ops;
mod cost;
mod dialect;
mod err_utils;
mod f_table;
mod gen;
Expand Down
28 changes: 13 additions & 15 deletions src/py/run_generator.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,18 @@
use crate::allocator::{Allocator, NodePtr};
use crate::chia_dialect::OperatorHandlerWithMode;
use crate::cost::Cost;
use crate::dialect::Dialect;
use crate::gen::conditions::{parse_spends, Condition, SpendConditionSummary};
use crate::gen::opcodes::{
ConditionOpcode, AGG_SIG_ME, AGG_SIG_UNSAFE, ASSERT_HEIGHT_ABSOLUTE, ASSERT_HEIGHT_RELATIVE,
ASSERT_SECONDS_ABSOLUTE, ASSERT_SECONDS_RELATIVE, CREATE_COIN, RESERVE_FEE,
};
use crate::gen::validation_error::{ErrorCode, ValidationErr};
use crate::int_to_bytes::u64_to_bytes;
use crate::py::run_program::OperatorHandlerWithMode;
use crate::reduction::{EvalErr, Reduction};
use crate::run_program::{run_program, STRICT_MODE};
use crate::run_program::STRICT_MODE;
use crate::serialize::node_from_bytes;

use crate::f_table::f_lookup_for_hashmap;
use crate::py::lazy_node::LazyNode;

use std::collections::HashMap;
Expand Down Expand Up @@ -215,24 +215,22 @@ pub fn run_generator(
flags: u32,
) -> PyResult<(Option<ErrorCode>, Vec<PySpendConditionSummary>, Cost)> {
let mut allocator = Allocator::new();
let f_lookup = f_lookup_for_hashmap(opcode_lookup_by_name);
let strict: bool = (flags & STRICT_MODE) != 0;
let f = OperatorHandlerWithMode::new(f_lookup, strict);
let program = node_from_bytes(&mut allocator, program)?;
let args = node_from_bytes(&mut allocator, args)?;
let dialect = Dialect::new(
&[quote_kw],
&[apply_kw],
Box::new(OperatorHandlerWithMode::new_with_hashmap(
opcode_lookup_by_name,
strict,
)),
);

let r = py.allow_threads(
|| -> Result<(Option<ErrorCode>, Cost, Vec<SpendConditionSummary>), EvalErr> {
let Reduction(cost, node) = run_program(
&mut allocator,
program,
args,
&[quote_kw],
&[apply_kw],
max_cost,
&f,
None,
)?;
let Reduction(cost, node) =
dialect.run_program(&mut allocator, program, args, max_cost)?;
// we pass in what's left of max_cost here, to fail early in case the
// cost of a condition brings us over the cost limit
match parse_spends(&allocator, node, max_cost - cost, flags) {
Expand Down
124 changes: 49 additions & 75 deletions src/py/run_program.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,54 +1,44 @@
use std::collections::HashMap;
use std::rc::Rc;

use crate::allocator::{Allocator, NodePtr};
use crate::allocator::Allocator;
use crate::chia_dialect::OperatorHandlerWithMode;
use crate::cost::Cost;
use crate::err_utils::err;
use crate::f_table::{f_lookup_for_hashmap, FLookup};
use crate::more_ops::op_unknown;
use crate::dialect::Dialect;
use crate::node::Node;
use crate::py::lazy_node::LazyNode;
use crate::reduction::Response;
use crate::run_program::{run_program, OperatorHandler, STRICT_MODE};
use crate::run_program::STRICT_MODE;
use crate::serialize::{node_from_bytes, node_to_bytes, serialized_length_from_bytes};

use pyo3::prelude::*;
use pyo3::types::{PyBytes, PyDict};

pub struct OperatorHandlerWithMode {
f_lookup: FLookup,
strict: bool,
}

impl OperatorHandlerWithMode {
pub fn new(l: FLookup, strict: bool) -> OperatorHandlerWithMode {
OperatorHandlerWithMode {
f_lookup: l,
#[allow(clippy::too_many_arguments)]
pub fn run_serialized_program(
py: Python,
allocator: &mut Allocator,
opcode_lookup_by_name: HashMap<String, Vec<u8>>,
quote_kw: &[u8],
apply_kw: &[u8],
program: &[u8],
args: &[u8],
max_cost: Cost,
flags: u32,
) -> PyResult<Response> {
let strict: bool = (flags & STRICT_MODE) != 0;
let program = node_from_bytes(allocator, program)?;
let args = node_from_bytes(allocator, args)?;
let dialect = Dialect::new(
quote_kw,
apply_kw,
Box::new(OperatorHandlerWithMode::new_with_hashmap(
opcode_lookup_by_name,
strict,
}
}
}
)),
);

impl OperatorHandler for OperatorHandlerWithMode {
fn op(
&self,
allocator: &mut Allocator,
o: NodePtr,
argument_list: NodePtr,
max_cost: Cost,
) -> Response {
let b = &allocator.atom(o);
if b.len() == 1 {
if let Some(f) = self.f_lookup[b[0] as usize] {
return f(allocator, argument_list, max_cost);
}
}
if self.strict {
err(o, "unimplemented operator")
} else {
op_unknown(allocator, o, argument_list, max_cost)
}
}
Ok(py.allow_threads(|| dialect.run_program(allocator, program, args, max_cost)))
}

#[pyfunction]
Expand Down Expand Up @@ -125,25 +115,17 @@ pub fn deserialize_and_run_program(
flags: u32,
) -> PyResult<(Cost, Py<PyBytes>)> {
let mut allocator = Allocator::new();
let f_lookup = f_lookup_for_hashmap(opcode_lookup_by_name);
let strict: bool = (flags & STRICT_MODE) != 0;
let f = OperatorHandlerWithMode { f_lookup, strict };
let program = node_from_bytes(&mut allocator, program)?;
let args = node_from_bytes(&mut allocator, args)?;

let r = py.allow_threads(|| {
run_program(
&mut allocator,
program,
args,
&[quote_kw],
&[apply_kw],
max_cost,
&f,
None,
)
});
match r {
match run_serialized_program(
py,
&mut allocator,
opcode_lookup_by_name,
&[quote_kw],
&[apply_kw],
program,
args,
max_cost,
flags,
)? {
Ok(reduction) => {
let node_as_blob = node_to_bytes(&Node::new(&allocator, reduction.1))?;
let node_as_bytes: Py<PyBytes> = PyBytes::new(py, &node_as_blob).into();
Expand Down Expand Up @@ -192,25 +174,17 @@ pub fn deserialize_and_run_program2(
flags: u32,
) -> PyResult<(Cost, LazyNode)> {
let mut allocator = Allocator::new();
let f_lookup = f_lookup_for_hashmap(opcode_lookup_by_name);
let strict: bool = (flags & STRICT_MODE) != 0;
let f = OperatorHandlerWithMode { f_lookup, strict };
let program = node_from_bytes(&mut allocator, program)?;
let args = node_from_bytes(&mut allocator, args)?;

let r = py.allow_threads(|| {
run_program(
&mut allocator,
program,
args,
&[quote_kw],
&[apply_kw],
max_cost,
&f,
None,
)
});
match r {
match run_serialized_program(
py,
&mut allocator,
opcode_lookup_by_name,
&[quote_kw],
&[apply_kw],
program,
args,
max_cost,
flags,
)? {
Ok(reduction) => {
let val = LazyNode::new(Rc::new(allocator), reduction.1);
Ok((reduction.0, val))
Expand Down
Loading