Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: improve error handling in LoadTxInfo #1571

Open
wants to merge 5 commits into
base: v0.34.x-celestia
Choose a base branch
from
Open

refactor: improve error handling in LoadTxInfo #1571

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ce7ac3c
fix: improve error handling in LoadTxInfo
tiendn Dec 20, 2024
615f5c2
fix: add test
tiendn Dec 20, 2024
e26f2b0
fix: add more tests
tiendn Dec 20, 2024
dc150e9
fix: update test transaction not found
tiendn Dec 23, 2024
b4b304d
fix: update test more clearful
tiendn Dec 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions store/store.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
tiendn marked this conversation as resolved.
Show resolved Hide resolved
}
rootulp marked this conversation as resolved.
Show resolved Hide resolved

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)
rootulp marked this conversation as resolved.
Show resolved Hide resolved
}
if len(bz) == 0 {
return nil
return nil, nil
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[question] should we return an error in this situation? Also needs a unit test.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think it makes consistent.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still doesn't return an error. should it?

}

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
Expand Down
16 changes: 11 additions & 5 deletions store/store_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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))
Expand Down Expand Up @@ -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")
}
Expand All @@ -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)
}
}
Expand All @@ -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)
Expand Down
Loading