zoneconcierge API: find the BTC-finalised chain info before specific CZ height #264

SebastianElvis · 2023-01-09T07:03:03Z

Context: point 2 of https://babylon-chain.atlassian.net/wiki/spaces/BABYLON/pages/50955614/The+Oracle#1.-Using-the-Oracle

This PR provides the API that given a CZ height, return the BTC-finalized chain info no later than this CZ height.
Along the way, this PR

implements a function FindClosestHeader that given a CZ height, return the indexed header that is closest (but no later than) this height
refactors FinalizedChainInfo so that FinalizedChainInfoUntilHeight can reuse some functionalities in it

This PR comes with a fuzz test for FindClosestHeader.

Logic of FinalizedChainInfoUntilHeight

Given the CZ height,

find the last finalised chain info
if the latest header in the chain info is no higher than the given height, do the same thing as in FinalizedChainInfo
if the latest header in the chain info is higher than the given height, then
- find the closest header of the given height
- get the epoch number of the header
- do the same thing as in FinalizedChainInfo, except for using the new epoch number

vitsalis

Great work! Few comments:

vitsalis · 2023-01-09T09:27:33Z

proto/babylon/zoneconcierge/query.proto

@@ -151,3 +155,44 @@ message QueryFinalizedChainInfoResponse {
  // It is the two TransactionInfo in the best (i.e., earliest) checkpoint submission
  repeated babylon.btccheckpoint.v1.TransactionInfo proof_epoch_submitted = 8;
 }
+
+


Extra empty line.

vitsalis · 2023-01-09T09:27:59Z

x/zoneconcierge/keeper/canonical_chain_indexer.go

 	sdkerrors "cosmossdk.io/errors"
 	"github.com/babylonchain/babylon/x/zoneconcierge/types"
 	"github.com/cosmos/cosmos-sdk/store/prefix"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )

+// FindClosestHeader finds the IndexedHeader that is closest to (but not after) the given height
+// TODO: test


I see tests for this in this PR.

vitsalis · 2023-01-09T09:28:58Z

x/zoneconcierge/keeper/canonical_chain_indexer.go

+		return nil, fmt.Errorf("chain with ID %s does not have a timestamped header before height %d", chainID, height)
+	}
+	// find the header in bytes, decode and return
+	headerBytes := iter.Value()


Very nice way of finding the closest header 👏

vitsalis · 2023-01-09T09:29:36Z

x/zoneconcierge/keeper/canonical_chain_indexer_test.go

+		require.NoError(t, err)
+		require.Equal(t, headers[len(headers)-1].Header.LastCommitHash, header.Hash)
+
+		// skip some a non-zero number of headers in between


I don't entirely understand this sentence

Sorry that's a typo 😢 I was meaning that here we skip a non-zero number of headers in between, in order to create a gap of non-timestamped headers. Fixed

vitsalis · 2023-01-09T09:34:58Z

x/zoneconcierge/keeper/grpc_query.go

+	ctx := sdk.UnwrapSDKContext(c)
+	resp := &types.QueryFinalizedChainInfoUntilHeightResponse{}
+
+	// find and assign the last finalised chain info and the earliest epoch that snapshots this epoch


Suggested change

// find and assign the last finalised chain info and the earliest epoch that snapshots this epoch

// find and assign the last finalised chain info and the earliest epoch that snapshots this chain ID

vitsalis · 2023-01-09T09:35:50Z

x/zoneconcierge/keeper/grpc_query.go

+			return nil, err
+		}
+	} else { // the requested height is before the last finalised chain info
+		// starting from the requested height, find backward until a timestamped header


Suggested change

// starting from the requested height, find backward until a timestamped header

// starting from the requested height, iterate backward until a timestamped header

vitsalis · 2023-01-09T09:37:13Z

x/zoneconcierge/keeper/grpc_query.go

+	return resp, nil
+}
+
+// ProveFinalizedChainInfo generates proofs that a chainInfo has been finalised by the given epoch with epochInfo


Suggested change

// ProveFinalizedChainInfo generates proofs that a chainInfo has been finalised by the given epoch with epochInfo

// proveFinalizedChainInfo generates proofs that a chainInfo has been finalised by the given epoch with epochInfo

KonradStaniec · 2023-01-09T12:53:19Z

x/zoneconcierge/keeper/canonical_chain_indexer.go

+	// the requested height is lower than the latest header, trace back until finding a timestamped header
+	store := k.canonicalChainStore(ctx, chainID)
+	heightBytes := sdk.Uint64ToBigEndian(height)
+	iter := store.ReverseIterator(nil, heightBytes)


iterators need to be closed, defer iter.close() after this line should do the trick.

Nice catch!

KonradStaniec · 2023-01-09T13:00:14Z

x/zoneconcierge/keeper/epochs.go

 	epochingtypes "github.com/babylonchain/babylon/x/epoching/types"
 	"github.com/babylonchain/babylon/x/zoneconcierge/types"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 )

+// GetCkptInfoForFinalizedEpoch gets the raw checkpoint and the associated best submission of a finalised epoch
+// CONTRACT: the function can only take an epoch that has already been finalised as input


this is public function, at some point someone will do not read this comment and break this invariant. In general I would avoid panicking in public function. I would either return error in case of providing not finalized epoch number or restructure code somehow.

Good point. This code snippet was not a function and panicking was expected in that scenario (specifically in RPC FinalizedChainInfo and FinalizedChainInfoUntilHeight). Given that this is just a private helper function dedicated for ZoneConcierge, I changed it to a private function. Then panic might make sense. Wdyt?

Sounds good. I have nothing against panicking in private helpers, if some module invariants are broken due to programming error

KonradStaniec · 2023-01-09T13:17:56Z

x/zoneconcierge/keeper/epochs.go

+		err := fmt.Errorf("finalized epoch %d should have at least 1 checkpoint submission", epochNumber)
+		panic(err) // this can only be a programming error
+	}
+	bestSubmissionKey := ed.Key[0] // index of checkpoint tx on BTC


hmm the fact that finalized epoch must have one submission keys, which is best submission is a bit implementation detail and the fact that we are doing this check:

if len(ed.Key) == 0 { err := fmt.Errorf("finalized epoch %d should have at least 1 checkpoint submission", epochNumber) panic(err) // this can only be a programming error }

is a bit of a smell that something is not right.

Maybe btccheckpoing module should have another query like getEpochBestSubmission which would return something along:

EpochInfo { epochStatus: Status bestCheckpointBytes: []bytes }

?
Then all this checks would become internal to btcheckpoint module. In general GetEpochData should be private to btcheckpoint module (my bad 😅 ) , and there should be separate query for epoch info, which would honor all invariants.

Yeah I feel it's better to make the btccheckpoint module to provide the functionality of choosing the best submission. I have implemented a function GetEpochDataWithBestSubmission in btccheckpoint to this end, and removed the contracts/panics in the original implementation.

Nevertheless, I left GetEpochData to be public, since this will lead to a lot of changes (e.g., btccheckpoint's testkeeper requires it to be public). Perhaps we can do this in a future PR.

…-before-height

SebastianElvis · 2023-01-10T00:33:24Z

Thanks Vitalis and Konrad for the comments! I have addressed all comments and please feel free to review again.

KonradStaniec · 2023-01-10T06:58:50Z

x/btccheckpoint/keeper/keeper.go

+	if len(ed.Key) == 0 {
+		return 0, nil, nil, types.ErrNoCheckpointsForPreviousEpoch
+	}
+	bestSubmissionKey := ed.Key[0] // index of checkpoint tx on BTC


take in mind that this holds only for finalized epoch i.e only for finalized epoch we can be sure that ed.Key[0] is the best submission. There are 2 ways around it:

make this function super specific like GetEpochDataWithBestSubmissionFromFinalizedEpoch and if epoch is not finalized return error

return ed.Key[0] if epoch is finalized, otherwise chose best submission. There is a function for that getEpochChanges which checks current status of submissions against light client and choses the best one. (You would need to pass nil as parentEpochBestSubmission to avoid check for parenthood)

I see. Thanks for clarifying the choice of best submissions. Since the function is only used in ZoneConcierge, the first option might be cleaner. I have changed the function name to GetFinalizedEpochDataWithBestSubmission and added an extra check. Please have a look again.

given specific usage of this function I also think so, if it ever become used in any other context we may need to re-think that

* zoneconcierge: API for querying the chain info of a given epoch (#260) * zoneconcierge: API for querying headers in a given epoch (#261) * zoneconcierge API: adding total number of timestamped headers to chainInfo (#263) * zoneconcierge API: find the BTC-finalised chain info before specific CZ height (#264) * zoneconcierge API: find header and fork headers at a given height (#266) * checkpointing API: add checkpoint lifecycle in `RawCheckpointWithMeta` (#267) * chore: refactor `FinalizedChainInfo` API (#268) * Add integration test for zoneconcierge checkpointing (#269) * Add integration test checking info about opposing cz chain * Fix: Increase gas in e2e test (#270) Increase gas * fix: checkpointing: Do not make the `home` flag a required one and unmarshall PubKey (#271) * Fix build Co-authored-by: Runchao Han <me@runchao.rocks> Co-authored-by: Vitalis Salis <VitSalis@gmail.com>

SebastianElvis added 4 commits January 9, 2023 15:38

init

f58de1c

simplify

7443392

init impl

f03de7b

test FindClosestHeader

3d96534

SebastianElvis requested a review from vitsalis January 9, 2023 07:03

vitsalis approved these changes Jan 9, 2023

View reviewed changes

KonradStaniec reviewed Jan 9, 2023

View reviewed changes

Base automatically changed from api-count-checkpoints to dev January 9, 2023 23:13

SebastianElvis added 4 commits January 10, 2023 10:14

fix a part of comments

b2a331f

Merge branch 'dev' into zoneconcierge-api-latest-finalised-epoch-info…

65e8792

…-before-height

get epoch data with best submission

9d958d8

minor

04e16ce

SebastianElvis requested review from KonradStaniec and vitsalis January 10, 2023 00:33

SebastianElvis force-pushed the zoneconcierge-api-latest-finalised-epoch-info-before-height branch from 81840db to 04e16ce Compare January 10, 2023 02:46

KonradStaniec reviewed Jan 10, 2023

View reviewed changes

fix

0518e1c

KonradStaniec approved these changes Jan 10, 2023

View reviewed changes

SebastianElvis merged commit c2fd760 into dev Jan 10, 2023

SebastianElvis deleted the zoneconcierge-api-latest-finalised-epoch-info-before-height branch January 10, 2023 09:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

zoneconcierge API: find the BTC-finalised chain info before specific CZ height #264

zoneconcierge API: find the BTC-finalised chain info before specific CZ height #264

SebastianElvis commented Jan 9, 2023 •

edited

Loading

vitsalis left a comment

vitsalis Jan 9, 2023

vitsalis Jan 9, 2023

vitsalis Jan 9, 2023

vitsalis Jan 9, 2023

SebastianElvis Jan 9, 2023

vitsalis Jan 9, 2023

vitsalis Jan 9, 2023

vitsalis Jan 9, 2023

KonradStaniec Jan 9, 2023

SebastianElvis Jan 9, 2023

KonradStaniec Jan 9, 2023

SebastianElvis Jan 9, 2023

KonradStaniec Jan 10, 2023

KonradStaniec Jan 9, 2023

SebastianElvis Jan 10, 2023

SebastianElvis commented Jan 10, 2023

KonradStaniec Jan 10, 2023

SebastianElvis Jan 10, 2023

KonradStaniec Jan 10, 2023

	// find and assign the last finalised chain info and the earliest epoch that snapshots this epoch
	// find and assign the last finalised chain info and the earliest epoch that snapshots this chain ID

	// starting from the requested height, find backward until a timestamped header
	// starting from the requested height, iterate backward until a timestamped header

	// ProveFinalizedChainInfo generates proofs that a chainInfo has been finalised by the given epoch with epochInfo
	// proveFinalizedChainInfo generates proofs that a chainInfo has been finalised by the given epoch with epochInfo

zoneconcierge API: find the BTC-finalised chain info before specific CZ height #264

zoneconcierge API: find the BTC-finalised chain info before specific CZ height #264

Conversation

SebastianElvis commented Jan 9, 2023 • edited Loading

Logic of FinalizedChainInfoUntilHeight

vitsalis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis commented Jan 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SebastianElvis commented Jan 9, 2023 •

edited

Loading