Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Add tests for the Debug and activation Vecs#11926

Merged
ryoqun merged 2 commits intosolana-labs:masterfrom
ryoqun:program-activation-tests
Sep 1, 2020
Merged

Add tests for the Debug and activation Vecs#11926
ryoqun merged 2 commits intosolana-labs:masterfrom
ryoqun:program-activation-tests

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented Aug 31, 2020

Problem

  • We could be in bad scheduling when specifying epochs to activate native programs
  • Debug for Programs and MessageProcessor isn't tested. (The Debug impls aren't derived; it's manually written due to rustc bug/limitation around function pointer).

Summary of Changes

  • Extend sanity checks for the scheduling data of programs by epoch. Let's make scheduling less error-prone.
  • Add very simple tests for Debug.

Context

follow up of #11736

@ryoqun ryoqun requested review from jackcmay and mvines August 31, 2020 08:03
@ryoqun ryoqun added the v1.3 label Aug 31, 2020
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Aug 31, 2020

fyi: Part of my patches were already got in #11916 ;)

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 31, 2020

Codecov Report

Merging #11926 into master will increase coverage by 0.0%.
The diff coverage is 83.9%.

@@           Coverage Diff           @@
##           master   #11926   +/-   ##
=======================================
  Coverage    82.1%    82.2%           
=======================================
  Files         332      332           
  Lines       78095    78142   +47     
=======================================
+ Hits        64153    64235   +82     
+ Misses      13942    13907   -35     

@ryoqun ryoqun changed the title Add tests for the Debug and activation Vecs and Add tests for the Debug and activation Vecs Aug 31, 2020
// guard against sysvars being made
if sysvar::check_id(&owner) {
// guard against sysvars and native loader programs being made
if sysvar::check_id(&owner) || native_loader::check_id(&owner) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is sneaky. For real safety here I think we need a compile-time check to inform us when additional restrictions are being added here so we can gate them. For example if there's a new sysvar or native_loader added, during the 33%-66% upgrade window, anybody can kill the cluster with a simple solana transfer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Would also be nice to reserve an upgrade path for our known ids like BPFLoader2111111111.... BPFLoader31111111....., etc...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops, odd. This is intended to be pushed to #11928...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mvines

For real safety here I think we need a compile-time check to inform us when additional restrictions are being added here so we can gate them.

This is interesting and timely. :) Well, since Rust 1.46, we can do this to some extent thanks to const fn improvements: https://blog.rust-lang.org/2020/08/27/Rust-1.46.0.html#const-fn-improvements. Namely, we can digest the ast tokens at the compile-time and create sha256 and compare it against annootated frozen_code(digest = "fooBarbase58") as an auto-generated test just like frozen_abi(...).

For example if there's a new sysvar or native_loader added, during the 33%-66% upgrade window, anybody can kill the cluster with a simple solana transfer.

Well, the attack isn't involved with SystemInstruction::Transfer. This is related to SystemInstruction::Assign. Anyway, it's very easy to kill cluster without gating... I'll gate it.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Would also be nice to reserve an upgrade path for our known ids like BPFLoader2111111111.... BPFLoader31111111....., etc...

@jackcmay gotcha!

@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Sep 1, 2020

silly me...:

$ git add --patch
...
==> 2020-08-31 16:47:29.316707928|0
$ git commit -m "Add tests for the Debug and activation Vecs and " -a
[for-small-patches cc2169a] Add tests for the Debug and activation Vecs and
5 files changed, 99 insertions(+), 13 deletions(-)
==> 2020-08-31 16:49:29.752882042|0

@ryoqun ryoqun force-pushed the program-activation-tests branch from cc2169a to abb9eb1 Compare September 1, 2020 02:05
@ryoqun ryoqun force-pushed the program-activation-tests branch from abb9eb1 to ef2a286 Compare September 1, 2020 02:13
assert!(next_start_epoch >= prev_start_epoch);
match program {
Program::Native((name, id)) => assert!(unique.insert((name, id))),
Program::BuiltinLoader((name, id, _)) => assert!(unique.insert((name, id))),
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well, we must require the uniqueness separately both for name and id, not for the tuple of (name, id).

@ryoqun ryoqun requested a review from mvines September 1, 2020 03:40
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented Sep 1, 2020

@mvines @jackcmay this is ready for review again! I think this is strictly non-functional change now. Sorry for sneaky changes...

@ryoqun ryoqun merged commit 11ac4eb into solana-labs:master Sep 1, 2020
mergify Bot pushed a commit that referenced this pull request Sep 1, 2020
* Add tests for the Debug and activation Vecs

* Rename a bit

(cherry picked from commit 11ac4eb)
mergify Bot added a commit that referenced this pull request Sep 1, 2020
* Add tests for the Debug and activation Vecs

* Rename a bit

(cherry picked from commit 11ac4eb)

Co-authored-by: Ryo Onodera <ryoqun@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants