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.

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

bug: opcode 0x08 SIGNEXTEND not conform to spec #677

Closed
Tracked by #28
enitrat opened this issue Aug 11, 2023 · 0 comments · Fixed by #808
Closed
Tracked by #28

bug: opcode 0x08 SIGNEXTEND not conform to spec #677

enitrat opened this issue Aug 11, 2023 · 0 comments · Fixed by #808
Assignees

Comments

@enitrat
Copy link
Collaborator

enitrat commented Aug 11, 2023

Bug Report

Kakarot version:
last version

Current behavior:
The current implementation is

    // @notice 0x0B - SIGNEXTEND
    // @dev Exp operation
    // @custom:since Frontier
    // @custom:group Stop and Arithmetic Operations
    // @custom:gas 5
    // @custom:stack_consumed_elements 2
    // @custom:stack_produced_elements 1
    // @param ctx The pointer to the execution context.
    // @return ExecutionContext The pointer to the execution context.
    func exec_signextend{
        syscall_ptr: felt*,
        pedersen_ptr: HashBuiltin*,
        range_check_ptr,
        bitwise_ptr: BitwiseBuiltin*,
    }(ctx: model.ExecutionContext*) -> model.ExecutionContext* {
        alloc_locals;

        // Stack input:
        // 0 - a: number.
        // 1 - b: exponent.
        let stack = ctx.stack;
        let (stack, popped) = Stack.pop_n(self=stack, n=2);
        let b = popped[0];
        let a = popped[1];

        // Value is already a uint256
        let stack: model.Stack* = Stack.push(self=stack, element=a);
        let ctx = apply_context_changes(ctx=ctx, stack=stack, gas_cost=GAS_COST_SIGNEXTEND);
        return ctx;
    }

https://github.com/kkrt-labs/kakarot/blob/main/src/kakarot/instructions/stop_and_arithmetic_operations.cairo#L414-L443

Expected behavior:
The expected behavior is the one explained in the whitepaper, further explained in this post https://ethereum.stackexchange.com/questions/63062/evm-signextend-opcode-explanation

For example, executing the following opcodes

// Example 1
PUSH1 0xFF
PUSH1 0
SIGNEXTEND

Should result in 0xffff....ffff (u256_max -1)

Steps to reproduce:

Related code:

Other information:

@github-project-automation github-project-automation bot moved this to 🆕 Backlog in Kakarot on Starknet Aug 11, 2023
@Eikix Eikix added this to the Official Ethereum Conformance milestone Aug 29, 2023
@Eikix Eikix self-assigned this Nov 14, 2023
@Eikix Eikix moved this from 🆕 Backlog to 👀 In review in Kakarot on Starknet Nov 14, 2023
ClementWalter pushed a commit that referenced this issue Nov 16, 2023
<!--- Please provide a general summary of your changes in the title
above -->

<!-- Give an estimate of the time you spent on this PR in terms of work
days.
Did you spend 0.5 days on this PR or rather 2 days?  -->

Time spent on this PR: 0.5d

## Pull request type

<!-- Please try to limit your pull request to one type,
submit multiple pull requests if needed. -->

Please check the type of change your PR introduces:

- [x] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe):

## What is the current behavior?

<!-- Please describe the current behavior that you are modifying,
or link to a relevant issue. -->

Resolves #677
@github-project-automation github-project-automation bot moved this from 👀 In review to ✅ Done in Kakarot on Starknet Nov 16, 2023
Eikix added a commit to Eikix/kakarot that referenced this issue Nov 16, 2023
<!--- Please provide a general summary of your changes in the title
above -->

<!-- Give an estimate of the time you spent on this PR in terms of work
days.
Did you spend 0.5 days on this PR or rather 2 days?  -->

Time spent on this PR: 0.5d

<!-- Please try to limit your pull request to one type,
submit multiple pull requests if needed. -->

Please check the type of change your PR introduces:

- [x] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe):

<!-- Please describe the current behavior that you are modifying,
or link to a relevant issue. -->

Resolves kkrt-labs#677
Eikix added a commit to Eikix/kakarot that referenced this issue Nov 16, 2023
<!--- Please provide a general summary of your changes in the title
above -->

<!-- Give an estimate of the time you spent on this PR in terms of work
days.
Did you spend 0.5 days on this PR or rather 2 days?  -->

Time spent on this PR: 0.5d

<!-- Please try to limit your pull request to one type,
submit multiple pull requests if needed. -->

Please check the type of change your PR introduces:

- [x] Bugfix
- [ ] Feature
- [ ] Code style update (formatting, renaming)
- [ ] Refactoring (no functional changes, no api changes)
- [ ] Build related changes
- [ ] Documentation content changes
- [ ] Other (please describe):

<!-- Please describe the current behavior that you are modifying,
or link to a relevant issue. -->

Resolves kkrt-labs#677
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants