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

Jump to bottom

feat: btclightclient: Refactor keepers based on needs of btcheckpoint module #72

Merged
merged 4 commits into from
Jul 27, 2022
Merged

feat: btclightclient: Refactor keepers based on needs of btcheckpoint module #72

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
dc3ad96
Refactor expected keepers
vitsalis Jul 25, 2022
053a412
minor
vitsalis Jul 26, 2022
59f3528
minor
vitsalis Jul 26, 2022
c58833e
Tests fix
vitsalis Jul 26, 2022
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
14 changes: 7 additions & 7 deletions testutil/datagen/btc_header_tree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ func (t *BTCHeaderTree) Contains(node *blctypes.BTCHeaderInfo) bool {
// GetRoot returns the root of the tree -- i.e. the node without an existing parent
func (t *BTCHeaderTree) GetRoot() *blctypes.BTCHeaderInfo {
for _, header := range t.headers {
if t.getParent(header) == nil {
if t.GetParent(header) == nil {
return header
}
}
Expand Down Expand Up @@ -154,7 +154,7 @@ func (t *BTCHeaderTree) getNodeAncestryUpToUtil(ancestry *[]*blctypes.BTCHeaderI
return
}
*ancestry = append(*ancestry, node)
parent := t.getParent(node)
parent := t.GetParent(node)
if parent != nil {
t.getNodeAncestryUpToUtil(ancestry, parent, upTo)
}
Expand All @@ -176,9 +176,9 @@ func (t *BTCHeaderTree) GetNodeAncestry(node *blctypes.BTCHeaderInfo) []*blctype
return t.GetNodeAncestryUpTo(node, nil)
}

// GetRandomAncestor retrieves the ancestry list and returns an ancestor from it.
// RandomAncestor retrieves the ancestry list and returns an ancestor from it.
// Can include the node itself.
func (t *BTCHeaderTree) GetRandomAncestor(node *blctypes.BTCHeaderInfo) *blctypes.BTCHeaderInfo {
func (t *BTCHeaderTree) RandomAncestor(node *blctypes.BTCHeaderInfo) *blctypes.BTCHeaderInfo {
ancestry := t.GetNodeAncestry(node)
idx := RandomInt(len(ancestry))
return ancestry[idx]
Expand All @@ -192,7 +192,7 @@ func (t *BTCHeaderTree) IsOnNodeChain(node *blctypes.BTCHeaderInfo, ancestor *bl
}
ancestryUpTo := t.GetNodeAncestryUpTo(node, ancestor)
lastElement := ancestryUpTo[len(ancestryUpTo)-1]
parent := t.getParent(lastElement)
parent := t.GetParent(lastElement)
if parent != nil && parent.Eq(ancestor) {
return true
}
Expand Down Expand Up @@ -244,8 +244,8 @@ func (t *BTCHeaderTree) Size() int {
return len(t.headers)
}

// getParent returns the parent of the node, or nil if it doesn't exist
func (t *BTCHeaderTree) getParent(node *blctypes.BTCHeaderInfo) *blctypes.BTCHeaderInfo {
// GetParent returns the parent of the node, or nil if it doesn't exist
func (t *BTCHeaderTree) GetParent(node *blctypes.BTCHeaderInfo) *blctypes.BTCHeaderInfo {
if header, ok := t.headers[node.Header.ParentHash().String()]; ok {
return header
}
Expand Down
6 changes: 4 additions & 2 deletions types/btc_header_hash_bytes_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,8 @@ func FuzzBTCHeaderHashBytesHexOps(f *testing.F) {
if datagen.OneInN(10) {
if datagen.OneInN(2) {
// 1/4 times generate an invalid hash size
hex = datagen.GenRandomHexStr(datagen.RandomInt(types.BTCHeaderHashLen * 20))
bzSz := datagen.RandomIntOtherThan(types.BTCHeaderHashLen, types.BTCHeaderHashLen*20)
hex = datagen.GenRandomHexStr(bzSz)
} else {
// 1/4 times generate an invalid hex
hex = string(datagen.GenRandomByteArray(types.BTCHeaderHashLen * 2))
Expand Down Expand Up @@ -135,7 +136,8 @@ func FuzzBTCHeaderHashBytesJSONOps(f *testing.F) {
if datagen.OneInN(10) {
if datagen.OneInN(2) {
// 1/4 times generate an invalid hash size
hex = datagen.GenRandomHexStr(datagen.RandomInt(types.BTCHeaderHashLen * 20))
bzSz := datagen.RandomIntOtherThan(types.BTCHeaderHashLen, types.BTCHeaderHashLen*20)
hex = datagen.GenRandomHexStr(bzSz)
} else {
// 1/4 times generate an invalid hex
hex = string(datagen.GenRandomByteArray(types.BTCHeaderHashLen * 2))
Expand Down
50 changes: 40 additions & 10 deletions x/btclightclient/keeper/keeper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,21 +142,20 @@ func (k Keeper) InsertHeader(ctx sdk.Context, header *bbl.BTCHeaderBytes) error
}

// BlockHeight returns the height of the provided header
func (k Keeper) BlockHeight(ctx sdk.Context, header *bbl.BTCHeaderBytes) (uint64, error) {
if header == nil {
func (k Keeper) BlockHeight(ctx sdk.Context, headerHash *bbl.BTCHeaderHashBytes) (uint64, error) {
if headerHash == nil {
return 0, types.ErrEmptyMessage
}
headerHash := header.Hash()
return k.headersState(ctx).GetHeaderHeight(headerHash)
}

// MainChainDepth returns the depth of the header in the main chain or -1 if it does not exist in it
func (k Keeper) MainChainDepth(ctx sdk.Context, headerBytes *bbl.BTCHeaderBytes) (int64, error) {
if headerBytes == nil {
func (k Keeper) MainChainDepth(ctx sdk.Context, headerHashBytes *bbl.BTCHeaderHashBytes) (int64, error) {
if headerHashBytes == nil {
return -1, types.ErrEmptyMessage
}
// Retrieve the header. If it does not exist, return an error
headerInfo, err := k.headersState(ctx).GetHeaderByHash(headerBytes.Hash())
headerInfo, err := k.headersState(ctx).GetHeaderByHash(headerHashBytes)
if err != nil {
return -1, err
}
Expand Down Expand Up @@ -185,11 +184,11 @@ func (k Keeper) MainChainDepth(ctx sdk.Context, headerBytes *bbl.BTCHeaderBytes)
}

// IsHeaderKDeep returns true if a header is at least k-deep on the main chain
func (k Keeper) IsHeaderKDeep(ctx sdk.Context, headerBytes *bbl.BTCHeaderBytes, depth uint64) (bool, error) {
if headerBytes == nil {
func (k Keeper) IsHeaderKDeep(ctx sdk.Context, headerHashBytes *bbl.BTCHeaderHashBytes, depth uint64) (bool, error) {
if headerHashBytes == nil {
return false, types.ErrEmptyMessage
}
mainchainDepth, err := k.MainChainDepth(ctx, headerBytes)
mainchainDepth, err := k.MainChainDepth(ctx, headerHashBytes)
if err != nil {
return false, err
}
Expand All @@ -198,6 +197,37 @@ func (k Keeper) IsHeaderKDeep(ctx sdk.Context, headerBytes *bbl.BTCHeaderBytes,
return false, nil
}
// return true if the provided depth is more than equal the mainchain depth
//panic(fmt.Sprintf("%d %d", depth, mainchainDepth))
return depth >= uint64(mainchainDepth), nil
}

// IsAncestor returns true/false depending on whether `parent` is an ancestor of `child`.
// Returns false if the parent and the child are the same header.
func (k Keeper) IsAncestor(ctx sdk.Context, parentHashBytes *bbl.BTCHeaderHashBytes, childHashBytes *bbl.BTCHeaderHashBytes) (bool, error) {
// nil checks
if parentHashBytes == nil || childHashBytes == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

One comment to arguments, why using passing by pointer not by value ? In general using pointer should be used when passing larger structs to avoid copying or when arguments are optional. Imo both are not true here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I do this mostly out of convenience. Due to the usage of the customtype attribute on the proto files, most elements are pointers to BTCHeaderBytes or BTCHeaderHashBytes objects. If the parameters are not passed as pointers, then we would have a bunch of pointer dereferences in various places and extra checks, so I prefer to delegate those checks to when the elements are actually getting used.

Copy link
Collaborator

Choose a reason for hiding this comment

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

so I understand this convenience argument although not fully agree with it as:

  • using IsAncestor with nil hashes does not really have any semantic meaning.
  • IsAncestor does not have full context of the caller if nil pointer is really error or rather a panic for example, when receiving input form external source like lets say json rpc this is really an error , but retrieving this hash from our local database it should rather be panic as we should not save nil stuf in our database. (unelss of course we use pointer to represent optional value)

Nevertheless as was merged first I will adapt my pr to use pointers, so that interfaces will allign,

return false, types.ErrEmptyMessage
}
// Retrieve parent and child header
parentHeader, err := k.headersState(ctx).GetHeaderByHash(parentHashBytes)
if err != nil {
return false, types.ErrHeaderDoesNotExist.Wrapf("parent does not exist")
}
childHeader, err := k.headersState(ctx).GetHeaderByHash(childHashBytes)
if err != nil {
return false, types.ErrHeaderDoesNotExist.Wrapf("child does not exist")
}

// If the height of the child is equal or less than the parent, then the input is invalid
if childHeader.Height <= parentHeader.Height {
return false, nil
}

// Retrieve the ancestry
ancestry := k.headersState(ctx).GetHeaderAncestryUpTo(childHeader, childHeader.Height-parentHeader.Height)
// If it is empty, return false
if len(ancestry) == 0 {
return false, nil
}
// Return whether the last element of the ancestry is equal to the parent
return ancestry[len(ancestry)-1].Eq(parentHeader), nil
}
100 changes: 93 additions & 7 deletions x/btclightclient/keeper/keeper_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ func FuzzKeeperIsHeaderKDeep(f *testing.F) {

// Test header not existing
nonExistentHeader := datagen.GenRandomBTCHeaderBytes(nil, nil)
isDeep, err = blcKeeper.IsHeaderKDeep(ctx, &nonExistentHeader, depth)
isDeep, err = blcKeeper.IsHeaderKDeep(ctx, nonExistentHeader.Hash(), depth)
if err == nil {
t.Errorf("Non existent header led to nil error")
}
Expand All @@ -62,7 +62,7 @@ func FuzzKeeperIsHeaderKDeep(f *testing.F) {
mainchain := tree.GetMainChain()
// Select a random depth based on the main-chain length
randDepth := uint64(rand.Int63n(int64(len(mainchain))))
isDeep, err = blcKeeper.IsHeaderKDeep(ctx, header.Header, randDepth)
isDeep, err = blcKeeper.IsHeaderKDeep(ctx, header.Hash, randDepth)
// Identify whether the function should return true or false
headerDepth := tip.Height - header.Height
// If the random depth that we chose is more than the headerDepth, then it should return true
Expand All @@ -76,7 +76,7 @@ func FuzzKeeperIsHeaderKDeep(f *testing.F) {
} else {
// The depth provided does not matter, we should always get false.
randDepth := rand.Uint64()
isDeep, err = blcKeeper.IsHeaderKDeep(ctx, header.Header, randDepth)
isDeep, err = blcKeeper.IsHeaderKDeep(ctx, header.Hash, randDepth)
if err != nil {
t.Errorf("Existent header led to a non-nil error %s", err)
}
Expand Down Expand Up @@ -117,7 +117,7 @@ func FuzzKeeperMainChainDepth(f *testing.F) {

// Test header not existing
nonExistentHeader := datagen.GenRandomBTCHeaderBytes(nil, nil)
depth, err = blcKeeper.MainChainDepth(ctx, &nonExistentHeader)
depth, err = blcKeeper.MainChainDepth(ctx, nonExistentHeader.Hash())
if err == nil {
t.Errorf("Non existent header led to nil error")
}
Expand All @@ -134,7 +134,7 @@ func FuzzKeeperMainChainDepth(f *testing.F) {
// Otherwise, the result should always be -1
tip := tree.GetTip()
// Get the depth
depth, err = blcKeeper.MainChainDepth(ctx, header.Header)
depth, err = blcKeeper.MainChainDepth(ctx, header.Hash)
if err != nil {
t.Errorf("Existent and header led to error")
}
Expand Down Expand Up @@ -182,7 +182,7 @@ func FuzzKeeperBlockHeight(f *testing.F) {

// Test header not existing
nonExistentHeader := datagen.GenRandomBTCHeaderBytes(nil, nil)
height, err = blcKeeper.BlockHeight(ctx, &nonExistentHeader)
height, err = blcKeeper.BlockHeight(ctx, nonExistentHeader.Hash())
if err == nil {
t.Errorf("Non existent header led to nil error")
}
Expand All @@ -192,7 +192,7 @@ func FuzzKeeperBlockHeight(f *testing.F) {

tree := genRandomTree(blcKeeper, ctx, 1, 10)
header := tree.RandomNode()
height, err = blcKeeper.BlockHeight(ctx, header.Header)
height, err = blcKeeper.BlockHeight(ctx, header.Hash)
if err != nil {
t.Errorf("Existent header led to an error")
}
Expand All @@ -202,6 +202,92 @@ func FuzzKeeperBlockHeight(f *testing.F) {
})
}

func FuzzKeeperIsAncestor(f *testing.F) {
/*
Checks:
1. If the child hash or the parent hash are nil, an error is returned
2. If the child has a lower height than the parent, an error is returned
3. If the child and the parent are the same, false is returned
4. If the parent is an ancestor of child then `true` is returned.

Data generation:
- Generate a random tree of headers and insert it into storage.
- Select a random header and select a random descendant and a random ancestor to test (2-4).
*/
datagen.AddRandomSeedsToFuzzer(f, 100)
f.Fuzz(func(t *testing.T, seed int64) {
rand.Seed(seed)
blcKeeper, ctx := testkeeper.BTCLightClientKeeper(t)

nonExistentParent := datagen.GenRandomBTCHeaderInfo()
nonExistentChild := datagen.GenRandomBTCHeaderInfo()

// nil inputs test
isAncestor, err := blcKeeper.IsAncestor(ctx, nil, nil)
if err == nil {
t.Errorf("Nil input led to nil error")
}
if isAncestor {
t.Errorf("Nil input led to true result")
}
isAncestor, err = blcKeeper.IsAncestor(ctx, nonExistentParent.Hash, nil)
if err == nil {
t.Errorf("Nil input led to nil error")
}
if isAncestor {
t.Errorf("Nil input led to true result")
}
isAncestor, err = blcKeeper.IsAncestor(ctx, nil, nonExistentChild.Hash)
if err == nil {
t.Errorf("Nil input led to nil error")
}
if isAncestor {
t.Errorf("Nil input led to true result")
}

// non-existent test
isAncestor, err = blcKeeper.IsAncestor(ctx, nonExistentParent.Hash, nonExistentChild.Hash)
if err == nil {
t.Errorf("Non existent headers led to nil error")
}
if isAncestor {
t.Errorf("Non existent headers led to true result")
}

// Generate random tree of headers
tree := genRandomTree(blcKeeper, ctx, 1, 10)
header := tree.RandomNode()
ancestor := tree.RandomNode()

if ancestor.Eq(header) {
// Same headers test
isAncestor, err = blcKeeper.IsAncestor(ctx, ancestor.Hash, header.Hash)
if err != nil {
t.Errorf("Valid input led to an error")
}
if isAncestor {
t.Errorf("Same header input led to true result")
}
} else if ancestor.Height >= header.Height { // Descendant test
isAncestor, err = blcKeeper.IsAncestor(ctx, ancestor.Hash, header.Hash)
if err != nil {
t.Errorf("Providing a descendant as a parent led to a non-nil error")
}
if isAncestor {
t.Errorf("Providing a descendant as a parent led to a true result")
}
} else { // Ancestor test
isAncestor, err = blcKeeper.IsAncestor(ctx, ancestor.Hash, header.Hash)
if err != nil {
t.Errorf("Valid input led to an error")
}
if isAncestor != tree.IsOnNodeChain(header, ancestor) { // The result should be whether it is an ancestor or not
Copy link
Contributor

Choose a reason for hiding this comment

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

For completeness sake I will say it it again: instead of having an algorithm to check, you should be able to generate data knowing what the outcome can be by picking a random ancestor and extending with another tree. Then you can pick any node in the extension and it's a descendant. The only question is is whether you can easily pick a node that doesn't have other descendants in the original tree.

But if you want to keep it this way that's fine as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Overall, I agree. In this case, we can get an ancestor or a descendant through the usage of the tree.RandomAncestor() and tree.RandomDescendant() methods, so extra tree building is not required.

However, in this test I follow this approach in order to get two birds with one stone. Specifically, I want to test that the function returns true if the node is an anecstor and false otherwise. Randomness ensures that both the test cases are going to be tested. I don't see any difference on using the IsOnNodeChain compared to using the RandomAncestor() or the RandomDescendant functions that are used extensivelly throughout the tests and testing the two cases respectively.

t.Errorf("Got invalid ancestry result. Expected %t, got %t", tree.IsOnNodeChain(header, ancestor), isAncestor)
}
}
})
}

func FuzzKeeperInsertHeader(f *testing.F) {
/*
Checks:
Expand Down
Loading