Gas used sanity check #2567

sasurobert · 2020-12-08T13:53:14Z

added check on vmInput and vmOutput for gasProvided and gas used
smart contract results do not need to consume move balance gas - as that part was already taken out at the sneder shard.

iulianpascalau · 2020-12-08T14:19:52Z

cmd/node/factory/structs.go

@@ -1841,7 +1841,20 @@ func newMetaBlockProcessor(
 	rater sharding.PeerAccountListAndRatingHandler,
 ) (process.BlockProcessor, error) {

-	builtInFuncs := builtInFunctions.NewBuiltInFunctionContainer()
+	argsBuiltIn := builtInFunctions.ArgsCreateBuiltInFunctionContainer{


this will cause conflicts with soft-restart feature branch (not having a great idea how to overcome this)

integrationTests/multiShard/relayedTx/relayedTx_test.go

iulianpascalau · 2020-12-08T14:23:46Z

integrationTests/testProcessorNode.go

@@ -1719,7 +1727,7 @@ func (tpn *TestProcessorNode) LoadTxSignSkBytes(skBytes []byte) {
 // ProposeBlock proposes a new block
 func (tpn *TestProcessorNode) ProposeBlock(round uint64, nonce uint64) (data.BodyHandler, data.HeaderHandler, [][]byte) {
 	startTime := time.Now()
-	maxTime := time.Second * 2
+	maxTime := time.Second * 20000


We might revert the values here and on L1953 and L1983

iulianpascalau · 2020-12-08T14:27:40Z

process/smartContract/process.go

+	totalGasInVMOutput := vmOutput.GasRemaining
+	for _, outAcc := range vmOutput.OutputAccounts {
+		for _, outTransfer := range outAcc.OutputTransfers {
+			transferGas, err := safeAddUint64(outTransfer.GasLocked, outTransfer.GasLimit)


safeSubUint64 ?

no. these are added.

iulianpascalau · 2020-12-08T14:29:27Z

process/smartContract/process.go

@@ -481,10 +528,15 @@ func (sc *scProcessor) computeTotalConsumedFeeAndDevRwd(
 	}

 	senderInSelfShard := sc.isSelfShard(tx.GetSndAddr())
-	consumedGas := safeSubUint64(tx.GetGasLimit(), vmOutput.GasRemaining)
+	consumedGas, err := safeSubUint64(tx.GetGasLimit(), vmOutput.GasRemaining)
+	log.LogIfError(err, "computeTotalConsumedFeeAndDevRwd")


You need a third parameter here, otherwise computeTotalConsumedFeeAndDevRwd won't be displayed. Add more info as to easily pin-point the errors in case they appear.

iulianpascalau · 2020-12-08T14:34:24Z

process/smartContract/process.go

+	err = sc.gasConsumedChecks(tx, vmOutput)
+	if err != nil {
+		log.Error("gasConsumedChecks with problem ", "err", err.Error(), "txHash", txHash)
+		return vmcommon.ExecutionFailed, sc.ProcessIfError(acntSnd, txHash, tx, err.Error(), []byte("gas consumed exceeded"), snapshot, vmInput.GasLocked)


is this still backwards-compatible?

yes. it is. only in asynccall we had these issues. plus this shouldn't ever happen

process/smartContract/process.go

iulianpascalau · 2020-12-08T14:37:51Z

process/smartContract/process.go

@@ -1144,6 +1213,8 @@ func (sc *scProcessor) processVMOutput(
 	if !createdAsyncCallback && callType == vmcommon.AsynchronousCall {


if L1213-L1218 were to be extracted to another function, then we could have avoided the else-if situation

tried, but does not look good. thanks golang.

iulianpascalau · 2020-12-08T14:39:23Z

process/smartContract/process.go

+		log.Error("gasConsumedChecks with problem ", "err", err.Error(), "txHash", txHash)
+		return vmcommon.ExecutionFailed, sc.ProcessIfError(acntSnd, txHash, tx, err.Error(), []byte("gas consumed exceeded"), snapshot, vmInput.GasLocked)
+	}
+
 	createdAsyncCallback := false
 	scrResults := make([]data.TransactionHandler, 0, len(vmOutput.OutputAccounts)+1)
 	outputAccounts := process.SortVMOutputInsideData(vmOutput)
 	for _, outAcc := range outputAccounts {


looks like duplicated code L167x

it has subtle differences.

iulianpascalau · 2020-12-08T14:40:52Z

process/smartContract/vmInput.go

@@ -100,6 +109,10 @@ func (sc *scProcessor) createVMCallInput(
 	vmCallInput.VMInput.Arguments = finalArguments
 	vmCallInput.GasProvided = vmCallInput.GasProvided - gasLocked

+	if vmCallInput.GasProvided+vmCallInput.GasLocked > tx.GetGasLimit() {


can't we use here the safe add? What happens if the sum overflows the uint64 max value?

AdoAdoAdo · 2020-12-08T16:02:00Z

process/errors.go

+var ErrAdditionOverflow = errors.New("uint64 addition overflow")
+
+// ErrSubtractionOverflow signals that uint64 subtraction overflowed
+var ErrSubtractionOverflow = errors.New("signals the uint64 subtraction overflowed")


fix error message "uint64 subtraction overflowed"

AdoAdoAdo · 2020-12-08T16:07:23Z

process/smartContract/process.go

+	vmOutput *vmcommon.VMOutput,
+) error {
+	if tx.GetGasLimit() == 0 && sc.shardCoordinator.ComputeId(tx.GetSndAddr()) == core.MetachainShardId {
+		// special case for issueing and minting ESDT tokens for normal users


typo issueing -> issuing

AdoAdoAdo · 2020-12-08T16:31:01Z

process/smartContract/process.go

+	}
+
+	result.CallType = vmcommon.AsynchronousCallBack
+	result.GasLimit += vmOutput.GasRemaining


error was treated before on gasConsumedChecks which is called before this function.

AdoAdoAdo · 2020-12-08T16:33:59Z

process/smartContract/vmInput.go

@@ -100,6 +109,14 @@ func (sc *scProcessor) createVMCallInput(
 	vmCallInput.VMInput.Arguments = finalArguments
 	vmCallInput.GasProvided = vmCallInput.GasProvided - gasLocked


maybe use here safeSub instead of adding a check with safeAdd afterwards?

done. left both of them.

SebastianMarian · 2020-12-08T15:57:15Z

process/smartContract/process.go

@@ -587,7 +641,8 @@ func (sc *scProcessor) ExecuteBuiltInFunction(
 		log.Debug("processed built in functions error", "error", err.Error())
 		return 0, err
 	}
-	builtInFuncGasUsed := safeSubUint64(vmInput.GasProvided, vmOutput.GasRemaining)
+	builtInFuncGasUsed, err := safeSubUint64(vmInput.GasProvided, vmOutput.GasRemaining)
+	log.LogIfError(err, "computeTotalConsumedFeeAndDevRwd", "executeBuildInFunction")


log.LogIfError(err, "ExecuteBuildInFunction", "builtInFuncGasUsed")

SebastianMarian · 2020-12-08T17:39:07Z

process/smartContract/process.go

@@ -1454,6 +1523,34 @@ func (sc *scProcessor) createSmartContractResults(
 	return createdAsyncCallBack, scResults
 }

+func (sc *scProcessor) useLastTransferAsAsyncCallBackWhenNeeded(


shouldUseLastTransferAsAsyncCallBack

would leave it as it is.

SebastianMarian · 2020-12-08T17:45:22Z

process/smartContract/process.go

 		isLastOutTransfer := i == lenOutTransfers-1
-		if isLastOutTransfer && isAsyncTransferBackToSender && sc.isTransferWithNoDataOrBuiltInCall(outputTransfer.Data) {
-			addVMOutputResultsToSCR(vmOutput, result)


Not need to call addVMOutputResultsToSCR? In line 1548 and 1549 it is done now only part of this whole method. Also
is done result.GasLimit += vmOutput.GasRemaining instead result.GasLimit = vmOutput.GasRemaining like it is done in addVMOutputResultsToSCR

no need to call. that method was wrongly called here and would have deleted the actual issueing of ESDT token from an smart contract. in case of outPutTransfer you need different operations here.

SebastianMarian · 2020-12-08T17:52:58Z

process/smartContract/process.go

-		if !createdAsyncCallback {
-			createdAsyncCallback = tmpCreatedAsyncCallback
-		}
+		createdAsyncCallback = createdAsyncCallback || tmpCreatedAsyncCallback

 		scrResults = append(scrResults, newSCRTxs...)


if len(newSCRTxs) != 0 {

SebastianMarian · 2020-12-08T17:53:35Z

process/smartContract/process.go

-		if !createdAsyncCallback {
-			createdAsyncCallback = tmpCreatedAsyncCallback
-		}
+		createdAsyncCallback = createdAsyncCallback || tmpCreatedAsyncCallback
 		scrResults = append(scrResults, scTxs...)


if len(scTxs) != 0 {

iulianpascalau · 2020-12-08T21:33:40Z

process/block/postprocess/intermediateResults.go

@@ -210,6 +210,12 @@ func (irp *intermediateResultsProcessor) AddIntermediateTransactions(txs []data.
 			return process.ErrWrongTypeAssertion
 		}

+		err := irp.checkSmartContractResultIntegrity(addScr)
+		if err != nil {
+			log.Error("AddIntermediateTransaction", "error", err, "dump", spew.Sdump(addScr))


iulianpascalau · 2020-12-08T21:44:07Z

process/block/postprocess/intermediateResults.go

@@ -235,6 +241,31 @@ func (irp *intermediateResultsProcessor) AddIntermediateTransactions(txs []data.
 	return nil
 }

+func (irp *intermediateResultsProcessor) checkSmartContractResultIntegrity(scr *smartContractResult.SmartContractResult) error {
+	if len(scr.RcvAddr) == 0 {


we could have made InterceptedUnsignedTransaction.integrity() exported function without pointer receiver (with the SmartContractResult parameter) and used it here.

I do not think it will work - maybe we will have some cyclic imports.

SebastianMarian · 2020-12-08T21:50:00Z

process/block/postprocess/intermediateResults.go

@@ -257,6 +257,10 @@ func checkSmartContractResultIntegrity(scr *smartContractResult.SmartContractRes
 	if len(scr.PrevTxHash) == 0 {
 		return process.ErrNilTxHash
 	}
+	sndId := irp.shardCoordinator.ComputeId(scr.SndAddr)


SebastianMarian · 2020-12-08T21:54:13Z

process/block/postprocess/intermediateResults.go

@@ -258,7 +258,8 @@ func (irp *intermediateResultsProcessor) checkSmartContractResultIntegrity(scr *
 		return process.ErrNilTxHash
 	}
 	sndId := irp.shardCoordinator.ComputeId(scr.SndAddr)
-	if !core.IsEmptyAddress(scr.SndAddr) && sndId != irp.shardCoordinator.SelfId() {
+	dstId := irp.shardCoordinator.ComputeId(scr.RcvAddr)


SebastianMarian · 2020-12-08T22:00:27Z

process/block/postprocess/intermediateResults.go

@@ -258,7 +258,8 @@ func (irp *intermediateResultsProcessor) checkSmartContractResultIntegrity(scr *
 		return process.ErrNilTxHash
 	}
 	sndId := irp.shardCoordinator.ComputeId(scr.SndAddr)
-	if !core.IsEmptyAddress(scr.SndAddr) && sndId != irp.shardCoordinator.SelfId() {
+	dstId := irp.shardCoordinator.ComputeId(scr.RcvAddr)


What happens if scr.SndAddr or scr.RcvAddr are nil. Maybe you should move this inside

if !core.IsEmptyAddress(scr.SndAddr) && !core.IsEmptyAddress(scr.RcvAddr) { sndShardID := irp.shardCoordinator.ComputeId(scr.SndAddr) dstShardID := irp.shardCoordinator.ComputeId(scr.RcvAddr) if sndShardID != irp.shardCoordinator.SelfId() && dstShardID != irp.shardCoordinator.SelfId() { return process.ErrShardIdMissmatch } }

nil checks are done above. for both sender, receiver and prevtxhash

modified to your suggestion

AdoAdoAdo · 2020-12-09T09:17:05Z

process/smartContract/process.go

@@ -1600,11 +1604,16 @@ func (sc *scProcessor) createSCRForSenderAndRelayer(
 	relayedSCR, isRelayed := isRelayedTx(tx)
 	shouldRefundGasToRelayerSCR := isRelayed && callType != vmcommon.AsynchronousCall && gasRefund.Cmp(zero) > 0
 	if shouldRefundGasToRelayerSCR {
+		senderForRelayerRefund := tx.GetRcvAddr()


can we have instead some hardcoded (constant) sender addresses for relayed txs that are constructed for each shard?

sasurobert added 5 commits December 8, 2020 14:27

fix after review and unit tests

49db9a0

implementation

e625d1d

small fixes

7ed8422

unit tests

19d5916

new arwen

ce6f075

iulianpascalau self-requested a review December 8, 2020 14:19

iulianpascalau requested changes Dec 8, 2020

View reviewed changes

fix after review - new arwen

55e38fb

iulianpascalau previously approved these changes Dec 8, 2020

View reviewed changes

reverted test values

c0f266b

sasurobert dismissed iulianpascalau’s stale review via c0f266b December 8, 2020 15:15

AdoAdoAdo self-requested a review December 8, 2020 15:31

iulianpascalau previously approved these changes Dec 8, 2020

View reviewed changes

AdoAdoAdo reviewed Dec 8, 2020

View reviewed changes

fix after review

a38e5bd

sasurobert dismissed iulianpascalau’s stale review via a38e5bd December 8, 2020 16:47

iulianpascalau previously approved these changes Dec 8, 2020

View reviewed changes

AdoAdoAdo previously approved these changes Dec 8, 2020

View reviewed changes

SebastianMarian reviewed Dec 8, 2020

View reviewed changes

fix after review

e47d180

sasurobert dismissed stale reviews from AdoAdoAdo and iulianpascalau via e47d180 December 8, 2020 18:06

iulianpascalau previously approved these changes Dec 8, 2020

View reviewed changes

SebastianMarian previously approved these changes Dec 8, 2020

View reviewed changes

mihaib79 previously approved these changes Dec 8, 2020

View reviewed changes

scr check validity

9b1ced5

sasurobert dismissed stale reviews from mihaib79, SebastianMarian, and iulianpascalau via 9b1ced5 December 8, 2020 20:03

this is a try only

03bf072

fix one bug. added more invariants.

654e3ed

iulianpascalau reviewed Dec 8, 2020

View reviewed changes

added unit test

83d0564

SebastianMarian reviewed Dec 8, 2020

View reviewed changes

fix after review

a69f26b

SebastianMarian approved these changes Dec 8, 2020

View reviewed changes

iulianpascalau approved these changes Dec 9, 2020

View reviewed changes

AdoAdoAdo reviewed Dec 9, 2020

View reviewed changes

AdoAdoAdo approved these changes Dec 9, 2020

View reviewed changes

sasurobert merged commit c233ae1 into development Dec 9, 2020

LucianMincu deleted the gasUsed-sanity-check branch December 29, 2020 10:54

		@@ -1144,6 +1213,8 @@ func (sc *scProcessor) processVMOutput(
		if !createdAsyncCallback && callType == vmcommon.AsynchronousCall {

		@@ -100,6 +109,14 @@ func (sc *scProcessor) createVMCallInput(
		vmCallInput.VMInput.Arguments = finalArguments
		vmCallInput.GasProvided = vmCallInput.GasProvided - gasLocked

Gas used sanity check #2567

Gas used sanity check #2567

Conversation

sasurobert commented Dec 8, 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

sasurobert Dec 8, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sasurobert commented Dec 8, 2020 •

edited

sasurobert Dec 8, 2020 •

edited