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

Amend transactions v2 proposal with dependent accounts #17901

Closed
wants to merge 1 commit into from

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Jun 11, 2021

Problem

One of the short comings of the original tx format was that it wasn't able to explicitly describe CPI dependencies. The result is the need to pass ALL the accounts and programs needed by an instruction execution chain to the top level program. Doing so creates a lot more overhead since all the accounts needed below have to serialized/mapped into the program space of each caller. Most accounts probably don't have to be passed in in their entirety or at least their data does not. If the tx included a dependent account field then the runtime could keep those accounts in the wings and reference them when needed without requiring the programs to ferry them all along. We should consider this now that we are bumping the tx format.

Originally posted by @jackcmay in #17103 (comment)

Summary of Changes

Add inner_programs and inner_program_accounts to transaction instruction structure to separate dependent accounts from account inputs needed by the outer instruction

Fixes #

@jstarry jstarry requested a review from jackcmay June 11, 2021 23:51
/// instruction's execution context. They are available for passing to inner
/// instructions
#[serde(with = "short_vec")]
pub inner_program_accounts: Vec<u8>,
Copy link
Member

Choose a reason for hiding this comment

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

I think we can avoid this additional churn to solve the problem described in this pr's description by providing yet another pathway (#17796 ) to access to tx's accounts in addition to the entrypoint one.

Copy link
Member Author

Choose a reason for hiding this comment

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

This can serve two purposes. One is avoiding the serialization overhead. The other (not mentioned in the PR description) is a stronger framework for isolating the scope of what an instruction can do. If we allow an instruction to dynamically load any account from the transaction, a malicious program might have enough resources for more damage. Imagine for instance, a program dynamically loads the fee payer signer account.

Comment on lines +194 to +195
For extra user protection, only the programs listed in `inner_programs` may be
invoked by the outer transaction instruction.
Copy link
Member

@ryoqun ryoqun Jun 12, 2021

Choose a reason for hiding this comment

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

is this strict requirement? is there any concrete threat in mind? if that's the case, I'll try to address this at #17796 as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think I already outlined a general threat here: #17901 (comment) but more concretely: If a user inadvertently calls a compromised or otherwise malicious program in their transaction, sandboxing can help limit what damage may be done. Transactions can restrict which programs are called and which signers can be used in an instruction to avoid a program being able to invoke a system or token program CPI with a signer account.

@stale
Copy link

stale bot commented Jun 22, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Jun 22, 2021
@stale
Copy link

stale bot commented Jul 1, 2021

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Jul 1, 2021
@jstarry jstarry reopened this Sep 2, 2021
@stale
Copy link

stale bot commented Sep 21, 2021

This stale pull request has been automatically closed. Thank you for your contributions.

@stale stale bot closed this Sep 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale [bot only] Added to stale content; results in auto-close after a week.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants