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

feat: e2e upgrade btc headers #4

Merged
merged 99 commits into from
Aug 9, 2024
Merged

Conversation

RafilxTenfen
Copy link
Contributor

@RafilxTenfen RafilxTenfen commented Jul 30, 2024

Add upgrade to insert BTC headers

Should be merged after #16

@RafilxTenfen RafilxTenfen self-assigned this Jul 30, 2024
Copy link
Member

Choose a reason for hiding this comment

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

I know this PR is still WIP, but still wanted to ask: is it okay to have the BTC headers in Json rather than in Go code? The json could not be a part of the binary of the Go code, so the upgrade no longer depends on the new binary, but also this json file. Also the path might be hardcoded in the code and it's not intuitive for people to move the json file to the right path and conduct upgrade

As an example, Osmosis wraps a lot of data into Go code https://github.com/osmosis-labs/osmosis/blob/main/app/upgrades/v4/prop12_data.go

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, I was trying to use as json, but in the upgrade we start to have issues like

panic: open /home/babylon/data/btc_headers.json: no such file or directory

goroutine 1 [running]:
github.com/babylonlabs-io/babylon/app/upgrades/signetlaunch.CreateUpgradeHandler.func1({0x3ddc6e8?, 0xc000c30700?}, {{0xc003726490, 0xd}, {0x0, 0x0, 0x0}, 0x19, {0xc0037264a0, 0x8}, ...}, ...)
        github.com/babylonlabs-io/babylon/app/upgrades/signetlaunch/upgrades.go:44 +0x19a

which since it is a json it does not compiles together with go... So I might need to research a good way to convert json to go code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the example, having it as a string might work pretty well, I will test that out

Copy link
Collaborator

Choose a reason for hiding this comment

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

a bit of higher level comment, while working on this epic we need to define data format we expect from other tools to make the job of chain upgrade as easy as possible, and later build some tooling to automate transforming data we get into usable upgrade.

If we define it as string in go file, then this will make job of data transformation from indexer to upgrade a bit tricky as now we will need to:

  1. run indexer/bitcoind to get required block headers
  2. retrieve those headers from indexer, probably by querying api and save it into json
  3. somehow transform this json file/files into this go file with this long string

Ideally whole pipeline would be fully automated.

And this is only about btc headers, there is so much other data to collect. Some of this data will be collected by web services team (messages signed by stakers), and it would be great if we can transfrom it relatively easily into the upgrade.

tldr; we can for now leave this string in e2e test to progress forward, but ultimately we need to define:

  • in what format we expect all this data to come to core team
  • transformation function from data format to viable upgrade
  • automate this process as much as possible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeap, I totally agree with this
that is why at first I was trying to have a json, but that creates a bit more trouble to put it together with the build. But, having the data as a string was very easy to handle

So, my plan once this e2e upgrade to insert BTC Headers is working properly is to create a script for the deployments repo that runs this upgrade with a script that gets the data from the BTC and writes the content of the file app/upgrades/signetlaunch/data_btc_headers.go to run the upgrade

The BTC Headers are the low-hanging fruit at the moment, but the next case might be harder as it could be the finality providers, I am thinking of using the same strategy, gathering it all as json, put in a file as a string and parsing it inside the upgrade itself

What do you guys think?

@RafilxTenfen RafilxTenfen marked this pull request as ready for review July 30, 2024 17:27
LICENSE Outdated Show resolved Hide resolved
app/upgrades/signetlaunch/upgrades.go Outdated Show resolved Hide resolved
app/upgrades/signetlaunch/upgrades.go Outdated Show resolved Hide resolved
@KonradStaniec
Copy link
Collaborator

Maybe I was missing some important context. Why do we want to upgrade btc headers on signet? Do we want to cover the case where we launch testnetwork upgraded from v0.9.0 on mainnet?

Yup exactly that, there will be big passage of time between TGE version and phase2 version, so if we are doing upgrade already it is good occasion to insert a lot of headers at once instead of burning fees by reporter.

@gitferry
Copy link
Member

gitferry commented Aug 8, 2024

Yup exactly that, there will be big passage of time between TGE version and phase2 version, so if we are doing upgrade already it is good occasion to insert a lot of headers at once instead of burning fees by reporter.

@KonradStaniec we have base btc header in the parameters. Can we cover this case by changing this parameter? As for TGE, there won't be any btc headers relayed to Babylon. The db for storing btc headers is empty

@RafilxTenfen
Copy link
Contributor Author

Yup exactly that, there will be big passage of time between TGE version and phase2 version, so if we are doing upgrade already it is good occasion to insert a lot of headers at once instead of burning fees by reporter.

@KonradStaniec we have a base btc header in the parameters. Can we cover this case by changing this parameter? As for TGE, there won't be any btc headers relayed to Babylon. The db for storing btc headers is empty

I thought we needed the BTC headers to be included for the BTC delegations done in the past (phase-1) be valid

@gitferry
Copy link
Member

gitferry commented Aug 8, 2024

I thought we needed the BTC headers to be included for the BTC delegations done in the past (phase-1) be valid

Ah, this makes sense. So, basically pior launch, we need to insert headers from some height ahead of the first activation height in phase-1 to the base height of the launch. Am I understanding it correctly?

@RafilxTenfen
Copy link
Contributor Author

I thought we needed the BTC headers to be included for the BTC delegations done in the past (phase-1) be valid

Ah, this makes sense. So, basically pior launch, we need to insert headers from some height ahead of the first activation height in phase-1 to the base height of the launch. Am I understanding it correctly?

This was my understanding, but I might be wrong
do we have some correlation for btc staking tx block height of inclusion and btc headers in x/btclightclient?
@SebastianElvis @KonradStaniec ?

@KonradStaniec
Copy link
Collaborator

This was my understanding, but I might be wrong
do we have some correlation for btc staking tx block height of inclusion and btc headers in x/btclightclient?
@SebastianElvis @KonradStaniec ?

I think your understanding is correct. In phase-2, we will need to be able to insert all delegations from phase-1 , this means we will need all header from phase-1 activation block to be inserted into phase-2 blockchain.

And the easiest way to do it is through the upgrade we will doing eitherway.

Copy link
Member

@gitferry gitferry left a comment

Choose a reason for hiding this comment

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

Great work!

app/test_helpers.go Outdated Show resolved Hide resolved
@RafilxTenfen RafilxTenfen merged commit f2508cf into dev Aug 9, 2024
18 checks passed
vitsalis pushed a commit that referenced this pull request Aug 14, 2024
Add upgrade to insert BTC headers

Should be merged after #16
@maurolacy maurolacy deleted the rafilx/e2e-upgrade-btc-headers branch August 26, 2024 06:23
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.

4 participants