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

Add CheckTx API #1675

Merged
merged 5 commits into from
May 10, 2019
Merged

Add CheckTx API #1675

merged 5 commits into from
May 10, 2019

Conversation

tjanez
Copy link
Member

@tjanez tjanez commented May 2, 2019

Closes: #1555.

@tjanez tjanez requested review from kostko and Yawning May 2, 2019 16:31
@tjanez tjanez force-pushed the tjanez/feature/add-checktx-api branch from 789b6f1 to b95bf35 Compare May 2, 2019 18:05
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

I think we should not handle checks as separate API methods. The reasoning is that execution also must run the exact same check before proceeding with execution which would lead to some duplication and could be easily forgotten by the runtime implementors.

So instead we should just add a flag (e.g., check_only) to the transaction Context and then format the methods as follows:

pub fn insert(/* ... */) -> Fallible<Option<FooBar>> {
    // ... perform checks here ...
    if ctx.check_only {
        return Ok(None);
    }
    // ... actual execution here ...
    Ok(Some(result))
}

The only downside is that this adds Options to all results as otherwise you need to generate a valid return value in that check only branch, so for the example insert call it would look like this (since the API call also uses an Option in its result type):

pub fn insert(args: &KeyValue, ctx: &mut TxnContext) -> Fallible<Option<Option<String>>> {
    // ...
}

An alternative could be defining a special error that would signify that the check was successful and this error would be transparently handled by the dispatcher and convert it into a successful result with no output value, for example:

use ekiden_runtime::transaction::dispatcher::CheckOnlySuccess;

pub fn insert(/* ... */) -> Fallible<FooBar> {
    // ... perform checks here ...
    if ctx.check_only {
        return Err(CheckOnlySuccess.into());
    }
    // ... actual execution here ...
    Ok(result)
}

I think I would prefer this last approach.

go/worker/common/host/protocol/types.go Outdated Show resolved Hide resolved
go/worker/common/host/sandboxed_test.go Outdated Show resolved Hide resolved
go/worker/common/host/sandboxed_test.go Outdated Show resolved Hide resolved
go/worker/common/host/sandboxed_test.go Outdated Show resolved Hide resolved
runtime/src/dispatcher.rs Outdated Show resolved Hide resolved
runtime/src/dispatcher.rs Outdated Show resolved Hide resolved
runtime/src/transaction/dispatcher.rs Outdated Show resolved Hide resolved
tests/runtimes/simple-keyvalue/src/main.rs Outdated Show resolved Hide resolved
tests/runtimes/simple-keyvalue/api/src/api.rs Outdated Show resolved Hide resolved
@tjanez tjanez force-pushed the tjanez/feature/add-checktx-api branch 2 times, most recently from 1f077d4 to b68b700 Compare May 6, 2019 16:28
@tjanez
Copy link
Member Author

tjanez commented May 6, 2019

An alternative could be defining a special error that would signify that the check was successful and this error would be transparently handled by the dispatcher and convert it into a successful result with no output value, for example:

use ekiden_runtime::transaction::dispatcher::CheckOnlySuccess;

pub fn insert(/* ... */) -> Fallible<FooBar> {
    // ... perform checks here ...
    if ctx.check_only {
        return Err(CheckOnlySuccess.into());
    }
    // ... actual execution here ...
    Ok(result)
}

I think I would prefer this last approach.

I've refactored my PR to follow this approach. @kostko, please take another look.

@tjanez tjanez force-pushed the tjanez/feature/add-checktx-api branch from b68b700 to efc26fc Compare May 7, 2019 08:26
@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #1675 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1675   +/-   ##
=======================================
  Coverage   55.22%   55.22%           
=======================================
  Files         200      200           
  Lines       19082    19082           
=======================================
  Hits        10538    10538           
  Misses       7461     7461           
  Partials     1083     1083
Impacted Files Coverage Δ
go/worker/common/host/protocol/types.go 20% <ø> (ø) ⬆️
go/worker/common/host/mock.go 85.36% <100%> (ø) ⬆️
go/worker/compute/committee/node.go 48.78% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0335143...efc26fc. Read the comment docs.

@codecov
Copy link

codecov bot commented May 7, 2019

Codecov Report

Merging #1675 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1675      +/-   ##
==========================================
- Coverage   56.46%   56.45%   -0.02%     
==========================================
  Files         206      206              
  Lines       19548    19548              
==========================================
- Hits        11038    11036       -2     
- Misses       7360     7361       +1     
- Partials     1150     1151       +1
Impacted Files Coverage Δ
go/worker/common/host/protocol/types.go 20% <ø> (ø) ⬆️
go/common/runtime/runtime.go 100% <ø> (ø) ⬆️
go/worker/compute/committee/node.go 48.78% <100%> (ø) ⬆️
go/worker/common/host/mock.go 85.36% <100%> (ø) ⬆️
go/roothash/tests/tester.go 94.25% <0%> (-0.96%) ⬇️
go/storage/client/client.go 59% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e31a1dc...5f3c681. Read the comment docs.

@tjanez tjanez force-pushed the tjanez/feature/add-checktx-api branch 2 times, most recently from 1bc7dd4 to e6420ab Compare May 7, 2019 09:19
Copy link
Member

@kostko kostko left a comment

Choose a reason for hiding this comment

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

Nice work, this looks pretty good! Only some minor nits and after you fix those and mark the PR as ready for review it is good to merge!

go/common/runtime/runtime.go Outdated Show resolved Hide resolved
go/worker/common/host/protocol/types.go Outdated Show resolved Hide resolved
go/worker/common/host/sandboxed_test.go Outdated Show resolved Hide resolved
tests/runtimes/simple-keyvalue/src/main.rs Outdated Show resolved Hide resolved
@tjanez tjanez force-pushed the tjanez/feature/add-checktx-api branch from e6420ab to 1cf2633 Compare May 9, 2019 15:13
tjanez added 5 commits May 10, 2019 11:48
This is in preparation to adding the CheckTx API to the worker-host
protocol and ensuring we use a consistent naming scheme.
This will allow worker host to request runtime-specific light-weight
transaction checks.
Add a simple check to the insert() method.
@tjanez tjanez force-pushed the tjanez/feature/add-checktx-api branch from 1cf2633 to 5f3c681 Compare May 10, 2019 09:52
@tjanez tjanez changed the title [WIP] Add CheckTx API Add CheckTx API May 10, 2019
@tjanez tjanez marked this pull request as ready for review May 10, 2019 09:59
@tjanez tjanez requested a review from peterjgilbert as a code owner May 10, 2019 10:00
@tjanez tjanez merged commit 5f3c681 into master May 10, 2019
@tjanez tjanez deleted the tjanez/feature/add-checktx-api branch May 10, 2019 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtimes should expose a CheckTx API
2 participants