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.

Sign up for GitHub

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

Jump to bottom

Dont call precompiled programs #19930

Merged
merged 1 commit into from
Sep 29, 2021
Merged

Dont call precompiled programs #19930

merged 1 commit into from
Sep 29, 2021

Conversation

jackcmay
Copy link
Contributor

Problem

Should not be able to call a precompiled program directly

Summary of Changes

  • Prevent calling precompiled program's via CPI
  • Short-circuit runtime to returning Ok for all precompiled programs

Fixes #19843

@jackcmay jackcmay added work in progress This isn't quite right yet noCI Suppress CI on this Pull Request labels Sep 15, 2021
@jackcmay jackcmay requested a review from ryoqun September 16, 2021 19:24
@jackcmay jackcmay force-pushed the dont-call-precompiled-programs branch from 5b8c882 to a8c405b Compare September 20, 2021 21:29
@jackcmay jackcmay added CI Pull Request is ready to enter CI and removed work in progress This isn't quite right yet noCI Suppress CI on this Pull Request labels Sep 20, 2021
@jackcmay jackcmay force-pushed the dont-call-precompiled-programs branch from a8c405b to c172deb Compare September 20, 2021 21:32
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Sep 20, 2021
@jackcmay jackcmay added the CI Pull Request is ready to enter CI label Sep 20, 2021
@solana-grimes solana-grimes removed the CI Pull Request is ready to enter CI label Sep 20, 2021
@jackcmay jackcmay force-pushed the dont-call-precompiled-programs branch from c172deb to e03aea2 Compare September 21, 2021 02:28
@jackcmay jackcmay force-pushed the dont-call-precompiled-programs branch from e03aea2 to 64b2b9d Compare September 21, 2021 17:26
@codecov
Copy link

codecov bot commented Sep 21, 2021 •
edited
Loading

Codecov Report

Merging #19930 (64b2b9d) into master (3aba89d) will increase coverage by 0.0%.
The diff coverage is 90.2%.

❗ Current head 64b2b9d differs from pull request most recent head 2417852. Consider uploading reports for the commit 2417852 to get more accurate results

@@           Coverage Diff            @@
##           master   #19930    +/-   ##
========================================
  Coverage    82.7%    82.8%            
========================================
  Files         487      485     -2     
  Lines      135326   134906   -420     
========================================
- Hits       112036   111765   -271     
+ Misses      23290    23141   -149

@jackcmay jackcmay requested a review from ryoqun September 21, 2021 23:42
@jackcmay
Copy link
Contributor Author

Hey @ryoqun Can you take a detailed look at the capitalization and bank hash-related changes to make sure I didn't mess anything up? Thanks!

ryoqun
ryoqun reviewed Sep 23, 2021
View reviewed changes
runtime/src/bank.rs Outdated Show resolved Hide resolved
@ryoqun
Copy link
Member

ryoqun commented Sep 23, 2021

to be sure, this pr is compatible with mainnet-beta/testnet/devnet?

ryoqun
ryoqun reviewed Sep 23, 2021
View reviewed changes
runtime/src/bank.rs Outdated Show resolved Hide resolved
ryoqun
ryoqun reviewed Sep 28, 2021
View reviewed changes
programs/bpf/c/src/invoke/invoke.c Outdated
@@ -588,6 +591,16 @@ extern uint64_t entrypoint(const uint8_t *input) {
*accounts[ARGUMENT_INDEX].lamports += 1;
break;
}
case TEST_CALL_PRECOMPILE: {
sol_log("Test calling builtin from cpi");
Copy link
Member

Choose a reason for hiding this comment

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

nit: builtin => precompiled program like the rust counterpart?

@ryoqun
Copy link
Member

ryoqun commented Sep 28, 2021

to be sure, this pr is compatible with mainnet-beta/testnet/devnet?

That's the question I was hoping you could help verify with a second set of eyes :-)

I stumbled on devnet check (testnet, mainnet-beta isn't done still):

it seems master got incompatible?

arguments: --identity validator-keypair-devnet.json --known-validator dv1ZAGvdsz5hHLwWXsVnM94hWf1pjbKVau1QVkaMJ92 --known-validator dv2eQHeP4RFrJZ6UeiZWoc3XTtmtZCUKxxCApCDcRNV --known-validator dv4ACNkpYPcE3aKmYDqZm9G5EB3J4MRoeE7WNDRBVJB --known-validator dv3qDFk1DTF36Z62bNvrCXe9sKATA6xvVy6A798xxAS --only-known-rpc --ledger ledger-devnet --rpc-port 8899 --dynamic-port-range 8001-8011 --entrypoint entrypoint.devnet.solana.com:8001 --entrypoint entrypoint2.devnet.solana.com:8001 --entrypoint entrypoint3.devnet.solana.com:8001 --entrypoint entrypoint4.devnet.solana.com:8001 --entrypoint entrypoint5.devnet.solana.com:8001 --expected-genesis-hash EtWTRABZaYq6iMfeYKouRu166VU2xqa1wcaWoxPkrZBG --wal-recovery-mode skip_any_corrupted_record --limit-ledger-size

# both build failed to catchup; observable ever widening slot distance from:

$ ./solana-validator-no-cpi-precompile-before --version
solana-validator 1.8.0 (src:9d91a6f1; feat:173977236)
$ ./solana-validator-no-cpi-precompile --version
solana-validator 1.8.0 (src:09b9de1b; feat:3245883770)

$ solana catchup -ud --our-localhost=8899 --log --follow --commitment root

fyi, the same argument managed to catch up to the cluster with solana-validator 1.7.12 (src:ca83167c; feat:2013646575)

@CriesofCarrots
Copy link
Contributor

it seems master got incompatible?

I believe @jeffwashington is looking into this as well.

@jeffwashington
Copy link
Contributor

it seems master got incompatible?

I believe @jeffwashington is looking into this as well.

I'm bisecting now. mnb and master with hack
failure results in not making roots at least.

@jackcmay
Copy link
Contributor Author

it seems master got incompatible?

I believe @jeffwashington is looking into this as well.

I'm bisecting now. mnb and master with hack failure results in not making roots at least.

So probably unrelated to this pr, aka, not blocking the commit of this pr?

@ryoqun
Copy link
Member

ryoqun commented Sep 28, 2021

@jackcmay

good work. code looks ok after all comments are addressed. thanks for working nicely according to my review comments.

the remaining task before merging would be the rather very tedious live cluster manual testing (seems to be blocked by incompatible master..).

@jackcmay
Copy link
Contributor Author

@jackcmay

good work. code looks ok after all comments are addressed. thanks for working nicely according to my review comments.

the remaining task before merging would be the rather very tedious live cluster manual testing (seems to be blocked by incompatible master..).

@jeffwashington Can you ping me when you get to the bottom of master's incompatibility?

@ryoqun
Copy link
Member

ryoqun commented Sep 28, 2021

it seems master got incompatible?

I believe @jeffwashington is looking into this as well.

I'm bisecting now. mnb and master with hack failure results in not making roots at least.

So probably unrelated to this pr, aka, not blocking the commit of this pr?

if this pr won't be backported soon, I think it's ok to merge this pr, forgoing the live-cluster compatibility stamp. as far as I reviewed, this should be okay... ;)

@jackcmay jackcmay force-pushed the dont-call-precompiled-programs branch from 09b9de1 to 1f8ff23 Compare September 28, 2021 20:46
@jackcmay jackcmay force-pushed the dont-call-precompiled-programs branch from 1f8ff23 to f592f0d Compare September 28, 2021 22:24
@jackcmay jackcmay force-pushed the dont-call-precompiled-programs branch from f592f0d to 84ee410 Compare September 29, 2021 00:17
@jackcmay jackcmay added the automerge Merge this Pull Request automatically once CI passes label Sep 29, 2021
@mergify mergify bot merged commit 8fee9a2 into solana-labs:master Sep 29, 2021
@jackcmay jackcmay deleted the dont-call-precompiled-programs branch September 29, 2021 15:25
dankelleher pushed a commit to identity-com/solana that referenced this pull request Nov 24, 2021
@jackcmay @dankelleher
Dont call precompiled programs (solana-labs#19930)
b578765
frits-metalogix added a commit to identity-com/solana that referenced this pull request Nov 24, 2021
@frits-metalogix
Revert "Dont call precompiled programs (solana-labs#19930)"
9968635 
This reverts commit b578765.
@ryoqun ryoqun mentioned this pull request Feb 17, 2022
Merged
@Lichtso Lichtso mentioned this pull request Apr 27, 2022
Closed
@ryoqun ryoqun mentioned this pull request Aug 17, 2022
Merged
ryoqun
ryoqun reviewed Aug 17, 2022
View reviewed changes
sdk/src/feature_set.rs
@@ -223,6 +223,12 @@ pub mod do_support_realloc {
solana_sdk::declare_id!("75m6ysz33AfLA5DDEzWM1obBrnPQRSsdVQ2nRmc8Vuu1");
}

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

Choose a reason for hiding this comment

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

cross ref: #27100 (comment) at #27100

@zfedoran zfedoran mentioned this pull request Mar 26, 2024
Open
joncinque added a commit to joncinque/solana that referenced this pull request Nov 14, 2024
@joncinque
sdk: Rename ed25519-instructions -> ed25519-program
99ee736 
#### Problem

The precompiles behave more like programs than instructions, as was
decided with the new secp256r1 precompile in solana-labs#3152, but the ed25519
precompile was split as `ed25519-instructions`.

#### Summary of changes

Rename the crate from `solana-ed25519-instructions` to
`solana-ed25519-program`, which is a previous crate that used to be used
until solana-labs#19930.
@joncinque joncinque mentioned this pull request Nov 14, 2024
Merged
joncinque added a commit to anza-xyz/agave that referenced this pull request Nov 14, 2024
@joncinque
sdk: Rename ed25519-instructions -> ed25519-program (#3632)
8652ecc 
* sdk: Rename ed25519-instructions -> ed25519-program

#### Problem

The precompiles behave more like programs than instructions, as was
decided with the new secp256r1 precompile in #3152, but the ed25519
precompile was split as `ed25519-instructions`.

#### Summary of changes

Rename the crate from `solana-ed25519-instructions` to
`solana-ed25519-program`, which is a previous crate that used to be used
until solana-labs#19930.

* Run cargo fmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seanyoung seanyoung seanyoung left review comments

@ryoqun ryoqun ryoqun left review comments

Assignees
No one assigned
Labels
automerge Merge this Pull Request automatically once CI passes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Shouldn't be possible to invoke ed25519 and secp256k1 instructions from a program
6 participants
@jackcmay @ryoqun @CriesofCarrots @jeffwashington @seanyoung @solana-grimes