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 1 commit
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
55 changes: 45 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,42 @@ 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 less than the parent, then the input is invalid
if childHeader.Height < parentHeader.Height {
return false, types.ErrInvalidAncestor.Wrapf("ancestor height is larger than descendant height")
}

// If they have the same height, then the result is false
if childHeader.Height == parentHeader.Height {
return false, nil
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just curious why you don't consider this to be an error as well, or the why isn't the previous not an error if this one isn't.

It sounds like either the caller should know that the block behind parentHashBytes can be an ancestor of the one behind childHashBytes, or they don't. If they know that it should be okay to call this because the height is <, then they should also know not to call when the height is ==. OTOH if they don't know, then both should not be considered errors, just plain false.


// 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 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
62 changes: 24 additions & 38 deletions x/btclightclient/keeper/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,12 @@ func (s headersState) HeadersByHeight(height uint64, f func(*types.BTCHeaderInfo
}

// getDescendingHeadersUpTo returns a collection of descending headers according to their height
func (s headersState) getDescendingHeadersUpTo(tipHeight uint64, depth uint64) []*types.BTCHeaderInfo {
func (s headersState) getDescendingHeadersUpTo(startHeight uint64, depth uint64) []*types.BTCHeaderInfo {
var headers []*types.BTCHeaderInfo
s.iterateReverseHeaders(func(header *types.BTCHeaderInfo) bool {
// Use `depth+1` because we want to first gather all the headers
// with a depth of `depth`.
if tipHeight-header.Height == depth+1 {
if startHeight-header.Height == depth+1 {
return true
}
headers = append(headers, header)
Expand All @@ -184,15 +184,9 @@ func (s headersState) getDescendingHeadersUpTo(tipHeight uint64, depth uint64) [
return headers
}

// GetMainChainUpTo returns the current canonical chain as a collection of block headers
// starting from the tip and ending on the header that has a depth distance from it.
func (s headersState) GetMainChainUpTo(depth uint64) []*types.BTCHeaderInfo {
// If there is no tip, there is no base header
if !s.TipExists() {
return nil
}
currentHeader := s.GetTip()

// GetHeaderAncestryUpTo returns a list of headers starting from the header parameter and leading to
// the header that has a `depth` distance from it.
func (s headersState) GetHeaderAncestryUpTo(currentHeader *types.BTCHeaderInfo, depth uint64) []*types.BTCHeaderInfo {
// Retrieve a collection of headers in descending height order
// Use depth+1 since we want all headers at the depth height.
headers := s.getDescendingHeadersUpTo(currentHeader.Height, depth)
Expand All @@ -214,6 +208,16 @@ func (s headersState) GetMainChainUpTo(depth uint64) []*types.BTCHeaderInfo {
return chain
}

// GetMainChainUpTo returns the current canonical chain as a collection of block headers
// starting from the tip and ending on the header that has a depth distance from it.
func (s headersState) GetMainChainUpTo(depth uint64) []*types.BTCHeaderInfo {
// If there is no tip, there is no base header
if !s.TipExists() {
return nil
}
return s.GetHeaderAncestryUpTo(s.GetTip(), depth)
}

// GetMainChain retrieves the main chain as a collection of block headers starting from the tip
// and ending on the base BTC header.
func (s headersState) GetMainChain() []*types.BTCHeaderInfo {
Expand Down Expand Up @@ -294,38 +298,20 @@ func (s headersState) GetInOrderAncestorsUntil(descendant *types.BTCHeaderInfo,
return []*types.BTCHeaderInfo{}
}

currentHeader := descendant

var ancestors []*types.BTCHeaderInfo
ancestors = append(ancestors, descendant)
if descendant.HasParent(ancestor) {
return ancestors
return []*types.BTCHeaderInfo{descendant}
}

found := false
s.iterateReverseHeaders(func(header *types.BTCHeaderInfo) bool {
if header.Eq(ancestor) {
found = true
return true
}
if currentHeader.HasParent(header) {
currentHeader = header
ancestors = append(ancestors, header)
}
// Abandon the iteration if the height of the current header is lower
// than the height of the provided ancestor
if currentHeader.Height < ancestor.Height {
return true
}
return false
})

// If the header was not found, discard the ancestors list
if !found {
ancestors = []*types.BTCHeaderInfo{}
ancestors := s.GetHeaderAncestryUpTo(descendant, descendant.Height-ancestor.Height)
if !ancestors[len(ancestors)-1].Eq(ancestor) {
// `ancestor` is not an ancestor of `descendant`, return an empty list
return []*types.BTCHeaderInfo{}
}

// Reverse the array
// Discard the last element of the ancestry which corresponds to `ancestor`
ancestors = ancestors[:len(ancestors)-1]

// Reverse the ancestry
for i, j := 0, len(ancestors)-1; i < j; i, j = i+1, j-1 {
ancestors[i], ancestors[j] = ancestors[j], ancestors[i]
}
Expand Down
Loading