Exchanges integration: return invalid transactions as well, refactor status #2237

andreibancioiu · 2020-08-25T12:18:09Z

refactor computation of transaction status (encapsulate in StatusComputer)
add more tests, refactor tests
include invalid transactions in response of /block?withTxs
deprecated field Status in MiniblockMetadata storage - will not be used anymore. Though, now, miniblock Type is required to compute status - thus, a breaking change for the blockAPI - but since we will implement replay & index rebuild, this is a non-issue.
added new field Type in MiniblockMetadata storage (see above)
also return miniblock hash in transaction response, where possible

How to test:

send both valid and invalid transactions
inspect transactions by hash (valid, invalid, rewards, possibly SCs as well)
inspect hyperblock wrt. to hyperblockNonce from step above
assert that invalid conditions are included in hyperblocks
assert that miniblock type is correctly set on get transaction responses
assert that miniblock hash, source shard, destination shard are present on get transaction responses

Get transaction route: http://proxy:8080/transaction/abracadabra

Get hyperblock route: http://proxy:8080/hyperblock/by-nonce/42

miiu96 · 2020-08-25T12:39:00Z

data/transaction/status.go

+	// TxStatusExecuted = received and executed
+	TxStatusExecuted TxStatus = "executed"
+	// TxStatusNotExecuted = received and executed with error
+	// Question for review: not executed means "executed with error", correct?


yes for transaction that are indexed in elasticseach 'not executed' is status for transaction executed with error.

miiu96 · 2020-08-25T12:41:35Z

data/transaction/status.go

+
+// ComputeStatusWhenInStorageKnowingMiniblock computes the transaction status for a historical transaction
+func (params *StatusComputer) ComputeStatusWhenInStorageKnowingMiniblock() TxStatus {
+	if params.MiniblockType == block.InvalidBlock {


maybe you can add a method isMiniblockInvalid

miiu96 · 2020-08-25T12:51:16Z

node/blockAPI/baseBlock.go

+		tx.MiniBlockType = miniblock.Type
+		tx.MiniBlockHash = hex.EncodeToString([]byte(miniblockHash))
+
+		tx.Status = (&transaction.StatusComputer{


nice approach

miiu96 · 2020-08-25T12:52:15Z

node/errors.go

+// ErrTransactionNotFound signals that a transaction was not found
+var ErrTransactionNotFound = errors.New("transaction not found")
+
+// ErrCannotRetrieveTransaction signals that a transaction was not found


update comment

miiu96 · 2020-08-25T13:02:15Z

node/nodeTransactions.go

 	}

-	// merge data about transaction from history storer and transaction storer
-	tx = putHistoryFieldsInTransaction(tx, miniblockMetadata)
+	putHistoryFieldsInTransaction(tx, miniblockMetadata)


maybe you can add this method inside structure ApiTransactionResult
func (ats *ApiTransactionResult) PutHistoryFieldsInTransaction(miniblockMetadata *fullHistory.MiniblockMetadata) just a suggestion

That is a good suggestion, indeed, to encapsulate this logic in ApiTransactionResult. The issue is, though, that ApiTransactionResult is defined in transaction package, while miniblockMetadata in fullHistory package - and that would require an additional interface - we can do in the future.

sasurobert · 2020-08-25T12:52:47Z

data/transaction/status.go

+	// TxStatusExecuted = received and executed
+	TxStatusExecuted TxStatus = "executed"
+	// TxStatusNotExecuted = received and executed with error
+	// Question for review: not executed means "executed with error", correct?


not-executed is confusing. Not-executed would be those transaction which really do not get included in block (lower nonce for example - user sending two transactions with same nonce and only 1 getting executed).
Than we have the scenario when the transaction's status in invalid - in that case the gas was consumed from the account, and the nonce increased.

Currently, also Explorer displays not executed for executed transactions (but, say, with user error). Do we keep it like this for the moment or do we change this?

we should change that as well.

Only now I see that TxStatusNotExecuted is only used in the context of the transactions simulator:

node/txsimulator/txSimulator.go, line 63

And seems to be used well there.

The other occurrence, completely unrelated, is in the indexer component (which, actually, defines a separate set of statuses): txStatusNotExecuted = "Not Executed".

I don't want to change the indexer status in this PR (a bit risky), but we can do that in another PR.

Ok?

Ok. that is in another PR changing in the indexer.

sasurobert · 2020-08-25T13:11:24Z

data/transaction/status.go

+
+// ComputeStatusWhenInPool computes the transaction status when transaction is in pool
+func (params *StatusComputer) ComputeStatusWhenInPool() TxStatus {
+	if params.isContractDeploy() {


why is this check needed ?

params.isDestinationMe() && params.isCrossShard() would be evaluated to true for deployments, but still we don't want to return TxStatusPartiallyExecuted in that case (seen in previous PR with the status for deployments).

Does it make sense?

Fixed (moved condition).

isCrossShard should return false in case of deployment.

Currently, isCrossShard() is implemented as follows:

func (params *StatusComputer) isCrossShard() bool { return params.SourceShard != params.DestinationShard }

While ComputeStatusWhenInPool() is fed the parameters as follows - in a generic manner, not knowing anything special about the Zero address:

SourceShard: n.shardCoordinator.ComputeId(tx.Tx.GetSndAddr()), DestinationShard: n.shardCoordinator.ComputeId(tx.Tx.GetRcvAddr()), Receiver: tx.Tx.GetRcvAddr(), TransactionData: tx.Data, SelfShard: n.shardCoordinator.SelfId(),

Thus, the isContractDeploy() has to be checked first (because the check isDestinationMe() && isCrossShard() will be fooled otherwise).

We could change isCrossShard() to call isContractDeploy(), but perhaps that would be a bit harder to reason about in the future.

Fixed to be more explicit, as before. Is it all right?

in this case shardCoordinator computeID should have returned selfID. for the empty address.

in computestatuswheninPool you might use another function a wrapper on top of computeID where you add the empty check.

Let's keep compute ID completely out of the statusComputer, please.

Previous location, previous PR (reviewed):
https://github.com/ElrondNetwork/elrond-go/pull/2234/files

Sometimes it happens that these things get approved by a PR and the next one requests changes in the same zone. It would be better to have correct shardId computation at the base. In this case shard coordinator, or computestatus

Left as it is, as discussed separately.

sasurobert · 2020-08-25T13:18:00Z

data/transaction/status.go

+	// Question for review: here we cannot know if the transaction is, actually, "invalid".
+	// However, when "fullHistory" indexing is enabled, this function is not used.
+
+	if params.isDestinationMe() || params.isContractDeploy() {


we should attach the receipt / smart contract result to the transaction - as we have in elastic search - than it it simple to decide the status of the transaction. It is not a trivial task, this can stay as it is.

delete the comments question for review

Fixed comment, moved above function.

sasurobert · 2020-08-25T13:43:11Z

node/blockAPI/baseBlock.go

 	case block.SmartContractResultBlock:
-		return bap.getTxsFromMiniblock(miniBlock, epoch, "unsigned", dataRetriever.UnsignedTransactionUnit)
+		return bap.getTxsFromMiniblock(miniBlock, miniblockHash, epoch, "unsigned", dataRetriever.UnsignedTransactionUnit)


make some constants for magic strings

sasurobert · 2020-08-25T15:32:48Z

data/transaction/status.go

+	// TxStatusExecuted = received and executed
+	TxStatusExecuted TxStatus = "executed"
+	// TxStatusNotExecuted = received and executed with error
+	// Question for review: not executed means "executed with error", correct?


we should change that as well.

sasurobert · 2020-08-25T15:33:16Z

data/transaction/status.go

+	// Question for review: here we cannot know if the transaction is, actually, "invalid".
+	// However, when "fullHistory" indexing is enabled, this function is not used.
+
+	if params.isDestinationMe() || params.isContractDeploy() {


delete the comments question for review

node/nodeTransactions.go

sasurobert · 2020-08-26T11:50:36Z

data/transaction/status.go

+
+// ComputeStatusWhenInPool computes the transaction status when transaction is in pool
+func (params *StatusComputer) ComputeStatusWhenInPool() TxStatus {
+	if params.isContractDeploy() {


in computestatuswheninPool you might use another function a wrapper on top of computeID where you add the empty check.

sasurobert · 2020-08-26T11:51:17Z

node/errors.go

@@ -195,3 +190,16 @@ var ErrNilTxSimulatorProcessor = errors.New("nil transaction simulator processor

 // ErrNilIntermediateProcessorContainer signals that intermediate processors container is nil
 var ErrNilIntermediateProcessorContainer = errors.New("intermediate processor container is nil")
+
+// NewErrTransactionNotFound creates an error which signals that a transaction was not found
+func NewErrTransactionNotFound(originalErr error) error {


do we really need these ?

Let's have them, please.

sasurobert · 2020-08-26T11:52:06Z

node/nodeTransactions.go

 	}

 	txBytes, txType, found := n.getTxBytesFromStorageByEpoch(hash, miniblockMetadata.Epoch)
 	if !found {
 		log.Warn("getFullHistoryTransaction(): unexpected condition, cannot find transaction in storage")
-		return nil, ErrCannotRetrieveTransaction
+		return nil, NewErrCannotRetrieveTransaction(err)


error wrapping can happen directly here.

Let's use the wrappers defined in errors.go, please (a bit cleaner).

LucianMincu

System tests passed.

andreibancioiu added 12 commits August 24, 2020 13:42

Add extra field, "Type" (might be needed in the future).

d0edf5f

Return "miniblockhash" field as well.

e248efc

Preparatory refactoring.

13e1a7f

Preparatory refactoring.

4ba97c2

Sketch (WIP).

7cdefe0

Merge branch 'development' into exchanges-integration

457e782

Deprecate "status" field on miniblock metadata.

1173081

Refactor, as status computer.

923e93e

Add tests.

f0bbdb0

Add tests.

d1c79f1

Refactor tests.

d6e6c98

Tests.

8f6c44e

andreibancioiu self-assigned this Aug 25, 2020

andreibancioiu requested review from miiu96, AdoAdoAdo and sasurobert August 25, 2020 12:24

andreibancioiu marked this pull request as ready for review August 25, 2020 12:32

Format field (self-review).

cbb9f2a

miiu96 reviewed Aug 25, 2020

View reviewed changes

Fix after self review.

3444bf6

sasurobert requested changes Aug 25, 2020

View reviewed changes

andreibancioiu and others added 3 commits August 25, 2020 17:43

Merge branch 'development' into exchanges-integration

8495b93

Fix after review.

d7661d8

Enable full history for tests.

87850b0

andreibancioiu requested review from miiu96 and sasurobert August 25, 2020 15:29

sasurobert requested changes Aug 25, 2020

View reviewed changes

Merge branch 'development' into exchanges-integration

6e20ae3

AdoAdoAdo requested changes Aug 26, 2020

View reviewed changes

node/nodeTransactions.go Outdated Show resolved Hide resolved

node/nodeTransactions.go Outdated Show resolved Hide resolved

node/nodeTransactions.go Show resolved Hide resolved

Merge branch 'development' into exchanges-integration

3795183

Fix after review.

46de3b5

sasurobert requested changes Aug 26, 2020

View reviewed changes

AdoAdoAdo previously approved these changes Aug 26, 2020

View reviewed changes

andreibancioiu requested a review from sasurobert August 26, 2020 12:52

miiu96 previously approved these changes Aug 26, 2020

View reviewed changes

Only index genesis block if epoch is 0.

8bb656c

andreibancioiu dismissed stale reviews from miiu96 and AdoAdoAdo via 8bb656c August 26, 2020 14:31

andreibancioiu added 2 commits August 26, 2020 18:06

Fix after review.

1705203

Fix indexing of genesis block.

14c154e

andreibancioiu requested review from AdoAdoAdo and miiu96 August 26, 2020 15:08

Undo config.toml.

fafc43c

miiu96 previously approved these changes Aug 26, 2020

View reviewed changes

Disable "fullHistory".

f6732e9

andreibancioiu dismissed miiu96’s stale review via f6732e9 August 26, 2020 15:25

AdoAdoAdo approved these changes Aug 26, 2020

View reviewed changes

andreibancioiu requested a review from miiu96 August 26, 2020 15:27

miiu96 approved these changes Aug 26, 2020

View reviewed changes

sasurobert approved these changes Aug 26, 2020

View reviewed changes

LucianMincu approved these changes Aug 26, 2020

View reviewed changes

LucianMincu merged commit c059598 into development Aug 26, 2020

LucianMincu deleted the exchanges-integration branch August 26, 2020 16:14

andreibancioiu restored the exchanges-integration branch August 26, 2020 21:08

andreibancioiu deleted the exchanges-integration branch August 28, 2020 12:23

Exchanges integration: return invalid transactions as well, refactor status #2237

Exchanges integration: return invalid transactions as well, refactor status #2237

Conversation

andreibancioiu commented Aug 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Aug 26, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreibancioiu Aug 25, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LucianMincu left a comment

Choose a reason for hiding this comment

andreibancioiu commented Aug 25, 2020 •

edited

andreibancioiu Aug 26, 2020 •

edited

andreibancioiu Aug 25, 2020 •

edited