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

Fix program buffer account rent-exempt lamport calculation #34722

Merged
merged 1 commit into from
Jan 10, 2024

Conversation

HaoranYi
Copy link
Contributor

@HaoranYi HaoranYi commented Jan 9, 2024

Problem

When we use bpf_loader v2 to load a program, we create a buffer account to
upload the program data. However, the required minimum balance of the account
is calculated directly from the program data length, which is not the correct
length for the actual account's data. The actual accounts' data include the additional
45 bytes of metadata.

Therefore, this can result that buffer account doesn't have enough lamport to be rent
exempted and fails the transaction.

Example:

[2024-01-09T20:45:22.340224015Z DEBUG solana_metrics::metrics] CounterPoint { name: "rent_paying_err-new_account", count: 0, timestamp: SystemTime { tv_sec: 1704833122, tv_nsec: 340211290 } }
[2024-01-09T20:45:22.340219646Z DEBUG solana_runtime::accounts::account_rent_state] Account 2cdvBZpHx6sxz4gCaYaYdvT4aXduC8cLDQWHsTHcg8WR not rent exempt, state Account { lamports: 714263040, data.len: 102533, owner: BPFLoaderUpgradeab1e11111111111111111111111, executable: false, rent_epoch: 18446744073709551615, data: 010000000115a25dc58afd88ec11d0e139c02e40a359bc71e50dcfc02d370318a7618db852000000000000000000000000000000000000000000000000000000 }
[2024-01-09T20:45:22.340299829Z DEBUG solana_runtime::bank] check: 22us load: 85us execute: 1035us txs_len=1
[2024-01-09T20:45:22.340312332Z DEBUG solana_runtime::bank] tx error: InsufficientFundsForRent { account_index: 1 } SanitizedTransaction { message: Legacy(LegacyMessage { message: Message { header: MessageHeader { num_required_signatures: 2, num_readonly_signed_accounts: 0, num_readonly_unsigned_accounts: 3 }, account_keys: [Crq3FuY6kvwf1g4TGnE1ThH8ECXgAV98wVf2Tka4Q9Pi, 2cdvBZpHx6sxz4gCaYaYdvT4aXduC8cLDQWHsTHcg8WR, 11111111111111111111111111111111, BPFLoaderUpgradeab1e11111111111111111111111, 2TTAEeGBRBt8kDJ3Nk5M9vYTvQ7Y8cwuKmtZy24Pz8gR], recent_blockhash: 94KXuLHsb2jK5pV7hvfSaJbJGXoe2rEC2dSNGJC2GDdG, instructions: [CompiledInstruction { program_id_index: 2, accounts: [0, 1], data: [0, 0, 0, 0, 0, 202, 146, 42, 0, 0, 0, 0, 133, 144, 1, 0, 0, 0, 0, 0, 2, 168, 246, 145, 78, 136, 161, 176, 226, 16, 21, 62, 247, 99, 174, 43, 0, 194, 185, 61, 22, 193, 36, 210, 192, 83, 122, 16, 4, 128, 0, 0] }, CompiledInstruction { program_id_index: 3, accounts: [1, 4], data: [0, 0, 0, 0] }] }, is_writable_account_cache: [true, true, false, false, false] }), message_hash: 9ytdEfJXcMoof3HM58xfu25pKXB2GgB8x4A4jbdmB5Bi, is_simple_vote_tx: false, signatures: [6727fLhm33Dad7QVqtoPxHbAX5AFgXCf6tv2FTEZn1v4sz8FEMSf5CxCstzq2SjgpHK8bD1dNfdXDD8SFJX5HiGc, hpKa8mCzjQGssDcLHN72xxucy44mfkBqHkDYDKbJxjmz4rUxBvfVwB2gz68JuD8pJ8GmKrdZUmwQwty6pnYp1Bb] }
[2024-01-09T20:45:22.340545295Z DEBUG solana_runtime::bank] 1 errors of 1 txs
[2024-01-09T20:45:22.340579881Z DEBUG solana_runtime::bank] store: 19us txs_len=1
[2024-01-09T20:45:22.340677587Z DEBUG solana_accounts_db::accounts] bank unlock accounts
thread 'test_program_sbf_realloc_invoke' panicked at /home/sol/src/solana/runtime/src/loader_utils.rs:145:10:
called `Result::unwrap()` on an `Err` value: TransactionError(InsufficientFundsForRent { account_index: 1 })

In this example, the program data is 102496 bytes, while the account's data is
102533 bytes. The lamports (714263040) on the account is calculated based on 102496, which
is not enough for 102533 bytes, and makes this account rent-paying, and failed the transaction.

Summary of Changes

Include buffer meta-data when calculating the minimal required lamports for
program buffer account.

Fixes #

@HaoranYi HaoranYi changed the title fix program buffer size in minimul for rent exempt calculation Fix program buffer account rent-exempt lamport calculation Jan 9, 2024
@HaoranYi
Copy link
Contributor Author

HaoranYi commented Jan 9, 2024

I find this issue when working on #34691.

  • using bpf_loader_v2 to load solana_sbf_rust_realloc program.

Copy link

codecov bot commented Jan 9, 2024

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (30fa449) 81.8% compared to head (06a3961) 81.8%.
Report is 17 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master   #34722   +/-   ##
=======================================
  Coverage    81.8%    81.8%           
=======================================
  Files         824      824           
  Lines      222687   222687           
=======================================
+ Hits       182245   182286   +41     
+ Misses      40442    40401   -41     

@brooksprumo
Copy link
Contributor

Nice find. I wonder why this wasn't an issue for people deploying programs? Is it that no one uses loader v2?

Copy link
Contributor

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

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

Looks good to me. r+ with signoff from @Lichtso

@HaoranYi
Copy link
Contributor Author

Nice find. I wonder why this wasn't an issue for people deploying programs? Is it that no one uses loader v2?

I think people are using cli to deploy v2 programs, not load_util.rs. With cli, they would often choose the minimal rent exempt lamport.

@HaoranYi HaoranYi merged commit 61e42cb into solana-labs:master Jan 10, 2024
35 checks passed
@HaoranYi HaoranYi deleted the 2024_jan_9 branch January 10, 2024 17:32
HaoranYi pushed a commit that referenced this pull request Feb 10, 2024
sakridge added a commit that referenced this pull request Feb 10, 2024
…35162)

* Upgrade sbf tests to use bpf loader v3 (#34691)

* update sbf test to use bpf_loader v2

* update test_program_sbf_invoke_sanity test

* update test bpf program owner

* update test_program_sbf_invoke_upgradeable_via_cpi

* update test_program_sbf_disguised_as_sbf_loader

* update test_program_reads_from_program_account

* update test_program_sbf_program_id_spoofing

* update test_program_sbf_caller_has_access_to_cpi_program

* update 3 more tests

* fix program buffer size in minimul for rent exempt calculation

* more test updates

* more update

* more test updates

* comments

* undo c format

* typo

* add sol_alloc_free not deployable and deployable tests

* comments

* review feedback - move buffer_keypair and program_keypair inside callee
fn.

* more refactor

* delete sof_alloc_free_syscall enabled tests

* revert lamport change

---------

Co-authored-by: HaoranYi <[email protected]>
(cherry picked from commit 8869d0c)

# Conflicts:
#	programs/sbf/tests/programs.rs

* fix merge conflicts

* update tests to avoid using the new test api from 1.18

* manually backport #34722 to fix a test

* Ignore failing benchmark tests and fix compilation

---------

Co-authored-by: HaoranYi <[email protected]>
Co-authored-by: HaoranYi <[email protected]>
Co-authored-by: haoran <haoran@mbook>
Co-authored-by: Stephen Akridge <[email protected]>
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.

4 participants