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

SIMD-0163: Lift the CPI caller restriction #163

Merged
merged 2 commits into from
Nov 25, 2024

Conversation

Lichtso
Copy link
Contributor

@Lichtso Lichtso commented Aug 1, 2024

No description provided.

Copy link
Contributor

@buffalojoec buffalojoec 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 a great change! Eliminating the requirement for programs to contain the callee program account - in the cases of loaders v2 and v4 - will be a drastic improvement on CUs.

@Lichtso Lichtso force-pushed the lift-cpi-caller-restriction branch from f5abfcf to 55578ff Compare August 15, 2024 12:25
@cavemanloverboy
Copy link

cavemanloverboy commented Aug 21, 2024

Can you clarify why this is a massive improvement in CU cost? It seems to me like it's more of a general improvement to VM performance and just removes the need to construct one or a few more AccountInfos. Programs don't ever copy the program data.

As an extreme example, a noop program

pub extern "C" fn entrypoint(input: *mut u8) -> u64 {
    0
}

would see no reduction in CUs from being invoked with one less 10 MB account

@Lichtso
Copy link
Contributor Author

Lichtso commented Aug 21, 2024

@cavemanloverboy For top-level instructions this is in deed irrelevant. But, not so for nested CPI.

In nested CPI the program runtime does copy the program data (except for those owned by loader-v3) of instruction accounts, and taxes the caller for it here: https://github.com/anza-xyz/agave/blob/77b4d131502e095d18f3df7caab8b6a1cf7c1887/programs/bpf_loader/src/syscalls/cpi.rs#L886

Copy link
Contributor

@jstarry jstarry left a comment

Choose a reason for hiding this comment

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

I really like this SIMD but I'm wondering if we should also update the tx format to make it explicit which programs may be invoked during tx execution. Rather than just appending program ids to the readonly accounts in a transaction, we could have a new message header value which defines how many of the trailing readonly accounts are invokable and should therefore be loaded into the program cache before execution.

@Lichtso
Copy link
Contributor Author

Lichtso commented Nov 6, 2024

we could have a new message header value which defines how many of the trailing readonly accounts are invokable

Good idea, however outside of this SIMD. This is only about CPI, it does not affect the message accounts.

@cavemanloverboy
Copy link

cavemanloverboy commented Nov 6, 2024

I agree. New transaction format needs to be thought through in great detail alongside #172.

I will draft something up in the next few weeks.

@2501babe
Copy link
Member

should we specify that non-instruction non-payer accounts must be read-only and enforce this in sanitize? or specify that writable non-instruction non-payer accounts are demoted to read-only?

it wouldnt really affect lock contention because the only reason to append a writable account is to be annoying, and they could be just as annoying by taking it as an instruction account, but it would mean we dont need to check if a non-instruction account was invoked to skip it during account saving

@Lichtso
Copy link
Contributor Author

Lichtso commented Nov 14, 2024

I don't think this SIMD is that much related to account loading. It needs all accounts containing programs to be loaded in the program cache, that's it. And that happens anyway, independent of writeability and later account saving.

@joncinque
Copy link
Contributor

It looks like there's consensus here. If no one objects within the next week, I'll merge this on 25 November.

@joncinque joncinque merged commit fda3d17 into solana-foundation:main Nov 25, 2024
2 checks passed
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.

8 participants