L1 gas refunds for failing transactions. BaseFee not burned anymore. (#…

…1550)

* Fixed known issues and added gas test that ensures network hasnt produced bad batches.

* Added comment.

* Fixes for linter.

* Ran gofumpt.

* no gas.

* Disabled gas test.

---------

Co-authored-by: StefanIliev545 <[email protected]>

go/enclave/components/batch_executor.go

@@ -107,6 +107,27 @@ func (executor *batchExecutor) payL1Fees(stateDB *state.StateDB, context *BatchE
 	return transactions, freeTransactions
 }
 
+func (executor *batchExecutor) refundL1Fees(stateDB *state.StateDB, context *BatchExecutionContext, transactions []*common.L2Tx) {
+	block, _ := executor.storage.FetchBlock(context.BlockPtr)
+	for _, tx := range transactions {
+		cost, err := executor.gasOracle.EstimateL1StorageGasCost(tx, block)
+		if err != nil {
+			executor.logger.Warn("Unable to get gas cost for tx", log.TxKey, tx.Hash(), log.ErrKey, err)
+			continue
+		}
+
+		sender, err := core.GetAuthenticatedSender(context.ChainConfig.ChainID.Int64(), tx)
+		if err != nil {
+			// todo @siliev - is this critical? Potential desync spot
+			executor.logger.Warn("Unable to extract sender for tx", log.TxKey, tx.Hash())
+			continue
+		}
+
+		stateDB.AddBalance(*sender, cost)
+		stateDB.SubBalance(context.Creator, cost)
+	}
+}
+
 func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext) (*ComputedBatch, error) {
 	defer core.LogMethodDuration(executor.logger, measure.NewStopwatch(), "Batch context processed")
 
@@ -159,12 +180,14 @@ func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext) (*Co
 
 	crossChainTransactions = append(crossChainTransactions, freeTransactions...)
 
-	successfulTxs, txReceipts, err := executor.processTransactions(batch, 0, transactionsToProcess, stateDB, context.ChainConfig, false)
+	successfulTxs, excludedTxs, txReceipts, err := executor.processTransactions(batch, 0, transactionsToProcess, stateDB, context.ChainConfig, false)
 	if err != nil {
 		return nil, fmt.Errorf("could not process transactions. Cause: %w", err)
 	}
 
-	ccSuccessfulTxs, ccReceipts, err := executor.processTransactions(batch, len(successfulTxs), crossChainTransactions, stateDB, context.ChainConfig, true)
+	executor.refundL1Fees(stateDB, context, excludedTxs)
+
+	ccSuccessfulTxs, _, ccReceipts, err := executor.processTransactions(batch, len(successfulTxs), crossChainTransactions, stateDB, context.ChainConfig, true)
 	if err != nil {
 		return nil, err
 	}
@@ -176,7 +199,7 @@ func (executor *batchExecutor) ComputeBatch(context *BatchExecutionContext) (*Co
 	// we need to copy the batch to reset the internal hash cache
 	copyBatch := *batch
 	copyBatch.Header.Root = stateDB.IntermediateRoot(false)
-	copyBatch.Transactions = append(transactionsToProcess, freeTransactions...)
+	copyBatch.Transactions = append(successfulTxs, freeTransactions...)
 	copyBatch.ResetHash()
 
 	if err = executor.populateOutboundCrossChainData(&copyBatch, block, txReceipts); err != nil {
@@ -362,28 +385,37 @@ func (executor *batchExecutor) verifyInboundCrossChainTransactions(transactions
 	return nil
 }
 
-func (executor *batchExecutor) processTransactions(batch *core.Batch, tCount int, txs []*common.L2Tx, stateDB *state.StateDB, cc *params.ChainConfig, noBaseFee bool) ([]*common.L2Tx, []*types.Receipt, error) {
+func (executor *batchExecutor) processTransactions(
+	batch *core.Batch,
+	tCount int,
+	txs []*common.L2Tx,
+	stateDB *state.StateDB,
+	cc *params.ChainConfig,
+	noBaseFee bool,
+) ([]*common.L2Tx, []*common.L2Tx, []*types.Receipt, error) {
 	var executedTransactions []*common.L2Tx
+	var excludedTransactions []*common.L2Tx
 	var txReceipts []*types.Receipt
 
 	txResults := evm.ExecuteTransactions(txs, stateDB, batch.Header, executor.storage, cc, tCount, noBaseFee, executor.logger)
 	for _, tx := range txs {
 		result, f := txResults[tx.Hash()]
 		if !f {
-			return nil, nil, fmt.Errorf("there should be an entry for each transaction")
+			return nil, nil, nil, fmt.Errorf("there should be an entry for each transaction")
 		}
 		rec, foundReceipt := result.(*types.Receipt)
 		if foundReceipt {
 			executedTransactions = append(executedTransactions, tx)
 			txReceipts = append(txReceipts, rec)
 		} else {
 			// Exclude all errors
+			excludedTransactions = append(excludedTransactions, tx)
 			executor.logger.Info("Excluding transaction from batch", log.TxKey, tx.Hash(), log.BatchHashKey, batch.Hash(), "cause", result)
 		}
 	}
 	sort.Sort(sortByTxIndex(txReceipts))
 
-	return executedTransactions, txReceipts, nil
+	return executedTransactions, excludedTransactions, txReceipts, nil
 }
 
 func allReceipts(txReceipts []*types.Receipt, depositReceipts []*types.Receipt) types.Receipts {

go/enclave/evm/evm_facade.go

@@ -111,6 +111,16 @@ func executeTransaction(
 		for _, l := range receipt.Logs {
 			l.BlockHash = batchHash
 		}
+
+		// Do not increase the balance of zero address as it is the contract deployment address.
+		// Doing so might cause weird interactions.
+		if header.Coinbase.Big().Cmp(gethcommon.Big0) != 0 {
+			gasUsed := big.NewInt(0).SetUint64(receipt.GasUsed)
+			executionGasCost := big.NewInt(0).Mul(gasUsed, header.BaseFee)
+			// As the baseFee is burned, we add it back to the coinbase.
+			// Geth should automatically add the tips.
+			s.AddBalance(header.Coinbase, executionGasCost)
+		}
 	}
 
 	header.MixDigest = before

integration/networktest/actions/native_fund_actions.go

@@ -12,9 +12,11 @@ import (
 )
 
 type SendNativeFunds struct {
-	FromUser int
-	ToUser   int
-	Amount   *big.Int
+	FromUser   int
+	ToUser     int
+	Amount     *big.Int
+	GasLimit   *big.Int
+	SkipVerify bool
 
 	user   *userwallet.UserWallet
 	txHash *common.Hash
@@ -33,7 +35,11 @@ func (s *SendNativeFunds) Run(ctx context.Context, _ networktest.NetworkConnecto
 	if err != nil {
 		return ctx, err
 	}
-	txHash, err := user.SendFunds(ctx, target.Address(), s.Amount)
+	gas := uint64(1_000_000)
+	if s.GasLimit != nil {
+		gas = s.GasLimit.Uint64()
+	}
+	txHash, err := user.SendFunds(ctx, target.Address(), s.Amount, gas)
 	if err != nil {
 		return nil, err
 	}
@@ -43,6 +49,10 @@ func (s *SendNativeFunds) Run(ctx context.Context, _ networktest.NetworkConnecto
 }
 
 func (s *SendNativeFunds) Verify(ctx context.Context, _ networktest.NetworkConnector) error {
+	if s.SkipVerify {
+		return nil
+	}
+
 	receipt, err := s.user.AwaitReceipt(ctx, s.txHash)
 	if err != nil {
 		return fmt.Errorf("failed to fetch receipt - %w", err)

integration/networktest/env/testnet.go

@@ -110,7 +110,7 @@ func (t *testnetConnector) GetValidatorNode(_ int) networktest.NodeOperator {
 }
 
 func (t *testnetConnector) AllocateFaucetFundsWithWallet(ctx context.Context, account gethcommon.Address) error {
-	txHash, err := t.faucetWallet.SendFunds(ctx, account, _defaultFaucetAmount)
+	txHash, err := t.faucetWallet.SendFunds(ctx, account, _defaultFaucetAmount, 1_000_000)
 	if err != nil {
 		return err
 	}

integration/networktest/log.go

@@ -1,19 +1,21 @@
 package networktest
 
 import (
+	"os"
+
 	"github.com/ethereum/go-ethereum/log"
 	"github.com/obscuronet/go-obscuro/integration/common/testlog"
 )
 
 // EnsureTestLogsSetUp calls Setup if it hasn't already been called (some tests run tests within themselves, we don't want
 // the log folder flipping around for every subtest, so we assume this is called for the top level test that is running
 // and ignore subsequent calls
-func EnsureTestLogsSetUp(testName string) {
+func EnsureTestLogsSetUp(testName string) *os.File {
 	logger := testlog.Logger()
 	if logger != nil {
-		return // already setup, do not reconfigure
+		return nil // already setup, do not reconfigure
 	}
-	testlog.Setup(&testlog.Cfg{
+	return testlog.Setup(&testlog.Cfg{
 		// todo (@matt) - walk up the dir tree to find /integration/.build or find best practice solution
 		// bit of a hack - tests need to be in a package nested within /tests to get logs in the right place
 		LogDir:      "../../../.build/networktest/",

integration/networktest/runner.go

@@ -7,6 +7,12 @@ import (
 	"time"
 )
 
+type contextKey int
+
+const (
+	LogFileKey contextKey = 0
+)
+
 // Run provides a standardised way to run tests and provides a single place for changing logging/output styles, etc.
 //
 // The tests in `/tests` should typically only contain a single line, executing this method.
@@ -17,12 +23,13 @@ import (
 //	networktest.Run(t, env.DevTestnet(), tests.smokeTest())
 //	networktest.Run(t, env.LocalDevNetwork(WithNumValidators(8)), traffic.RunnerTest(traffic.NativeFundsTransfers(), 30*time.Second)
 func Run(testName string, t *testing.T, env Environment, action Action) {
-	EnsureTestLogsSetUp(testName)
+	logFile := EnsureTestLogsSetUp(testName)
 	network, envCleanup, err := env.Prepare()
 	if err != nil {
 		t.Fatal(err)
 	}
-	ctx, cancelCtx := context.WithCancel(context.Background())
+	initialCtx, cancelCtx := context.WithCancel(context.Background())
+	ctx := context.WithValue(initialCtx, LogFileKey, logFile)
 	defer func() {
 		envCleanup()
 		cancelCtx()

integration/networktest/tests/gas/gas_test.go

@@ -0,0 +1,81 @@
+package helpful
+
+import (
+	"bufio"
+	"context"
+	"fmt"
+	"math/big"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/ethereum/go-ethereum/common"
+	"github.com/obscuronet/go-obscuro/integration/networktest/actions"
+
+	"github.com/obscuronet/go-obscuro/integration/networktest"
+	"github.com/obscuronet/go-obscuro/integration/networktest/env"
+)
+
+// Smoke tests are useful for checking a network is live or checking basic functionality is not broken
+
+var _transferAmount = big.NewInt(100_000_000)
+
+// Transaction with insufficient gas limit for the intrinsic cost. It should result in no difference
+// to user balances, but network should remain operational.
+// Used to automatically detect batch desync based on transaction inclusion.
+// Sequencer and Validator will process different transactions, but state should be identical.
+func TestExecuteNativeFundsTransferNoGas(t *testing.T) {
+	networktest.TestOnlyRunsInIDE(t)
+	networktest.Run(
+		"gas-underlimit-test",
+		t,
+		env.LocalDevNetwork(),
+		actions.Series(
+			&actions.CreateTestUser{UserID: 0},
+			&actions.CreateTestUser{UserID: 1},
+			actions.SetContextValue(actions.KeyNumberOfTestUsers, 2),
+
+			&actions.AllocateFaucetFunds{UserID: 0},
+			actions.SnapshotUserBalances(actions.SnapAfterAllocation), // record user balances (we have no guarantee on how much the network faucet allocates)
+			&actions.SendNativeFunds{
+				FromUser:   0,
+				ToUser:     1,
+				Amount:     _transferAmount,
+				GasLimit:   big.NewInt(11_000),
+				SkipVerify: true,
+			},
+			&actions.VerifyBalanceAfterTest{
+				UserID:          1,
+				ExpectedBalance: common.Big0,
+			},
+			actions.VerifyOnlyAction(func(ctx context.Context, network networktest.NetworkConnector) error {
+				logFile, ok := (ctx.Value(networktest.LogFileKey)).(*os.File)
+				if !ok {
+					return fmt.Errorf("log file not provided in context")
+				}
+				fmt.Println(logFile.Name())
+
+				f, err := os.Open(logFile.Name())
+				if err != nil {
+					return err
+				}
+
+				scanner := bufio.NewScanner(f)
+
+				// https://golang.org/pkg/bufio/#Scanner.Scan
+				for scanner.Scan() {
+					if strings.Contains(scanner.Text(), "Error validating batch") {
+						return fmt.Errorf("found bad batches in test logs")
+					}
+				}
+
+				if err := scanner.Err(); err != nil {
+					// Handle the error
+					return err
+				}
+
+				return nil
+			}),
+		),
+	)
+}

integration/networktest/userwallet/userwallet.go

@@ -85,7 +85,7 @@ func (s *UserWallet) ChainID() *big.Int {
 	return big.NewInt(integration.ObscuroChainID)
 }
 
-func (s *UserWallet) SendFunds(ctx context.Context, addr gethcommon.Address, value *big.Int) (*gethcommon.Hash, error) {
+func (s *UserWallet) SendFunds(ctx context.Context, addr gethcommon.Address, value *big.Int, gas uint64) (*gethcommon.Hash, error) {
 	err := s.EnsureClientSetup(ctx)
 	if err != nil {
 		return nil, fmt.Errorf("unable to prepare client to send funds - %w", err)
@@ -94,7 +94,7 @@ func (s *UserWallet) SendFunds(ctx context.Context, addr gethcommon.Address, val
 	tx := &types.LegacyTx{
 		Nonce:    s.nonce,
 		Value:    value,
-		Gas:      uint64(1_000_000),
+		Gas:      gas,
 		GasPrice: gethcommon.Big1,
 		To:       &addr,
 	}

integration/simulation/devnetwork/dev_network.go

@@ -67,7 +67,7 @@ func (s *InMemDevNetwork) AllocateFaucetFunds(ctx context.Context, account gethc
 	s.faucetLock.Lock()
 	defer s.faucetLock.Unlock()
 
-	txHash, err := s.faucet.SendFunds(ctx, account, _defaultFaucetAmount)
+	txHash, err := s.faucet.SendFunds(ctx, account, _defaultFaucetAmount, 1_000_000)
 	if err != nil {
 		return err
 	}

integration/simulation/network/geth_utils.go

@@ -64,6 +64,8 @@ func StartGethNetwork(wallets *params.SimWallets, startPort int, blockDurationSe
 		walletAddresses = append(walletAddresses, w.Address().String())
 	}
 
+	fmt.Printf("Prefunded wallet addresses: %d\n", len(walletAddresses))
+
 	// kickoff the network with the prefunded wallet addresses
 	eth2Network := eth2network.NewEth2Network(
 		path,
@@ -98,7 +100,7 @@ func DeployObscuroNetworkContracts(client ethadapter.EthClient, wallets *params.
 	}
 	mgmtContractReceipt, err := DeployContract(client, wallets.MCOwnerWallet, bytecode)
 	if err != nil {
-		return nil, fmt.Errorf("failed to deploy management contract. Cause: %w", err)
+		return nil, fmt.Errorf("failed to deploy management contract from %s. Cause: %w", wallets.MCOwnerWallet.Address(), err)
 	}
 
 	managementContract, err := ManagementContract.NewManagementContract(mgmtContractReceipt.ContractAddress, client.EthClient())