Yield Execution #10415
Yield Execution #10415
Conversation
This reverts commit bf543b6.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #10415 +/- ##
==========================================
- Coverage 72.42% 72.30% -0.13%
==========================================
Files 732 732
Lines 149935 150348 +413
Branches 149935 150348 +413
==========================================
+ Hits 108594 108705 +111
- Misses 36421 36710 +289
- Partials 4920 4933 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
saketh-are
requested review from
wacban and
nagisa
and removed request for
wacban
| wasm_yield_create_base: { old: 300_000_000_000_000, new: 50_000_000_000_000 } | ||
| wasm_yield_create_byte: { old: 300_000_000_000_000, new: 10_000_000 } | ||
| wasm_yield_resume_base: { old: 300_000_000_000_000, new: 50_000_000_000_000 } | ||
| wasm_yield_resume_byte: { old: 300_000_000_000_000, new: 100_000_000 } |
There was a problem hiding this comment.
Do these fees allow for testing? I remember you mentioning the fees in nightly were too large to test right now.
There was a problem hiding this comment.
Yes, I believe they are too high. To allow local testing I have been modifying this file and using much lower costs; it is not really a blocker if they continue to be set too high until we have the proper values.
| yield_create_base -> 61 [2% host] | ||
| yield_create_byte -> 62 [2% host] | ||
| yield_resume_base -> 63 [3% host] | ||
| yield_resume_byte -> 64 [3% host] |
There was a problem hiding this comment.
Note to self: check if these need to be excluded from the profiles while not stable.
| /// * Fees for reading the `method_name` and `arguments`; | ||
| /// * Fees for writing the Data ID to the output register; | ||
| /// * Fees for setting up the receipt. | ||
| pub fn promise_yield_create( |
There was a problem hiding this comment.
I added a note about this in the tracking issue task list.
core/store/src/lib.rs
Outdated
| pub fn get_yielded_promise_indices( | ||
| trie: &dyn TrieAccess, | ||
| ) -> Result<YieldedPromiseQueueIndices, StorageError> { | ||
| Ok(get(trie, &TrieKey::YieldedPromiseQueueIndices)?.unwrap_or_default()) | ||
| } | ||
|
|
||
| pub fn set_yielded_promise_indices( | ||
| state_update: &mut TrieUpdate, | ||
| yielded_promise_indices: &YieldedPromiseQueueIndices, | ||
| ) { | ||
| set(state_update, TrieKey::YieldedPromiseQueueIndices, yielded_promise_indices); | ||
| } | ||
|
|
||
| // Records the yielded promise and appends it to the yielded promises queue. | ||
| pub fn set_yielded_promise( | ||
| state_update: &mut TrieUpdate, | ||
| yielded_promise_indices: &mut YieldedPromiseQueueIndices, | ||
| account_id: AccountId, | ||
| data_id: CryptoHash, | ||
| expires_at: BlockHeight, | ||
| ) { | ||
| set(state_update, TrieKey::YieldedPromise { data_id }, &YieldedPromise { account_id }); | ||
|
|
||
| set( | ||
| state_update, | ||
| TrieKey::YieldedPromiseQueueEntry { index: yielded_promise_indices.next_available_index }, | ||
| &YieldedPromiseQueueEntry { data_id, expires_at }, | ||
| ); | ||
| yielded_promise_indices.next_available_index = yielded_promise_indices | ||
| .next_available_index | ||
| .checked_add(1) | ||
| .expect("Next available index for yielded promise exceeded the integer limit"); | ||
| } | ||
|
|
||
| pub fn get_yielded_promise( | ||
| trie: &dyn TrieAccess, | ||
| data_id: CryptoHash, | ||
| ) -> Result<Option<YieldedPromise>, StorageError> { | ||
| get(trie, &TrieKey::YieldedPromise { data_id }) | ||
| } | ||
|
|
||
| pub fn remove_yielded_promise(state_update: &mut TrieUpdate, data_id: CryptoHash) { | ||
| state_update.remove(TrieKey::YieldedPromise { data_id }) | ||
| } |
There was a problem hiding this comment.
We should make sure there's absolutely no way for these columns to get created before the feature is stabilized. One way would be to still add the usual nightly compile-time feature gates -- it is only in the contract runtime that we gate using a runtime variables; or alternatively we could assert! here that the feature is enabled through parameters.
I'm not sure if these are created lazily (at least I didn't spot any code that creates the columns explicitly), and while the contract runtime gates the relevant host functions, adding additional layers of defense wouldn't hurt.
There was a problem hiding this comment.
The gated compilation turned out to be quite a long thread to pull on (needing to gate any dependent code, associated imports, etc.) so I went with some asserts for now.
| // Yields are only resumable by the account which created them | ||
| if yielded_promise.account_id != *self.account_id { | ||
| return Err( | ||
| HostError::YieldedPromiseNotFound { data_id: data_id.to_string() }.into() |
There was a problem hiding this comment.
We probably should maintain that the error is the same when the entry does not exist in the storage and when it is malformed for the function call. The information on the distinction is not particularly useful to a well-intentioned user. I filled this as a followup point in the tracking issue.
| @@ -1499,10 +1500,70 @@ impl Runtime { | |||
| prefetcher.clear(); | |||
| } | |||
|
|
|||
| // Resolve timed-out yielded promises | |||
There was a problem hiding this comment.
style: should perhaps consider splitting this out into a function. The apply is quite a bit too large as is.
There was a problem hiding this comment.
Makes sense, I will look into sending a follow-up PR refactoring apply.
|
On coverage: we're hoping to finish up the PR as a follow up, including test coverage. This PR has been becoming humongous and its less trivial than we'd like to work on our respective parts without this being in mainline. |
Implements the
promise_yield_createandpromise_yield_resumehost functions described in NEP 519.promise_yield_create: Creates a promise callback with a special input data dependency to be delivered via a resume call.promise_yield_resume: Delivers input data for a yielded promise, triggering execution of the callback.A fixed-length timeout
yield_timeout_length_in_blocksis configured at the protocol level. If a yielded promise is not resumed within the timeout length, the callback is triggered automatically.Co-authored by: Simonas Kazlauskas git@kazlauskas.me