fix: use contents of the tx as identifier in cache (#20533)

Co-authored-by: Alexander Peters <alpe@users.noreply.github.com>

tests/systemtests/system.go

@@ -44,7 +44,7 @@ type TestnetInitializer interface {
 type SystemUnderTest struct {
 	execBinary         string
 	blockListener      *EventListener
-	currentHeight      int64
+	currentHeight      atomic.Int64
 	outputDir          string
 	testnetInitializer TestnetInitializer
 
@@ -164,7 +164,7 @@ func (s *SystemUnderTest) StartChain(t *testing.T, xargs ...string) {
 		s.blockListener.Subscribe("tm.event='NewBlock'", func(e ctypes.ResultEvent) (more bool) {
 			newBlock, ok := e.Data.(tmtypes.EventDataNewBlock)
 			require.True(t, ok, "unexpected type %T", e.Data)
-			atomic.StoreInt64(&s.currentHeight, newBlock.Block.Height)
+			s.currentHeight.Store(newBlock.Block.Height)
 			return true
 		}),
 	)
@@ -362,12 +362,12 @@ func (s *SystemUnderTest) PrintBuffer() {
 // AwaitBlockHeight blocks until te target height is reached. An optional timeout parameter can be passed to abort early
 func (s *SystemUnderTest) AwaitBlockHeight(t *testing.T, targetHeight int64, timeout ...time.Duration) {
 	t.Helper()
-	require.Greater(t, targetHeight, s.currentHeight)
+	require.Greater(t, targetHeight, s.currentHeight.Load())
 	var maxWaitTime time.Duration
 	if len(timeout) != 0 {
 		maxWaitTime = timeout[0]
 	} else {
-		maxWaitTime = time.Duration(targetHeight-s.currentHeight+3) * s.blockTime
+		maxWaitTime = time.Duration(targetHeight-s.currentHeight.Load()+3) * s.blockTime
 	}
 	abort := time.NewTimer(maxWaitTime).C
 	for {
@@ -393,10 +393,10 @@ func (s *SystemUnderTest) AwaitNextBlock(t *testing.T, timeout ...time.Duration)
 	}
 	done := make(chan int64)
 	go func() {
-		for start, current := atomic.LoadInt64(&s.currentHeight), atomic.LoadInt64(&s.currentHeight); current == start; current = atomic.LoadInt64(&s.currentHeight) {
+		for start, current := s.currentHeight.Load(), s.currentHeight.Load(); current == start; current = s.currentHeight.Load() {
 			time.Sleep(s.blockTime)
 		}
-		done <- atomic.LoadInt64(&s.currentHeight)
+		done <- s.currentHeight.Load()
 		close(done)
 	}()
 	select {
@@ -434,7 +434,7 @@ func (s *SystemUnderTest) ResetChain(t *testing.T) {
 
 	// reset all validator nodes
 	s.ForEachNodeExecAndWait(t, []string{"comet", "unsafe-reset-all"})
-	s.currentHeight = 0
+	s.currentHeight.Store(0)
 	s.dirty = false
 }
 
@@ -465,7 +465,7 @@ func (s *SystemUnderTest) ModifyGenesisJSON(t *testing.T, mutators ...GenesisMut
 // modify json without enforcing a reset
 func (s *SystemUnderTest) modifyGenesisJSON(t *testing.T, mutators ...GenesisMutator) {
 	t.Helper()
-	require.Empty(t, s.currentHeight, "forced chain reset required")
+	require.Empty(t, s.currentHeight.Load(), "forced chain reset required")
 	current, err := os.ReadFile(filepath.Join(WorkDir, s.nodePath(0), "config", "genesis.json"))
 	require.NoError(t, err)
 	for _, m := range mutators {
@@ -727,6 +727,10 @@ func (s *SystemUnderTest) anyNodeRunning() bool {
 	return len(s.pids) != 0
 }
 
+func (s *SystemUnderTest) CurrentHeight() int64 {
+	return s.currentHeight.Load()
+}
+
 type Node struct {
 	ID      string
 	IP      string

tests/systemtests/unordered_tx_test.go

@@ -0,0 +1,52 @@
+//go:build system_test
+
+package systemtests
+
+import (
+	"strconv"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestUnorderedTXDuplicate(t *testing.T) {
+	// scenario: test unordered tx duplicate
+	// given a running chain with a tx in the unordered tx pool
+	// when a new tx with the same hash is broadcasted
+	// then the new tx should be rejected
+
+	sut.ResetChain(t)
+	cli := NewCLIWrapper(t, sut, verbose)
+	// add genesis account with some tokens
+	account1Addr := cli.AddKey("account1")
+	account2Addr := cli.AddKey("account2")
+	sut.ModifyGenesisCLI(t,
+		[]string{"genesis", "add-genesis-account", account1Addr, "10000000stake"},
+	)
+
+	sut.StartChain(t)
+
+	height := sut.CurrentHeight()
+	timeoutHeight := height + 15
+	timeoutHeightStr := strconv.Itoa(int(timeoutHeight))
+	// send tokens
+	rsp1 := cli.Run("tx", "bank", "send", account1Addr, account2Addr, "5000stake", "--from="+account1Addr, "--fees=1stake", "--timeout-height="+timeoutHeightStr, "--unordered", "--sequence=1", "--note=1")
+	RequireTxSuccess(t, rsp1)
+
+	assertDuplicateErr := func(xt assert.TestingT, gotErr error, gotOutputs ...interface{}) bool {
+		require.Len(t, gotOutputs, 1)
+		assert.Contains(t, gotOutputs[0], "is duplicated: invalid request")
+		return false // always abort
+	}
+	rsp2 := cli.WithRunErrorMatcher(assertDuplicateErr).Run("tx", "bank", "send", account1Addr, account2Addr, "5000stake", "--from="+account1Addr, "--fees=1stake", "--timeout-height="+timeoutHeightStr, "--unordered", "--sequence=1")
+	RequireTxFailure(t, rsp2)
+
+	// assert TX executed before timeout
+	for cli.QueryBalance(account2Addr, "stake") != 5000 {
+		t.Log("query balance")
+		if current := sut.AwaitNextBlock(t); current > timeoutHeight {
+			t.Fatal("TX was not executed before timeout")
+		}
+	}
+}