stacks-network / stacks-core

The Stacks blockchain implementation
https://docs.stacks.co
GNU General Public License v3.0
3.01k stars 667 forks source link

Include transactions with insufficient balance #3639

Open obycode opened 1 year ago

obycode commented 1 year ago

Is your feature request related to a problem? Please describe. Transactions that will fail due to an insufficient balance (ex. I transfer 10 STX but I only have 8 STX), are left pending in the mempool until they are dropped after 256 blocks. This causes a lot of confusion from users, because they don't know why their transaction is pending, and also holding up any future transactions (with higher nonces).

Describe the solution you'd like Instead of just leaving these transactions pending, I think the miner should include them in a block as a failing transaction. I believe the current implementation purposely does not do this so that it does not charge the fee for a transaction that is definitely going to fail, basically trying to save the sender from their mistake. In practice, the pain of this pending transaction is much worse than the pain of the wasted fee. A somewhat related change was implemented in 2.1, #3213.

Describe alternatives you've considered Leaving it as-is has proven to cause a lot of confusion from users.

Additional context See discussions in Discord.

kantai commented 1 year ago

I think this is a great idea. The current UX of pending transactions with insufficient balance is pretty bad. In the worst case, the transaction actually ends up in the miner/mempool blacklist and the "pending" transaction is just stuck for 48 hours or until it is RBFed.

donpdonp commented 1 year ago

I remember hearing in a status call months ago that even successfully mined transactions are not removed from the mempool, so that they'd be available if and when a re-org happened to be mined on the new fork.

jcnelson commented 1 year ago

@donpdonp I remember hearing in a status call months ago that even successfully mined transactions are not removed from the mempool, so that they'd be available if and when a re-org happened to be mined on the new fork.

Correct.

@kantai The current UX of pending transactions with insufficient balance is pretty bad.

Agree in principle, but isn't this a breaking change? To what extent can this be mitigated by better wallet support for e.g. allowing users to seamlessly RBF their transaction with a different one?

kantai commented 1 year ago

Agree in principle, but isn't this a breaking change? To what extent can this be mitigated by better wallet support for e.g. allowing users to seamlessly RBF their transaction with a different one?

Yep, this would be a breaking change and would need to be included in a consensus change to activate.

Regarding whether or not this could be mitigated by better wallet support, the answer there is theoretically yes, but for whatever reason, this has been challenging for wallets to do. Perhaps wallet developers should weigh in on this?

diwakergupta commented 1 year ago

Tagging @kyranjamie @yknl @markmhx to chime in from wallet perspective

yknl commented 1 year ago

The majority of users don't understand what is RBF or that you can even replace a transaction that is pending with a completely different one. All of the wallets already support replacing a pending transaction by customizing the nonce.

kyranjamie commented 1 year ago

The wallet prevents transferring less than your balance, but as Brice says there are plenty of edge cases that might cause this. Do we have any sense how often users run into this problem? Assuming it's infrequent, but debilitating when it does happen.

Is there more the wallet can do to prevent this from happening in the first place?

As far as fixing the problem, is best thing to do is RBFing to a legit dust transaction to a null/change address? I wonder if this might be a suitable explorer feature? Allowing the user to diagnose stuck transactions, and initiating an extension wallet transaction that'll get it mined.

jcnelson commented 1 year ago

What if the node offered an RPC endpoint that lets you query mempool transactions by sender address (or sponsor address), and returned the height of the canonical Stacks tip at the time the transaction was accepted into the mempool? We already track (or can efficiently obtain) this information. From there, the wallet can calculate the "confirmation age" of the transaction, and uses that to deduce that the transaction is stale. The confirmation age of a transaction is the difference between the height of the canonical chain tip when the transaction arrived, and whatever the current canonical chain tip height is.

The wallet could use the confirmation age to seamlessly RBF stuck transactions. The wallet would query /v2/info to get the current block height, and then use this hypothetical /v2/mempool/sender/[:stacks_address:] endpoint to get a list of transactions in the mempool sent by [:stacks_address:] and the Stacks heights at which they were accepted. From there, the wallet would find the transaction record with highest sender nonce, and calculate its confirmation age. If the confirmation age is too high (e.g. 3 or more), then the wallet would offer the user a choice to replace this transaction.

We'd offer a similar endpoint for sponsors, for the same reason.

obycode commented 1 year ago

I agree, that the suggested endpoint could be useful to wallets to help avoid this issue, but I would argue for a principle of never leaving a transaction with the next nonce in the mempool unprocessed. The miner is doing work to check whether this transaction is valid or not, so it is perfectly acceptable to take the fee. It's not worth a consensus-change just for this, but it could be included with one of the planned changes.

jcnelson commented 1 year ago

Instead of just leaving these transactions pending, I think the miner should include them in a block as a failing transaction. I believe the current implementation purposely does not do this so that it does not charge the fee for a transaction that is definitely going to fail, basically trying to save the sender from their mistake

I would argue for a principle of never leaving a transaction with the next nonce in the mempool unprocessed. The miner is doing work to check whether this transaction is valid or not, so it is perfectly acceptable to take the fee.

Adding context to these two points. The current implementation does this to not only save the user a fee, but also to avoid wasting block space. Once a transaction is mined, it's part of the chainstate forever -- every current and future node must evaluate and process it.

I think we should be investigating ways to help miners minimize the work they spend in detecting these kinds of failures. For example, at contract analysis time we could determine and cache the set of potentially-reachable token transfer calls from all public functions. Then, when we process a contract-call transaction, we can check to see if (a) the reachable set of token transfers is reasonably small, and (b) the sender's token balances are all sufficiently high that the entire reachable set of token transfers are guaranteed to succeed. Transactions for which both points are true could be prioritized for block inclusion over those that require subsequent analysis.

markmhendrickson commented 1 year ago

Thanks @obycode for opening up this discussion!

Just to be clear here about the general UX if we were to handle things wallet-side:

As for the detection scheme, instead of querying against confirmation age, might it be more effective for the wallet to query mempool transactions and compare their intended transfer amounts to the user's known balance, regardless of age?

That seems like perhaps a more direct way to determine the need for RBF (due to incongruent send vs. current balance amounts) than guessing there is a need due to staleness (wherein false positives might arise due to congestion)?

I'm also curious if @314159265359879 has seen this scenario much on the support side given the wallet does generally protect users from sending more than their balance. Though I suspect that users who are sending many transactions in a row might find that the wallet doesn't accurately calculate their balance in time for subsequent ones given delays on the API side due to mempool processing?

Overall, faced with the need to provide a manual option to RBF here (however better we prompt the user automatically), I'd lean towards having the miners automatically include / clean up failing transactions for the users instead. That would save users the need for manual intervention (which adds friction and risks only partial adoption), even if it comes at the cost of increased block space usage.

314159265359879 commented 1 year ago

It is especially in cases when a users sends transactions in rapid succession. And some cases that are not yet covered in the wallet to protect the user from double spending.

I have long thought that the best solution is to include these transaction in a block and make them fail with an "insufficient funds error". Because:

  1. If mistakes cost money they usually provide a memorable learning experience for the user. Wallet providers obviously still want to help prevent users from making the mistake (of double spending) but it could help prevent subsequent ones. Part of the issue is nothing happening; users sending new transactions (with higher nonces because the average users doesn't realize they need to replace the prior transaction... adding new issues rather than solving anything).

  2. Moreover though I believe making the transactions part of the history is helpful, rather than a disadvantage. I have helped users on numerous occasions to analyze their wallet history to prove they did not lose funds after a transaction was dropped (and became invisible). If the transaction is mined and confirmed with an error, that makes it obvious to anyone what happened.

  3. Mining these transactions to send unavailable STX is more consistent. If I try to double spend a SIP-010 token it does get mined and fee subtracted (failing with result error u1). Example of such a failing transaction: https://explorer.hiro.so/txid/0x3352ac13e5a7e0ac3fde427063c88d6103beb4c5f07ae26f43f7cbcf19831d74?chain=mainnet image

@jcnelson by not including them you can save the user some money, and you can save the network some blockspace (for now?). Saving block space is likely just a temporary advantage because when the network gets the traction we expect the blocks will be full regardless of how many (potential) double spend transactions are mined to fail. Full blocks lead to a functioning fee market and higher fees that make the mistakes more expensive and users more motivated to prevent them.

I do not think this needs a consensus breaking change right away but I am in favor of including it in a future hard fork as @obycode suggested.

Some of the work done on the wallet side to prevent these troublesome transactions

That will cover most cases but not all, and the edge cases (sending in quick succession, with another wallet, with a dapp that doesn't specify post conditions exactly, etc) always pop up when there are a lot of new users using the network. I think this solution could help give those new users a better first experience using the Stacks network.