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 ed25519 builtin program handling #23182

Merged
merged 5 commits into from
Feb 16, 2022

Conversation

jstarry
Copy link
Member

@jstarry jstarry commented Feb 16, 2022

Problem

Master and v1.9 branches are incompatible with v1.8.

The ed25519 program was originally added as a "builtin" program which means that ed25519 instructions would be loaded by the native loader. However, on master and v1.9, ed25519 was changed to be a "precompile" program to prevent it from being invoked by on-chain programs (it always returns success so this would be confusing).

On devnet/testnet/mainnet-beta, the ed25519 program has already been activated as a builtin, and so both precompile programs, secp256k1 and ed25519, are still invokable because the prevent_calling_precompiles_as_programs feature wasn't back ported to v1.8 and so hasn't been activated yet. In order to transition those clusters to v1.9, the precompiles need to be handled as builtins until the prevent_calling_precompiles_as_programs feature is activated.

Summary of Changes

At a high level, this PR just ensures that ed25519 is treated the same way as secp256k1. When the prevent_calling_precompiles_as_programs feature is activated, both programs will be transitioned from builtins to precompiles.

  • Re-add ed25519 to the list of builtin programs. Since the program is already activated on all clusters, it can be added directly to the genesis builtin list.
  • Ensure that ed25519 is removed as a builtin when the prevent_calling_precompiles_as_programs feature is activated
  • Ensure that ed25519 is added as a precompile when prevent_calling_precompiles_as_programs feature is activated
  • Add integration tests and the ability to deactivate features in program tests

Fixes #23180

this regression is introduced at #19918 and #19930

@jstarry jstarry requested a review from jackcmay February 16, 2022 08:54
@mvines mvines added the v1.9 label Feb 16, 2022
@jstarry jstarry added the automerge Merge this Pull Request automatically once CI passes label Feb 16, 2022
@mergify mergify bot merged commit 813725d into solana-labs:master Feb 16, 2022
mergify bot pushed a commit that referenced this pull request Feb 16, 2022
* Fix ed25519 builtin program handling

* Fix tests

* Add integration tests for processing transactions with ed25519 ixs

* Fix another test

* fix formatting

(cherry picked from commit 813725d)

# Conflicts:
#	runtime/src/bank.rs
#	runtime/src/genesis_utils.rs
t-nelson pushed a commit that referenced this pull request Feb 16, 2022
* Fix ed25519 builtin program handling

* Fix tests

* Add integration tests for processing transactions with ed25519 ixs

* Fix another test

* fix formatting

(cherry picked from commit 813725d)

[dev-dependencies]
assert_matches = "1.5.0"
ed25519-dalek = "=1.0.1"
Copy link

Choose a reason for hiding this comment

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

Is this typo?

Copy link
Contributor

Choose a reason for hiding this comment

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

Are you a bot? Elaborate

mergify bot added a commit that referenced this pull request Feb 17, 2022
* Fix ed25519 builtin program handling (#23182)

* Fix ed25519 builtin program handling

* Fix tests

* Add integration tests for processing transactions with ed25519 ixs

* Fix another test

* fix formatting

(cherry picked from commit 813725d)

* fix tests

Co-authored-by: Justin Starry <[email protected]>
Co-authored-by: Jack May <[email protected]>
recent_blockhash,
);

assert_matches!(client.process_transaction(transaction).await, Ok(()));
Copy link
Member

Choose a reason for hiding this comment

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

hehe, i'm not familiar with those tokio based integration tests. but it seems these are nice. :)

also, it's pretty fast compared to the dreadful local-cluster thing.. ;)

2842-running 3 tests
2843-test test_failure ... ok
2844-test test_success ... ok
2845:test test_success_call_builtin_program ... ok
2846-
2847-test result: ok. 3 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.30s

so, this test is checking ed25519 instruction still success as long as it's directly embedded into tx (i.e. not via CPI) regardless prevent_calling_precompiles_as_programs is activated or not.

Copy link
Member

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

lgtm

@jstarry jstarry deleted the ed25519-builtin branch February 18, 2022 06:06
@behzadnouri
Copy link
Contributor

behzadnouri commented Feb 18, 2022

Apparently this causing panic when starting a validator off master on testnet:

thread 'main' panicked at 'assertion failed: !self.freeze_started()', runtime/src/bank.rs:5383:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/6db0a0e9a4a2f55b1a85954e114ada0b45c32e45/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/6db0a0e9a4a2f55b1a85954e114ada0b45c32e45/library/core/src/panicking.rs:107:14
   2: core::panicking::panic
             at /rustc/6db0a0e9a4a2f55b1a85954e114ada0b45c32e45/library/core/src/panicking.rs:48:5
   3: solana_runtime::bank::Bank::add_builtin_account
   4: solana_runtime::bank::Bank::add_builtin
   5: solana_runtime::bank::Bank::finish_init
   6: solana_runtime::bank::Bank::new_from_fields
   7: solana_runtime::serde_snapshot::bank_from_streams
   8: solana_runtime::snapshot_utils::rebuild_bank_from_snapshots
   9: solana_runtime::snapshot_utils::bank_from_latest_snapshot_archives
  10: solana_ledger::bank_forks_utils::load
  11: solana_core::validator::new_banks_from_ledger
  12: solana_core::validator::Validator::new
  13: solana_validator::main

@t-nelson
Copy link
Contributor

Apparently this causing panic when starting a validator off master on testnet:

thread 'main' panicked at 'assertion failed: !self.freeze_started()', runtime/src/bank.rs:5383:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/6db0a0e9a4a2f55b1a85954e114ada0b45c32e45/library/std/src/panicking.rs:498:5
   1: core::panicking::panic_fmt
             at /rustc/6db0a0e9a4a2f55b1a85954e114ada0b45c32e45/library/core/src/panicking.rs:107:14
   2: core::panicking::panic
             at /rustc/6db0a0e9a4a2f55b1a85954e114ada0b45c32e45/library/core/src/panicking.rs:48:5
   3: solana_runtime::bank::Bank::add_builtin_account
   4: solana_runtime::bank::Bank::add_builtin
   5: solana_runtime::bank::Bank::finish_init
   6: solana_runtime::bank::Bank::new_from_fields
   7: solana_runtime::serde_snapshot::bank_from_streams
   8: solana_runtime::snapshot_utils::rebuild_bank_from_snapshots
   9: solana_runtime::snapshot_utils::bank_from_latest_snapshot_archives
  10: solana_ledger::bank_forks_utils::load
  11: solana_core::validator::new_banks_from_ledger
  12: solana_core::validator::Validator::new
  13: solana_validator::main

Yep! Fix is here #23233

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Feb 25, 2022
* Fix ed25519 builtin program handling

* Fix tests

* Add integration tests for processing transactions with ed25519 ixs

* Fix another test

* fix formatting
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Feb 25, 2022
* Fix ed25519 builtin program handling

* Fix tests

* Add integration tests for processing transactions with ed25519 ixs

* Fix another test

* fix formatting
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Mar 1, 2022
* Fix ed25519 builtin program handling

* Fix tests

* Add integration tests for processing transactions with ed25519 ixs

* Fix another test

* fix formatting
jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request Mar 4, 2022
* Fix ed25519 builtin program handling

* Fix tests

* Add integration tests for processing transactions with ed25519 ixs

* Fix another test

* fix formatting
@github-actions
Copy link
Contributor

This PR has been automatically locked since there has not been any activity in past 14 days after it was merged.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
automerge Merge this Pull Request automatically once CI passes locked PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consensus (bank hash) mismatch between v1.8.14 and v1.9.x/master (devnet)
7 participants