feat: initializing conditional stack opcodes #32

okhaimie-dev · 2024-09-11T16:07:56Z

Implement conditional opcodes and wip to enhance script code structure

Added the following opcodes with corresponding tests:

opIf
opNotIf
opElse
opEndIf

Others

opReserved
opVer

Improvements:

Structured opcode implementation to align with Shinigami's approach for better maintainability and collaboration
Introduced cond_stack for managing conditional execution

This commit addresses issue #29

src/script/cond_stack.zig

tdelabro · 2024-09-16T12:55:59Z

src/script/cond_stack.zig

+        if (self.stack.items.len == 0) {
+            return ConditionalStackError.EmptyConditionalStack;
+        }
+        _ = self.stack.pop();


Why don't you return the value?
Should it be named delete then? Not pop

src/script/cond_stack.zig

src/script/engine.zig

src/script/opcodes/flow.zig

src/script/cond_stack.zig

src/script/engine.zig

src/script/opcodes/flow.zig

tdelabro · 2024-09-23T10:03:56Z

@okhaimie-dev don't forget to ask for a new review when you are finished pushing your changes

src/script/cond_stack.zig

src/script/engine.zig

src/script/opcodes/flow_control.zig

src/script/cond_stack.zig

tdelabro · 2024-09-23T18:10:57Z

src/script/cond_stack.zig

+    ///
+    /// Returns:
+    ///     The popped value (u8) if the stack is not empty
+    ///     ConditionalStackError.EmptyConditionalStack if the stack is empty


we know what pop return. You can remove this long comment

tdelabro · 2024-09-23T18:11:57Z

src/script/cond_stack.zig

+    /// Push a conditional value onto the stack
+    ///
+    /// Args:
+    ///     value: The conditional value to push (typically 0, 1, or 2)


Can it be something else than 0, 1 or 2? You comment suggest it can

Also can you define an enum for more readability

OpCondFalse = 0 OpCondTrue = 1 OpCondSkip = 2

Can it be something else than 0, 1 or 2? You comment suggest it can

Reading other implementations it looks like it cannot. So please change your comment to not mean that it could

And ofcourse use those OpCond enum in your tests rather than 1, 2, 3

tdelabro · 2024-09-24T09:35:45Z

src/script/cond_stack.zig

+        if (self.stack.items.len == 0) {
+            return ConditionalStackError.UnbalancedConditional;
+        }
+        _ = self.stack.pop();


Don't you want to return the poped value?

@tdelabro, I don't see any need to pop it is possible to remove the value completely. what do you think?

tdelabro · 2024-09-24T09:40:36Z

src/script/opcodes/constant.zig

@@ -391,3 +391,27 @@ pub fn isUnnamedPushNDataOpcode(opcode: Opcode) ?u8 {
    const opcodeByte = opcode.toBytes();
    return if (opcodeByte > 0x00 and opcodeByte <= 0x4b) opcodeByte else null;
 }
+
+/// Conditional execution constants for script operations.
+pub const OpConditional = enum(u8) {


Those are not Opcodes so I don't like the name.
What about "CondStackValue" ?

tdelabro · 2024-09-24T09:41:56Z

src/script/cond_stack.zig

+    ///
+    /// Returns:
+    ///     Possible error if allocation fails
+    pub fn push(self: *Self, value: u8) !void {


So if values are strictly limited, change the signature to reflect that:

pub fn push(self: *Self, value: CondStackValue) !void

tdelabro · 2024-09-24T09:42:55Z

src/script/cond_stack.zig

+    const Self = @This();
+
+    /// Dynamic array to store conditional values
+    stack: std.ArrayList(u8),


std.ArrayList(CondStackValue) is more restrictive and more expressive

tdelabro · 2024-09-30T14:33:29Z

@okhaimie-dev are you still working on this?

okhaimie-dev · 2024-10-01T11:39:25Z

@okhaimie-dev are you still working on this?

I'll send a fix tomorrow, I have been traveling

tdelabro · 2024-10-03T09:13:46Z

@okhaimie-dev how is it going?

KhairallahA · 2024-10-03T09:25:52Z

@tdelabro It's already covered here I didn't notice this but I don't understand what the problem is that is still being faced here to now?

tdelabro · 2024-10-03T09:55:53Z

@KhairallahA there are a few comments I did that have to be fixed

KhairallahA · 2024-10-03T11:11:47Z

src/script/opcodes/flow_control.zig

+        var engine = Engine.init(allocator, script, ScriptFlags{});
+        defer engine.deinit();
+
+        try engine.stack.pushBool(true); // Push a true value onto the stack


I don't think it is necessary to push bool values.

OP_0 = FALSE
OP_1 = TRUE

OP_1 should be the value that returns TRUE and OP_0 should return FALSE.

(According to what is written in the ops implementation, let me know if there is a use case like this, I may be wrong)

KhairallahA · 2024-10-03T11:21:31Z

src/script/opcodes/flow_control.zig

+
+    // Test OP_ENDIF with matching OP_IF
+    {
+        const script_bytes = [_]u8{ Opcode.OP_IF.toBytes(), Opcode.OP_ENDIF.toBytes() };


You don't need to run opcodes functions directly to test it, you can test OP_ENDIF like this:

const script_bytes = [_]u8{ Opcode.OP_0.toBytes(), Opcode.OP_IF.toBytes(), Opcode.OP_1.toBytes(), Opcode.OP_ENDIF.toBytes(), };

What we expect:

engine.stack.len() == 0

KhairallahA · 2024-10-03T11:23:04Z

src/script/cond_stack.zig

+        if (self.stack.items.len == 0) {
+            return ConditionalStackError.UnbalancedConditional;
+        }
+        _ = self.stack.pop();


@tdelabro, I don't see any need to pop it is possible to remove the value completely. what do you think?

KhairallahA · 2024-10-03T11:39:38Z

@KhairallahA there are a few comments I did that have to be fixed

I think the problem is in understanding the function of Flow control because the complexity in running opcodes execute, not ops themselves

https://wiki.bitcoinsv.io/index.php/Opcodes_used_in_Bitcoin_Script#Flow_control

@okhaimie-dev, I always think it is better to go through these steps for implementation:

Implement the main goal (as it was) so that everything is clear.
Improve the implementation method in the best possible way with the help from @tdelabro.

It is possible to implement cond_stack by replacing the use of u8 with bool, but we will need to use loop, as the implementation method is many, but what matters in the beginning is achieving the main goal of the implementation process so that the improvement becomes easier

okhaimie-dev · 2024-10-03T11:53:17Z

Alright, thanks. Working on this rn

KhairallahA · 2024-10-03T11:56:49Z

@okhaimie-dev let me know if there's any problem you face
you can close it for the day, you have done a lot.

KhairallahA · 2024-10-03T12:07:56Z

src/script/opcodes/flow_control.zig

How about adding these opcodes in engine file? No need to clutter your mind like that, better have them all in one place.

KhairallahA · 2024-10-03T12:11:00Z

src/script/opcodes/constant.zig

+    /// Convert the enum to its underlying u8 value
+    pub fn toU8(self: OpConditional) u8 {
+        return @intFromEnum(self);
+    }
+
+    /// Create an OpConditional from a u8 value
+    pub fn fromU8(value: u8) !OpConditional {
+        return std.meta.intToEnum(OpConditional, value) catch {
+            return error.InvalidOpConditional;
+        };
+    }


By using the @tdelabro method to include CondStackValue as a data type as an alternative to u8, you will not need to use these functions.

@KhairallahA if I don't make a PR in the next 24hrs, you can take over this. I am still trying to get settled and not had enough time to dig into it so far.

cc: @tdelabro

tdelabro · 2024-10-07T11:43:09Z

let's close this as the project is put on hold. I will still pay you something for your work on OnlyDust, ping me if I forget

feat: initializing conditional stack opcodes

f6f6447

tcoratger reviewed Sep 12, 2024

View reviewed changes

okhaimie-dev added 3 commits September 12, 2024 23:45

feat: feedback

7b51c08

fix: retry build

14fad35

test build

0f04a73

tdelabro requested changes Sep 16, 2024

View reviewed changes

okhaimie-dev added 2 commits September 16, 2024 20:47

fix: addressed the PR feedbacks

ade3d32

fix: swapCondition removed from stack and impl in flow

749bfc3

okhaimie-dev requested a review from tdelabro September 17, 2024 06:26

okhaimie-dev and others added 3 commits September 17, 2024 02:54

feat: took out some incomplete impl

d4f3d98

Merge branch 'main' into opcode-flow

e0da5f2

fix: failing test add DisabledError

b2ba3fc

tdelabro requested changes Sep 18, 2024

View reviewed changes

okhaimie-dev and others added 3 commits September 18, 2024 09:35

Merge branch 'main' into opcode-flow

c045157

wip: addressing feedbacks

7c40d11

feat: feedback impl. renames and removed allocation in opReserve func

7e85ad5

okhaimie-dev requested a review from tdelabro September 18, 2024 15:34

mubarak23 mentioned this pull request Sep 19, 2024

Implement OPCODE for opResereved, opReserved1 and opReserved2 #46

Open

Merge branch 'main' into opcode-flow

f7e1704

tdelabro requested changes Sep 23, 2024

View reviewed changes

okhaimie-dev and others added 3 commits September 23, 2024 11:28

Merge branch 'main' into opcode-flow

9feaf49

fix: impl feedback

717c457

Merge branch 'main' into opcode-flow

f7c2de6

okhaimie-dev requested a review from tdelabro September 23, 2024 18:03

tdelabro requested changes Sep 23, 2024

View reviewed changes

okhaimie-dev added 3 commits September 23, 2024 17:54

wip: reassessing code w/ btcd

8c1fe98

feat: minor clean of flow_control & cond_stack

2a338af

feat: cleaned up comment a little

c232cdd

tdelabro requested changes Sep 24, 2024

View reviewed changes

This was referenced Oct 3, 2024

Implement opcodes 0x63-0x64-0x67-0x68 #153

Closed

Add op conditions 0x63-0x64-0x67-0x68 #155

Closed

KhairallahA suggested changes Oct 3, 2024

View reviewed changes

KhairallahA approved these changes Oct 3, 2024

View reviewed changes

tdelabro closed this Oct 7, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initializing conditional stack opcodes #32

feat: initializing conditional stack opcodes #32

okhaimie-dev commented Sep 11, 2024 •

edited

Loading

tdelabro Sep 16, 2024

tdelabro commented Sep 23, 2024

tdelabro Sep 23, 2024

tdelabro Sep 23, 2024

tdelabro Sep 23, 2024

tdelabro Sep 23, 2024

tdelabro Sep 23, 2024

tdelabro Sep 24, 2024

KhairallahA Oct 3, 2024

tdelabro Sep 24, 2024

tdelabro Sep 24, 2024

tdelabro Sep 24, 2024

tdelabro commented Sep 30, 2024

okhaimie-dev commented Oct 1, 2024

tdelabro commented Oct 3, 2024

KhairallahA commented Oct 3, 2024

tdelabro commented Oct 3, 2024

KhairallahA Oct 3, 2024 •

edited

Loading

KhairallahA Oct 3, 2024

KhairallahA Oct 3, 2024

KhairallahA commented Oct 3, 2024 •

edited

Loading

okhaimie-dev commented Oct 3, 2024

KhairallahA commented Oct 3, 2024 •

edited

Loading

KhairallahA Oct 3, 2024

KhairallahA Oct 3, 2024

okhaimie-dev Oct 3, 2024

tdelabro commented Oct 7, 2024

feat: initializing conditional stack opcodes #32

feat: initializing conditional stack opcodes #32

Conversation

okhaimie-dev commented Sep 11, 2024 • edited Loading

Choose a reason for hiding this comment

tdelabro commented Sep 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdelabro commented Sep 30, 2024

okhaimie-dev commented Oct 1, 2024

tdelabro commented Oct 3, 2024

KhairallahA commented Oct 3, 2024

tdelabro commented Oct 3, 2024

KhairallahA Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KhairallahA commented Oct 3, 2024 • edited Loading

okhaimie-dev commented Oct 3, 2024

KhairallahA commented Oct 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdelabro commented Oct 7, 2024

okhaimie-dev commented Sep 11, 2024 •

edited

Loading

KhairallahA Oct 3, 2024 •

edited

Loading

KhairallahA commented Oct 3, 2024 •

edited

Loading

KhairallahA commented Oct 3, 2024 •

edited

Loading