From ce7ac3c2e3a10274f3f8fd57ac4681f4cd0839f2 Mon Sep 17 00:00:00 2001 From: tiendn <15717476+tiendn@users.noreply.github.com> Date: Fri, 20 Dec 2024 17:54:49 +0700 Subject: [PATCH 1/5] fix: improve error handling in LoadTxInfo --- store/store.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/store/store.go b/store/store.go index 5ddea3bbfc..0c1ba266b7 100644 --- a/store/store.go +++ b/store/store.go @@ -561,20 +561,25 @@ func LoadBlockStoreState(db dbm.DB) cmtstore.BlockStoreState { } // LoadTxInfo loads the TxInfo from disk given its hash. -func (bs *BlockStore) LoadTxInfo(txHash []byte) *cmtstore.TxInfo { +// Returns nil if the transaction is not found. +func (bs *BlockStore) LoadTxInfo(txHash []byte) (*cmtstore.TxInfo, error) { + if len(txHash) == 0 { + return nil, fmt.Errorf("cannot load tx info with empty hash") + } + bz, err := bs.db.Get(calcTxHashKey(txHash)) if err != nil { - panic(err) + return nil, fmt.Errorf("failed to get tx info from db: %w", err) } if len(bz) == 0 { - return nil + return nil, nil } var txi cmtstore.TxInfo if err = proto.Unmarshal(bz, &txi); err != nil { - panic(fmt.Errorf("unmarshal to TxInfo failed: %w", err)) + return nil, fmt.Errorf("failed to unmarshal tx info: %w", err) } - return &txi + return &txi, nil } // mustEncode proto encodes a proto.message and panics if fails From 615f5c2c5fd409140e30e5d1acdec12e76027fed Mon Sep 17 00:00:00 2001 From: tiendn <15717476+tiendn@users.noreply.github.com> Date: Fri, 20 Dec 2024 18:07:42 +0700 Subject: [PATCH 2/5] fix: add test --- store/store_test.go | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/store/store_test.go b/store/store_test.go index be119dc3ff..eb1a29c48c 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -418,7 +418,9 @@ func TestSaveTxInfo(t *testing.T) { block := blockStore.LoadBlock(h) // Check that transactions exist in the block for i, tx := range block.Txs { - txInfo := blockStore.LoadTxInfo(tx.Hash()) + txInfo, err := blockStore.LoadTxInfo(tx.Hash()) + require.NoError(t, err) + require.NotNil(t, txInfo) require.Equal(t, block.Height, txInfo.Height) require.Equal(t, uint32(i), txInfo.Index) require.Equal(t, allTxResponseCodes[txIndex], txInfo.Code) @@ -435,7 +437,9 @@ func TestSaveTxInfo(t *testing.T) { // Get a random transaction and make sure it's indexed properly block := blockStore.LoadBlock(7) tx := block.Txs[0] - txInfo := blockStore.LoadTxInfo(tx.Hash()) + txInfo, err := blockStore.LoadTxInfo(tx.Hash()) + require.NoError(t, err) + require.NotNil(t, txInfo) require.Equal(t, block.Height, txInfo.Height) require.Equal(t, block.Height, int64(7)) require.Equal(t, txInfo.Height, int64(7)) @@ -625,7 +629,7 @@ func TestPruneBlocksPrunesTxs(t *testing.T) { // Check that the saved txs exist in the block store. for _, hash := range indexedTxHashes { - txInfo := blockStore.LoadTxInfo(hash) + txInfo, err := blockStore.LoadTxInfo(hash) require.NoError(t, err) require.NotNil(t, txInfo, "transaction was not saved in the database") } @@ -638,10 +642,12 @@ func TestPruneBlocksPrunesTxs(t *testing.T) { // removed 11 blocks, each block has 1 tx so 11 txs should no longer // exist in the db. for i, hash := range indexedTxHashes { - txInfo := blockStore.LoadTxInfo(hash) + txInfo, err := blockStore.LoadTxInfo(hash) if int64(i) < 11 { + require.NoError(t, err) require.Nil(t, txInfo) } else { + require.NoError(t, err) require.NotNil(t, txInfo) } } @@ -651,7 +657,7 @@ func TestPruneBlocksPrunesTxs(t *testing.T) { block := blockStore.LoadBlock(height) for i, tx := range block.Txs { hash := tx.Hash() - txInfo := blockStore.LoadTxInfo(hash) + txInfo, err := blockStore.LoadTxInfo(hash) require.NoError(t, err) require.NotNil(t, txInfo) require.Equal(t, height, txInfo.Height) From e26f2b06af3ea8f4117ce41e972ffdd2909caed4 Mon Sep 17 00:00:00 2001 From: tiendn <15717476+tiendn@users.noreply.github.com> Date: Fri, 20 Dec 2024 23:23:38 +0700 Subject: [PATCH 3/5] fix: add more tests --- store/store_test.go | 115 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 115 insertions(+) diff --git a/store/store_test.go b/store/store_test.go index eb1a29c48c..0533dad75e 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -782,3 +782,118 @@ func newBlock(hdr types.Header, lastCommit *types.Commit) *types.Block { LastCommit: lastCommit, } } + +func TestLoadTxInfoErrors(t *testing.T) { + config := cfg.ResetTestRoot("blockchain_reactor_test") + defer os.RemoveAll(config.RootDir) + bs := NewBlockStore(dbm.NewMemDB()) + + testCases := []struct { + name string + txHash []byte + expectedError string + dbError error + }{ + { + name: "empty hash", + txHash: []byte{}, + expectedError: "cannot load tx info with empty hash", + }, + { + name: "non-existent tx", + txHash: []byte("non_existent_hash"), + expectedError: "", // no error, just nil result + }, + { + name: "corrupted data", + txHash: []byte("corrupted_hash"), + expectedError: "failed to unmarshal tx info", + }, + { + name: "database error", + txHash: []byte("error_hash"), + dbError: fmt.Errorf("mock db error"), + expectedError: "failed to get tx info from db: mock db error", + }, + } + + // Test corrupted data case by manually inserting invalid data + err := bs.db.Set(calcTxHashKey([]byte("corrupted_hash")), []byte("invalid_data")) + require.NoError(t, err) + + // Create a BlockStore with mock DB for testing database errors + bsWithMockDB := &BlockStore{ + db: &mockDBWithError{}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if tc.dbError != nil { + // Use mock DB that returns an error + bsWithMockDB.db = &mockDBWithError{err: tc.dbError} + txInfo, err := bsWithMockDB.LoadTxInfo(tc.txHash) + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedError) + require.Nil(t, txInfo) + } else { + // Use regular DB + txInfo, err := bs.LoadTxInfo(tc.txHash) + if tc.expectedError != "" { + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedError) + require.Nil(t, txInfo) + } else { + require.NoError(t, err) + require.Nil(t, txInfo) + } + } + }) + } +} + +// mockDBWithError implements a mock database that returns errors +type mockDBWithError struct { + dbm.DB + err error +} + +func (m *mockDBWithError) Get(key []byte) ([]byte, error) { + return nil, m.err +} + +func TestLoadTxInfoNotFound(t *testing.T) { + bs := NewBlockStore(dbm.NewMemDB()) + + // Test cases for "not found" scenarios + testCases := []struct { + name string + txHash []byte + setup func(db dbm.DB) + }{ + { + name: "completely non-existent key", + txHash: []byte("non_existent_hash"), + }, + { + name: "key exists but value is empty", + txHash: []byte("empty_value_hash"), + setup: func(db dbm.DB) { + err := db.Set(calcTxHashKey([]byte("empty_value_hash")), []byte{}) + require.NoError(t, err) + }, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if tc.setup != nil { + tc.setup(bs.db) + } + + // Both cases should return (nil, nil) to indicate "not found" + txInfo, err := bs.LoadTxInfo(tc.txHash) + require.NoError(t, err) + require.Nil(t, txInfo) + }) + } +} From dc150e943fa6ee026a46955cb41d86efa2d49503 Mon Sep 17 00:00:00 2001 From: tiendn <15717476+tiendn@users.noreply.github.com> Date: Tue, 24 Dec 2024 00:25:30 +0700 Subject: [PATCH 4/5] fix: update test transaction not found --- store/store.go | 6 +++--- store/store_test.go | 18 ++++++++++-------- 2 files changed, 13 insertions(+), 11 deletions(-) diff --git a/store/store.go b/store/store.go index 0c1ba266b7..a58ac60f4b 100644 --- a/store/store.go +++ b/store/store.go @@ -561,10 +561,10 @@ func LoadBlockStoreState(db dbm.DB) cmtstore.BlockStoreState { } // LoadTxInfo loads the TxInfo from disk given its hash. -// Returns nil if the transaction is not found. +// Returns err notfound if the transaction is not found. func (bs *BlockStore) LoadTxInfo(txHash []byte) (*cmtstore.TxInfo, error) { if len(txHash) == 0 { - return nil, fmt.Errorf("cannot load tx info with empty hash") + return nil, fmt.Errorf("cannot load tx info for empty txHash") } bz, err := bs.db.Get(calcTxHashKey(txHash)) @@ -572,7 +572,7 @@ func (bs *BlockStore) LoadTxInfo(txHash []byte) (*cmtstore.TxInfo, error) { return nil, fmt.Errorf("failed to get tx info from db: %w", err) } if len(bz) == 0 { - return nil, nil + return nil, fmt.Errorf("transaction not found") } var txi cmtstore.TxInfo diff --git a/store/store_test.go b/store/store_test.go index 0533dad75e..f81bb4e27f 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -644,7 +644,8 @@ func TestPruneBlocksPrunesTxs(t *testing.T) { for i, hash := range indexedTxHashes { txInfo, err := blockStore.LoadTxInfo(hash) if int64(i) < 11 { - require.NoError(t, err) + require.Error(t, err) + require.Contains(t, err.Error(), "transaction not found") require.Nil(t, txInfo) } else { require.NoError(t, err) @@ -795,22 +796,22 @@ func TestLoadTxInfoErrors(t *testing.T) { dbError error }{ { - name: "empty hash", + name: "Empty txHash", txHash: []byte{}, - expectedError: "cannot load tx info with empty hash", + expectedError: "cannot load tx info for empty txHash", }, { - name: "non-existent tx", + name: "Non-existent tx", txHash: []byte("non_existent_hash"), - expectedError: "", // no error, just nil result + expectedError: "transaction not found", // no error, just nil result }, { - name: "corrupted data", + name: "Corrupted data", txHash: []byte("corrupted_hash"), expectedError: "failed to unmarshal tx info", }, { - name: "database error", + name: "Database error", txHash: []byte("error_hash"), dbError: fmt.Errorf("mock db error"), expectedError: "failed to get tx info from db: mock db error", @@ -892,7 +893,8 @@ func TestLoadTxInfoNotFound(t *testing.T) { // Both cases should return (nil, nil) to indicate "not found" txInfo, err := bs.LoadTxInfo(tc.txHash) - require.NoError(t, err) + require.Error(t, err) + require.Contains(t, err.Error(), "transaction not found") require.Nil(t, txInfo) }) } From b4b304db21d77dc630057273bf13325e6c0aa391 Mon Sep 17 00:00:00 2001 From: tiendn <15717476+tiendn@users.noreply.github.com> Date: Tue, 24 Dec 2024 09:25:47 +0700 Subject: [PATCH 5/5] fix: update test more clearful --- store/store_test.go | 108 ++++++++++++++++---------------------------- 1 file changed, 39 insertions(+), 69 deletions(-) diff --git a/store/store_test.go b/store/store_test.go index f81bb4e27f..7bbc801f59 100644 --- a/store/store_test.go +++ b/store/store_test.go @@ -784,7 +784,7 @@ func newBlock(hdr types.Header, lastCommit *types.Commit) *types.Block { } } -func TestLoadTxInfoErrors(t *testing.T) { +func TestLoadTxInfo(t *testing.T) { config := cfg.ResetTestRoot("blockchain_reactor_test") defer os.RemoveAll(config.RootDir) bs := NewBlockStore(dbm.NewMemDB()) @@ -793,61 +793,69 @@ func TestLoadTxInfoErrors(t *testing.T) { name string txHash []byte expectedError string - dbError error + setup func(db dbm.DB) }{ { - name: "Empty txHash", + name: "TestLoadTxInfoWithEmptyHash", txHash: []byte{}, expectedError: "cannot load tx info for empty txHash", }, { - name: "Non-existent tx", + name: "TestLoadTxInfoWithNonExistentTx", txHash: []byte("non_existent_hash"), - expectedError: "transaction not found", // no error, just nil result + expectedError: "transaction not found", }, { - name: "Corrupted data", + name: "TestLoadTxInfoWithCorruptedData", txHash: []byte("corrupted_hash"), expectedError: "failed to unmarshal tx info", + setup: func(db dbm.DB) { + err := db.Set(calcTxHashKey([]byte("corrupted_hash")), []byte("invalid_data")) + require.NoError(t, err) + }, + }, + { + name: "TestLoadTxInfoWithEmptyValueInDb", + txHash: []byte("empty_value_hash"), + expectedError: "transaction not found", + setup: func(db dbm.DB) { + err := db.Set(calcTxHashKey([]byte("empty_value_hash")), []byte{}) + require.NoError(t, err) + }, }, { - name: "Database error", + name: "TestLoadTxInfoWithDatabaseError", txHash: []byte("error_hash"), - dbError: fmt.Errorf("mock db error"), expectedError: "failed to get tx info from db: mock db error", + setup: func(db dbm.DB) { + // This case will be handled separately with mockDB + }, }, } - // Test corrupted data case by manually inserting invalid data - err := bs.db.Set(calcTxHashKey([]byte("corrupted_hash")), []byte("invalid_data")) - require.NoError(t, err) - - // Create a BlockStore with mock DB for testing database errors - bsWithMockDB := &BlockStore{ - db: &mockDBWithError{}, - } - for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { - if tc.dbError != nil { - // Use mock DB that returns an error - bsWithMockDB.db = &mockDBWithError{err: tc.dbError} + if tc.name == "TestLoadTxInfoWithDatabaseError" { + // Special case for database error using mock + mockDB := &mockDBWithError{err: fmt.Errorf("mock db error")} + bsWithMockDB := &BlockStore{db: mockDB} txInfo, err := bsWithMockDB.LoadTxInfo(tc.txHash) require.Error(t, err) require.Contains(t, err.Error(), tc.expectedError) require.Nil(t, txInfo) - } else { - // Use regular DB - txInfo, err := bs.LoadTxInfo(tc.txHash) - if tc.expectedError != "" { - require.Error(t, err) - require.Contains(t, err.Error(), tc.expectedError) - require.Nil(t, txInfo) - } else { - require.NoError(t, err) - require.Nil(t, txInfo) - } + return } + + // Run setup if provided + if tc.setup != nil { + tc.setup(bs.db) + } + + // Test the case + txInfo, err := bs.LoadTxInfo(tc.txHash) + require.Error(t, err) + require.Contains(t, err.Error(), tc.expectedError) + require.Nil(t, txInfo) }) } } @@ -861,41 +869,3 @@ type mockDBWithError struct { func (m *mockDBWithError) Get(key []byte) ([]byte, error) { return nil, m.err } - -func TestLoadTxInfoNotFound(t *testing.T) { - bs := NewBlockStore(dbm.NewMemDB()) - - // Test cases for "not found" scenarios - testCases := []struct { - name string - txHash []byte - setup func(db dbm.DB) - }{ - { - name: "completely non-existent key", - txHash: []byte("non_existent_hash"), - }, - { - name: "key exists but value is empty", - txHash: []byte("empty_value_hash"), - setup: func(db dbm.DB) { - err := db.Set(calcTxHashKey([]byte("empty_value_hash")), []byte{}) - require.NoError(t, err) - }, - }, - } - - for _, tc := range testCases { - t.Run(tc.name, func(t *testing.T) { - if tc.setup != nil { - tc.setup(bs.db) - } - - // Both cases should return (nil, nil) to indicate "not found" - txInfo, err := bs.LoadTxInfo(tc.txHash) - require.Error(t, err) - require.Contains(t, err.Error(), "transaction not found") - require.Nil(t, txInfo) - }) - } -}