Fix/sc txn errors #523
Fix/sc txn errors #523
Conversation
| return | ||
| txn.TransactionOutput = err.Error() | ||
| txn.Status = transaction.TxnFail | ||
| return nil |
There was a problem hiding this comment.
Please do not ignore the returned error if the transaction execute failed.
There was a problem hiding this comment.
It was the intention that failed smart contract transactions not be considered internal failures and instead become part of the block. If an error is returned, a transaction will be ignored.
Could you suggest some unintended consequences if you're aware of something?
There was a problem hiding this comment.
Transactions could fail for different reasons, and we can not ignore all of the transactions directly. Check the following situations:
a) The transactions are invalid by themself, we should and must ignore them (The question is whether this transactions could be packed into a block, need further check)
b) The transactions are valid, and it could be executed successfully by most of the miners and sharders, except some miners that do not have sufficient state in MPT. In this situation, if we ignore the transaction, and continue the block state compute, the block's MPT state would be different from others, and we will see the mismatch state hash error. What we should do is return, instead of continue the block state computing.
There was a problem hiding this comment.
So basically, we need to differ the transaction fail errors.
a) invalid transactions by themself
b) Transactions are valid, but the miner is not ready yet to execute the transactions.
There was a problem hiding this comment.
Might be a dumb question, but could we check the transaction validity when the client submits it? Then it'd be no surprise for the txn to be discarded and not included in the block.
There was a problem hiding this comment.
valid point by @peterlimg.I can add a little bit, there can be 3 main cases:
- transaction is malformed or has invalid signature or anything else that can be validated at the time of receiving
- transaction can't be executed now, but can be executed in the future (no funds yet for transfer and so on) not sure that there are lots of cases, since we use transaction creation time for sequencing.
- transaction causes error to smart contract
regarding this cases
- should not be included into the block and should be excluded from tx_pool asap, we have to collect statistics on malicious senders by ip or something similar
- should be stored in the tx_pool for some time and if can't be executed successfully should be included into the block
- should be included into the block
|
Am i right, that this change is needed first of all to charge clients even for failed SC transactions? |
Yes. I expect there to be problems with repeatability of a transaction output between generator and verifiers, which can lead to discarded blocks because of failed verification. We need to make sure that miners get same error when processing same transaction, both during generation and verification. When it comes to clients, for now I've changed gosdk to treat failed transaction as errors, and use transaction output as error message. |
Miners must validate the transactions no matter whether the transactions have been validated in clients end, we can't guarantee that all clients are honest. |
What I meant is, as soon as the backend receives a txn, he runs validation, and on the response to the request informs the client that it is not valid. Then it'd be no surprise for the client when txn is discarded. |
It needs to be part of the finalized block otherwise it can be a Byzantine miner |
|
Fixed in PR #930 |
This PR fixes the problem pointed out in 0chain/zboxcli#53, but is part of general solution that handles all smartcontract errors.
As the failed smartcontract transaction (+ it's error) become part of a block, the application should ensure that both the block generator as well as the block verifier gets same error while processing same transaction. Otherwise, the verification would fail and the block would be discarded.