stacks-network / stacks-core

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

Incorrect tx_status after error #2777

Open friedger opened 2 years ago

friedger commented 2 years ago

Describe the bug The tx_status of https://stacks-node-api.mainnet.stacks.co/extended/v1/tx/0x38e2f50a4c410e2835a3497d0a7f68c2e81150a3f95956333c31d7b68beb647b?chain=mainnet is shown as abort_by_post_condition. However, the tx result is (err u5).

Expected behavior I'd expect the tx status as abort_by_response

Additional context First reported on https://github.com/blockstack/stacks-blockchain-api/issues/671 because the explorer shows the status of the tx wrongly.

stale[bot] commented 1 year ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

friedger commented 1 year ago

Has this been fixed?

jcnelson commented 1 year ago

Temporarily assigning @zone117x so this issue has an owner. Please feel free to re-assign as you see fit.

zone117x commented 1 year ago

I doubt I'm going to be working on code around this anytime soon. @friedger do you feel strongly that this behavior should be changed? I'm inclined to close this unless someone more familiar with this area of the code agrees with the change request.

friedger commented 1 year ago

Yes, this is very visible and confusing for developers like the explorer developers.

Users of the explorer see the big warning that the post conditions were not satisfied.

As a workaround we could update the documentation and say that abort_by_poatxonditions does not imply that the tx was aborted by post conditions. This is only the case if the tx result is ok.

zone117x commented 1 year ago

As a workaround we could update the documentation and say that abort_by_post_conditions does not imply that the tx was aborted by post conditions

Would you agree that this is somewhat of an order of precedence issue with what we declare as the cause of the abort? If the tx had an error result, and also did not fulfill the post-condition requirements, then it sounds like the abort "reason" could arguably be both? Either way, I agree a more helpful error message should be displayed.

It sounds like this is something that could be handled client side. Both the tx status and result properties are available for clients to display a more helpful message like you've described.

If we changed this on the stacks-node, it would technically be breaking change to the event-stream history. And on the API side, I'd prefer not to override/transform data from the stacks-node. That usually backfires and causes more confusion.

friedger commented 1 year ago

However, this is a bug.

zone117x commented 1 year ago

Assigning to @kantai to give some input. I'm not going to be working on this area of the code either way.

kantai commented 1 year ago

Is this a bug, or just something that needs more information in the specification?

The transaction runs, and the result of the execution is (err u5). The transaction does not transfer the asset SP497E7RX3233ATBS2AB9G4WTHB63X5PBSP5VGAQ.boomboxes-cycle-12.b-12, which is required by the post condition. The stacks-node sees that the post condition failed and aborts the transaction.

It's true that the transaction would have been aborted because of (err u5) anyways, but regardless of this, the transaction's post conditions are not met: the stacks-node always checks post conditions, even if the transaction was being aborted.

The value result of the transaction is always provided exactly so that clients can differentiate between these kinds of cases.

friedger commented 1 year ago

Thank you for the clarification.

Are there well known use cases where post conditions are met and the tx aborts due to an error?

kantai commented 1 year ago

I’ll find a specific transaction to link, but you could always create such a case with the post conditions set to ALLOW mode. Contracts that have side effects without assets could also exhibit this case (for example, a PoX transaction can abort without any post conditions) even if you were using post conditions in DENY mode.

kantai commented 1 year ago

Here's an example: https://explorer.hiro.so/txid/0xdbf8ae41ef3e9e709cda18f42ae3ea46783dcc5e6435fc246a4a28e8ce407722?chain=mainnet

friedger commented 1 year ago

There is never a tx with a post condition that is satisfied when the tx fails with an error.

kantai commented 1 year ago

Not exactly -- a transaction with one of [SENT_GT, SENT_GE, SENT_EQ] (for FTs) or [SENT] (for NFTs) condition codes cannot have their post condition satisfied when the tx fails with an error (because the post condition is asserting that something got sent, but because the tx failed with error, nothing did).

However, transactions with [SENT_LE, SENT_LT] or [NOT_SENT] condition codes can have their post condition satisfied when the tx fails with an error.

friedger commented 1 year ago

I see. Thank you