Skip to content

flamenco: refactor txn_ctx/instr_ctx account getters API #4606

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

Merged
merged 1 commit into from
Mar 28, 2025

Conversation

cali-jumptrading
Copy link
Contributor

@cali-jumptrading cali-jumptrading commented Mar 21, 2025

Notable changes:

  • simplify txn_ctx account getter APIs to match Agave semantically and allow for users to pass in an account pre-condition checker function.
  • Add try_borrow_last_program_account in the instr_ctx to match agave semantics and account borrowing checks.

@cali-jumptrading cali-jumptrading changed the title flamenco: clean up account getter API in txn ctx flamenco: refactor txn ctx account getters Mar 21, 2025
@cali-jumptrading cali-jumptrading linked an issue Mar 21, 2025 that may be closed by this pull request
@cali-jumptrading cali-jumptrading changed the title flamenco: refactor txn ctx account getters flamenco: refactor txn ctx account getters API Mar 21, 2025
@cali-jumptrading cali-jumptrading marked this pull request as ready for review March 21, 2025 22:07
@cali-jumptrading cali-jumptrading force-pushed the cali/clean-up-txn-ctx-borrowed-accounts-api branch 10 times, most recently from a114adf to 5b3ca09 Compare March 24, 2025 15:13
@cali-jumptrading cali-jumptrading changed the title flamenco: refactor txn ctx account getters API flamenco: refactor txn_ctx/instr_ctx account getters API Mar 24, 2025
@cali-jumptrading cali-jumptrading force-pushed the cali/clean-up-txn-ctx-borrowed-accounts-api branch 10 times, most recently from 7d588cd to cea7474 Compare March 24, 2025 17:29
@cali-jumptrading cali-jumptrading force-pushed the cali/clean-up-txn-ctx-borrowed-accounts-api branch 2 times, most recently from c7c10f6 to f9b677f Compare March 25, 2025 21:15
@cali-jumptrading cali-jumptrading force-pushed the cali/clean-up-txn-ctx-borrowed-accounts-api branch 5 times, most recently from cbe19cb to 11274c7 Compare March 26, 2025 14:29
jumpsiegel
jumpsiegel previously approved these changes Mar 26, 2025
Copy link
Contributor

@ibhatt-jumptrading ibhatt-jumptrading left a comment

Choose a reason for hiding this comment

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

Leaving some comments, happy to chat more/clarify any of these points

@cali-jumptrading cali-jumptrading force-pushed the cali/clean-up-txn-ctx-borrowed-accounts-api branch 6 times, most recently from 86a80a7 to 875c33b Compare March 27, 2025 18:46

if ( FD_UNLIKELY( use_sysvar_instructions ) ) {
ret = fd_sysvar_instructions_update_current_instr_idx( txn_ctx, i );
if( ret != FD_ACC_MGR_SUCCESS ) {
FD_LOG_WARNING(( "sysvar instructions failed to update instruction index" ));
txn_ctx->instr_err_idx = i;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was to prevent a segfault when debugging a mismatch in test-vectors.

fd_exec_txn_ctx_get_account_modify_fee_payer( fd_exec_txn_ctx_t * ctx,
fd_txn_account_t * * account );
static inline int
fd_exec_txn_ctx_find_index_of_account( fd_exec_txn_ctx_t const * ctx,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason we do a reverse iteration here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suspect it's for performance reasons to compare to 0 instead of N. The slight performance edge probably is negligible, so I'm happy to change to forward iteration if it's clearer.

Copy link
Contributor

@ibhatt-jumptrading ibhatt-jumptrading left a comment

Choose a reason for hiding this comment

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

this is great!

@cali-jumptrading cali-jumptrading force-pushed the cali/clean-up-txn-ctx-borrowed-accounts-api branch from fd24d11 to 7e99720 Compare March 28, 2025 18:29
@cali-jumptrading cali-jumptrading added this pull request to the merge queue Mar 28, 2025
Merged via the queue into main with commit 8b2586e Mar 28, 2025
11 checks passed
@cali-jumptrading cali-jumptrading deleted the cali/clean-up-txn-ctx-borrowed-accounts-api branch March 28, 2025 18:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make txn_ctx/instr_ctx APIs match Agave
4 participants