Skip to content

Fix StakingEscrow state for upgrading#1345

Closed
vzotova wants to merge 1 commit intonucypher:masterfrom
vzotova:fix-upgrade
Closed

Fix StakingEscrow state for upgrading#1345
vzotova wants to merge 1 commit intonucypher:masterfrom
vzotova:fix-upgrade

Conversation

@vzotova
Copy link
Member

@vzotova vzotova commented Sep 18, 2019 •
edited
Loading

Deployed in testnet:

struct StakerInfo {
        uint256 value;
        uint16 confirmedPeriod1;
        uint16 confirmedPeriod2;
        bool reStake;
        uint16 lockReStakeUntilPeriod;
        address worker;
        // period when worker was set
        uint16 workerStartPeriod;
        // last confirmed active period
        uint16 lastActivePeriod;
        Downtime[] pastDowntime;
        SubStakeInfo[] subStakes;
}

and in master:

struct StakerInfo {
        uint256 value;
        uint16 confirmedPeriod1;
        uint16 confirmedPeriod2;
        bool reStake;
        uint16 lockReStakeUntilPeriod;
        address worker;
        // period when worker was set
        uint16 workerStartPeriod;
        // last confirmed active period
        uint16 lastActivePeriod;
        bool measureWork;
        uint256 completedWork;
        Downtime[] pastDowntime;
        SubStakeInfo[] subStakes;
}

and require with delegateGet(_testTarget, "getPastDowntimeLength(address)", staker) fails in verifyState because pastDowntime in new contract in a wrong slot
measureWork and completedWork must be after subStakes

@vzotova vzotova added Bug 🐛 Broken functionality ETH Contracts labels Sep 18, 2019
@KPrasch
Copy link
Member

KPrasch commented Sep 18, 2019

Failing CI/CLI test is resolved in #1339

michwill
michwill reviewed Sep 19, 2019
View reviewed changes
nucypher/blockchain/eth/sol/source/contracts/StakingEscrow.sol
result := memoryAddress
// copy data to the right position after the array pointer place
// measureWork
mstore(add(memoryAddress, 0x140), mload(add(memoryAddress, 0x100)))
Copy link
Contributor

@michwill michwill Sep 19, 2019

Choose a reason for hiding this comment

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

Is this direct memory addressing the result of previous version not having those two fields? E.g. is it how we will handle similar upgrades in future?

Copy link
Member Author

@vzotova vzotova Sep 19, 2019

Choose a reason for hiding this comment

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

It's independent from previous version, issue that structure has array pointers between fields. And function stakerInfo returns without array pointers but without gap. To correct translate from returned value to StakerInfo struct need to move tail of memory. In future need to add copying for each new slot of memory

Copy link
Member

@KPrasch KPrasch Sep 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Still not understanding fully - So this is specific to the upgrade issue with different struct layouts? If we deploy on another network, with a fresh dispatcher and this as the initially deployed contract, then this code will have no effect (rather what effect will it have)?

Copy link
Member

@cygnusv cygnusv Sep 20, 2019

Choose a reason for hiding this comment

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

I'm afraid I too don't understand. Why is measureWork (L1220) relevant here?

Copy link
Member

@cygnusv cygnusv Sep 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, is it because pastDowntime and subStakes are arrays? and these arrays in the struct have a different memory structure in storage than when returned by stakerInfo()?

Copy link
Member

@KPrasch KPrasch Sep 21, 2019

Choose a reason for hiding this comment

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

This means we'd also need to redeploy the token, start with 0 stakes, redistribute, etc. considering our current standing - I think this fix is still the best course of action for this deployment set.

Copy link
Member

@KPrasch KPrasch Sep 21, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

One of the best responses we can make is some tooling to help mitigate this class of problem in the future. How can we prevent this in CI? (Perhaps another discussion)

Copy link
Member Author

@vzotova vzotova Sep 21, 2019

Choose a reason for hiding this comment

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

I'm for having this code maybe even in production, because it shows narrow places and we will meet with those type of upgrading on mainnet.
Other idea - do hardfork test (before mainnet), not just redeploy but also break contract and copy information from old to new one

Copy link
Member

@cygnusv cygnusv Sep 21, 2019

Choose a reason for hiding this comment

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

Well, we don't need to be attached for this deployment set. If we deploy this new contract then at some point we'll still have to do hard fork if we want to deploy in mainnet with the correct structure from the start.

My point is that now is the moment to do unorthodox changes, not just before mainnet.

Copy link
Contributor

@michwill michwill Sep 21, 2019

Choose a reason for hiding this comment

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

@cygnusv I totally agree with that. Nevertheless, it is very valuable to test, how we will do upgrades in similar cases.
I think, incentivized testnet is a good candidate for a "clean" deployment without this patch

@KPrasch KPrasch mentioned this pull request Sep 19, 2019
Merged
@michwill
Copy link
Contributor

michwill commented Sep 26, 2019

Since this fix is deployed anyway... Do we want to go and merge it?

@cygnusv
Copy link
Member

cygnusv commented Sep 26, 2019

I think we will merge it together 1344. However, there's something I still don't have clear: we're going to merge this but we will deploy at some moment before mainnet (e g. Incentivized testnet) a new version with the correct memory layout from the start, and hence, reverting this fix. Is my understanding correct? In that case, we'd like to open an issue

@michwill
Copy link
Contributor

michwill commented Sep 26, 2019

@cygnusv Yes, that's correct. We need an issue

@cygnusv cygnusv mentioned this pull request Sep 27, 2019
Closed
@cygnusv
Copy link
Member

cygnusv commented Sep 27, 2019

Created issue #1363, and assigned it a milestone for next testnet release.

@tuxxy
Copy link
Contributor

tuxxy commented Sep 30, 2019

Did we ever figure out tooling as a check for this in the future?

@KPrasch
Copy link
Member

KPrasch commented Sep 30, 2019

Included in #1344

@KPrasch KPrasch closed this Sep 30, 2019
@vzotova vzotova deleted the fix-upgrade branch April 28, 2020 13:06
@vzotova vzotova mentioned this pull request Jun 27, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cygnusv cygnusv cygnusv approved these changes

@KPrasch KPrasch Awaiting requested review from KPrasch

+2 more reviewers

@michwill michwill michwill approved these changes

@tuxxy tuxxy tuxxy approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Bug 🐛 Broken functionality

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@vzotova @KPrasch @michwill @cygnusv @tuxxy