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.

Sign up for GitHub

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

Jump to bottom

Stake contract migration #1464

Merged
merged 18 commits into from
Mar 13, 2024
Merged

Stake contract migration #1464

merged 18 commits into from
Mar 13, 2024

Conversation

miloszm
Copy link
Contributor

@miloszm miloszm commented Feb 27, 2024 •
edited
Loading

Stake contract migration

Implements issue #1448

Migration is not active unless an argument --migration-height N is given

@miloszm miloszm requested a review from herr-seppia February 27, 2024 11:10
@miloszm miloszm force-pushed the issue-1448-stake-migration branch 2 times, most recently from df33294 to 9d9b69e Compare February 27, 2024 16:58
@miloszm miloszm marked this pull request as ready for review February 27, 2024 17:31
@miloszm miloszm force-pushed the issue-1448-stake-migration branch 2 times, most recently from 5c93392 to 0a84a90 Compare February 27, 2024 19:10
@herr-seppia
Copy link
Member

Can you target "ITN" branch for this PR?

@autholykos
Copy link
Member

Blocked by #1516

@herr-seppia herr-seppia force-pushed the issue-1448-stake-migration branch from 8bd7218 to 262feb7 Compare March 8, 2024 11:46
@herr-seppia herr-seppia changed the base branch from master to ITN2 March 8, 2024 11:46
@miloszm miloszm force-pushed the issue-1448-stake-migration branch from 0b454a3 to 06b698e Compare March 8, 2024 14:33
@miloszm miloszm requested a review from herr-seppia March 8, 2024 15:29
herr-seppia
herr-seppia requested changes Mar 8, 2024
View reviewed changes
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.

Migration should not be triggered by rusk, it should be triggered by the chain.
Therefore there is no need to pass migration height to rusk, but it should be configure into the chain.

Furthermore, any state transition should accept a closure to execute before create or check the state root (this closure will be the migration code triggered by the chain)

rusk/src/lib/chain/vm.rs Outdated Show resolved Hide resolved
@miloszm miloszm requested a review from herr-seppia March 11, 2024 16:39
herr-seppia
herr-seppia requested changes Mar 12, 2024
View reviewed changes
rusk/src/lib/chain/rusk.rs Outdated Show resolved Hide resolved
rusk/src/lib/chain/rusk.rs Outdated Show resolved Hide resolved
@herr-seppia herr-seppia requested a review from goshawk-3 March 12, 2024 08:33
@herr-seppia herr-seppia requested a review from autholykos March 12, 2024 10:02
@herr-seppia herr-seppia force-pushed the issue-1448-stake-migration branch from c587a32 to 109dcc5 Compare March 12, 2024 16:16
@herr-seppia herr-seppia force-pushed the issue-1448-stake-migration branch from 837679f to 6c1612a Compare March 12, 2024 20:46
@herr-seppia
Copy link
Member

Tracking down migration results on DO 4GB 2CPU Standard

2024-03-13T11:22:08.180153Z  INFO rusk::chain::vm::migration: MIGRATING STAKE CONTRACT
2024-03-13T11:22:16.374086Z  INFO rusk::chain::vm::migration: MIGRATION FINISHED: 8.193867389s
2024-03-13T11:22:16.374404Z  INFO rusk::chain::vm::migration: Performing sanity checks
2024-03-13T11:22:16.381258Z  INFO rusk::chain::vm::migration: Get new list: 6.773122ms
2024-03-13T11:22:16.386942Z  INFO rusk::chain::vm::migration: Sanity checks OK: 5.447941ms

@herr-seppia
Copy link
Member

Blocked by #1516

Changes in contract have been implemented. Still missing the rusk part, but we can consider this not blocked anymore

ureeves
ureeves reviewed Mar 13, 2024
View reviewed changes
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

Some questions

node/src/chain/acceptor.rs Show resolved Hide resolved
rusk/src/bin/args.rs Show resolved Hide resolved
ureeves
ureeves approved these changes Mar 13, 2024
View reviewed changes
Copy link
Member

@ureeves ureeves left a comment

Choose a reason for hiding this comment

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

Approved for ITN

@herr-seppia herr-seppia requested a review from ureeves March 13, 2024 14:07
@herr-seppia herr-seppia merged commit 4381289 into ITN2 Mar 13, 2024
6 checks passed
@herr-seppia herr-seppia deleted the issue-1448-stake-migration branch March 13, 2024 14:07
herr-seppia added a commit to dusk-network/node-installer that referenced this pull request Mar 13, 2024
@herr-seppia
Upgrade rusk binary
48c3824 
See also:
- dusk-network/rusk#1517
- dusk-network/rusk#1500
- dusk-network/rusk#1529
- dusk-network/rusk#1464
@herr-seppia herr-seppia mentioned this pull request Mar 13, 2024
Merged
@herr-seppia herr-seppia restored the issue-1448-stake-migration branch March 15, 2024 12:46
@HDauven HDauven mentioned this pull request Apr 8, 2024
Closed
@herr-seppia herr-seppia deleted the issue-1448-stake-migration branch August 9, 2024 10:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@herr-seppia herr-seppia herr-seppia approved these changes

@goshawk-3 goshawk-3 Awaiting requested review from goshawk-3

@autholykos autholykos Awaiting requested review from autholykos

@HDauven HDauven Awaiting requested review from HDauven

@ureeves ureeves Awaiting requested review from ureeves

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@miloszm @herr-seppia @autholykos @ureeves