Skip to content

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.

@vercel
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?

@zilayo zilayo requested a review from acheroncrypto March 20, 2024 10:14
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 solana-foundation:master Mar 20, 2024
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