reorder transfer checks so as to ensure sending more money than you…

… have to yourself fails with an error (fixing issue 7596)

PR #7637, also adds tests to make sure behavior is correct across versions.

.circleci/config.yml

@@ -895,6 +895,11 @@ workflows:
           suite: itest-sector_terminate
           target: "./itests/sector_terminate_test.go"
 
+      - test:
+          name: test-itest-self_sent_txn
+          suite: itest-self_sent_txn
+          target: "./itests/self_sent_txn_test.go"
+
       - test:
           name: test-itest-tape
           suite: itest-tape

chain/vm/runtime.go

@@ -332,7 +332,7 @@ func (rt *Runtime) DeleteActor(beneficiary address.Address) {
 		}
 
 		// Transfer the executing actor's balance to the beneficiary
-		if err := rt.vm.transfer(rt.Receiver(), beneficiary, act.Balance); err != nil {
+		if err := rt.vm.transfer(rt.Receiver(), beneficiary, act.Balance, rt.NetworkVersion()); err != nil {
 			panic(aerrors.Fatalf("failed to transfer balance to beneficiary actor: %s", err))
 		}
 	}

chain/vm/vm.go

@@ -339,7 +339,7 @@ func (vm *VM) send(ctx context.Context, msg *types.Message, parent *Runtime,
 		defer rt.chargeGasSafe(newGasCharge("OnMethodInvocationDone", 0, 0))
 
 		if types.BigCmp(msg.Value, types.NewInt(0)) != 0 {
-			if err := vm.transfer(msg.From, msg.To, msg.Value); err != nil {
+			if err := vm.transfer(msg.From, msg.To, msg.Value, vm.ntwkVersion(ctx, vm.blockHeight)); err != nil {
 				return nil, aerrors.Wrap(err, "failed to transfer funds")
 			}
 		}
@@ -869,50 +869,89 @@ func (vm *VM) incrementNonce(addr address.Address) error {
 	})
 }
 
-func (vm *VM) transfer(from, to address.Address, amt types.BigInt) aerrors.ActorError {
-	if from == to {
-		return nil
-	}
+func (vm *VM) transfer(from, to address.Address, amt types.BigInt, networkVersion network.Version) aerrors.ActorError {
+	var f *types.Actor
+	var fromID, toID address.Address
+	var err error
+	// switching the order around so that transactions for more than the balance sent to self fail
+	if networkVersion >= network.Version15 {
+		if amt.LessThan(types.NewInt(0)) {
+			return aerrors.Newf(exitcode.SysErrForbidden, "attempted to transfer negative value: %s", amt)
+		}
 
-	fromID, err := vm.cstate.LookupID(from)
-	if err != nil {
-		return aerrors.Fatalf("transfer failed when resolving sender address: %s", err)
-	}
+		fromID, err = vm.cstate.LookupID(from)
+		if err != nil {
+			return aerrors.Fatalf("transfer failed when resolving sender address: %s", err)
+		}
 
-	toID, err := vm.cstate.LookupID(to)
-	if err != nil {
-		return aerrors.Fatalf("transfer failed when resolving receiver address: %s", err)
-	}
+		f, err = vm.cstate.GetActor(fromID)
+		if err != nil {
+			return aerrors.Fatalf("transfer failed when retrieving sender actor: %s", err)
+		}
 
-	if fromID == toID {
-		return nil
-	}
+		if f.Balance.LessThan(amt) {
+			return aerrors.Newf(exitcode.SysErrInsufficientFunds, "transfer failed, insufficient balance in sender actor: %v", f.Balance)
+		}
 
-	if amt.LessThan(types.NewInt(0)) {
-		return aerrors.Newf(exitcode.SysErrForbidden, "attempted to transfer negative value: %s", amt)
-	}
+		if from == to {
+			log.Infow("sending to same address: noop", "from/to addr", from)
+			return nil
+		}
 
-	f, err := vm.cstate.GetActor(fromID)
-	if err != nil {
-		return aerrors.Fatalf("transfer failed when retrieving sender actor: %s", err)
+		toID, err = vm.cstate.LookupID(to)
+		if err != nil {
+			return aerrors.Fatalf("transfer failed when resolving receiver address: %s", err)
+		}
+
+		if fromID == toID {
+			log.Infow("sending to same actor ID: noop", "from/to actor", fromID)
+			return nil
+		}
+	} else {
+		if from == to {
+			return nil
+		}
+
+		fromID, err = vm.cstate.LookupID(from)
+		if err != nil {
+			return aerrors.Fatalf("transfer failed when resolving sender address: %s", err)
+		}
+
+		toID, err = vm.cstate.LookupID(to)
+		if err != nil {
+			return aerrors.Fatalf("transfer failed when resolving receiver address: %s", err)
+		}
+
+		if fromID == toID {
+			return nil
+		}
+
+		if amt.LessThan(types.NewInt(0)) {
+			return aerrors.Newf(exitcode.SysErrForbidden, "attempted to transfer negative value: %s", amt)
+		}
+
+		f, err = vm.cstate.GetActor(fromID)
+		if err != nil {
+			return aerrors.Fatalf("transfer failed when retrieving sender actor: %s", err)
+		}
 	}
 
 	t, err := vm.cstate.GetActor(toID)
 	if err != nil {
 		return aerrors.Fatalf("transfer failed when retrieving receiver actor: %s", err)
 	}
 
-	if err := deductFunds(f, amt); err != nil {
+	if err = deductFunds(f, amt); err != nil {
 		return aerrors.Newf(exitcode.SysErrInsufficientFunds, "transfer failed when deducting funds (%s): %s", types.FIL(amt), err)
 	}
 	depositFunds(t, amt)
 
-	if err := vm.cstate.SetActor(fromID, f); err != nil {
-		return aerrors.Fatalf("transfer failed when setting receiver actor: %s", err)
+	if err = vm.cstate.SetActor(fromID, f); err != nil {
+		return aerrors.Fatalf("transfer failed when setting sender actor: %s", err)
 	}
 
-	if err := vm.cstate.SetActor(toID, t); err != nil {
-		return aerrors.Fatalf("transfer failed when setting sender actor: %s", err)
+	if err = vm.cstate.SetActor(toID, t); err != nil {
+		return aerrors.Fatalf("transfer failed when setting receiver actor: %s", err)
 	}
 
 	return nil

itests/self_sent_txn_test.go

@@ -0,0 +1,102 @@
+package itests
+
+import (
+	"context"
+	"testing"
+	"time"
+
+	"github.com/filecoin-project/go-state-types/network"
+
+	"github.com/filecoin-project/go-state-types/big"
+	"github.com/filecoin-project/go-state-types/exitcode"
+	"github.com/filecoin-project/lotus/api"
+	"github.com/filecoin-project/lotus/chain/types"
+	"github.com/filecoin-project/lotus/itests/kit"
+	"github.com/stretchr/testify/require"
+)
+
+
+// these tests check that the versioned code in vm.transfer is functioning correctly across versions! 
+// we reordered the checks to make sure that a transaction with too much money in it sent to yourself will fail instead of succeeding as a noop
+// more info in this PR! https://github.com/filecoin-project/lotus/pull/7637
+func TestSelfSentTxnV15(t *testing.T) {
+	ctx := context.Background()
+
+	kit.QuietMiningLogs()
+
+	client15, _, ens := kit.EnsembleMinimal(t, kit.MockProofs(), kit.GenesisNetworkVersion(network.Version15))
+	ens.InterconnectAll().BeginMining(10 * time.Millisecond)
+
+	bal, err := client15.WalletBalance(ctx, client15.DefaultKey.Address)
+	require.NoError(t, err)
+
+	// send self half of account balance
+	msgHalfBal := &types.Message{
+		From:  client15.DefaultKey.Address,
+		To:    client15.DefaultKey.Address,
+		Value: big.Div(bal, big.NewInt(2)),
+	}
+	smHalfBal, err := client15.MpoolPushMessage(ctx, msgHalfBal, nil)
+	require.NoError(t, err)
+	mLookup, err := client15.StateWaitMsg(ctx, smHalfBal.Cid(), 3, api.LookbackNoLimit, true)
+	require.NoError(t, err)
+	require.Equal(t, exitcode.Ok, mLookup.Receipt.ExitCode)
+
+	msgOverBal := &types.Message{
+		From:       client15.DefaultKey.Address,
+		To:         client15.DefaultKey.Address,
+		Value:      big.Mul(big.NewInt(2), bal),
+		GasLimit:   10000000000,
+		GasPremium: big.NewInt(10000000000),
+		GasFeeCap:  big.NewInt(100000000000),
+		Nonce:      1,
+	}
+	smOverBal, err := client15.WalletSignMessage(ctx, client15.DefaultKey.Address, msgOverBal)
+	require.NoError(t, err)
+	smcid, err := client15.MpoolPush(ctx, smOverBal)
+	require.NoError(t, err)
+	mLookup, err = client15.StateWaitMsg(ctx, smcid, 3, api.LookbackNoLimit, true)
+	require.NoError(t, err)
+	require.Equal(t, exitcode.SysErrInsufficientFunds, mLookup.Receipt.ExitCode)
+}
+
+func TestSelfSentTxnV14(t *testing.T) {
+	ctx := context.Background()
+
+	kit.QuietMiningLogs()
+
+	client14, _, ens := kit.EnsembleMinimal(t, kit.MockProofs(), kit.GenesisNetworkVersion(network.Version14))
+	ens.InterconnectAll().BeginMining(10 * time.Millisecond)
+
+	bal, err := client14.WalletBalance(ctx, client14.DefaultKey.Address)
+	require.NoError(t, err)
+
+	// send self half of account balance
+	msgHalfBal := &types.Message{
+		From:  client14.DefaultKey.Address,
+		To:    client14.DefaultKey.Address,
+		Value: big.Div(bal, big.NewInt(2)),
+	}
+	smHalfBal, err := client14.MpoolPushMessage(ctx, msgHalfBal, nil)
+	require.NoError(t, err)
+	mLookup, err := client14.StateWaitMsg(ctx, smHalfBal.Cid(), 3, api.LookbackNoLimit, true)
+	require.NoError(t, err)
+	require.Equal(t, exitcode.Ok, mLookup.Receipt.ExitCode)
+
+	msgOverBal := &types.Message{
+		From:       client14.DefaultKey.Address,
+		To:         client14.DefaultKey.Address,
+		Value:      big.Mul(big.NewInt(2), bal),
+		GasLimit:   10000000000,
+		GasPremium: big.NewInt(10000000000),
+		GasFeeCap:  big.NewInt(100000000000),
+		Nonce:      1,
+	}
+	smOverBal, err := client14.WalletSignMessage(ctx, client14.DefaultKey.Address, msgOverBal)
+	require.NoError(t, err)
+	smcid, err := client14.MpoolPush(ctx, smOverBal)
+	require.NoError(t, err)
+	mLookup, err = client14.StateWaitMsg(ctx, smcid, 3, api.LookbackNoLimit, true)
+	require.NoError(t, err)
+	require.Equal(t, exitcode.Ok, mLookup.Receipt.ExitCode)
+}