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

fix: fix panic in event parsing due to empty stack #2856

Merged
merged 2 commits into from Mar 20, 2024

Conversation

zilayo
Copy link
Contributor

@zilayo zilayo commented Mar 18, 2024

fixes #2855 - please see linked issue for more details

Issue

When subscribing to logs via Program::on -> Program::on_internal -> parse_logs_response the thread panics.

The issue is due to calling Execution::Program in parse_logs_response which asserts that Self::stack isn't empty.

However, the current logic empties the stack upon the return of the first outer instruction matching the subscribed Program ID, and so when there are multiple outer instructions within the logs, a panic occurs.

Fix

This PR introduces a few changes to parse_logs_response and handle_system_log

handle_system_log

"cpi" is only pushed as a new_program when the invoke instruction isn't invoking at a depth of [1]. We'll handle these outer instruction invocations in parse_logs_response

parse_logs_response

The for loop is now a peekable iterator which is manually managed via next().

If the current instruction returned did_pop = true (and hence was a program success log) then we peek at the next log (if one exists).

If this ends with "invoke [1] then it means the current instruction was the end of an outer instruction, and a new outer instruction will begin on the next iteration.

At this stage the stack is empty due to the pop, so we capture the program ID of the next instruction and push this to the stack.

Tests

Added a minimal test case to confirm the new logic now allows this to run without any panics. The logs used in the test are from a real transaction on mainnet, but with dummy program IDs and event names.

Copying the test case to the current commit will replicate the panic behavior.

Copy link

vercel bot commented Mar 18, 2024

@zilayo is attempting to deploy a commit to the coral-xyz Team on Vercel.

A member of the Team first needs to authorize it.

Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Hey, thank you for the comphrehensive explanation of the problem and the solution.

This looks good to me overall, but before we merge, could you also note this fix in the CHANGELOG?

client/src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@acheroncrypto acheroncrypto left a comment

Choose a reason for hiding this comment

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

Thank you!

@acheroncrypto acheroncrypto merged commit 3514216 into coral-xyz:master Mar 20, 2024
46 of 47 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

client: Panic in parse_logs_response / Program::on due to incorrect Execution logic
2 participants