smite-scenarios: build invalid programs using raw_program

[morehouse]

Add a raw_program helper to build programs that are malformed and therefore can't use ProgramBuilder (which would panic).

[erickcestari]

LGTM!

[erickcestari]

nit: The module doc is not true anymore, since raw_program doesn't append to a ProgramBuilder

[morehouse]

Fixed.

[NishantBansal2003]

LGTM

[ekzyis]

ACK 8444224

nit: If ProgramBuilder had an append_unchecked() method:

diff --git a/smite-ir/src/builder.rs b/smite-ir/src/builder.rs
index fd4843b..5a7f185 100644
--- a/smite-ir/src/builder.rs
+++ b/smite-ir/src/builder.rs
@@ -127,6 +127,21 @@ impl ProgramBuilder {
         idx
     }

+    /// Appends an instruction without the well-formedness checks
+    /// [`Self::append`] applies. Returns the instruction index.
+    ///
+    /// For tests that need an invalid instruction `append` would reject. It can
+    /// follow checked `append` calls, so the invalid instruction can be
+    /// appended after an otherwise valid prefix.
+    pub fn append_unchecked(&mut self, operation: Operation, inputs: &[usize]) -> usize {
+        let idx = self.instructions.len();
+        self.instructions.push(Instruction {
+            operation,
+            inputs: inputs.to_vec(),
+        });
+        idx
+    }
+
     /// Selects or creates a variable of the given type using probabilistic
     /// variable selection (75% most recent, 15% any existing, 10% fresh).
     /// For affine variables the most recent candidate is always selected.

Then we could use the same pattern in execute_affine_overuse_panics to build invalid programs. We could then also highlight in the tests which instruction is invalid with append() vs append_unchecked(), see full patch. append_unchecked() would need to be public, though, so it would expose an unsafe method (not sure if we care about that). WDYT?

full patch

diff --git a/smite-ir/src/builder.rs b/smite-ir/src/builder.rs
index fd4843b..84a6f16 100644
--- a/smite-ir/src/builder.rs
+++ b/smite-ir/src/builder.rs
@@ -127,6 +127,19 @@ impl ProgramBuilder {
         idx
     }
 
+    /// Appends an instruction without the well-formedness checks
+    /// [`Self::append`] applies. Returns the instruction index.
+    ///
+    /// For tests that need an invalid instruction `append` would reject.
+    pub fn append_unchecked(&mut self, operation: Operation, inputs: &[usize]) -> usize {
+        let idx = self.instructions.len();
+        self.instructions.push(Instruction {
+            operation,
+            inputs: inputs.to_vec(),
+        });
+        idx
+    }
+
     /// Selects or creates a variable of the given type using probabilistic
     /// variable selection (75% most recent, 15% any existing, 10% fresh).
     /// For affine variables the most recent candidate is always selected.
diff --git a/smite-scenarios/src/executor/tests.rs b/smite-scenarios/src/executor/tests.rs
index 1f00c9b..ee3fe11 100644
--- a/smite-scenarios/src/executor/tests.rs
+++ b/smite-scenarios/src/executor/tests.rs
@@ -9,7 +9,6 @@ use bitcoin::secp256k1::{Secp256k1, SecretKey};
 use harness::*;
 use programs::*;
 use smite::bolt::{AcceptChannelTlvs, GossipTimestampFilter, Init, Ping};
-use smite_ir::Instruction;
 use smite_ir::builder::ProgramBuilder;
 use smite_ir::operation::ShutdownScriptVariant;
 
@@ -550,43 +549,47 @@ fn execute_records_open_channel_for_duplicate_id_after_funding() {
 #[should_panic(expected = "expected 1 inputs, got 0")]
 fn execute_wrong_input_count_panics() {
     // `DerivePoint` expects one input.
-    Fixture::new().run(&raw_program(&[(Operation::DerivePoint, &[])]));
+    let mut b = ProgramBuilder::new();
+    b.append_unchecked(Operation::DerivePoint, &[]);
+    Fixture::new().run(&b.build());
 }
 
 #[test]
 #[should_panic(expected = "expected PrivateKey, got Amount")]
 fn execute_type_mismatch_panics() {
-    // v0 is an Amount; `DerivePoint` wants a PrivateKey.
-    Fixture::new().run(&raw_program(&[
-        (Operation::LoadAmount(42), &[]),
-        (Operation::DerivePoint, &[0]),
-    ]));
+    let mut b = ProgramBuilder::new();
+    let amount = b.append(Operation::LoadAmount(42), &[]);
+    // `DerivePoint` wants a PrivateKey, but `amount` is an Amount.
+    b.append_unchecked(Operation::DerivePoint, &[amount]);
+    Fixture::new().run(&b.build());
 }
 
 #[test]
 #[should_panic(expected = "out of bounds")]
 fn execute_variable_out_of_bounds_panics() {
-    Fixture::new().run(&raw_program(&[(Operation::SendMessage, &[99])]));
+    let mut b = ProgramBuilder::new();
+    b.append_unchecked(Operation::SendMessage, &[99]);
+    Fixture::new().run(&b.build());
 }
 
 #[test]
 #[should_panic(expected = "out of bounds")]
 fn execute_forward_variable_reference_panics() {
-    // v0 refers forward to v1.
-    Fixture::new().run(&raw_program(&[
-        (Operation::DerivePoint, &[1]),
-        (Operation::LoadPrivateKey([0x11; 32]), &[]),
-    ]));
+    let mut b = ProgramBuilder::new();
+    // v0 refers forward to v1, which isn't defined yet.
+    b.append_unchecked(Operation::DerivePoint, &[1]);
+    b.append(Operation::LoadPrivateKey([0x11; 32]), &[]);
+    Fixture::new().run(&b.build());
 }
 
 #[test]
 #[should_panic(expected = "is void")]
 fn execute_void_variable_reference_panics() {
-    // `MineBlocks` produces no variable, so v0 is void.
-    Fixture::new().run(&raw_program(&[
-        (Operation::MineBlocks(1), &[]),
-        (Operation::SendMessage, &[0]),
-    ]));
+    let mut b = ProgramBuilder::new();
+    let void = b.append(Operation::MineBlocks(1), &[]);
+    // `MineBlocks` produced no variable, so `void` refers to a void slot.
+    b.append_unchecked(Operation::SendMessage, &[void]);
+    Fixture::new().run(&b.build());
 }
 
 #[test]
@@ -602,10 +605,11 @@ fn execute_invalid_private_key_panics() {
 #[test]
 #[should_panic(expected = "expected OpenChannelMessage, got Amount")]
 fn execute_send_open_channel_wrong_type_panics() {
-    Fixture::new().run(&raw_program(&[
-        (Operation::LoadAmount(42), &[]),
-        (Operation::SendOpenChannel, &[0]),
-    ]));
+    let mut b = ProgramBuilder::new();
+    let amount = b.append(Operation::LoadAmount(42), &[]);
+    // `SendOpenChannel` wants an OpenChannelMessage, but `amount` is an Amount.
+    b.append_unchecked(Operation::SendOpenChannel, &[amount]);
+    Fixture::new().run(&b.build());
 }
 
 #[test]
@@ -613,18 +617,17 @@ fn execute_send_open_channel_wrong_type_panics() {
 fn execute_affine_overuse_panics() {
     let mut b = ProgramBuilder::new();
     let negotiated = negotiate_channel(&mut b, &announced_open_channel());
-    let mut program = b.build();
 
-    // `ProgramBuilder` rejects the reuse itself, so we manually append the
-    // second receive instruction.
-    program.instructions.push(Instruction {
-        operation: Operation::RecvAcceptChannel,
-        inputs: vec![negotiated.open_channel.sent],
-    });
+    // `ProgramBuilder::append` rejects the reuse itself, so append the second
+    // receive instruction unchecked.
+    b.append_unchecked(
+        Operation::RecvAcceptChannel,
+        &[negotiated.open_channel.sent],
+    );
 
     Fixture::new()
         .queue(&Message::AcceptChannel(sample_accept_channel()))
-        .run(&program);
+        .run(&b.build());
 }
 
 // MineBlocks should track calls to mine_blocks
@@ -645,11 +648,11 @@ fn execute_mine_blocks_invokes_cli() {
 #[test]
 #[should_panic(expected = "expected 0 inputs, got 1")]
 fn execute_mine_blocks_wrong_input() {
+    let mut b = ProgramBuilder::new();
+    let amount = b.append(Operation::LoadAmount(1), &[]);
     // `MineBlocks` takes no inputs.
-    Fixture::new().run(&raw_program(&[
-        (Operation::LoadAmount(1), &[]),
-        (Operation::MineBlocks(6), &[0]),
-    ]));
+    b.append_unchecked(Operation::MineBlocks(6), &[amount]);
+    Fixture::new().run(&b.build());
 }
 
 #[test]
diff --git a/smite-scenarios/src/executor/tests/programs.rs b/smite-scenarios/src/executor/tests/programs.rs
index c551628..f149c28 100644
--- a/smite-scenarios/src/executor/tests/programs.rs
+++ b/smite-scenarios/src/executor/tests/programs.rs
@@ -6,7 +6,6 @@
 
 use super::harness::{PointSource, SampleOpenChannel, acceptor_funding_sk, opener_funding_sk};
 use crate::executor::*;
-use smite_ir::Instruction;
 use smite_ir::builder::ProgramBuilder;
 
 // -- open_channel --
@@ -339,22 +338,3 @@ pub fn send_channel_announcement(b: &mut ProgramBuilder, scid: usize) {
     );
     b.append(Operation::SendMessage, &[announcement]);
 }
-
-// -- Malformed programs --
-
-/// Builds a program from `(operation, inputs)` pairs, skipping the
-/// well-formedness checks [`ProgramBuilder`] applies.
-///
-/// Only for tests asserting the executor rejects a malformed program, which
-/// can't use `ProgramBuilder` since it too panics on malformed programs.
-pub fn raw_program(instructions: &[(Operation, &[usize])]) -> Program {
-    Program {
-        instructions: instructions
-            .iter()
-            .map(|(operation, inputs)| Instruction {
-                operation: operation.clone(),
-                inputs: inputs.to_vec(),
-            })
-            .collect(),
-    }
-}

[morehouse]

@ekzyis I considered that approach as well. Since tests that need invalid programs are generally quite short, I decided the raw_program approach was a better tradeoff so that we don't have to add a test-only method to ProgramBuilder.