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 self call (for advanced access keys) #1005

Merged
merged 3 commits into from Jun 18, 2019

Conversation

@evgenykuzyakov
Copy link
Collaborator

commented Jun 14, 2019

Self call is used when the account calls the its own contract. In this case no receipts needed, since all information is available immediately. It can be used by access keys pointed at the owner's account to proxy transactions further.

Added simple test. Need to validate liquid balance is correctly used and accounted. But this issue is present with other calls as well.

@evgenykuzyakov evgenykuzyakov requested review from bowenwang1996 and nearmax Jun 14, 2019

@@ -319,12 +403,13 @@ impl Runtime {
block_index,
nonce.as_ref().to_vec(),
false,
public_key,
),
)
.map_err(|e| format!("wasm async call preparation failed with error: {:?}", e))?;

This comment has been minimized.

Copy link
@nearmax

nearmax Jun 14, 2019

Collaborator

executor::execute returns Err also when the contract code fails (e.g. due to running out of liquid balance) not only when preparation fails. We should change the error message here.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 14, 2019

Author Collaborator

This error should only be a preparation error. Since we need to keep track of the liquid balance in case of contract failure (for whatever reason).

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 14, 2019

Author Collaborator

We return Ok from execute in case the contract run out of liquid balance

This comment has been minimized.

Copy link
@nearmax

nearmax Jun 18, 2019

Collaborator

So when charge_balance_with_limit returns Err then execute returns Ok?

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 18, 2019

Author Collaborator

Yes, but it includes this error within .return_data

),
)
.map_err(|e| format!("wasm async call preparation failed with error: {:?}", e))?;
transaction_result.logs.append(&mut wasm_res.logs);
let balance = wasm_res.frozen_balance;
*leftover_balance += wasm_res.liquid_balance;
*leftover_balance = wasm_res.liquid_balance;

This comment has been minimized.

Copy link
@nearmax

nearmax Jun 14, 2019

Collaborator

Before this change if wasm execution fails , e.g. due to running out of liquid balance, the leftover_balance equals 0, because we lose all liquid balance. With this change if execution fails the leftover balance is equal to the entire liquid balance that was before the execution started, which creates an abuse vector since users can grind validators by sending transactions that fail and get all liquid balance back through the refund.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 14, 2019

Author Collaborator

Contract can fail by calling assert internally. That's why this entire logic has to be changed to account for balances correctly, like it was before. Which means execution runtime should not be able to remove exceed the initial liquid balance, and frozen balance withdrawals should not modify the account balance in case of contract failure

This comment has been minimized.

Copy link
@nearmax

nearmax Jun 18, 2019

Collaborator

Is this the spec that you are currently suggesting?

  • We use liquid balance for: wasm execution, fee for creating initial transaction;
  • We use frozen balance for: promise creation and other transaction creation within contract, rent;

Failure modes:

  • When contract fails due to running out of liquid balance the liquid balance is lost and frozen balance is reverted to value before the execution;
  • When contract fails due to panic (assert or out of bounds) the same rules applies.

Correct?

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 18, 2019

Author Collaborator

Correct. But we also need to account for deposited liquid balance. In case of failure we need to return it back to the liquid balance (since we reverting frozen balance) and the rest of the liquid balance has to be refunded

@@ -321,6 +322,10 @@ pub struct RuntimeContext {
pub random_seed: Vec<u8>,
/// Whether the execution should not charge any costs.
pub free_of_charge: bool,
/// In case this is a function call on by the account owner on their account, we provide

This comment has been minimized.

Copy link
@nearmax

nearmax Jun 14, 2019

Collaborator

Could we always pass public_key to avoid making it an Option? This will make code more consistent.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 14, 2019

Author Collaborator

It's not available for receipts, only for self-calls

This comment has been minimized.

Copy link
@nearmax

nearmax Jun 18, 2019

Collaborator

It looks more consistent to me to have the same context fields available for self and regular contract calls. In case of regular contract calls public_key has the same meaning:
"public key to identify which access key or the public key was used to sign the initial transaction"

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 18, 2019

Author Collaborator

Then we'd have pass it into all receipts. It maybe okay, but it's additional traffic

This comment has been minimized.

Copy link
@nearmax

nearmax Jun 18, 2019

Collaborator

I suggest for now we prioritize consistency over efficiency. It is in line with how we are replacing all transactions with contract calls. We can do optimizations later.

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 18, 2019

Author Collaborator

Makes sense

@@ -108,6 +108,26 @@ pub fn test_smart_contract_simple(node: impl Node) {
validate_tx_result(node_user, root, &hash, 2);
}

pub fn test_smart_contract_self_call(node: impl Node) {

This comment has been minimized.

Copy link
@nearmax

nearmax Jun 14, 2019

Collaborator

Should we have one more test that uses access key to sign the transaction?

This comment has been minimized.

Copy link
@evgenykuzyakov

evgenykuzyakov Jun 14, 2019

Author Collaborator

Yes. We also need tests to verify account balances post execution depending whether it failed or succeeded.

@evgenykuzyakov evgenykuzyakov force-pushed the add-self-call branch from d325d44 to 89d5c8f Jun 17, 2019

@evgenykuzyakov evgenykuzyakov requested a review from nearmax Jun 17, 2019

@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 18, 2019

@nearmax Can you take a look again?

@nearmax
Copy link
Collaborator

left a comment

Replied.

@nearmax
Copy link
Collaborator

left a comment

I suggest we pipe public_key into receipts for consistency and optimize it out later, once we have enough iterations on use cases.

@evgenykuzyakov

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 18, 2019

I suggest we pipe public_key into receipts for consistency and optimize it out later, once we have enough iterations on use cases.

Will work on it in the next PR

@evgenykuzyakov evgenykuzyakov merged commit 8645aae into master Jun 18, 2019

1 check passed

ci/gitlab/add-self-call Pipeline passed on GitLab
Details

@evgenykuzyakov evgenykuzyakov deleted the add-self-call branch Jun 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.