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

Move checkpoint tag to params #356

Merged
merged 5 commits into from
Apr 25, 2023
Merged

Move checkpoint tag to params #356

merged 5 commits into from
Apr 25, 2023

Conversation

KonradStaniec
Copy link
Contributor

@KonradStaniec KonradStaniec commented Apr 21, 2023

  • moves checkpoint tag to params
  • make checkpoint tag hex encoded string at babylon node level. It is job of some high level protocols to give those bytes a meaning.
  • removes file with default tags

@KonradStaniec KonradStaniec marked this pull request as ready for review April 24, 2023 06:02
Copy link
Member

@vitsalis vitsalis left a comment

Choose a reason for hiding this comment

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

Very clean! As we discussed offline, it would be nice if the BTC network was also moved to params as well, but given that the btccheckpoint module also needs to verify the work of headers as it receives them, this is not entirely straightforward. One case in which we could keep this feature (i.e. btccheckpoint receives headers as well) would be that the module doesn't do any PoW checks itself, but instead just forwards the header and the btclightclient module performs the check, returning error if it's not valid. We can further discuss this as a future optimization.

@@ -375,9 +375,10 @@ func GenRandomBabylonTxPair() ([]*wire.MsgTx, *btctxformatter.RawBtcCheckpoint)

// fake a raw checkpoint
rawBTCCkpt := GetRandomRawBtcCheckpoint()
tag := []byte{1, 2, 3, 4}
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
tag := []byte{1, 2, 3, 4}
tag := datagen.GenRandomByteArray(4)

will resolve the TODO below.

@@ -14,7 +14,7 @@ func (k Keeper) SetParams(ctx sdk.Context, p types.Params) error {
store := ctx.KVStore(k.storeKey)
bz := k.cdc.MustMarshal(&p)
store.Set(types.ParamsKey, bz)

Copy link
Member

Choose a reason for hiding this comment

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

extra whitespace

@KonradStaniec
Copy link
Contributor Author

One case in which we could keep this feature (i.e. btccheckpoint receives headers as well) would be that the module doesn't do any PoW checks itself, but instead just forwards the header and the btclightclient module performs the check, returning error if it's not valid. We can further discuss this as a future optimization

It would be doable, but it would require a bit of refactoring on btccheckpoint module part, I am not sure if it won't be better to just receive headers hashes instead of whole headers as we do not use headers as second source eitherway :)

@KonradStaniec KonradStaniec merged commit 4ace11f into dev Apr 25, 2023
@KonradStaniec KonradStaniec deleted the improve-tag-handling branch April 25, 2023 05:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants