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: Finalization Issue with Mismatched return_data_length and Empty return_data #718

Closed
jobez opened this issue Aug 31, 2023 · 4 comments · Fixed by #765
Closed

bug: Finalization Issue with Mismatched return_data_length and Empty return_data #718

jobez opened this issue Aug 31, 2023 · 4 comments · Fixed by #765
Assignees

Comments

@jobez
Copy link
Contributor

jobez commented Aug 31, 2023

Bug Report

Kakarot version:

Current behavior:

The sha ef tests presented a flow where a called context is finalized without ever executing the exec_return opcode

  • in this case, the return_data of the execution context is empty, but the return_data_length is nonzero (in this tests case, it is 64
  • when we finalize, we attempt to store the return data in the calling contexts memory
  • we hit an error because the length of the return data does not match the empty return data array

Expected behavior:

  • execution context finalization should be able to handle the case where an executing context has no return data, even when called with a caller with a retSize
  • we need to differentiate the caller's retSize and the called's return_data_length

Steps to reproduce/related code:

class TestSha3:
@pytest.mark.skip(
"TODO: need to fix when return_data is shorter than retSize in CallHelper.finalize_calling_context"
)
async def test_sha3_d0g0v0_Shanghai(
self,
owner,
create_account_with_bytecode,
kakarot: StarknetContract,
):
called_contract = await create_account_with_bytecode("0x600060002060005500")
caller_contract = await create_account_with_bytecode(
"0x604060206010600f6000600435610100016001600003f100"
)
res = await kakarot.eth_send_transaction(
to=int(caller_contract.evm_contract_address, 16),
gas_limit=1_000_000,
gas_price=0,
value=0,
data=hex_string_to_bytes_array(
# In the original EF test, the called contract is supposed to be set in genesis
# at address 0x000000000000000000000000000000000000010{i} while the payload of
# the tx uses {i}, hence we sub 0x100 to the real deployed called_address
f"0x693c6139{int(called_contract.evm_contract_address, 16) - 0x100:064x}"
),
).execute(caller_address=owner.starknet_address)
sha3 = called_contract.storage(0).call()
assert res == sha3

Other information:

@jobez
Copy link
Contributor Author

jobez commented Sep 20, 2023

Problem Statement:
In our current logic, we treat the ret_data parsed from the opcode level argument as the "actual return data length" (return_data_len). This causes issues when exec_return is not called to update it, especially when the compiler has to read from uninitialized memory cells.

Proposed Solution:
Add yet another incredible field to ExecutionContext: add in the execution context to requested_return_data_len. This variable will be set during the prepare_args function.

return_data_len will be treated as the actual return data len, and will only be updated when the exec_return opcode is called: https://github.com/kkrt-labs/kakarot/blob/main/src/kakarot/instructions/system_operations.cairo#L176

Finalization Logic: In the finalization phase, compare return_data_len with requested_return_data_len implicitly via using

https://github.com/kkrt-labs/kakarot/blob/main/src/utils/utils.cairo#L253

let padded_return_data = Helpers.slice_data(data_len=return_data_len, data=return_data, data_offset=0, slice_len=requested_return_data_len)

And thus we achieve world peace.

@Eikix
Copy link
Member

Eikix commented Sep 20, 2023

reproducing here our convo in telegram

greg is proposing a little refactor where the execution context has a return_data_info field that points to a ReturnDataInfo struct with the following dimensions

  • len
  • requested_len
  • offset
  • data

i am fine with doing this, but would like collective buyin before doing so

@Eikix
Copy link
Member

Eikix commented Sep 20, 2023

Elias:


yes, i’m asking if in https://github.com/jobez/kakarot/blob/main/src/kakarot/instructions/system_operations.cairo#L585,
the bug comes from ctx.sub_context.return_data_len == ret_size
while ctx.sub_context.return_data is empty



ok ok, could we, without changing much, add a check here
https://github.com/jobez/kakarot/blob/main/src/kakarot/instructions/system_operations.cairo#L585
to check that return_data_len and return_data are coherent

@ClementWalter
Copy link
Member

reproducing here our convo in telegram

greg is proposing a little refactor where the execution context has a return_data_info field that points to a ReturnDataInfo struct with the following dimensions

  • len
  • requested_len
  • offset
  • data

i am fine with doing this, but would like collective buyin before doing so

I’m fin with the struct, and it’ll save steps overall

This was referenced Sep 20, 2023
ClementWalter added a commit that referenced this issue Oct 10, 2023
Time spent on this PR: 0.4

## Pull request type

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?

Fails to finalize a calling context when `ret_size` is greater than
`return_data_len`.

Resolves #718 

## What is the new behavior?

Use `Helpers.slice` to make sure that the stored `return_data` is big
enough.

Note: we previously stored the `ret_offset` in the
`sub_context.return_data[0]`. I have put both `ret_offset` and
`ret_size` in the calling_context return data, as:

- this is more logical: they belong to the calling context and not the
sub context
- it is safe because return_data is only written in RETURN, which stops
the context, so no data could have been put before a call
- actually I've seen that geth and the execution spec reads
RETURNDATASIZE and RETURNDATACOPY directly from the execution context
and don't keep a ref to the previous sub_context. Might be an idea for a
future refacto
@github-project-automation github-project-automation bot moved this from 🆕 Backlog to ✅ Done in Kakarot on Starknet Oct 10, 2023
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.

3 participants