feat: Add btclightclient events and hooks #53

vitsalis · 2022-07-07T14:41:47Z

Mostly influenced by similar pull requests by @SebastianElvis and @gitferry

x/btclightclient/keeper/keeper.go

aakoshh · 2022-07-07T20:07:56Z

x/btclightclient/keeper/keeper.go

+		// Trigger TipUpdated hook
+		k.AfterTipUpdated(ctx, height+1)


Trying to remember the discussions we had around this.

One suggestion was from @fishermanymc who I believe wanted to emit all the blocks that the chains got extended with.

My suggestion was that we should do the callbacks one by one, but make sure that all heights are emitted, one after the other, so that the listeners who have unstable checkpoints can say "aha, now the difference between the block I am interested in and the tip is exactly k or w, let's see if my block is on the main chain".

The current logic of just emitting the height can skip over some heights, in theory, making that simple rule I laid out unworkable.

It would be a pathological case, but imagine we have a chain like this, where capital letters mean "a lot of work" and small case letter mean "very little work"

B0 - B1 - B2* \ b1' - b2' - b3' - B4

So, imagine that B2 is the current tip, but b3' didn't have enough work to take over. Only when B4 arrives does the lower fork become the tip. In that case, the tip height would go from 2 to 4.

To fully and unambiguously describe the chain, we would need two events:

RollBackward(to_hash, to_height)

RollForward(header, height)

When going from B2 to B4 we would see the following callbacks:

RollBackward(hash(B0), 0)

RollForward(b1', 1)

RollForward(b2', 2)

RollForward(b3', 3)

RollForward(B4, 4)

For this you first have to find the common ancestor between the old tip and the new tip. That is easiest to achieve by first going to the same height as the old tip, then walk both forks together until the parents are the same. That's where we have to roll back to.

By telling everyone to roll back, they should be told about a block they already know about. Attaching the height to the event makes it easier for anyone who is already deeper than that to ignore this, as they must still be on the main chain. Then rolling forward one by one means at least one callback will hit the condition that is easy to check, and eventually the whole BTC chain will be emitted.

Deciding whether this "reorg" logic needs to kick in can be done by looking at the parent of the header you are adding: if it's the tip, just roll forward. If not, and the tip changed (as indicated by the return value from your method), then you have to find the common ancestor and roll forward on the new fork.

To give another illustration to why a rollback is nice, imagine we have this chain and I know my checkpoint has been submitted to BTC in B3, and the current tips is B4.

B0 - B1 - B2 - B3* - B4 \ B3' - B4' - B5'

That means the status of my checkpoint is currently SUBMITTED. But now say we extend the chain to B5', so we need a reorg, a rollback to B2, then a rollforward to B3', B4' and B5'.

If all we emit to the rest of the application is that height went from 4 to 5, then in the code monitoring the checkpoint status, I may think that everything is good, or I'm forced to query every time whether I'm still on the main chain.

Why do I want to know? Because if it's no longer on the main chain I want the status to revert to SIGNED, so that submitters can pick it up again and submit to BTC, if need to.

By emitting Rollback(hash(B2), 2), I can see that "wait, I was on the main chain at height 3, but now we are rolling back to 2, so my status needs to be reverted as well, if there are no other submissions on the main chain".

cc @gitferry

I think the Rollback is cool. But I'm not sure it is critical to us.

I don't think checkpoints should care about status revert. Consider Akosh's example. Although B3*-B4 is no longer the mainchain, I don't think we should revert the status of the checkpoint on B3* to SIGNED because in my understanding, these transactions that are forked (e.g., contained in B3* and B4) will be collected back to the miners' mempool and be proposed again. This means the checkpoint will eventually appear on the mainchain without submitters submitting it again. Changing status between SUBMITTED and SIGNED does not affect unbonding. So, I don't think we need to revert a checkpoint's status.

Nevertheless, maybe a safe card is to let the submitter re-submit a checkpoint if it notices a fork that could potentially affect a checkpoint. This ensures that a checkpoint will be eventually CONFIRMED.

Having those sets of events is cool and would allow to build more complex functionality, but I think we can be super strict about this or follow a more simple approach.

Currently, the btclightclient module does not have the concept of a rollback or a rollforward. More specifically, when a new header is included, it's accumulative PoW is calculated. If it is more than the accumulative PoW of the tip, then it becomes the tip. We do not go back to check which headers of the previous main chain are not included in the new main chain and which headers are new ones.

The main reason that we want to just output the height of the new tip header is because the other modules care whether a header is k-deep. Using the height of the tip one can check whether they should make a query about whether a particular header is k-deep on the current main-chain. The height that is emitted is only an optimization step. The chain can work even without it by asking each time the tip is updated whether the unconfirmed checkpoints are included on the main-chain.

That's the simple approach. Once the status of a checkpoint is changed to a more "safe" state (e.g. Submitted -> Signed or ) then it cannot be rolled back (without requerying which the events do not output any useful information about whether it should happen). If we want to be able to roll-back then I agree a more complex set of events is required. However, this would require some more complex operations on the btclightclient module to compute the new headers on the main chain and the ones that were removed (which would not be that performant, although a re-org would happen quite infrequently). I suggest that we see exactly what is needed by other modules and add this functionality if it is necessary or has an obvious performance or code complexity improvement.

I fact this roll back/forward reactive scheme can completely negate the need for downstream modules to query the mainchain.

They know they are on the mainchain now , and they know when they are rolled back by looking at just the height. So the status can be updated accordingly without further queries.

Agreed. Implemented and pushed the new methodology -- please take a look. The GetHighestCommonAncestor is a little bit ugly so improvement suggestions would be appreciated.

Thanks!

Appreciate your open mind here. I know I probably suggested exactly what you implemented, as a rough idea, but it was just an intuition and I think it wasn't complete without emitting all blocks at least once, at which point the rollback becomes necessary for the full picture. Sorry for the confusion.

With such an event stream in place, the btccheckpoint module that @KonradStaniec is doing can have a hook along the lines of:

on RollForward(header):

check if any BTC submissions we are tracking are included in that header, their status is turns to SUBMITTED; if so the checkpoint they have goes from SIGNED to SUBMITTED (unless already higher status)

check if any BTC submissions we are tracking are in a header whose height is k deep relative to header; if so their status goes to CONFIRMED, and the checkpoint they contain also goes to CONFIRMED unless already higher

if the checkpoint went CONFIRMED we can pay out rewards; we can remove all but the winning transaction

check if any BTC submissions we are tracking are in a header whose height is w deep relative to header; if so the checkpoint status goes to FINALIZED and we don't need to track the transactions any more for this epoch

on RollBackward(hash, height):

check if any BTC submission we are tracking is included in a header whose height is higher than the one in the event; if so, reset their status to ORPHANED

if all submissions for a checkpoint are ORPHANED then the status of the checkpoint goes back to SIGNED.

So, no need to do any queries, which should overall make it more efficient. The only part where we need the main chain check is when a checkpoint is first submitted, to determine its starting status.

There could be other ways, but at least it gives the consumer the tools to decide what works best for them.

@vitsalis give this change, have you implemented/updated what "X-deep" related query that the btc_ckpt_oracle can make?

Added it now.

aakoshh

Nicely done. I would suggest a more elaborate event emission scheme to allow the rest of the code to be as dumb as possible.

x/btclightclient/types/expected_keepers.go

gitferry

LGTM! Thanks. I only left some discussions with @aakoshh. No blocker from my side.

x/btclightclient/keeper/keeper.go

x/btclightclient/keeper/state.go

aakoshh · 2022-07-08T14:12:54Z

x/btclightclient/keeper/state.go

+		// We have already found what we're looking for, no need to proceed further
+		if found != nil {
+			if *found == btcdHeader.BlockHash() {
+				height, err := s.GetHeaderHeight(found)


Isn't this part of the keys? It would really help the header with height was available together.

Maybe the height could be looked up at the end, outside the iterator, so it's obvious that it's just an extra return value.

I'm still not entirely sure how the height can be retrieved through the (height, hash) key in a reliable manner. I recall you mentioning something about manipulating the bytes of the key in order to extract it.

Based on issue #57 , I am gonna include more information in storage so that we can avoid doing magic with bytes and improve code quality. Let's leave this for the next PR

Okay, if you're already enhancing it. What I said was that there must be a way to parse the height from the key because if there wasn't, it could not be used for sorting by height. It's either encoded as a fixed number of bytes, or there's a length prefix. In any case you know the rightmost bytes are 32 long for the hash.

x/btclightclient/keeper/state.go

x/btclightclient/keeper/keeper.go

x/btclightclient/keeper/triggers.go

aakoshh · 2022-07-08T17:26:15Z

This would be a nice feature to fuzz test.

The way I would possibly do it:

Generate a random block tree:
- Generate a maximum depth in the beginning (e.g. 1-10)
- Generate a random root block
- Generate a random number of children (use probabilities like 0: 25%, 1: 50%, 2: 25%, avoid exponential branching or it will easily take ages, with 10 depth it could be 1000 blocks if it picks 2 children often)
- Recursively generate random grandchildren for those until maximum depth is reached
Shuffle the tree by (height, hash) so it's topologically sorted, but you can't expect any particular order at the same height
Feed them in that order to the keeper
At the end, or each step, check that the last emitted RollForward event is the same as the block returned by GetTip.
At the end, or each step, check that GetTip returns a block with the maximum height observed from the stream so far

Other invariants can surely be tested by different tree generation, e.g.

generate a prefix of a tree
pick the last block
generate a short sub tree for it (max 2-3 deep)
generate a longer subtree for it (between 4-6 deep)
feed it to the keeper in this order
There should be a rollback event to the last block picked, some time after we switch from the short to the long subtree.

vitsalis · 2022-07-09T12:31:14Z

@aakoshh thanks for the insightful comments! Answered some of them and resolved others, please take a look.

About fuzzing: Completely agree, we need a methodology for building random trees to test all of the state methods. In a following PR (after some code cleanup) I will implement such a methodology and test all of the related methods.

proto/babylon/btclightclient/event.proto

aakoshh · 2022-07-11T08:38:22Z

x/btclightclient/keeper/grpc_query.go

 	// If a starting key has not been set, then the first header is the tip
 	prevHeader := k.HeadersState(sdkCtx).GetTip()
+	prevHeaderHash := prevHeader.BlockHash()
+	prevHeaderHeight, err := k.HeadersState(sdkCtx).GetHeaderHeight(&prevHeaderHash)


This handling of the "previous" doesn't look right. The comment says "if the starting key has not been set" but there is no if to check it, GetTip is called every time, and prevHeaderHeight is set to be that of the tip. Then, if the key is not 0 (is that the right check to do?) we retrieve the header, but not the corresponding height - that stays the height of the tip.

Can we either:

delay retrieving the height until we know which header we are dealing with, or

return the "info" struct which has the height, so there's no chance to mix it up?

Working on that on the refactoring PR for #57

aakoshh · 2022-07-11T08:44:21Z

x/btclightclient/keeper/keeper.go

+			// tipHeight + 1 - len(addedToMainChain) -> height of the highest common ancestor
+			addedHeight := tipHeight - uint64(len(addedToMainChain)) + 1 + uint64(idx)


Suggested change

// tipHeight + 1 - len(addedToMainChain) -> height of the highest common ancestor

addedHeight := tipHeight - uint64(len(addedToMainChain)) + 1 + uint64(idx)

addedHeight := hcaHeight + 1 + uint64(idx)

Since it's already available as a variable, this is much easier to grok.

This will entirely be removed on the PR that resolves #57

aakoshh · 2022-07-11T08:52:46Z

x/btclightclient/keeper/keeper.go

+// HeaderKDeep returns true if a header is at least k-deep on the main chain
+func (k Keeper) HeaderKDeep(ctx sdk.Context, header *wire.BlockHeader, depth uint64) bool {
+	// TODO: optimize to not traverse the entire mainchain by storing the height along with the header
+	mainchain := k.HeadersState(ctx).GetMainChain()
+	if depth > uint64(len(mainchain)) {
+		return false
+	}
+	// k-deep -> k headers built on top of the BTC header
+	// Discard the first `depth` headers
+	kDeepMainChain := mainchain[depth:]
+	for _, mainChainHeader := range kDeepMainChain {
+		if sameBlock(header, mainChainHeader) {
+			return true
+		}
+	}
+	return false
+}


We really need to rationalise how we handle headers. The wire format is that - a wire format. Since we already store the height when we insert the data into the store, there's little excuse for not using it to our advantage.

I see this retrieves the whole mainchain, which is much more than what we needed if just used the height of the header we are interested in. Based on that knowledge we would know exactly how far we have to traverse back on the main chain: if we are beyond that height, and we have not reached the header in question, it's not on the main chain.

So, a query like func MainChainDepth(blockHash: BTCBlockHash) -> uint64 can express what we need:

Retrieve the header; if it can't be found, return -1 (or an error)

Retrieve the tip

Traverse the main chain up to tip.height - header.height + 1 deep (should be an input to a variant of GetMainChain)

Check that the last block is the one we are looking for, if so, return the depth, otherwise -1 (0 would be the tip itself I suppose, there are 0 blocks building on it)

The caller can project the "at least k-deep" query by getting the depth and comparing.

x/btclightclient/keeper/state.go

aakoshh · 2022-07-11T09:03:15Z

x/btclightclient/keeper/state.go

+
+	return ancestors


This iteration is not guaranteed to return a path to the ancestor.

Once again, it would be much better to work with the "info" versions that have height, so you could quit the iteration early if you are past the point where you have a chance of meeting the ancestor.

If I ask the impossible, e.g. by mixing up the order of child and ancestor, or asking with a non-existent ancestor, or passing blocks which are on different forks, this query will dutifully iterate all the way back to the current oldest block and return the whole path to a block which is not the ancestor.

Ideally it should:

Panic if the height of the ancestor is not less than the height of the child.

Abandon the iteration if we went lower than the height of the ancestor without finding it and return an empty array.

Check that the last block in the array has the ancestor as its parent, otherwise return empty array. (That means we reached the root without visiting the ancestor; maybe they share the height).

x/btclightclient/keeper/state.go

aakoshh · 2022-07-11T09:04:50Z

x/btclightclient/keeper/state.go

+	defer iter.Close()
+
+	for ; iter.Valid(); iter.Next() {
+		btcdHeader := blockHeaderFromStoredBytes(iter.Value())


They height is in the key, but ideally we should just store the "info" as the Value so we can put more stuff in it than what we can retrieve from the key. Or you can pass the header and the height to the function.

Yep, working on that for #57

vitsalis · 2022-07-11T11:52:23Z

Thanks for the updated comments @aakoshh ! I resolved the minor ones, but left the other ones to be addressed on a PR that I'm currently working on to resolve #57 . This PR involves a major refactoring that entirely abstracts the wire.BlockHeader and chainhash.Hash interfaces in favor of storing BTCHeaderInfo objects in storage.

If there are no further comments, I suggest we merge this PR and resolve the rest in the next PR that involves the major refactoring.

aakoshh · 2022-07-11T12:05:49Z

Sure, if you want to do some of the refactorings as part of the other issue, that's totally fine.

Strictly speaking #53 (comment) is not a refactoring, the code is incorrect as it is. I added a link in the issue as a reminder.

vitsalis requested review from aakoshh, gitferry, SebastianElvis and KonradStaniec July 7, 2022 14:41

aakoshh reviewed Jul 7, 2022

View reviewed changes

x/btclightclient/keeper/keeper.go Outdated Show resolved Hide resolved

aakoshh reviewed Jul 7, 2022

View reviewed changes

gitferry reviewed Jul 8, 2022

View reviewed changes

x/btclightclient/types/expected_keepers.go Outdated Show resolved Hide resolved

gitferry approved these changes Jul 8, 2022

View reviewed changes

vitsalis force-pushed the btclightclient-events-hooks branch from 38849c9 to 88005d4 Compare July 8, 2022 12:23