Initial integration of dynamic contracts and native module loading #1256

jackcmay · 2018-09-18T18:25:26Z

Addresses first three boxes of #1255.

Notes:

Should have no effect on execution unless a dynamic contract transaction is issued (which isn't fully supported by the bank yet anyway)
Tested on MacOs only so far
Waiting for dynamic contract transaction support to fully test

jackcmay · 2018-09-18T18:39:40Z

src/dynamic_contract.rs

+    }
+
+    #[test]
+    fn test_contract_move_funds_success() {


Looks like automation failed on this test (rest pass). I didn't see any failures locally, any ideas?

test dynamic_contract::tests::test_contract_move_funds_success ... error: process didn't exit successfully: /solana/target/debug/deps/solana-2161c8890c108436 (signal: 11, SIGSEGV: invalid memory reference)
🚨 Error: The command exited with status 101

jackcmay · 2018-09-18T19:28:18Z

src/dynamic_contract.rs

+    #[test]
+    fn test_create_library_path_1() {
+        assert_eq!(
+            "target/debug/deps/libfoo.dylib",


Not portable and caught by automation on Linux, will fix

mvines · 2018-09-18T20:14:48Z

src/dynamic_contract.rs

+                        Ok(s) => s,
+                        Err(e) => panic!("{:?} Unable to find {:?} in {:?}", e, ENTRYPOINT, &path),
+                    };
+                    if let Err(e) = entrypoint(tx, accounts) {


The contract only needs tx.userdata and accounts, no need to pass in the entire Transaction

mvines · 2018-09-18T20:25:11Z

src/dynamic_contract.rs

+        account.tokens
+    }
+
+    pub fn process_transaction(tx: &Transaction, accounts: &mut [Account]) {


I would structure this a little differently where DynamicContract is a new LoadContract instruction on SystemContract. LoadContract takes the .so name and the contract_id to associate it with (for now, until we have on-chain storage of contract binaries).

bank.rs then looks up incoming Transaction contract_id's against the list of loaded contracts and invokes the appropriate entrypoint().

Then in a future PR, an internal LoadContract instruction is performed to load the current budget_contract so the only hard coded contract that bank.rs knows about is the system contract (or we just boot the budget_contract from src/ entirely -- need to refactor the Vote instruction before that's possible though...).

Two or more different transactions involved. First one to load the contract (tx usedata contains filename and id) and subsequent ones to call the loaded contract by id (tx contains contract userdata only). Did you have thoughts on unloading, a second new instruction to SystemContract?

Also, are you thinking that budget_contract would also be dynamic?

Two or more different transactions involved...
Yep, exactly.

Did you have thoughts on unloading...
For now let's punt on unloading. But I think unloading can be implicit -- once there are no active Accounts with the contract_id for the given .so in bank.rs, that .so can be unloaded from memory. It would be reloaded again once a new active Account is tagged with that contract_id. Unloading will probably be associated with a token value as well. The user pays 100 (N) tokens to make that contract_id available for 100 (M) days.

Also, are you thinking that budget_contract would also be dynamic?

It certainly could be eventually. There's a bunch of code detangling that needs to happen in transaction.rs/bank.rs/.. before that'll be possible though.

Sounds good, will give it a shot

@mvines, would you like me to merge this PR and then start a new one for the structure changes you suggested?

Also, I'll make the changes to pass the keys in this PR, agreed?

There’s no rush to land this so I’d rather the structural changes are made in this PR (keys too)

garious

Awesome. Merge once @mvines is happy with it.

mvines · 2018-09-19T03:32:46Z

Hey Jack, I realized that we also need to pass the Transaction keys[] into the entry point as well. The entries in keys[] correspond 1-1 to the entries in accounts[], so maybe we zip those two vectors together before passing them in (I don’t feel strongly about zipping either way though).

garious · 2018-09-19T03:44:26Z

zip them so that we don't need to document a semantic coupling

jackcmay · 2018-09-19T04:47:36Z

rust noob question, you guys are suggesting that I zip (iter zip) both keys and accounts together into a vector (to be passed as a buffer) of structs of type [ key: Pubkey, account: Account ]? Also, any name suggestions? :-)

mvines · 2018-09-19T05:47:27Z

Yeah that sounds great. Name ideas: AccountInfo maybe?

jackcmay · 2018-09-20T07:48:29Z

@mvines Restructured, let me know what you think

mvines · 2018-09-20T15:18:02Z

src/system_contract.rs

+}
+
+// TODO where should this live?
+struct Temporary {


This probably should be a part of bank.rs

mvines · 2018-09-20T15:19:16Z

src/system_contract.rs

+                    }
+                }
+            }
+            SystemContract::Call { contract_id, data } => {


ah, so instead of Call we just dispatch the dynamic contracts from https://github.com/solana-labs/solana/blob/master/src/bank.rs#L363
(which is also why the Temporary stuff can just move into bank.rs)

Something like this?

// Call the contract method // It's up to the contract to implement its own rules on moving funds if SystemContract::check_id(&tx.contract_id) { SystemContract::process_transaction(&tx, accounts) } else if BudgetContract::check_id(&tx.contract_id) { // TODO: the runtime should be checking read/write access to memory // we are trusting the hard coded contracts not to clobber or allocate BudgetContract::process_transaction(&tx, accounts) } else if { if let Some(dynamic_contract) = self::check_dynanmic_id(tx.contract_id) { dynamic_contract.call(tx, accounts); } } else { return Err(BankError::UnknownContractId(tx.contract_id)); }

This poses some structural issues. SystemContract::process_transaction()' does not return a value so no way to return the loaded module context back to the bank to be saved in the bank's list. Maybe bank should export both a load_contract()andget_contract_by_id()methods rather then these operations being done bySystemContract?`

hmm, yeah. I think it's ok if the SystemContract::process_transaction() is a little special and take in another argument from the bank that allows the system contract to save loaded contracts in. I wouldn't expose the entire bank to the system contract, just what it needs

Like, pass in the hash to be populated?

Only slightly less ugly is having system contract call a bank function to register the loaded contract.

passing in the hash seems nice

garious · 2018-09-20T15:22:46Z

contracts/move_funds/src/lib.rs

+            let mut infos: Vec<AccountInfo> = Vec::new();
+            for (key, account) in keys.iter().zip(&mut accounts).collect::<Vec<_>>() {
+                infos.push(AccountInfo { key, account });
+            }


We use the suffix Info throughout the codebase to mean something like Meta. So the name AccountInfo suggests isn't information/metadata about that account, which it's not. How about OwnedAccount? Suggesting it's an Account with some additional information (the public key of its owner). Or maybe AddressedAccount? Or perhaps punt on a name and leave it as a tuple?

Also, that code can be written without mut:

let owned_accounts: Vec<_> = keys.into_iter().zip(&accounts).map(|(key, account)| OwnedAccount {key, account}).collect();

Awesome, was looking fror a one-liner for the zip :-)

For the name, maybe KeyedAccount?

no strong opinion. I'd lean toward the tuple. :)

garious · 2018-09-20T15:22:58Z

contracts/move_funds/src/lib.rs

+        let keys = vec![Pubkey::default(); 2];
+        let mut accounts = vec![Account::default(), Account::default()];
+        accounts[0].tokens = 100;
+        accounts[1].tokens = 1;


What happens if this is zero?

'this' being the value we assign to `accounts[1].tokens'?

then the result of that test would be that accounts[0].tokens == 0 and accounts[1].tokens == 100. What issue did you notice?

garious · 2018-09-20T15:24:05Z

contracts/noop/Cargo.toml

+
+[dependencies]
+libloading = "0.5.0"
+solana = { path = "../.." }


This is rough. Would it be a huge amount of work to move the noop dependencies out of solana into their own workspace?

The amount of work is not what I'm concerned about. I would like to have example dynamic programs built into the solana project so they are part of the normal dev/test/ci/etc... cycle.

What is your concern with referencing solana in this way?

The circular reference will make it so it's impossible to get noop out on its own.

In or out noop will have to depend on Solana (or a subset) in order to share the common interface definitions

garious · 2018-09-20T15:24:53Z

src/dynamic_contract.rs

+    /// * Transaction::keys[0..] - contract dependent
+    /// * TODO BPF specific stuff
+    /// * userdata - contract specific user data
+    BPF { userdata: Vec<u8> },


Rust convention is to only capitalize the first letter of acronyms. Can you make this Bpf?

garious · 2018-09-20T15:26:24Z

src/system_contract.rs

+#[derive(Debug)]
+pub struct AccountInfo<'a> {
+    pub key: &'a Pubkey,
+    pub account: &'a mut Account,


Hmm, how about dumping the references here?

Not sure how to how since they are both borrowed by SystemContract::process_transaction(). Are you suggesting a rework of SystemContract?

src/system_contract.rs

jackcmay · 2018-09-23T22:16:13Z

Worked around thread safety issue: #1314

mvines · 2018-09-23T23:44:52Z

src/system_program.rs

@@ -25,6 +28,10 @@ pub enum SystemProgram {
    /// * Transaction::keys[0] - source
    /// * Transaction::keys[1] - destination
    Move { tokens: i64 },
+    /// Load a program
+    /// programn_id - id to associate this program


nit: programn_id

mvines

r+ with the one minor speling nit picked

jackcmay · 2018-09-24T05:12:56Z

Looks like unrelated CI test failure, filed issue #1315

…end_and_verify_all_nodes_env_num_nodes` (solana-labs#1256)

jackcmay added the work in progress This isn't quite right yet label Sep 18, 2018

jackcmay self-assigned this Sep 18, 2018

jackcmay requested review from garious and mvines September 18, 2018 18:25

jackcmay commented Sep 18, 2018

View reviewed changes

mvines reviewed Sep 18, 2018

View reviewed changes

garious approved these changes Sep 19, 2018

View reviewed changes

jackcmay force-pushed the integrate_dynamic_contracts branch from 6011d16 to 4887376 Compare September 20, 2018 07:19

mvines reviewed Sep 20, 2018

View reviewed changes

garious reviewed Sep 20, 2018

View reviewed changes

jackcmay force-pushed the integrate_dynamic_contracts branch 3 times, most recently from bf9db06 to f002faf Compare September 21, 2018 06:34

jackcmay added 5 commits September 23, 2018 11:13

Integration of native dynamic programs

e22c6c9

fix order of fn def, test nit

55f4051

unable to reproduce ci failure, submit debug

be78e4d

compiler warning

6f583da

linux bug workaround, never unload native modules

cc320fb

jackcmay force-pushed the integrate_dynamic_contracts branch from 0f7f0b4 to cc320fb Compare September 23, 2018 18:37

fix fmt

dffb77b

mvines reviewed Sep 23, 2018

View reviewed changes

mvines approved these changes Sep 23, 2018

View reviewed changes

spelling

6170c9c

jackcmay removed the work in progress This isn't quite right yet label Sep 24, 2018

jackcmay merged commit 26b1466 into solana-labs:master Sep 24, 2018

jackcmay deleted the integrate_dynamic_contracts branch September 24, 2018 05:13

jeffwashington pushed a commit to jeffwashington/solana that referenced this pull request May 9, 2024

Unignore test_local_cluster_signature_subscribe and remove `test_sp…

d36954c

…end_and_verify_all_nodes_env_num_nodes` (solana-labs#1256)

Initial integration of dynamic contracts and native module loading #1256

Initial integration of dynamic contracts and native module loading #1256

Conversation

jackcmay commented Sep 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garious left a comment

Choose a reason for hiding this comment

mvines commented Sep 19, 2018

garious commented Sep 19, 2018

jackcmay commented Sep 19, 2018 • edited Loading

mvines commented Sep 19, 2018

jackcmay commented Sep 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jackcmay commented Sep 23, 2018

Choose a reason for hiding this comment

mvines left a comment

Choose a reason for hiding this comment

jackcmay commented Sep 24, 2018

jackcmay commented Sep 19, 2018 •

edited

Loading