Skip to content

Comments

Use an invalid table index as the callback for one-way functions#2942

Merged
mergify[bot] merged 3 commits intomasterfrom
joachim/oneway
Dec 10, 2021
Merged

Use an invalid table index as the callback for one-way functions#2942
mergify[bot] merged 3 commits intomasterfrom
joachim/oneway

Conversation

@nomeata
Copy link
Contributor

@nomeata nomeata commented Nov 27, 2021

this fixes #2938, and allows developers to write canisters that are
zero-downtime upgradeable, as long as they stick to making one-way calls
onlys (i.e. plain actor model)

this fixes #2938, and allows developers to write canisters that are
zero-downtime upgradeable, as long as they stick to making one-way calls
onlys (i.e. plain actor model)
@github-actions
Copy link
Contributor

github-actions bot commented Nov 27, 2021

Comparing from 27476cb to 90705e4:
In terms of gas, no changes are observed in 3 tests.
In terms of size, no changes are observed in 3 tests.

@nomeata nomeata marked this pull request as ready for review November 27, 2021 14:23
@crusso
Copy link
Contributor

crusso commented Nov 27, 2021

Very nice but we should double check this doesn't somehow interfere with the work that @ggreif did with cleanup of the closure table after a trapping callback. Probably fine, but best to double check.

@nomeata
Copy link
Contributor Author

nomeata commented Nov 27, 2021

Double checking is appreciated. I believe that the callback table wasn't used at all for one-ways, and no cleanup callback needed or used. (This also means that the “is it safe to upgrade” check was incomplete, as it would ignore outstanding callbacks to one-way calls.)

Copy link
Contributor

@crusso crusso left a comment

Choose a reason for hiding this comment

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

Looks ok to me, but I don't quite see why the error message has changed in the test? @ggreif have you taken a look at this?

@nomeata
Copy link
Contributor Author

nomeata commented Nov 30, 2021

Because with this change, when the callback comes back, the system now complains about the callback is to be invalid. that's expected (and drun is even silent about this)

@ggreif
Copy link
Contributor

ggreif commented Nov 30, 2021

Sorry for the delay, I am a bit behind. I'll have a look soon.

@nomeata
Copy link
Contributor Author

nomeata commented Dec 10, 2021

Ping, @ggreif?

@ggreif
Copy link
Contributor

ggreif commented Dec 10, 2021

This got buried again :-(, sorry!

I have double-checked, #2663 is compatible with this change, as it will try to access the continuation table in the same way as the other callbacks.

Copy link
Contributor

@ggreif ggreif left a comment

Choose a reason for hiding this comment

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

LGTM!

@nomeata nomeata added the automerge-squash When ready, merge (using squash) label Dec 10, 2021
@mergify mergify bot merged commit 4d775f4 into master Dec 10, 2021
@mergify mergify bot deleted the joachim/oneway branch December 10, 2021 10:01
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Dec 10, 2021
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.

Upgradeable compilation of one-shot calls

3 participants