[Discussion] Staking should behave like Transfer

[MaksymZavershynskyi]

This is an expanded continuation of the discussion here: https://github.com/nearprotocol/nearcore/issues/1713

It is not clear to me what is the benefit of not making staking behave in exactly the same way the transfer action behaves. Here is how I think it should work:

• First we change StakeAction to be:

  
  pub struct StakeAction {
  amount: BalanceDiff,
  public_key: Option<PublicKey>
  }

pub type BalanceDiff = i128;

* When a person wants to stake an additional X amount of tokens they issue a transaction with `StakeAction` with `amount=X`. Additionally, they might specify a new public key that they want to be used once the staking goes through. If nothing is specified the old key is used.
* When we convert the staking transaction to the staking receipt if `StakeAction::amount` is positive we deduct it from the `Account::amount` if it is negative we deduct it from `Account::locked`, similarly to `total_deposit` we can have `total_locked`, see: https://github.com/nearprotocol/nearcore/blob/9b66d2b4231a2607f2ace3a4d44be6ab46e28cf9/runtime/runtime/src/config.rs#L184
* When the receipt with staking action arrives we deposit the amount either to the `amount` or to the `locked` field of the account, depending on its sign.

Benefits compared to the current implementation:
1. It is exactly the same mental model as transfers;
2. Processing of staking action becomes as simple as processing of transfer action, compare the difference between the two now: https://github.com/nearprotocol/nearcore/blob/e76609702177767f2ca5d1e2b5a45006ac61b6e8/runtime/runtime/src/actions.rs#L202
3. **The account information is not up to date while the staking receipt is in flight!** Currently if person A issues a staking transaction and concurrently someone calls a smart contract method on A (could be called by a different account or the same account) that creates a transfer action that takes X amount of tokens and sends it to account C, the staking transaction might fail upon the processing of the receipt;
4. We can benefit from all the checks that filter out invalid transactions early on. These checks currently work only for transfers and balances attached to function calls.
5. It is clear from the discussion in #1713 that people agree that we need to change how we do unstaking. The change proposed here removes the need for the separate unstaking action altogether;
6. Staking receipt cannot fail, only staking transaction can. Similarly to how transfers work.

Disadvantages:
* I've heard people were saying that the current model is intuitive -- a user needs to specify what their stake should be after once it is applied. However, I am not sure what can be more intuitive than an existing mental model that everyone is already used to, which is transfers.

[MaksymZavershynskyi]

Places that do the checks that verify and discard invalid transactions early on. With the proposed change these checks will also discard invalid staking actions early on:

• A check after converting transactions to receipts: https://github.com/nearprotocol/nearcore/blob/9b66d2b4231a2607f2ace3a4d44be6ab46e28cf9/runtime/runtime/src/lib.rs#L1061
• A check when converting an individual transaction to receipt: https://github.com/nearprotocol/nearcore/blob/9b66d2b4231a2607f2ace3a4d44be6ab46e28cf9/runtime/runtime/src/lib.rs#L360
• A check before we add a transaction to the mempool: https://github.com/nearprotocol/nearcore/blob/e0dcbb9fb3be06bd8569c0aeb18a1b4a4cd0497e/chain/client/src/client.rs#L1054
• A check when we remove transactions from the mempool: https://github.com/nearprotocol/nearcore/blob/e7b430eafec13127764e5e6c99259df068ad8162/near/src/runtime.rs#L667

[bowenwang1996]

Unfortunately this doesn't quite work. When you want to decrease your stake, since the balance does not change immediately, we have to record your intention, which means that we have to at least add another field in the account. I would rather that we don't add anything into account that is not strictly necessary because people do get charged for it.

[MaksymZavershynskyi]

When you want to decrease your stake, since the balance does not change immediately

Are you suggesting that in addition to subtracting the staked amount from Account::amount we need to record somewhere on the account level what is the amount of stake is currently in-flight with the receipt? What do we need it for?

I would rather that we don't add anything into account that is not strictly necessary because people do get charged for it.

Please consider again the list of the benefits this change offers -- it is a pretty long list.

[MaksymZavershynskyi]

@bowenwang1996 I just checked Account::locked it is not used outside the runtime. That seems to contradict what you are saying here:

When you want to decrease your stake, since the balance does not change immediately, we have to record your intention, which means that we have to at least add another field in the account.

[bowenwang1996]

We don't touch your account right now if you already staked and want to decrease your stake. Because of that, we need to record your intention in case you send another staking transaction in the same epoch, assuming we don't change how staking is handled from the blockchain side (I would highly recommend that we don't change it as doing so might very well delay our progress by several weeks, if not months).

As for the benefits, I don't completely agree that the transfer model is better. When I stake I am more interested in the total stake that is locked right now rather than thinking about how much I should adjust. Also even if we do this, the logic would still be similar and I don't see how it is as simple as account transfer.

[MaksymZavershynskyi]

We don't touch your account right now if you already staked and want to decrease your stake. Because of that, we need to record your intention in case you send another staking transaction in the same epoch, assuming we don't change how staking is handled from the blockchain side (I would highly recommend that we don't change it as doing so might very well delay our progress by several weeks, if not months).

The whole point of this change is to avoid having this "declaration of intent" thing. There is no need to declare an intent if we subtracted the tokens that are going to be staked immediately when the transaction is processed.

assuming we don't change how staking is handled from the blockchain side

I will write here again what I wrote above: Account::locked is not used by the chain at all. So this change should be completely transparent to anything happening outside the Runtime.

I would highly recommend that we don't change it as doing so might very well delay our progress by several weeks, if not months

Could you elaborate on exactly how is it going to delay us by weeks or months?

As for the benefits, I don't completely agree that the transfer model is better. When I stake I am more interested in the total stake that is locked right now rather than thinking about how much I should adjust. Also even if we do this, the logic would still be similar and I don't see how it is as simple as account transfer.

Okay, I see you addressed the first bullet of benefits (the mental model one). Could you please address the remaining 5 bullets describing the benefits of this change?

[bowenwang1996]

The whole point of this change is to avoid having this "declaration of intent" thing. There is no need to declare an intent if we subtracted the tokens that are going to be staked immediately when the transaction is processed.

I don't understand how you do it when you already staked and want to decrease your stake.

I will write here again what I wrote above: Account::locked is not used by the chain at all. So this change should be completely transparent to anything happening outside the Runtime.

I understand. That's why I said "assuming".

Okay, I see you addressed the first bullet of benefits (the mental model one). Could you please address the remaining 5 bullets describing the benefits of this change?

I also addressed second point. For this

The account information is not up to date while the staking receipt is in flight! Currently if person A issues a staking transaction and concurrently someone calls a smart contract method on A (could be called by a different account or the same account) that creates a transfer action that takes X amount of tokens and sends it to account C, the staking transaction might fail upon the processing of the receipt; I don't quite understand. Staking transaction creates a local receipt that either succeeds or fails. If it succeeds then the account information is updated. If it fails then the account information is not updated. I think I am missing something here.

It is clear from the discussion in #1713 that people agree that we need to change how we do unstaking. The change proposed here removes the need for the separate unstaking action altogether;

How does this work? If you want to unstake you send a transaction with amount = 0 or amount = the total amount staked?

We can benefit from all the checks that filter out invalid transactions early on. These checks currently work only for transfers and balances attached to function calls.

I agree. But we can also add this check for staking.

[MaksymZavershynskyi]

I don't understand how you do it when you already staked and want to decrease your stake.

Suppose you want to decrease your stake by 10 then you send a transaction with StakeAction::amount=-10.

I also addressed second point.

The second point states that fn action_stake is much more complex than fn action_transfer and this change will fix this. I do not see where you addressed this, could you please paste a quote or rephrase?

How does this work? If you want to unstake you send a transaction with amount = 0 or amount = the total amount staked?

You send the staking action with StakeAction::amount=-<currently locked amount>.

I agree. But we can also add this check for staking.

Notice, that there are four checks. You can have these checks with the current model, but they will not be able to catch some failures. For instance, after we convert a staking transaction into a staking receipt it can still fail when receipt is processed. With transfers, it cannot happen. With the current staking method, a contract can create an invalid staking receipt that will fail once it is processed. With transfers and the proposed change, it will not be possible. Once we implement this change ActionError::TriesToUnstake and ActionError::TriesToStake will be gone.

Please also address the benefits 3-6.

[bowenwang1996]

Suppose you want to decrease your stake by 10 then you send a transaction with StakeAction::amount=-10.

Sorry for being unclear. I meant I don't understand how it works on the account level.

The second point states that fn action_stake is much more complex than fn action_transfer and this change will fix this. I do not see where you addressed this, could you please paste a quote or rephrase?

I don't see how with this change the logic will be simpler.

You send the staking action with StakeAction::amount=- I would say that's not as intuitive as sending a unstaking transaction or even a staking transaction with amount 0 because you first need to look up the locked amount. Another problem is that in case reward is added in the same block, things will be messed up since reward is added before transaction get processed and you can just unintentionally unstake a large chunk of your stake.

I agree with 4. For 3 and 6 I see the point, but I am not sure I understand why transaction failure is better than receipt failure. 5 is addressed above.

[MaksymZavershynskyi]

Sorry for being unclear. I meant I don't understand how it works on the account level.

The same way it works with transfers. When we process a transaction that contains StakeAction if StakeAction::amount > 0 we deduct it from Account::amount if StakeAction::amount < 0 we deduct if from Account::locked; we then create a receipt. When we process the receipt if StakeAction::amount > 0 we deposit it to Account::locked, if StakeAction::amount < 0 we deposit it to Account::amount.

I don't see how with this change the logic will be simpler.

Here is the current code:

pub(crate) fn action_stake(
    account: &mut Option<Account>,
    result: &mut ActionResult,
    account_id: &AccountId,
    stake: &StakeAction,
) {
    let mut account = account.as_mut().unwrap();
    let increment = stake.stake.saturating_sub(account.locked);
    if account.amount >= increment {
        if account.locked == 0 && stake.stake == 0 {
            // if the account hasn't staked, it cannot unstake
            result.result = Err(ActionError::TriesToUnstake(account_id.clone()));
            return;
        }
        result.validator_proposals.push(ValidatorStake {
            account_id: account_id.clone(),
            public_key: stake.public_key.clone(),
            amount: stake.stake,
        });
        if stake.stake > account.locked {
            account.amount -= increment;
            account.locked = stake.stake;
        }
    } else {
        result.result = Err(ActionError::TriesToStake(
            account_id.clone(),
            stake.stake,
            account.locked,
            account.amount,
        ));
    }
}

This is how the code will look like after the change:

pub(crate) fn action_stake(account: &mut Option<Account>, stake: &StakeAction) {
    let mut account = account.as_mut().unwrap();
    if stake.amount > 0 {
        account.locked += stake.amount; 
    } else {
        account.amount += stake.amount;
    }
}

Forget that it is much shorter, the proposed code not only cannot throw errors, but it does not need for us to define custom errors specifically for this place in code that we have to process somewhere upstream.

I would say that's not as intuitive as sending a unstaking transaction or even a staking transaction with amount 0 because you first need to look up the locked amount.

You need to do the same action with the transfer. If Alice wants Bob to have 100 tokens she needs to first lookup his account balance and then send the remaining tokens.

Another problem is that in case reward is added in the same block, things will be messed up since reward is added before transaction get processed and you can just unintentionally unstake a large chunk of your stake.

How exactly an accidental unstaking of your own stake is going to happen?

but I am not sure I understand why transaction failure is better than receipt failure

Because:

• Contract that issue staking receipts can be certain that they will not fail. Without it contract developers need to deal with error handling;
• Failures that happen earlier in the code are generally better than the errors that happen later. In general fail-fast mode is always better: https://en.wikipedia.org/wiki/Fail-fast

Do you agree with 3?

5 is addressed above.

I have replied to your argument about 5. Does it clarify how unstaking works?

[bowenwang1996]

The same way it works with transfers. When we process a transaction that contains StakeAction if StakeAction::amount > 0 we deduct it from Account::amount if StakeAction::amount < 0 we deduct if from Account::locked; we then create a receipt. When we process the receipt if StakeAction::amount > 0 we deposit it to Account::locked, if StakeAction::amount < 0 we deposit it to Account::amount.

This unfortunately doesn't work because we cannot reduce your locked amount right away. Otherwise staking invariant will break. That is also why the logic you see for processing staking action is not as simple as token transfer.

[MaksymZavershynskyi]

This unfortunately doesn't work because we cannot reduce your locked amount right away.

Are you saying we can only increase the stake and unstake entirely but not decrease the stake? What is the reason for that?

[bowenwang1996]

Are you saying we can only increase the stake and unstake entirely but not decrease the stake? What is the reason for that?

Yes. The reason is how stake return and rewards are calculated. It is not very easy to explain but basically there is some invariant we maintain to make sure rewards and stake are returned properly and it relies on the assumption that staking cannot decrease your stake in the same epoch. Even though you might think this is very suboptimal, I have to say that this is what we agreed on and it is a collective effort between more than one person who work on chain. I am for sure not saying that a better solution doesn't exist, but we have spent some time on this and have not come up with a better solution. Also, depending on how the solution works, at this point it might be too late to change as it might involve refactoring multiple parts of chain.

[frol]

StakeAction::amount=-10 is not intuitive to me. I would rather forbid decreasing action to avoid confusion (there are no transfer transactions of a negative amount).

As a note, I have seen questions on the staking transactions like “why can I stake 10N twice?”, so a cumulative amount seem to make sense to people. However, we can easily fix this on the tooling side and I also believe that we can implement both cumulative and absolute amount staking on top of both the current and the proposed design (+ unstaking)

[lexfrl]

I think the main issue is that account.amount is "The sum of amount and staked is the total value of the account.". The fact that account.amount mixes liquid and locked tokens makes it harder to reason about and forces to keep in mind about the locked part each time when user works with. If we separate locked and amount things become much easier.

Idempotent APIs are much easier to reason about. "I want to stake 'x' amount of my tokens" vs "I want to change amount of staked tokens by".

[evgenykuzyakov]

account.amount is only liquid tokens account.locked is currently locked tokens. This value can only be decreased by passing ValidatorAccountsUpdate from epoch manager.

To have incremental updates we need to record the latest desired stake in the account. Something like account.desired which we considered before, but it was not needed for the final solution.

This creates an issue since desire to unstake some or all doesn't modify locked or amount from the account immediately. While positive increment should take the different from the amount immediately.

To achieve what @nearmax want, we may add an additional field to the Stake action that indicates the prepaid balance that is taken at the moment when a Stake is created from the amount. It wouldn't break the invariance and we can basically compute the stake properly. But we already have a Transfer option that does exactly the desired action, so there are no need to modify Stake action to increment current stake.

Now to have a permanent Unstake is not as simple. We can't get the current locked balance out immediately and have to wait for 3 epochs to get it unlocked back to the amount. The issue is the there can be conflict of Stake and Unstake that can happen in the next receipt. The only way to resolve it now is to rely that the receipts are processed in the same order as they are generated from 1 account. So if we unstaked first and then tried to stake again, the stake call will fail because the contract no longer allows to stake. But if we staked first and then unstaked again, then the receipt of unstake should arrive later and override the stake action.

[evgenykuzyakov]

Should we close it?