clean feature: `prevent_calling_precompiles_as_programs` #27100

jstarry · 2022-08-12T12:30:59Z

Problem

prevent_calling_precompiles_as_programs is now activated on all clusters and the feature gate code can be removed

Summary of Changes

Removed usages of prevent_calling_precompiles_as_programs

jstarry · 2022-08-12T14:06:37Z

runtime/src/genesis_utils.rs

@@ -27,6 +27,7 @@ pub fn bootstrap_validator_stake_lamports() -> u64 {
 // Number of lamports automatically used for genesis accounts
 pub const fn genesis_sysvar_and_builtin_program_lamports() -> u64 {
    const NUM_BUILTIN_PROGRAMS: u64 = 4;
+    const NUM_PRECOMPILES: u64 = 2;


Now that the feature ids passed to Precompile::new are both None, Bank::finish_init will always add the precompile accounts if they don't exist yet. This should be safe on live clusters because those precompile accounts already exist.

seems so:

$ ./target/release/solana -ud account KeccakSecp256k11111111111111111111111111111 Public Key: KeccakSecp256k11111111111111111111111111111 Balance: 0.000000001 SOL Owner: NativeLoader1111111111111111111111111111111 Executable: true Rent Epoch: 0 Length: 17 (0x11) bytes 0000: 73 65 63 70 32 35 36 6b 31 5f 70 72 6f 67 72 61 secp256k1_progra 0010: 6d m $ ./target/release/solana -ut account KeccakSecp256k11111111111111111111111111111 Public Key: KeccakSecp256k11111111111111111111111111111 Balance: 0.000000001 SOL Owner: NativeLoader1111111111111111111111111111111 Executable: true Rent Epoch: 0 Length: 17 (0x11) bytes 0000: 73 65 63 70 32 35 36 6b 31 5f 70 72 6f 67 72 61 secp256k1_progra 0010: 6d m $ ./target/release/solana -um account KeccakSecp256k11111111111111111111111111111 Public Key: KeccakSecp256k11111111111111111111111111111 Balance: 0.000000001 SOL Owner: NativeLoader1111111111111111111111111111111 Executable: true Rent Epoch: 0 Length: 17 (0x11) bytes 0000: 73 65 63 70 32 35 36 6b 31 5f 70 72 6f 67 72 61 secp256k1_progra 0010: 6d m $ ./target/release/solana -ud account Ed25519SigVerify111111111111111111111111111 Public Key: Ed25519SigVerify111111111111111111111111111 Balance: 0.000000001 SOL Owner: NativeLoader1111111111111111111111111111111 Executable: true Rent Epoch: 0 Length: 15 (0xf) bytes 0000: 65 64 32 35 35 31 39 5f 70 72 6f 67 72 61 6d ed25519_program $ ./target/release/solana -ut account Ed25519SigVerify111111111111111111111111111 Public Key: Ed25519SigVerify111111111111111111111111111 Balance: 0.000000001 SOL Owner: 11111111111111111111111111111111 Executable: true Rent Epoch: 0 $ ./target/release/solana -um account Ed25519SigVerify111111111111111111111111111 Public Key: Ed25519SigVerify111111111111111111111111111 Balance: 0.000000001 SOL Owner: NativeLoader1111111111111111111111111111111 Executable: true Rent Epoch: 0 Length: 15 (0xf) bytes 0000: 65 64 32 35 35 31 39 5f 70 72 6f 67 72 61 6d ed25519_program

jstarry · 2022-08-12T14:07:01Z

runtime/src/bank.rs

@@ -14540,25 +14540,25 @@ pub(crate) mod tests {
            if bank.slot == 0 {
                assert_eq!(
                    bank.hash().to_string(),
-                    "9tLrxkBoNE7zEUZ2g72ZwE4fTfhUQnhC8A4Xt4EmYhP1"
+                    "5gY6TCgB9NymbbxgFgAjvYLpXjyXiVyyruS1aEwbWKLK"


These had to be updated because the genesis block now includes the precompile accounts

runtime/src/builtins.rs

ryoqun · 2022-08-17T07:55:24Z

(not finished reviewing yet) prevent_calling_precompiles_as_programs in sdk/src/feature_set.rs can be cleaned up too?

jstarry · 2022-08-17T07:56:46Z

(not finished reviewing yet) prevent_calling_precompiles_as_programs in sdk/src/feature_set.rs can be cleaned up too?

It's nice to keep entries around to see which features were activated in the past rather than removing all trace

ryoqun · 2022-08-17T12:54:34Z

(not finished reviewing yet) prevent_calling_precompiles_as_programs in sdk/src/feature_set.rs can be cleaned up too?

It's nice to keep entries around to see which features were activated in the past rather than removing all trace

so, i think the comment at least need to be tweaked a bit?:

solana/sdk/src/feature_set.rs

Lines 204 to 205 in dbff6d1

    
           // Note: when this feature is cleaned up, also remove the secp256k1 program from 
        
           // the list of builtins and remove its files from /programs

=>

> // Note: when this feature id removed from this file, also remove the secp256k1 program from 
> // the list of builtins and remove its files from /programs

Hmm, it seems the referenced files actually gone already at the pr ( #19930) this comment is added?: https://github.com/solana-labs/solana/pull/19930/files#diff-eeb0f84b8b2aed64cf67da3bf301359c9a708d1d15fde7a3a872e9874763ef83 , so, is this just a stale comment, which confuses innocent reader ;) cc: @jackcmay

so, I think we should just remove this outdated comment (if my read is correct).

programs/bpf_loader/src/syscalls/cpi.rs

ryoqun

lgtm with nits

jstarry · 2022-08-17T18:38:29Z

Note: when this feature id removed from this file, also remove the secp256k1 program from
the list of builtins and remove its files from /programs

The comment here was still partially addressed by this PR because the ed25519 and secp256k1 programs remained in the builtins list until the prevent_calling_precompiles_as_programs feature was activated and transitioned them to be handled as precompiles. This was represented by the now removed BuiltinFeatureTransition::RemoveOrRetain variant.

Pull request has been modified.

mergify · 2022-08-17T21:26:43Z

automerge label removed due to a CI failure

ryoqun · 2022-08-18T05:20:58Z

re-lgtm! also added automerge

…#27100) * clean feature: prevent_calling_precompiles_as_programs * fix tests * fix test * remove comment * fix test * feedback

* refactor: extract store_stake_accounts fn * refactor: extract store_vote_account fn * refactor: extract reward history update fn * remove avg point value from pay_valiator fn. not used * clippy: slice * clippy: slice * remove abort() from test-validator (#27124) * chore: bump bytes from 1.1.0 to 1.2.1 (#27172) * chore: bump bytes from 1.1.0 to 1.2.1 Bumps [bytes](https://github.com/tokio-rs/bytes) from 1.1.0 to 1.2.1. - [Release notes](https://github.com/tokio-rs/bytes/releases) - [Changelog](https://github.com/tokio-rs/bytes/blob/master/CHANGELOG.md) - [Commits](tokio-rs/bytes@v1.1.0...v1.2.1) --- updated-dependencies: - dependency-name: bytes dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * [auto-commit] Update all Cargo lock files Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot-buildkite <[email protected]> * Share Ancestors API get with contains_key (#27161) consolidate similar fns * Rename to `MAX_BLOCK_ACCOUNTS_DATA_SIZE_DELTA` (#27175) * chore: bump libc from 0.2.129 to 0.2.131 (#27162) * chore: bump libc from 0.2.129 to 0.2.131 Bumps [libc](https://github.com/rust-lang/libc) from 0.2.129 to 0.2.131. - [Release notes](https://github.com/rust-lang/libc/releases) - [Commits](rust-lang/libc@0.2.129...0.2.131) --- updated-dependencies: - dependency-name: libc dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * [auto-commit] Update all Cargo lock files Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot-buildkite <[email protected]> * reverts wide fanout in broadcast when the root node is down (#26359) A change included in #20480 was that when the root node in turbine broadcast tree is down, the leader will broadcast the shred to all nodes in the first layer. The intention was to mitigate the impact of dead nodes on shreds propagation, because if the root node is down, then the entire cluster will miss out the shred. On the other hand, if x% of stake is down, this will cause 200*x% + 1 packets/shreds ratio at the broadcast stage which might contribute to line-rate saturation and packet drop. To avoid this bandwidth saturation issue, this commit reverts that logic and always broadcasts shreds from the leader only to the root node. As before we rely on erasure codes to recover shreds lost due to staked nodes being offline. * add getTokenLargestAccounts rpc method to rust client (#26840) * add get token largest accounts rpc call to client * split to include with commitment * Bump spl-token-2022 (#27181) * Bump token-2022 to 0.4.3 * Allow cargo to bump stuff to v1.11.5 * VoteProgram.safeWithdraw function to safeguard against accidental vote account closures (#26586) feat: safe withdraw function Co-authored-by: aschonfeld <[email protected]> * chore: bump futures from 0.3.21 to 0.3.23 (#27182) * chore: bump futures from 0.3.21 to 0.3.23 Bumps [futures](https://github.com/rust-lang/futures-rs) from 0.3.21 to 0.3.23. - [Release notes](https://github.com/rust-lang/futures-rs/releases) - [Changelog](https://github.com/rust-lang/futures-rs/blob/master/CHANGELOG.md) - [Commits](rust-lang/futures-rs@0.3.21...0.3.23) --- updated-dependencies: - dependency-name: futures dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> * [auto-commit] Update all Cargo lock files Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot-buildkite <[email protected]> * chore: bump nix from 0.24.2 to 0.25.0 (#27179) * chore: bump nix from 0.24.2 to 0.25.0 Bumps [nix](https://github.com/nix-rust/nix) from 0.24.2 to 0.25.0. - [Release notes](https://github.com/nix-rust/nix/releases) - [Changelog](https://github.com/nix-rust/nix/blob/master/CHANGELOG.md) - [Commits](nix-rust/nix@v0.24.2...v0.25.0) --- updated-dependencies: - dependency-name: nix dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <[email protected]> * [auto-commit] Update all Cargo lock files Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot-buildkite <[email protected]> * Parse ConfidentialTransaction instructions (#26825) Parse ConfidentialTransfer instructions * snapshots: serialize version file first (#27192) serialize version file first * serialize incremental_snapshot_hash (#26839) * serialize incremental_snapshot_hash * pr feedback * derives Error trait for ClusterInfoError and core::result::Error (#27208) * Add clean_accounts_for_tests() (#27200) * Rust v1.63.0 (#27148) * Upgrade to Rust v1.63.0 * Add nightly_clippy_allows * Resolve some new clippy nightly lints * Increase QUIC packets completion timeout Co-authored-by: Michael Vines <[email protected]> * docs: updated "transaction fees" page (#26861) * docs: transaction fees, compute units, compute budget * docs: added messages definition * Revert "docs: added messages definition" This reverts commit 3c56156. * docs: added messages definition * Update docs/src/transaction_fees.md Co-authored-by: Jacob Creech <[email protected]> * fix: updates from feedback Co-authored-by: Jacob Creech <[email protected]> * sdk: Fix args after "--" in build-bpf and test-bpf (#27221) * Flaky Unit Test test_rpc_subscriptions (#27214) Increase unit test timeout from 5 seconds to 10 seconds * chore: only buildkite pipelines use sccache in docker-run.sh (#27204) chore: only buildkite ci use sccache * clean feature: `prevent_calling_precompiles_as_programs` (#27100) * clean feature: prevent_calling_precompiles_as_programs * fix tests * fix test * remove comment * fix test * feedback * Add get_account_with_commitment to BenchTpsClient (#27176) * Fix a corner-case panic in get_entries_in_data_block() (#27195) #### Problem get_entries_in_data_block() panics when there's inconsistency between slot_meta and data_shred. However, as we don't lock on reads, reading across multiple column families is not atomic (especially for older slots) and thus does not guarantee consistency as the background cleanup service could purge the slot in the middle. Such panic was reported in #26980 when the validator serves a high load of RPC calls. #### Summary of Changes This PR makes get_entries_in_data_block() panic only when the inconsistency between slot-meta and data-shred happens on a slot older than lowest_cleanup_slot. * Verify snapshot slot deltas (#26666) * store-tool: log lamports for each account (#27168) log lamports for each account * add an assert for a debug feature to avoid wasted time (#27210) * remove redundant call that bumps age to future (#27215) * Use from_secs api to create duration (#27222) use from_secs api to create duration * reorder slot # in debug hash data path (#27217) * create helper fn for clarity (#27216) * Verifying snapshot bank must always specify the snapshot slot (#27234) * Remove `Bank::ensure_no_storage_rewards_pool()` (#26468) * cli: Add subcommands for address lookup tables (#27123) * cli: Add subcommand for creating address lookup tables * cli: Add additional subcommands for address lookup tables * short commands * adds hash domain to ping-pong protocol (#27193) In order to maintain backward compatibility, for now the responding node will hash the token both with and without domain so that the other node will accept the response regardless of its upgrade status. Once the cluster has upgraded to the new code, we will remove the legacy domain = false case. * Revert "Rust v1.63.0 (#27148)" (#27245) This reverts commit a2e7bdf. * correct double negation (#27240) * Enable QUIC client by default. Add arg to disable QUIC client. (Forward port #26927) (#27194) Enable QUIC client by default. Add arg to disable QUIC client. * Enable QUIC client by default. Add arg to disable QUIC client. * Deprecate --disable-quic-servers arg * Add #[ignore] annotation to failing tests * slots_connected: check if the range is connected (>= ending_slot) (#27152) * create-snapshot check if snapshot slot exists (#27153) * Add Bank::clean_accounts_for_tests() (#27209) * Call `AccountsDb::shrink_all_slots()` directly (#27235) * add ed25519_program to built-in instruction cost list (#27199) * add ed25519_program to built-in instruction cost list * Remove unnecessary and stale comment * simple refactorings to disk idx (#27238) * add _inclusive for clarity (#27239) * eliminate unnecessary ZERO_RAW_LAMPORTS_SENTINEL (#27218) * make test code more clear (#27260) * banking stage: actually aggregate tracer packet stats (#27118) * aggregated_tracer_packet_stats_option was alwasys None * Actually accumulate tracer packet stats * Refactor epoch reward 1 (#27253) * refactor: extract store_stake_accounts fn * clippy: slice Co-authored-by: haoran <haoran@mbook> * recovers merkle shreds from erasure codes (#27136) The commit * Identifies Merkle shreds when recovering from erasure codes and dispatches specialized code to reconstruct shreds. * Coding shred headers are added to recovered erasure shards. * Merkle tree is reconstructed for the erasure batch and added to recovered shreds. * The common signature (for the root of Merkle tree) is attached to all recovered shreds. * Simplify `Bank::clean_accounts()` by removing params (#27254) * Account files remove (#26910) * Create a new function cleanup_accounts_paths, a trivial change * Remove account files asynchronously * Update and simplify the implementation after the validator test runs. * Fixes after testing on the dev device * Discard tokio. Use thread instead * Fix comments format * Fix config type to pass the github test * Fix failed tests. Handle the case of non-existing path * Final cleanup, addressing the review comments Avoided OsString. Made the function more generic with "impl AsRef<Path>" Co-authored-by: Jeff Washington <[email protected]> * Refactor: Flattens `TransactionContext::instruction_trace` (#27109) * Flattens TransactionContext::instruction_trace. * Stop the search at transaction level. * Renames get_instruction_context_at => get_instruction_context_at_nesting_level. * Removes TransactionContext::get_instruction_trace(). Adds TransactionContext::get_instruction_trace_length() and TransactionContext::get_instruction_context_at_index(). * Have TransactionContext::instruction_accounts_lamport_sum() accept an iterator instead of a slice. * Removes instruction_trace from ExecutionRecord. * make InstructionContext::new() private * Parallel insertion of dirty store keys during clean (#27058) parallelize dirty store key insertion * Refactor epoch reward 2 (#27257) * refactor: extract store_stake_accounts fn * refactor: extract store_vote_account fn * clippy: slice * clippy: slice * fix merge error Co-authored-by: haoran <haoran@mbook> * Standardize thread names Tenets: 1. Limit thread names to 15 characters 2. Prefix all Solana-controlled threads with "sol" 3. Use Camel case. It's more character dense than Snake or Kebab case * cleanup comment on filter_zero_lamport_clean_for_incremental_snapshots (#27273) * remove inaccurate log (#27255) * patches metrics for invalid cached vote/stake accounts (#27266) patches invalid cached vote/stake accounts metrics Invalid cached vote accounts is overcounting actual mismatches, and invalid cached stake accounts is undercounting. * Refactor epoch reward 3 (#27259) * refactor: extract store_stake_accounts fn * refactor: extract store_vote_account fn * refactor: extract reward history update fn * clippy: slice * clippy: slice Co-authored-by: haoran <haoran@mbook> * fix merges Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: haoran <haoran@mbook> Co-authored-by: Jeff Biseda <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: dependabot-buildkite <[email protected]> Co-authored-by: Brooks Prumo <[email protected]> Co-authored-by: behzad nouri <[email protected]> Co-authored-by: AJ Taylor <[email protected]> Co-authored-by: Tyera Eulberg <[email protected]> Co-authored-by: Andrew Schonfeld <[email protected]> Co-authored-by: aschonfeld <[email protected]> Co-authored-by: apfitzge <[email protected]> Co-authored-by: Jeff Washington (jwash) <[email protected]> Co-authored-by: Brennan Watt <[email protected]> Co-authored-by: Michael Vines <[email protected]> Co-authored-by: Nick Frostbutter <[email protected]> Co-authored-by: Jacob Creech <[email protected]> Co-authored-by: Jon Cinque <[email protected]> Co-authored-by: Yihau Chen <[email protected]> Co-authored-by: Justin Starry <[email protected]> Co-authored-by: kirill lykov <[email protected]> Co-authored-by: Yueh-Hsuan Chiang <[email protected]> Co-authored-by: leonardkulms <[email protected]> Co-authored-by: Will Hickey <[email protected]> Co-authored-by: Tao Zhu <[email protected]> Co-authored-by: Xiang Zhu <[email protected]> Co-authored-by: Jeff Washington <[email protected]> Co-authored-by: Alexander Meißner <[email protected]>

clean feature: prevent_calling_precompiles_as_programs

3bcc636

jstarry requested review from ryoqun and Lichtso August 12, 2022 12:30

fix tests

d58495e

jstarry commented Aug 12, 2022

View reviewed changes

fix test

333ffd4

Lichtso reviewed Aug 12, 2022

View reviewed changes

runtime/src/builtins.rs Outdated Show resolved Hide resolved

jstarry added 2 commits August 12, 2022 17:09

remove comment

4d83c81

fix test

dbff6d1

ryoqun mentioned this pull request Aug 17, 2022

Dont call precompiled programs #19930

Merged

ryoqun reviewed Aug 17, 2022

View reviewed changes

programs/bpf_loader/src/syscalls/cpi.rs Show resolved Hide resolved

ryoqun previously approved these changes Aug 17, 2022

View reviewed changes

feedback

6e6bae6

jstarry added the automerge Merge this Pull Request automatically once CI passes label Aug 17, 2022

mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Aug 17, 2022

ryoqun added the automerge Merge this Pull Request automatically once CI passes label Aug 18, 2022

mergify bot merged commit 7d765e3 into solana-labs:master Aug 18, 2022

jstarry deleted the clean-feature/prevent_calling_precompiles_as_programs branch August 18, 2022 08:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean feature: `prevent_calling_precompiles_as_programs` #27100

clean feature: `prevent_calling_precompiles_as_programs` #27100

jstarry commented Aug 12, 2022 •

edited

Loading

jstarry Aug 12, 2022

ryoqun Aug 17, 2022

jstarry Aug 12, 2022

ryoqun commented Aug 17, 2022

jstarry commented Aug 17, 2022

ryoqun commented Aug 17, 2022 •

edited

Loading

ryoqun left a comment

jstarry commented Aug 17, 2022

mergify bot commented Aug 17, 2022

ryoqun commented Aug 18, 2022

clean feature: prevent_calling_precompiles_as_programs #27100

clean feature: prevent_calling_precompiles_as_programs #27100

Conversation

jstarry commented Aug 12, 2022 • edited Loading

Problem

Summary of Changes

jstarry Aug 12, 2022

Choose a reason for hiding this comment

ryoqun Aug 17, 2022

Choose a reason for hiding this comment

jstarry Aug 12, 2022

Choose a reason for hiding this comment

ryoqun commented Aug 17, 2022

jstarry commented Aug 17, 2022

ryoqun commented Aug 17, 2022 • edited Loading

ryoqun left a comment

Choose a reason for hiding this comment

jstarry commented Aug 17, 2022

mergify bot commented Aug 17, 2022

ryoqun commented Aug 18, 2022

clean feature: `prevent_calling_precompiles_as_programs` #27100

clean feature: `prevent_calling_precompiles_as_programs` #27100

jstarry commented Aug 12, 2022 •

edited

Loading

ryoqun commented Aug 17, 2022 •

edited

Loading