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

[chainstate] deduct fee before running the transaction #3213

Closed
jcnelson opened this issue Jul 22, 2022 · 14 comments
Closed

[chainstate] deduct fee before running the transaction #3213

jcnelson opened this issue Jul 22, 2022 · 14 comments

Comments

@jcnelson
Copy link
Member

Right now, we deduct the transaction fee after running the transaction. This can lead to problematic transactions which run to completion successfully, but where the sender or sponsor doesn't have enough STX to pay the fee. The inclusion of these transactions invalidate the block, so miners are forced to run these transactions to completion before they know not to include them. #3212 implements a mitigation in a backwards-compatible way, but ideally we would simply bill transaction fees before running transactions in the first place. However, doing so is a consensus-breaking change.

@jcnelson jcnelson self-assigned this Jul 22, 2022
@jcnelson jcnelson added this to To do in Stacks 2.1 via automation Jul 22, 2022
@project-bot project-bot bot added this to New Issues in Stacks Blockchain Board Jul 22, 2022
@jcnelson
Copy link
Member Author

One option is to make it so that if you get an ::InvalidFee error, we could roll back the tx and just debit the fee and materialize no other changes. This is similar to how Ethereum works. But, we'll want to ask smart contract devs how they feel about this change. Note that wallets could still determine that a transaction is likely (or definitely will) hit this case, because Clarity can be statically analyzed.

@jcnelson jcnelson moved this from To do to In progress in Stacks 2.1 Jul 25, 2022
@jcnelson
Copy link
Member Author

jcnelson commented Jul 26, 2022

There are basically two ways we can make sure miners can collect the fee even if the transaction's sender runs out of STX while the transaction is being processed:

  • Option 1:

    • Deduct the transaction fee from the sender. If this fails due to a lack of STX, then the whole block is invalid (i.e. these transactions are never mined).
    • Process the transaction payload. If this fails, then all changes are rolled back, but the transaction is mined and the fee remains deducted.
  • Option 2:

    • Check to see that the sender has enough STX to pay the fee. If not, then the whole block is invalid (i.e. these transactions are never mined).
    • Process the transaction payload. If this fails, then all changes are rolled back, but the transaction is mined and the fee remains deducted.
    • Deduct the fee from the sender. If this fails (e.g. suppose the transaction spent all the sender's STX), then all changes are rolled back, the fee is deducted from the sender's account, and the transaction is mined.

The difference between these two options is seen in the following example.

Suppose Alice wants to register five BNS names at once. To do so, she sends a smart contract transaction that preorders them all in a (begin) block:

(begin
   (if (is-err (contract-call? SP000000000000000000002Q6VF78.bns name-preorder 0x399c7f0cf9f90d08da6c96c605a9166c2c2f5138 u1000000))
      true
      true
   )
   (if (is-err (contract-call? SP000000000000000000002Q6VF78.bns name-preorder 0x6248ebcf673117980830191b06000a3a7eea7311 u1000000))
      true
      true
   )
   (if (is-err (contract-call? SP000000000000000000002Q6VF78.bns name-preorder 0xfb8b09d8f56532776ca6f07473522e185f8a094b u1000000))
      true
      true
   )
   (if (is-err (contract-call? SP000000000000000000002Q6VF78.bns name-preorder 0x13a8a3ac65155072b680973ac1b9fec5f4e64525 u1000000))
      true
      true
   )
   (if (is-err (contract-call? SP000000000000000000002Q6VF78.bns name-preorder 0x8955ce2197f58580a88aa6f3544a1c88768801a7 u1000000))
      true
      true
   )
)

As you can see, she's spending 5 STX (5,000,000 uSTX) in total. So, suppose she loads up her wallet with exactly 5 STX. But, suppose that she pays a transaction fee of 0.1 STX, meaning that she actually needed 5.1 STX total in her wallet.

Today, this transaction would not be mined at all. Alice's transaction would run to completion, and when the software tries to deduct the fee from her account, it fails for lack of STX. When a miner discovers this, it omits the transaction from the block entirely (because its presence in the block would invalidate the block).

With Option 1, the transaction gets mined, and Alice successfully preorders the first four names. The fifth one fails, because she only has 0.9 STX available.

With Option 2, the transaction gets mined, but none of the preorders materialize since Alice didn't have enough STX to pay the fee at the end.

My ask to the community is this: which fee-processing option would you like to see in Stacks 2.1? Please emoji-vote:

  • 👍 for Option 1
  • 😄 for Option 2

@jiga
Copy link

jiga commented Jul 26, 2022

option 2 may be better from apps sending transaction as they don't have to track which parts of previous transaction were successful and which weren't.

@jcnelson
Copy link
Member Author

jcnelson commented Jul 27, 2022

@jiga Smart contract authors today already have to deal with the possibility that any asset-moving operation could fail for lack of sufficient funds (Clarity doesn't let you avoid handling the error case), which is why it's not clear to me why "they don't have to track which parts of previous transaction were successful and which weren't" is a win -- they're already tracking this, and implementing Option 2 doesn't change that requirement.

@radicleart
Copy link

@jcnelson option 1 sounds more intelligent and expected behaviour to me - especially if this were say a bulk transfer of 200 NFTs and it just fails because of the 199th.

Are there complexities though that the miner makes this calculation but Alice is running other transactions concurrently, which affect her balance and the outcome of the miners calculation? Are there different technical risks associated with implementation of each option?

@BowTiedDeployer
Copy link

@jiga Smart contract authors today already have to deal with the possibility that any asset-moving operation could fail for lack of sufficient funds (Clarity doesn't let you avoid handling the error case), which is why it's not clear to me why "they don't have to track which parts of previous transaction were successful and which weren't" is a win -- they're already tracking this, and implementing Option 2 doesn't change that requirement.

I think what @jiga is saying is related to the fact that right now the operation is either successful or failed. With the first implementation, it gets partially successful as some things happened but not the intended action ( to mint exactly 5 bns ).

From an app dev perspective, it is cleaner to know if happened exactly what was intended or if it failed. Let's say
instead of having 5 claims of the same type the contract call is a wrapper for other 5 claims each with 1 stx from different contracts ( also applies with 5 bns calls, gets harder with different contracts in the app implementation ). In my app after you claim those 5 you get access to something. If it fails (because of fees) you get nothing. What happens if it only claims the first 2, 3 or 4?

  1. Make the user claim again all of them and the user lost what was successfully spent
  2. Create extra cases that take into consideration every single option from the place it claimed successfully and continue specifically from there

@friedger
Copy link
Collaborator

friedger commented Jul 27, 2022

@BowTiedDeployer The handling of partially failed sequences of contract calls (fails after first 2 or 3 or so) is the same with option 1 and 2.

The question is more about whether to deduce the fees from the users balance first and the execute as far as possible, or to execute as far as possible and then fail everything due to low balance.

For the case of the 5 claims, if we assume that the sequence of 5 claims needs to be executed the only difference would be different error codes. Option 1) would generate a custom error code from your function because only 4 of the 5 claims succeeded. Option 2) would generate an invalid fees error because the 5 claims succeeded but there were not enough stx for the fees.

I am for option 1) because it gives the developers the option to make use of as much stx as possible from the user. Also, a function call to wipe a wallet would be able to determine the correct value to transfer:

Assume the wipe call costs 0.1 STX and the user has a balance of 5 STX before the call. With option 1) the wipe function checks the balance and sees 4.9 STX, the call would succeed. With option 2) the balance would be 5 STX and the tx would fail.

@hozzjss
Copy link

hozzjss commented Jul 27, 2022

I'm for option two
@friedger I think the smart contract dev can add a checker first if possible where the function checks if the user has enough funds to execute
Option 2 is also clearer and aligns with the philosophy of post conditions where what you see is what you pay, with the current stacks implementation it would suck to send another transaction but it is still a much much clearer way

@jcnelson
Copy link
Member Author

jcnelson commented Jul 27, 2022

As @friedger points out, one consequence of Option 2 from a contract implementation standpoint is that the contract doesn't know how much STX to leave in the account to pay the fee (since the fee isn't exposed to Clarity). This could lead to some surprising transaction failures for users who don't have quite enough STX to do what they want to do. Wallets would have a hard time knowing what fee to set, since the contract could spend an arbitrary amount of STX in ways that can't be predicted in advance. While it's possible to expose the fee as a Clarity global variable, there's no way to force contract authors to use it (and it doesn't help you if the STX spend is dependent on data that isn't known at the time the tx is submitted). Option 1 does not have this problem, since the fee is paid in advance.

@jcnelson
Copy link
Member Author

It seems the major argument for Option 2 is that it provides "all or nothing" semantics for transactions. In this example, only four of the five preorders would get mined.

I'd like to remind folks that you still have this in Option 1 -- all you'd need to do is wrap any STX transfer or burn with unwrap-panic. Then if the STX transfer fails due to insufficient funds, the whole transaction aborts. In my example snippet, Alice would simply wrap the contract-call?s to .bns with unwrap-panic to get these semantics (which is less verbose than what I have here).

@friedger
Copy link
Collaborator

friedger commented Jul 27, 2022

Options 2 is more along the current implementation, i.e. that the fees are deducted after the transaction and the the contract sees the full stx balance before deducting the fees.

@hozzjss This is not about sending more txs, but about the stx balance that is available in the tx.

@hozzjss
Copy link

hozzjss commented Jul 28, 2022

Alright I’m convinced opt out is better

@MarvinJanssen
Copy link
Member

MarvinJanssen commented Jul 28, 2022

Option one seems like the more sane choice. Friedger's balance wipe is the perfect example. Without a new constant exposing the transaction fee it will be rather tricky to pull it off with option two.

@fiftyeightandeight
Copy link

Option 1 seems simple enough and straightforward. So the only difference between right now and Option 1 is the switch between the first step and the second step (more or less), correct?

@jcnelson jcnelson moved this from In progress to Review in progress in Stacks 2.1 Aug 1, 2022
Stacks Blockchain Board automation moved this from New Issues to Done Aug 15, 2022
Stacks 2.1 automation moved this from Review in progress to Done Aug 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

8 participants