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 bytecode migrations #320

Merged
merged 3 commits into from
Feb 6, 2024
Merged

Add bytecode migrations #320

merged 3 commits into from
Feb 6, 2024

Conversation

ureeves
Copy link
Member

@ureeves ureeves commented Feb 5, 2024

Bytecode migrations can now be performed using the Session::migrate function. This new function allows for passing in a closure that encodes a migration procedure. The procedure will have both contracts available in the state and is passed the new contract's temporary ID such that it can be called in just the same way as a regular contract. Once the procedure is executed without error the old contract is replaced with the new one and the session returned to the user.

Since it is possible that the Session is in an inconsistent state after an error in Session::migrate, the function takes the session by value, consuming it, and only returning it if no error was encountered.

Resolves: #313

Eduardo Leegwater Simões added 3 commits February 5, 2024 17:08
This contract is intended to be a near copy of the `counter` contract,
but containing two different integers as opposed to just one. It will be
used to test migrating from the counter contract to this new contract -
i.e. proof that it is possible to substitute contracts in the state.
The `migration` test proves that it is possible to swap one contract for
another, and maintain the data of the old contract - even if in a
different form - based on a procedure that simply codes the migration
using the already existing `Session`.
The new `Session::migrate` function allows for swapping contract
bytecode for a different code and perform a migration of the data by
allowing a sequence of operations on a `Session` to be done whilst both
contracts are available, and, once this sequence of operations is done,
swapping the old contract for the new one.
Copy link
Member

@herr-seppia herr-seppia left a comment

Choose a reason for hiding this comment

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

LGTM

However I've some questions:

  • should the metadata be replaced too?
  • is there a way to prevent a contract to be replaced? Or replaced only in certain circumstances?
  • should we support having the closure being a contract itself?

I agree in advance that, except the first, questions can be related to the piecrust consumer (aka rusk)

@ureeves
Copy link
Member Author

ureeves commented Feb 5, 2024

LGTM

However I've some questions:

* should the metadata be replaced too?

* is there a way to prevent a contract to be replaced? Or replaced only in certain circumstances?

* should we support having the closure being a contract itself?

I agree in advance that, except the first, questions can be related to the piecrust consumer (aka rusk)

I'll answer the questions in order:

  • Right now it can be replaced. That said we could prevent the function from replacing, if that's what we wish. As if is "should" be or not, I think that's up to downstream to decide, which is essentially how it is right now.
  • I'm not sure if I get this one. If you're worried that contracts could call this themselves - making it a clear security nightmare - don't, because they can't.
  • This would require an entire different subsystem of imports to be added to support contracts being able to define these transitions. It is possible to do so, but I fail to see a use-case downstream.

These questions seem to be written under the assumption that this functionality is made to be callable by contracts. As I say in the second point, and expand upon in the third, it is most definitely not. The API for the contracts remains exactly the same as before, and only the host can decide to call this under certain circumstances.
My view is that this should be called when we decide to do a genesis contract upgrade at a certain block height. Given that after the procedure the state tree is altered, it is mandatory that all nodes process this upgrade in exactly the same way, meaning that the upgrade code is then essentially a part of the protocol.

@herr-seppia
Copy link
Member

I've written those questions to figure out if it's possible to handle protocol upgrades without touching the binary.
Indeed, if the clojure could be a contract/wasm, the migration code could be put in the migration-block sidebyside with the new contract code.

But this, indeed, should not be a piecrust problem (we can handle it in rusk properly)

@miloszm
Copy link
Contributor

miloszm commented Feb 6, 2024

LGTM - A question though - what if we migrate, say, transfer contract, it takes some time, and in the meantime there are some transactions happening in other threads, changing state. Then, when the migration is committed, there could be some data conflicts, or some data could be lost. Should we have some kind of guarantee that other parties are not still using the old contract?

@ureeves
Copy link
Member Author

ureeves commented Feb 6, 2024

LGTM - A question though - what if we migrate, say, transfer contract, it takes some time, and in the meantime there are some transactions happening in other threads, changing state. Then, when the migration is committed, there could be some data conflicts, or some data could be lost. Should we have some kind of guarantee that other parties are not still using the old contract?

While it is indeed possible to have multiple Sessions running in parallel, it is not possible to process multiple Transactions in the context of the same session in parallel. This is because every function that writes to a session requires &mut Session, and Rust's borrow checker prevents multiple mutable reference from existing. As such the conflicts you speak of are impossible.

On your point about the timing though, you're correct. Such a migration will take time, of the order of the amount of data we want to keep. This will mean that the consensus will need to tolerate the block in which we do this migration taking longer to process than "normal".

@miloszm
Copy link
Contributor

miloszm commented Feb 6, 2024

LGTM - A question though - what if we migrate, say, transfer contract, it takes some time, and in the meantime there are some transactions happening in other threads, changing state. Then, when the migration is committed, there could be some data conflicts, or some data could be lost. Should we have some kind of guarantee that other parties are not still using the old contract?

While it is indeed possible to have multiple Sessions running in parallel, it is not possible to process multiple Transactions in the context of the same session in parallel. This is because every function that writes to a session requires &mut Session, and Rust's borrow checker prevents multiple mutable reference from existing. As such the conflicts you speak of are impossible.

On your point about the timing though, you're correct. Such a migration will take time, of the order of the amount of data we want to keep. This will mean that the consensus will need to tolerate the block in which we do this migration taking longer to process than "normal".

That is nice that a single session is safe. How about multiple sessions though? Do we have a possibility of them? - Just asking as I am not sure at the moment. With the timing problem - I think it'd be nice to have some rough estimate how long the migration could take, say, after few months of the blockchain being up with some average traffic.

@ureeves
Copy link
Member Author

ureeves commented Feb 6, 2024

That is nice that a single session is safe. How about multiple sessions though? Do we have a possibility of them? - Just asking as I am not sure at the moment. With the timing problem - I think it'd be nice to have some rough estimate how long the migration could take, say, after few months of the blockchain being up with some average traffic.

Yes, it is possible to have multiple sessions in parallel. However, these never conflict with each other, even if they are committed to disk. The consensus is responsible for which branch it takes.
It would be nice to have such a benchmark, I agree.

@miloszm
Copy link
Contributor

miloszm commented Feb 6, 2024

Could we be in trouble if consensus decides to pick some other state than a given migration was based upon?

@ureeves
Copy link
Member Author

ureeves commented Feb 6, 2024

Could we be in trouble if consensus decides to pick some other state than a given migration was based upon?

I mean, then the upgrade is just not performed. I wouldn't say this is "trouble", more like "network choice".

@ureeves ureeves merged commit 0f81d4a into main Feb 6, 2024
6 checks passed
@ureeves ureeves deleted the migrate-bytecode-313 branch February 6, 2024 13: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.

Add ability to migrate contract bytecode
3 participants