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

Cross-program balanced instruction handling is too restrictive #9711

Open
jackcmay opened this issue Apr 24, 2020 · 13 comments
Open

Cross-program balanced instruction handling is too restrictive #9711

jackcmay opened this issue Apr 24, 2020 · 13 comments
Assignees
Labels
runtime Issues related to runtime, BPF, and LLVM
Milestone

Comments

@jackcmay
Copy link
Contributor

Problem

The MessageProcessor's balanced instruction check ensures the Lamport sum of all accounts in the instruction remains the same before and after the instruction. Cross-program invocations only include the accounts required by the invoked instruction and thus leaves out any transfers the invoking program may have made to other accounts before the call to invoke.

For example:

For programs A and B and accounts X, Y, Z

  • A transfers lamports from X to Y
  • A invokes B passing Y, Z
  • balanced instruction check fails because it does not take into account the lamports in X

It requires developers to be aware of when they transfer tokens relative to any invocations they are making.

Proposed Solution

Include all accounts in the balance calculations

@jackcmay
Copy link
Contributor Author

@jstarry

@mvines mvines added this to the v1.2.0 milestone Apr 30, 2020
@mvines
Copy link
Member

mvines commented May 22, 2020

@jackcmay - do you want to keep this in 1.2 or punt to 1.3?

@jackcmay
Copy link
Contributor Author

@mvines I think given the other priorities punting to 1.3 is more realistic

@mvines mvines modified the milestones: v1.2.0, v1.3.0 May 22, 2020
@mvines mvines modified the milestones: v1.3.0, v1.4.0 Aug 5, 2020
@mvines mvines modified the milestones: v1.4.0, v1.5.0 Oct 8, 2020
@mvines mvines modified the milestones: v1.5.0, v1.6.0 Dec 17, 2020
@leoluk
Copy link
Contributor

leoluk commented Feb 2, 2021

Looks like this is breaking Wormhole guardian set upgrades, which - if the subsidizer pool is full - do:

  • A CPI call.
  • Transfer lamports from the bridge's subsidizer pool.
  • Another CPI call.
[devnet] [2021-02-01T19:09:41.671877046Z DEBUG solana_runtime::message_processor] Program Bridge1p5gheXUvJ6jGWGeCsgPKgnE3YgdGKRVCMY9o invoke [1]
[devnet] [2021-02-01T19:09:41.672078448Z DEBUG solana_runtime::message_processor] Program log: Instruction: PostVAA
[devnet] [2021-02-01T19:09:41.673453480Z DEBUG solana_runtime::message_processor] Program log: deriving key
[devnet] [2021-02-01T19:09:41.673568861Z DEBUG solana_runtime::message_processor] Program log: deploying contract
[devnet] [2021-02-01T19:09:41.673794033Z DEBUG solana_runtime::message_processor] Program 11111111111111111111111111111111 invoke [2]
[devnet] [2021-02-01T19:09:41.673816843Z TRACE solana_runtime::system_instruction_processor] process_instruction: CreateAccount { lamports: 3814080, space: 420, owner: Bridge1p5gheXUvJ6jGWGeCsgPKgnE3YgdGKRVCMY9o }
[devnet] [2021-02-01T19:09:41.673826873Z TRACE solana_runtime::system_instruction_processor] keyed_accounts: [KeyedAccount { is_signer: true, is_writable: true, key: 6sbzC1eH4FTujJXWj51eQe25cYvr4xfXbJ1vAj7j2k5J, account: RefCell { value: Account { lamports: 99776482800 data.len: 0 owner: 11111111111111111111111111111111 executable: false rent_epoch: 0 } } }, KeyedAccount { is_signer: true, is_writable: true, key: 7h2RXNxM9r6NDvsxRXkn7rtGt2VNe6zcs5b74vbrhjkg, account: RefCell { value: Account { lamports: 0 data.len: 0 owner: 11111111111111111111111111111111 executable: false rent_epoch: 0 } } }]
[devnet] [2021-02-01T19:09:41.673851953Z DEBUG solana_runtime::message_processor] Program 11111111111111111111111111111111 success
[devnet] [2021-02-01T19:09:41.674085315Z DEBUG solana_runtime::message_processor] Program log: deriving key
[devnet] [2021-02-01T19:09:41.674340378Z DEBUG solana_runtime::message_processor] Program log: deploying contract
[devnet] [2021-02-01T19:09:41.674586211Z DEBUG solana_runtime::message_processor] Program Bridge1p5gheXUvJ6jGWGeCsgPKgnE3YgdGKRVCMY9o consumed 100351 of 200000 compute units
[devnet] [2021-02-01T19:09:41.674612091Z DEBUG solana_runtime::message_processor] Program Bridge1p5gheXUvJ6jGWGeCsgPKgnE3YgdGKRVCMY9o BPF VM error: sum of account balances before and after instruction do not match
[devnet] [2021-02-01T19:09:41.674620231Z DEBUG solana_runtime::message_processor] Program Bridge1p5gheXUvJ6jGWGeCsgPKgnE3YgdGKRVCMY9o failed: sum of account balances before and after instruction do not match

Reproduction instructions:

  • spawn fresh devnet with tilt up --update-mode=exec -- --num=1
  • run the E2E test suite to completion
  • run scripts/test-injection.sh 0 0, boom

@jackcmay
Copy link
Contributor Author

jackcmay commented Feb 2, 2021

@leoluk One workaround is to do all in-program transfers at the end of a program's execution, is that possible here?

hendrikhofstadt added a commit to wormhole-foundation/wormhole that referenced this issue Feb 2, 2021
This works around solana-labs/solana#9711 which causes issues when the guardian set creation is subsidized and another CPI call is done subsequently
leoluk pushed a commit to wormhole-foundation/wormhole that referenced this issue Feb 2, 2021
* don't subsidize guardian set creation

This works around solana-labs/solana#9711 which causes issues when the guardian set creation is subsidized and another CPI call is done subsequently
@mvines mvines modified the milestones: v1.6.0, v1.7.0 Mar 11, 2021
@mvines mvines modified the milestones: v1.7.0, v1.8.0 May 10, 2021
@pgarg66 pgarg66 added the runtime Issues related to runtime, BPF, and LLVM label Oct 26, 2022
@billythedummy
Copy link
Contributor

Does anyone ever intend to fix this? I have not tried it yet but I think this means programs cannot do the following basic procedure:

  • transfer lamports to a wSOL token account by directly decrementing lamports from a program owned account
  • SyncNative the wSOL account

@outdoteth
Copy link

+1 trying to do the exact same use case that @billythedummy speaks about.

transfer + sync native

It's not possible right now, so what is a workaround?

@NikaTheEngineer
Copy link

Any updates on this one?
I am also trying to:

  • transfer lamports to a wSOL token account by directly decrementing lamports from a program owned account
  • SyncNative the wSOL account

@toptalhook
Copy link

Any updates on this one? I am also trying to:

  • transfer lamports to a wSOL token account by directly decrementing lamports from a program owned account
  • SyncNative the wSOL account

Now, I am also trying to do this.
Do you have any resolution?

@NikaTheEngineer
Copy link

@toptalhook The only thing you can do is wrap the SOL on a normal(non-program-owned) account and transfer wSOL to the program-owned account.

@b1acKr0se
Copy link

Does anyone ever intend to fix this? I have not tried it yet but I think this means programs cannot do the following basic procedure:

  • transfer lamports to a wSOL token account by directly decrementing lamports from a program owned account
  • SyncNative the wSOL account

This is still not possible in 2024. Please, someone take a look.

@andreisilviudragnea
Copy link

@jstarry @mvines Do you have any update on this issue?

@jstarry
Copy link
Member

jstarry commented Sep 11, 2024

To understand how to work around this runtime constraint, it's important to understand that whenever an instruction is processed in the SVM, the latest lamport balance of each instruction account is recorded and tracked. Later when a CPI is made with just a subset of those accounts, it performs balanced lamport check by adding the latest lamports for the subset of accounts passed to the CPI instruction to the sum of the last known lamport balances of the accounts that were passed to the parent instruction but not to the child CPI instruction. This sum must be the same as the sum of all last known account lamport balances when the parent instruction began processing.

So let's say you had an instruction A with accounts X (90 lamports) and Y (10 lamports) which at the beginning of processing had a sum of 100 lamports. Then, instruction A internally moves 10 lamports from account X to account Y and invokes a CPI instruction B with only account Y. When the runtime starts processing instruction B, it doesn't know what changes instruction A made to account X because it only sees the latest state of account Y which was passed as an input to instruction B. So it calculates the latest sum of accounts as 90 + 20 = 110 which is greater than 100. In order to work around this constraint, you can pass account X to the CPI (despite it not being used by instruction B) so that the runtime sees its latest balance of 80 lamports and calculates the correct sum of 100 lamports.

TL;DR: append all accounts with lamport changes to the end of your CPI instruction accounts list.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
runtime Issues related to runtime, BPF, and LLVM
Projects
None yet
Development

No branches or pull requests