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

Jump to bottom

Potential fix for softfork guard issue #491

Closed
wants to merge 2 commits into from
Closed

Potential fix for softfork guard issue #491

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
fe356d6
Potential fix for softfork guard issue
Rigidity Nov 4, 2024
ad539f5
Get tests passing
Rigidity Nov 5, 2024
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
8 changes: 4 additions & 4 deletions src/chia_dialect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,8 @@ impl Dialect for ChiaDialect {
) -> Response {
let flags = self.flags
| match extension {
OperatorSet::Unknown => 0,
OperatorSet::Default => 0,
OperatorSet::BLS => 0,
OperatorSet::Keccak => ENABLE_KECCAK_OPS_OUTSIDE_GUARD,
};

Expand Down Expand Up @@ -186,10 +186,10 @@ impl Dialect for ChiaDialect {
match ext {
// The BLS extensions (and coinid) operators were brought into the
// main operator set as part of the hard fork
0 => OperatorSet::BLS,
1 if (self.flags & ENABLE_KECCAK) != 0 => OperatorSet::Keccak,
0 | 1 => OperatorSet::Default,
Copy link
Contributor

Choose a reason for hiding this comment

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

When I reviewed your first PR, I (for some reason) was thinking that the BLS softfork had id 0, so 1 was the next one. I realize I was wrong. Can you think of ways to prevent this mistake in the future?
There should probably be a comment here explaining what operator set 1 is (or was, prior to hard fork).
Maybe it even makes sense to preserve the OperatorSet::BLS enum value.

2 if (self.flags & ENABLE_KECCAK) != 0 => OperatorSet::Keccak,
// new extensions go here
_ => OperatorSet::Default,
_ => OperatorSet::Unknown,
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/dialect.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ use crate::reduction::Response;
#[derive(Clone, Copy, Eq, PartialEq)]
pub enum OperatorSet {
Default,
BLS,
Keccak, // keccak256 operator
Unknown,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make sense to, briefly, describe the issue we just introduced and how Default and Unknown differ and solve the problem.

}

pub trait Dialect {
Expand Down
69 changes: 45 additions & 24 deletions src/run_program.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,7 +336,7 @@ impl<'a, D: Dialect> RunProgramContext<'a, D> {
let extension =
self.dialect
.softfork_extension(uint_atom::<4>(self.allocator, extension, "softfork")? as u32);
if extension == OperatorSet::Default {
if extension == OperatorSet::Unknown {
Copy link
Contributor

Choose a reason for hiding this comment

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

this failure must depend on whether we're in mempool or not, right? Do we ignore the "unknown softfork extension" error we issue below, in the caller, if we're in consensus mode?

err(args, "unknown softfork extension")
} else {
Ok((extension, program, env))
Expand Down Expand Up @@ -1187,7 +1187,7 @@ mod tests {

// keccak256 is available when the softfork has activated
RunProgramTest {
prg: "(softfork (q . 1134) (q . 1) (q a (i (= (keccak256 (q . \"foobar\")) (q . 0x38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e)) (q . 0) (q x)) (q . ())) (q . ()))",
prg: "(softfork (q . 1134) (q . 2) (q a (i (= (keccak256 (q . \"foobar\")) (q . 0x38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e)) (q . 0) (q x)) (q . ())) (q . ()))",
args: "()",
flags: ENABLE_KECCAK,
result: Some("()"),
Expand All @@ -1196,7 +1196,7 @@ mod tests {
},
// make sure keccak is actually executed, by comparing with the wrong output
RunProgramTest {
prg: "(softfork (q . 1134) (q . 1) (q a (i (= (keccak256 (q . \"foobar\")) (q . 0x58d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e)) (q . 0) (q x)) (q . ())) (q . ()))",
prg: "(softfork (q . 1134) (q . 2) (q a (i (= (keccak256 (q . \"foobar\")) (q . 0x58d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e)) (q . 0) (q x)) (q . ())) (q . ()))",
args: "()",
flags: ENABLE_KECCAK,
result: None,
Expand All @@ -1205,13 +1205,22 @@ mod tests {
},
// keccak is ignored when the softfork has not activated
RunProgramTest {
prg: "(softfork (q . 1134) (q . 1) (q a (i (= (keccak256 (q . \"foobar\")) (q . 0x58d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e)) (q . 0) (q x)) (q . ())) (q . ()))",
prg: "(softfork (q . 1134) (q . 2) (q a (i (= (keccak256 (q . \"foobar\")) (q . 0x58d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e)) (q . 0) (q x)) (q . ())) (q . ()))",
args: "()",
flags: 0,
result: Some("()"),
cost: 1215,
err: "",
},
// keccak fails when the softfork has not activated and you're using a previous extension
RunProgramTest {
prg: "(softfork (q . 1134) (q . 1) (q a (i (= (keccak256 (q . \"foobar\")) (q . 0x38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e)) (q . 0) (q x)) (q . ())) (q . ()))",
args: "()",
flags: 0,
result: None,
cost: 1215,
err: "clvm raise",
},

// === HARD FORK ===
// new operators *outside* the softfork guard
Expand Down Expand Up @@ -1314,32 +1323,42 @@ mod tests {
res.0
}

fn run_test_case(t: &RunProgramTest) {
fn run_test_case(test_case: &RunProgramTest) {
use crate::chia_dialect::ChiaDialect;
use crate::test_ops::node_eq;
let mut allocator = Allocator::new();

let program = check(parse_exp(&mut allocator, t.prg));
let args = check(parse_exp(&mut allocator, t.args));
let expected_result = &t.result.map(|v| check(parse_exp(&mut allocator, v)));
let program = check(parse_exp(&mut allocator, test_case.prg));
let args = check(parse_exp(&mut allocator, test_case.args));
let expected_result = &test_case
.result
.map(|v| check(parse_exp(&mut allocator, v)));

let dialect = ChiaDialect::new(test_case.flags);

let dialect = ChiaDialect::new(t.flags);
println!("prg: {}", t.prg);
match run_program(&mut allocator, &dialect, program, args, t.cost) {
match run_program(&mut allocator, &dialect, program, args, test_case.cost) {
Ok(Reduction(cost, prg_result)) => {
assert!(node_eq(&allocator, prg_result, expected_result.unwrap()));
assert_eq!(cost, t.cost);
assert_eq!(cost, test_case.cost);

// now, run the same program again but with the cost limit 1 too low, to
// ensure it fails with the correct error
let expected_cost_exceeded =
run_program(&mut allocator, &dialect, program, args, t.cost - 1).unwrap_err();
run_program(&mut allocator, &dialect, program, args, test_case.cost - 1)
.unwrap_err();
assert_eq!(expected_cost_exceeded.1, "cost exceeded");
}
Err(err) => {
println!("FAILED: {}", err.1);
assert_eq!(err.1, t.err);
assert!(expected_result.is_none());
Err(actual) => {
assert_eq!(
actual.1, test_case.err,
"Expected program {} to fail",
test_case.prg
);
assert!(
expected_result.is_none(),
"Expected program {} to have no result",
test_case.prg
);
}
}
}
Expand Down Expand Up @@ -1431,14 +1450,14 @@ mod tests {
(3981700, 0, 0),
"atom is not a G2 point"
)]
#[case::keccak(
#[case::keccak_equal(
"(i (= (keccak256 (q . \"foobar\")) (q . 0x38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873e)) (q . 0) (q x))",
(1134, 1, ENABLE_KECCAK_OPS_OUTSIDE_GUARD),
(1134, 2, ENABLE_KECCAK_OPS_OUTSIDE_GUARD),
""
)]
#[case::keccak(
#[case::keccak_inequal(
"(i (= (keccak256 (q . \"foobar\")) (q . 0x38d18acb67d25c8bb9942764b62f18e17054f66a817bd4295423adf9ed98873f)) (q . 0) (q x))",
(1134, 1, ENABLE_KECCAK_OPS_OUTSIDE_GUARD),
(1134, 2, ENABLE_KECCAK_OPS_OUTSIDE_GUARD),
"clvm raise"
)]
fn test_softfork(
Expand All @@ -1457,13 +1476,15 @@ mod tests {

// softfork extensions that are enabled
let ext_enabled = match test_ext {
0 => true,
1 => (flags & ENABLE_KECCAK) != 0,
0 | 1 => true,
2 => (flags & ENABLE_KECCAK) != 0,
_ => false,
};
let supports_op = test_ext >= enabled;

println!("mempool: {mempool} ext: {test_ext} flags: {flags}");
let expect_err = match (ext_enabled as u8, (test_ext >= enabled) as u8) {
println!("enabled: {ext_enabled} supports: {supports_op}");
let expect_err = match (ext_enabled as u8, supports_op as u8) {
// the extension we're running has not been activated, and we're not
// running an extension that supports the operator
(0, 0) => {
Expand Down