Add return data to VM #19548

seanyoung · 2021-09-01T10:27:23Z

Implementation for #19318

Add test for returning simple data
Add test for returning data via CPI
Needs to be feature gated
Returning data via CPI needs to be costed (why?)

Signed-off-by: Sean Young [email protected]

codecov · 2021-09-01T12:17:58Z

Codecov Report

Merging #19548 (132dc2e) into master (eade497) will decrease coverage by 0.0%.
The diff coverage is 27.1%.

❗ Current head 132dc2e differs from pull request most recent head e9c7ff4. Consider uploading reports for the commit e9c7ff4 to get more accurate results

@@            Coverage Diff            @@
##           master   #19548     +/-   ##
=========================================
- Coverage    82.6%    82.6%   -0.1%     
=========================================
  Files         471      471             
  Lines      132557   132658    +101     
=========================================
+ Hits       109574   109611     +37     
- Misses      22983    23047     +64

Requires: - solana-labs/solana#19548 - solana-labs/solana#19318 Signed-off-by: Sean Young <[email protected]>

seanyoung · 2021-09-01T17:21:32Z

I've created a PR for Solang to use this new system, and it works very well. The result is much nicer. 🎉

seanyoung · 2021-09-02T16:33:07Z

Compared the proposal, there are two changes:

return data is always cleared on entry (so we don't "return" data that the caller set)
the syscalls charge for their arguments, according to the length of data. Passing return data is not charged in itself, as the calls already charge.

programs/bpf_loader/src/syscalls.rs

mvines · 2021-09-08T17:53:05Z

sdk/src/process_instruction.rs

@@ -309,6 +313,19 @@ pub mod stable_log {
        ic_logger_msg!(logger, "Program log: {}", message);
    }

+    /// Log return data as from the program itself.


Just to be complete please also document here that the Program return data: will not be present if no return data was provided (as opposed to an empty <program-generated-in-base64> value)

So with the program_id, the return_data becomes an Option<>. Now it is present, even if it is empty.

mvines · 2021-09-08T17:54:45Z

sdk/src/process_instruction.rs

+    /// The general form is:
+    ///
+    /// ```notrust
+    /// "Program return data: <program-generated-in-base64>"


Suggested change

/// "Program return data: <program-generated-in-base64>"

/// "Program return data: <program-generated-data-in-base64>"

If we end up adding the program id to the return data (cc: #19318 (comment)) then I think it'll be good to include the program id that generated the return data in this log as well

Absolutely. Now it included the program_id of the setter.

mvines

lgtm! Would want @jackcmay's careful 👁️ on this too

jackcmay · 2021-09-08T18:46:58Z

* the syscalls charge  for their arguments, according to the length of data. Passing return data is not charged in itself, as the calls already charge.

We want to avoid a program calling into the runtime and consuming cycles that are not accounted for. For example, a program could call sol_get_return_data in a loop, and the program would incur a small cost of calling from BPF but a lot more unaccounted cost doing work in the syscall runtime. That work in the runtime needs to be accounted for

programs/bpf/c/src/return_data/return_data.c

sdk/program/src/program.rs

runtime/src/message_processor.rs

programs/bpf_loader/src/syscalls.rs

jackcmay · 2021-09-09T16:40:23Z

programs/bpf_loader/src/syscalls.rs

+            // Return the actual length, rather the length returned
+            *result = Ok(return_data.len() as u64);
+        } else {
+            question_mark!(


Charge base cost always, then charge byte cost if there are bytes to get

programs/bpf_loader/src/syscalls.rs

sdk/program/src/pubkey.rs

sdk/src/compute_budget.rs

jackcmay · 2021-09-09T22:45:36Z

sdk/program/src/program.rs

+        if size == 0 {
+            None
+        } else {
+            let size = std::cmp::min(size as usize, MAX_RETURN_DATA);


nit: use a use for std::cmp

I agree having the full namespace there is a bit much. Since this is in a target == bpf block, we can't have a use std::cmp::min; at the top without `#[allow(unused)]'. So I've put the use in the same block.

jackcmay

Looks great besides one last nit ;-)

mvines · 2021-09-09T23:32:32Z

sdk/src/process_instruction.rs

+    /// The general form is:
+    ///
+    /// ```notrust
+    /// "Program return data: <program-generated-data-in-base64>"


Suggested change

/// "Program return data: <program-generated-data-in-base64>"

/// "Program return data: <program-id> <program-generated-data-in-base64>"

Thanks for catching that, fixed.

Pull request has been modified.

This consists of: - syscalls - passing return data from invoked to invoker - printing to stable log - rust and C SDK changes

Requires: - solana-labs/solana#19548 - solana-labs/solana#19318 Signed-off-by: Sean Young <[email protected]>

Requires solana-labs/solana#19548 Signed-off-by: Sean Young <[email protected]>

seanyoung marked this pull request as draft September 1, 2021 10:27

seanyoung force-pushed the return-data-impl branch from 1ccad05 to 57b42a1 Compare September 1, 2021 10:29

seanyoung force-pushed the return-data-impl branch from 57b42a1 to 99e1f45 Compare September 1, 2021 14:13

seanyoung added a commit to seanyoung/solang that referenced this pull request Sep 1, 2021

Use new solana mechanism for return data

c7514ad

Requires: - solana-labs/solana#19548 - solana-labs/solana#19318 Signed-off-by: Sean Young <[email protected]>

seanyoung mentioned this pull request Sep 1, 2021

Use new solana mechanism for return data hyperledger-solang/solang#485

Merged

seanyoung force-pushed the return-data-impl branch 3 times, most recently from dc92466 to d54e41d Compare September 2, 2021 16:23

seanyoung marked this pull request as ready for review September 2, 2021 16:24

seanyoung requested review from jackcmay and mvines September 2, 2021 16:24

seanyoung force-pushed the return-data-impl branch 4 times, most recently from e6c99ca to 516cdaa Compare September 8, 2021 17:08

mvines reviewed Sep 8, 2021

View reviewed changes

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

mvines reviewed Sep 8, 2021

View reviewed changes

mvines previously approved these changes Sep 8, 2021

View reviewed changes