From 849648500fb87321c761a241e3afedfce28cfe11 Mon Sep 17 00:00:00 2001 From: Stefan Iliev <46542846+StefanIliev545@users.noreply.github.com> Date: Wed, 27 Sep 2023 15:11:18 +0300 Subject: [PATCH] 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 --- go/enclave/components/batch_executor.go | 44 ++++++++-- go/enclave/evm/evm_facade.go | 10 +++ .../actions/native_fund_actions.go | 18 ++++- integration/networktest/env/testnet.go | 2 +- integration/networktest/log.go | 8 +- integration/networktest/runner.go | 11 ++- integration/networktest/tests/gas/gas_test.go | 81 +++++++++++++++++++ .../networktest/userwallet/userwallet.go | 4 +- .../simulation/devnetwork/dev_network.go | 2 +- integration/simulation/network/geth_utils.go | 4 +- 10 files changed, 164 insertions(+), 20 deletions(-) create mode 100644 integration/networktest/tests/gas/gas_test.go diff --git a/go/enclave/components/batch_executor.go b/go/enclave/components/batch_executor.go index 0b390dfc26..3d75a8ff95 100644 --- a/go/enclave/components/batch_executor.go +++ b/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(©Batch, block, txReceipts); err != nil { @@ -362,15 +385,23 @@ 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 { @@ -378,12 +409,13 @@ func (executor *batchExecutor) processTransactions(batch *core.Batch, tCount int 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 { diff --git a/go/enclave/evm/evm_facade.go b/go/enclave/evm/evm_facade.go index fa7b3e42f3..b35e0e9e79 100644 --- a/go/enclave/evm/evm_facade.go +++ b/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 diff --git a/integration/networktest/actions/native_fund_actions.go b/integration/networktest/actions/native_fund_actions.go index 5184eca0d7..8b52ec9295 100644 --- a/integration/networktest/actions/native_fund_actions.go +++ b/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) diff --git a/integration/networktest/env/testnet.go b/integration/networktest/env/testnet.go index e05a14a773..a90fda057a 100644 --- a/integration/networktest/env/testnet.go +++ b/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 } diff --git a/integration/networktest/log.go b/integration/networktest/log.go index fc133c1b48..8166aa8c25 100644 --- a/integration/networktest/log.go +++ b/integration/networktest/log.go @@ -1,6 +1,8 @@ package networktest import ( + "os" + "github.com/ethereum/go-ethereum/log" "github.com/obscuronet/go-obscuro/integration/common/testlog" ) @@ -8,12 +10,12 @@ import ( // 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/", diff --git a/integration/networktest/runner.go b/integration/networktest/runner.go index cb95b718de..d33d7e0cdb 100644 --- a/integration/networktest/runner.go +++ b/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() diff --git a/integration/networktest/tests/gas/gas_test.go b/integration/networktest/tests/gas/gas_test.go new file mode 100644 index 0000000000..ec35d16629 --- /dev/null +++ b/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 + }), + ), + ) +} diff --git a/integration/networktest/userwallet/userwallet.go b/integration/networktest/userwallet/userwallet.go index c6946a3556..5faba24d47 100644 --- a/integration/networktest/userwallet/userwallet.go +++ b/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, } diff --git a/integration/simulation/devnetwork/dev_network.go b/integration/simulation/devnetwork/dev_network.go index 2964c486c7..3a2fe89189 100644 --- a/integration/simulation/devnetwork/dev_network.go +++ b/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 } diff --git a/integration/simulation/network/geth_utils.go b/integration/simulation/network/geth_utils.go index c82f62906b..98bc58c920 100644 --- a/integration/simulation/network/geth_utils.go +++ b/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())