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

Add tx to restore wasm blob to get-settings-upgrade-txs #4351

Merged
merged 1 commit into from
Jun 7, 2024

Conversation

sisuresh
Copy link
Contributor

@sisuresh sisuresh commented Jun 6, 2024

Description

Resolves #4220

Quickstart and docs will need to be updated.

Checklist

  • Reviewed the contributing document
  • Rebased on top of master (no merge commits)
  • Ran clang-format v8.0.0 (via make format or the Visual Studio extension)
  • Compiles
  • Ran all tests
  • If change impacts performance, include supporting evidence per the performance document

@sisuresh sisuresh requested a review from dmkozh June 6, 2024 22:47
Copy link
Contributor

@dmkozh dmkozh left a comment

Choose a reason for hiding this comment

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

This is probably sufficient for a while, but when data starts getting actually evicted, we'll need to come up with a better solution

@dmkozh
Copy link
Contributor

dmkozh commented Jun 7, 2024

r+ 6935d82

@latobarita latobarita merged commit b3aeb14 into stellar:master Jun 7, 2024
15 checks passed
@leighmcculloch
Copy link
Member

Quickstart and docs will need to be updated.

Heads up that the quickstart script that currently uses this command and that needs updating will be a bit gnarly, so you may want to budget time for that as a slightly larger item rather than as a quick update. The code is linked below and is particularly brittle to the addition of new or less transactions. It probably needs updating to handle an arbitrary number of txs, but if I recall correctly that was challenging because the output of this command includes a variety of things: txs, txids, and a key at the end.

Also, to make it extra fun, anything we update here needs to support any stellar-core versions used in any image, or we need to add guards to the build process to distinguish them. We've done the latter before, but it would be good to avoid that if possible.

https://github.com/stellar/quickstart/blob/6cc1a0b9af10f7bef8f849f0b459832fffa60bef/start#L539-L562

@sisuresh
Copy link
Contributor Author

Also, to make it extra fun, anything we update here needs to support any stellar-core versions used in any image, or we need to add guards to the build process to distinguish them. We've done the latter before, but it would be good to avoid that if possible.

@leighmcculloch This is unfortunate. Would it be acceptable if we broke compatibility with older core versions for one release, and then in the next release we add a flag for the restore op?

@leighmcculloch
Copy link
Member

I don't think we can break it. We can add build arguments to quickstart to signal which is in use, we've done that before, it's just additional time to set up.

@sisuresh
Copy link
Contributor Author

We could also add a conditional that just checks the number of lines returned (7 vs 9). If it's 9, we just read more lines (among a few other changes).

@leighmcculloch
Copy link
Member

👍🏻 and stellar/quickstart#611 looks good. This logic should be temporary and once all images are using that version or newer we can remove it.

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.

Restore the wasm contract used for network settings upgrade if expired
4 participants