feat(ci): create disk image after a successful full sync test

[dconnolly]

Motivation

We need a fully-synced zebrad state (or pretty close) to do fast lightwalletd-zebrad tests (and maybe other things).

Solution

On successful full sync test, create a disk image in gcloud from the vm image that the test has been writing to, named zebrad-cache-$COMMIT_SHA_SHORT-mainnet.

Resolves #3545

Review

@gustavovalverde

Reviewer Checklist

• Works correctly

Follow Up Work

It would be nice/handy to include the block height where we stopped syncing in the name of the image, we probably need to extract that with a regex from the sync logs, similar to how we do to get the container name in earlier workflow steps. Done!

We should also include the state version (v14, etc) in the name/metadata. Done!

[gustavovalverde]

@dconnolly Are we going to add the following to this PR or in a separate one?

• Make sure the triggers for the workflow fire on changes to state format, lightwalletd support tweaks, etc
• Name the image with the block height
• Put the state version in the name

[teor2345]

• Make sure the triggers for the workflow fire on changes to state format, lightwalletd support tweaks, etc

The cached state database version should change whenever we make lightwalletd state changes. So these checks should be the same thing.

(And the same as the existing cached state triggers for the post-Canopy sync workflow.)

[teor2345]

If we want to run a testnet sync, we'll need to change:
FULL_SYNC_MAINNET_TIMEOUT_MINUTES to:
FULL_SYNC_TESTNET_TIMEOUT_MINUTES.

This is a missing feature in the current workflow.

[dconnolly]

@dconnolly Are we going to add the following to this PR or in a separate one?

• Make sure the triggers for the workflow fire on changes to state format, lightwalletd support tweaks, etc

Oh yes, we have to update the triggers to not just be pre-merge by Mergify, or manual, right? It currently runs every time *.rs changes, deps change, or the workflow yaml changes.

• Name the image with the block height

We can do some regex to extract this from the stdout as it is now

• Put the state version in the name

We may have to make Zebra changes to extract this in a nice way

[teor2345]

• Put the state version in the name

We may have to make Zebra changes to extract this in a nice way

Zebra logs the state version and network when it opens the state, it looks like:

Mar 28 19:18:49.565 INFO {zebrad="9e065b5" net="Main"}: zebra_state::service::finalized_state::disk_db: Opened Zebra state cache at /home/user/.cache/zebra/state/v18/mainnet

[dconnolly]

• Put the state version in the name

We may have to make Zebra changes to extract this in a nice way

Zebra logs the state version and network when it opens the state, it looks like:

Mar 28 19:18:49.565 INFO {zebrad="9e065b5" net="Main"}: zebra_state::service::finalized_state::disk_db: Opened Zebra state cache at /home/user/.cache/zebra/state/v18/mainnet

Nice ✨

[teor2345]

Is this blocked by making the full sync faster, so it actually finishes?
Or can we just merge this feature, and make the full sync faster later?

[dconnolly]

Is this blocked by making the full sync faster, so it actually finishes? Or can we just merge this feature, and make the full sync faster later?

We can merge as soon as the test run succeeds, I think everything is fast enough to finish

[teor2345]

Is this blocked by making the full sync faster, so it actually finishes? Or can we just merge this feature, and make the full sync faster later?

We can merge as soon as the test run succeeds, I think everything is fast enough to finish

We've only had one successful full sync test in the last few days, the others have timed out:
https://github.com/ZcashFoundation/zebra/actions/workflows/test-full-sync.yml

I don't think that's a blocker for adding cached state. But it is a blocker for using the cached state.

[teor2345]

Should we also be running a full sync when the workflow changes?
Or is it ok to just manually test this workflow?

[teor2345]

I'm happy to merge this, but I'd like Gustavo to check today's fixes as well.

[gustavovalverde]

I added some comments, most are oriented to keep those changes in a different PR, and around the use of variables.

[dconnolly]

Yay

[teor2345]

I'm happy with this PR, but I don't want to approve it before Gustavo has checked his comments.

[gustavovalverde]

@teor2345 @dconnolly I posted a few review comments, are you able to see those? (just to confirm is not happening as last time that I was the only one seeing the comments)

[teor2345]

@teor2345 @dconnolly I posted a few review comments, are you able to see those? (just to confirm is not happening as last time that I was the only one seeing the comments)

I can see your comments, but I figured Deirdre was the best person to fix them.

[teor2345]

Is this blocked by making the full sync faster, so it actually finishes?

Sync speed shouldn't be a blocker for this PR, so we can merge it whenever it's done.

The address index PR series (#3934 to #4038) makes the sync slower by about 2%, which makes the full sync time out more often. But PR #4040 speeds it up by about 7%, so I'm going to merge that PR before all the database changes.

[dconnolly]

@gustavovalverde reverted some of those step id removals, applied your suggested rename, left some comments on the step outputs vs env var questions 🙏