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.

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

Btccheckpoint oracle schema and processing #43

Merged
merged 3 commits into from
Jul 16, 2022
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
52 changes: 52 additions & 0 deletions proto/babylon/btccheckpoint/btccheckpoint.proto
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
syntax = "proto3";
package babylon.btccheckpoint;

import "gogoproto/gogo.proto";

option go_package = "github.com/babylonchain/babylon/x/btccheckpoint/types";

// Each provided OP_RETURN transaction can be idendtified by hash of block in
// which transaction was included and transaction index in the block
message TransactionKey {
uint32 index = 1;
bytes hash = 2 [
(gogoproto.customtype) = "github.com/babylonchain/babylon/types.BTCHeaderHashBytes"
];
}

// Checkpoint can be composed from multiple transactions, so to identify whole
// submission we need list of transaction keys.
// Each submission can generally be identified by this list of (txIdx, blockHash)
// tuples.
// Note: this could possibly be optimized as if transactions were in one block
// there would have the same block hash and different indexes, but each blockshash
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// there would have the same block hash and different indexes, but each blockshash
// they would have the same block hash and different indexes, but each blockhash

// is only 33 (1 bytes for prefix encoding and 32 byte hash), so there should
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// is only 33 (1 bytes for prefix encoding and 32 byte hash), so there should
// is only 33 (1 byte for prefix encoding and 32 byte hash), so there should

// be other strong arguments for this optimization
message SubmissionKey {
repeated TransactionKey key = 1;
}

// TODO: Determine if we should keep any block number or depth info.
// On one hand it may be usefull to determine if block is stable or not, on other
// depth/block number info, without context (i.e info about chain) is pretty useless
// and blockshash in enough to retrieve is from lightclient
message SubmissionData {
// TODO: this could probably be better typed
// Address of submitter of given checkpoint. Required to payup the reward to
// submitter of given checkpoint
bytes submitter = 1;
// Required to recover address of sender of btc transction to payup the reward.
// TODO: Maybe it is worth recovering senders while processing the InsertProof
// message, and store only those. Another point is that it is not that simple
// to recover sender of btc tx.
repeated bytes btctransaction = 2;
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we can add a custom type for this. If we follow the "fortress" approach which is defined for BTCHeaderBytes and BTCHeaderHashBytes, then verification would happen in only one place, while we could efficiently display it as a hex.

Copy link
Contributor Author

@KonradStaniec KonradStaniec Jul 14, 2022

Choose a reason for hiding this comment

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

tbh I am not sure this thing will ultimately be here. For now I keep it here to extract btc sender later, but I am not sure it is such each task to do, so maybe some other approach will be need (like having btc sender in the message directly ?).

That's why for now I would leave it like that and add the approach you mention in the future.

Copy link
Contributor

Choose a reason for hiding this comment

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

@fishermanymc suggested once to put the Babylon account to be rewarded into the OP_RETURN, mainly to do away with having to register the BTC->BBL mapping up front. It felt to me like that would be using a limited resource for something we can get around by other means. But it's an option.

}

// Data stored in db and indexed by epoch number
// TODO: Add btc blockheight at epooch end, when adding hadnling of epoching callbacks
message EpochData {
// List of all received checkpoints during this epoch, sorted by order of
// submission.
repeated SubmissionKey key = 1;
}

21 changes: 15 additions & 6 deletions x/btccheckpoint/btcutils/btcutils.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,18 @@ const (

// Parsed proof represent semantically valid:
// - Bitcoin Header
// - Bitcoin Header hash
// - Bitcoin Transaction
// - Bitcoin Transaction index in block
// - Non-empty OpReturnData
type ParsedProof struct {
BlockHeader wire.BlockHeader
Transaction *btcutil.Tx
OpReturnData []byte
BlockHeader wire.BlockHeader
Copy link
Member

Choose a reason for hiding this comment

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

In the btclightclient module, the concept of wire.BlockHeaders has been entirely abstracted by the BTCHeaderBytes object. I suggest that you use that here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sure.

I mean it makes not much difference to me, and if those are types which you prefer on light client level then lets go for it. Consider all following comments regarding those types solved 👍

Although at some point we may need to discuss also passing whole header at interface level, as a second source of headers from btccheckpoint module. (imo this is not critical right now, so we can just use the hash)

// keeping header hash to avoid recomputing it everytime
BlockHash chainhash.Hash
Copy link
Member

Choose a reason for hiding this comment

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

Same, but with the BTCHeaderHashBytes object. It provides methods for efficiently performing the important operations that are performed by a chainhash.Hash module and even more.

Transaction *btcutil.Tx
TransactionBytes []byte
TransactionIdx uint32
OpReturnData []byte
}

// Concatenates and double hashes two provided inputs
Expand Down Expand Up @@ -148,9 +154,12 @@ func ParseProof(
}

parsedProof := &ParsedProof{
BlockHeader: *header,
Transaction: tx,
OpReturnData: opReturnData,
BlockHeader: *header,
BlockHash: header.BlockHash(),
Copy link
Member

Choose a reason for hiding this comment

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

If header was of BTCHeaderBytes, then this would be header.Hash().

Transaction: tx,
TransactionBytes: btcTransaction,
TransactionIdx: transactionIndex,
OpReturnData: opReturnData,
}

return parsedProof, nil
Expand Down
45 changes: 33 additions & 12 deletions x/btccheckpoint/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ package keeper
import (
"fmt"

"github.com/btcsuite/btcd/wire"
"github.com/btcsuite/btcd/chaincfg/chainhash"
"github.com/tendermint/tendermint/libs/log"

"github.com/babylonchain/babylon/x/btccheckpoint/types"
Expand Down Expand Up @@ -50,26 +50,47 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
return ctx.Logger().With("module", fmt.Sprintf("x/%s", types.ModuleName))
}

func (k Keeper) GetBlockHeight(b wire.BlockHeader) (uint64, error) {
func (k Keeper) GetBlockHeight(b chainhash.Hash) (uint64, error) {
return k.btcLightClientKeeper.BlockHeight(b)
}

func (k Keeper) IsAncestor(parentHash chainhash.Hash, childHash chainhash.Hash) (bool, error) {
return k.btcLightClientKeeper.IsAncestor(parentHash, childHash)
}

func (k Keeper) GetCheckpointEpoch(c []byte) (uint64, error) {
return k.checkpointingKeeper.CheckpointEpoch(c)
}

// TODO for now we jsut store raw checkpoint with epoch as key. Ultimatly
// we should store checkpoint with some timestamp info, or even do not store
// checkpoint itelf but its status
func (k Keeper) StoreCheckpoint(ctx sdk.Context, e uint64, c []byte) {
func (k Keeper) SubmissionExists(ctx sdk.Context, sk types.SubmissionKey) bool {
store := ctx.KVStore(k.storeKey)
kBytes := k.cdc.MustMarshal(&sk)
return store.Has(kBytes)
}

// Return epoch data for given epoch, if there is not epoch data yet returns nil
func (k Keeper) GetEpochData(ctx sdk.Context, e uint64) *types.EpochData {
store := ctx.KVStore(k.storeKey)
bytes := store.Get(types.GetEpochIndexKey(e))
if len(bytes) == 0 {
return nil
} else {
Copy link
Member

Choose a reason for hiding this comment

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

else is not required here.

Copy link
Member

Choose a reason for hiding this comment

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

else is not required here.

ed := &types.EpochData{}
k.cdc.MustUnmarshal(bytes, ed)
return ed
}
}

func (k Keeper) SaveEpochData(ctx sdk.Context, e uint64, ed *types.EpochData) {
store := ctx.KVStore(k.storeKey)
key := sdk.Uint64ToBigEndian(e)
store.Set(key, c)
ek := types.GetEpochIndexKey(e)
eb := k.cdc.MustMarshal(ed)
store.Set(ek, eb)
Copy link
Member

Choose a reason for hiding this comment

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

What happens if the epoch data is already set? Is it ok to override it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I think that's the idea, that the list of submission keys gets expanded.

}

// TODO just return checkpoint if exists
func (k Keeper) GetCheckpoint(ctx sdk.Context, e uint64) []byte {
func (k Keeper) SaveSubmission(ctx sdk.Context, sk types.SubmissionKey, sd types.SubmissionData) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering if instead of having a separate SaveEpochData and SaveSubmission there should be one AddSubmission(epochNumber, submission) that calculates the keys on its own - it could even get the epoch number from the submission but I'm not sure what the state of your thinking is about who knows what about the op_return content.

That way I could not use public methods of the keeper to:

  • set submission data with the wrong key
  • insert submission data that doesn't get recorded in the epoch
  • insert submission keys in the epoch that don't exist

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was also thinking about it , and currently have method checkAndSaveEpochData in msg_server with todo // TODO maybe move it to keeper
but I wanted to consider it after pr which will use the hooks from epoching and lightclient to manipulate submission data in someway. I imagine that SaveSubmission will come in handy on its own then but we will see.

store := ctx.KVStore(k.storeKey)
key := sdk.Uint64ToBigEndian(e)
return store.Get(key)
kBytes := k.cdc.MustMarshal(&sk)
sBytes := k.cdc.MustMarshal(&sd)
store.Set(kBytes, sBytes)
}
Loading