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

FEATURE: Efficiency Refactoring and Slot Selection for Staking Contracts #321

Merged
merged 23 commits into from
Feb 2, 2023

Conversation

joshuahannan
Copy link
Member

@joshuahannan joshuahannan commented Nov 15, 2022

Feature branch for big changes to the staking contracts to improve efficiency for epoch phase transition transactions and algorithm for random slot selection for new nodes.

Current Changes:

Changes approved node ID arguments to dictionaries instead of arrays

Cleans up some unnecessary code and should help with performance of certain transactions

Stores stakedNodeIDs as a dictionary in account storage and uses it to iterate over instead of all IDs

Should improve the performance of the calculateRewards function because the function will only iterate through staked node IDs instead of all node IDs
Also significantly helps the performance of getter functions like getProposedNodes and getStakedNodes because they just return the list of node IDs instead of having to iterate through the list to construct the nodes that are staked

Adds Pending Token Movements Dictionary

Adds a stored dictionary that represents nodes and delegators who have submitted a transaction to move tokens between buckets in some way. They are added to this record when they submit their transaction, not when the system chunk checks.

At the end of the epoch, when the moveTokens function is called, the function will only iterate through nodes and delegators who have submitted requests to move tokens, instead of checking all of the nodes and delegators.
This dictionary is cleared when the moveTokens function is called. Nodes who's unstaked tokens are moved to unstaking are automatically added to the dictionary for the next epoch.

This should significantly improve performance for the end epoch transactions.

This did not require any new tests because the existing tests were sufficient.

Adds a Candidate Node List

A candidate node is defined as a node who is not participating/staked in the current epoch, but has committed enough tokens to participate in the next epoch.

The candidate node list is a dictionary that maps roles to an array of node IDs:

{UInt8: [String]}

This is added to make the random slot selection process easier. This process will be much more efficient with the candidate node list because it can select nodes for each role separately instead of having to do them all in one loop.

Some tests were added to test this list as well as some testing utility functions

Adds Events for Unstaking Requests

Events were added to mark when a node or delegator requests to unstake, indicating how much they requested
Additional TokensUnstaked event emissions were added on unstaking requests when tokens in the commited bucket are moved to the tokensUnstaked bucket. it isn't a full unstaking per se, but it matches the rest of the patterns that TokensUnstaked is emitted when tokens are moved into the tokens unstaked bucket.

Adds new storage fields to the contract to track staking slots for node roles:

Role Slot Limits

{UInt8: UInt16}
Indicates how many of each node role can participate in each epoch

Participant Node Role Counts

Indicates how many of each node role are currently staked and participating in the epoch

Separates removeAndRefundNodeRecord into its own function

This process is now done multiple times for checking for approval, staking minimum, and random slot selection, so it is now its own method

Adds fillNodeRoleSlots method to Admin resource

This method randomly select candidate nodes for each slot until there are no candidate nodes left or all the slots for a given role have been filled. Leftover nodes are refunded.
As part of this addition, the need for Access nodes (role=5) to be approved by the admin has been removed. Other node roles will receive this update in the future

Adds setNodeWeight method to Admin resource

Allows the admin to set a node's weight to any number less than 100

NOTES FOR REVIEWERS: Ignore the FlowIDTableStaking_new.cdc file. I'm currently using this for testing because the flow-go dependencies don't like it when I make changes to the main contract file and some of the transactions.

Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Approach is looking good - nice improvements.

  • Added a few naming/documentation suggestions
  • Added a suggestion to disallow approving an un-registered node ID (fixes an outstanding potential security issue, and would simplify some code added in this PR)

contracts/FlowIDTableStaking.cdc Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Outdated Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Outdated Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Outdated Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Outdated Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Outdated Show resolved Hide resolved
@joshuahannan
Copy link
Member Author

joshuahannan commented Jan 20, 2023

I just merged the other two branches into this feature branch, but the CI is failing because the integration with flow-go is still broken. I'll talk with Jordan today about getting that resolved.

I've also written a transaction that we can use for the upgrade. I'll start putting it in a PR in the service account repo and a forum post so we can have a formal plan and announcement for it, but I'm leaving it here so people can start reviewing it.
Basically, there are some storage paths that we introduce as part of the upgrades that need to be initialized, and some existing paths that are having their types and values changed. The transaction performs all of those initializations and then does the upgrade.

I think the only values we'll need to change for the different networks are the staking slot limits.

import FlowIDTableStaking from 0xIDENTITYTABLEADDRESS

/// Transaction to upgrade the FlowIDTableStaking contract
/// to include performance improvements
/// and support permissionless node operation for access nodes
/// This transaction needs to be sent during the epoch setup
/// or epoch commit phase, because it initializes important
/// storage paths that are used to store fields
/// in the contract to track candidate nodes, staking slots,
/// limits, and more

transaction(code: [UInt8]) {

    prepare(stakingAccount: AuthAccount) {

        // Initialize Candidate Node List
        let emptyCandidateNodes: {UInt8: {String: Bool}} = {1: {}, 2: {}, 3: {}, 4: {}, 5: {}}
        stakingAccount.save(emptyCandidateNodes, to: /storage/idTableCandidateNodes)

        // Save Staking Slot Limits to storage
        // These are the current node counts, which access nodes increased by 5
        // Will have different values on canary, testnet, and mainnet
        let slotLimits: {UInt8: UInt16} = {1: 108, 2: 117, 3: 7, 4: 85, 5: 117}
        stakingAccount.save(slotLimits, to: /storage/flowStakingSlotLimits)

        // Initialize the Moves pending list
        let movesPendingList: {String: {UInt32: Bool}} = {}
        stakingAccount.save<{String: {UInt32: Bool}}>(movesPendingList, to: /storage/idTableMovesPendingList)

        // Initialize the candidate Node Limits List
        // Currently, only access nodes are allowed as candidate nodes
        // because the other node roles are approval only
        let candidateNodeLimits: {UInt8: UInt64} = {1: 0, 2: 0, 3: 0, 4: 0, 5: 1000}
        stakingAccount.save<{UInt8: UInt64}>(candidateNodeLimits, to: /storage/idTableCandidateNodeLimits)

        // Borrow the admin to set minimum stake requirement for access nodes
        self.adminRef = acct.borrow<&FlowIDTableStaking.Admin>(from: FlowIDTableStaking.StakingAdminStoragePath)
            ?? panic("Could not borrow reference to staking admin")
        
        // Set Access node minimum stake requirement to 100 FLOW
        let minimums: {UInt8: UFix64} = FlowIDTableStaking.getMinimumStakeRequirements()
        minimums[5] = 100
        self.adminRef.setMinimumStakeRequirements(minimums)

        // Initialize the Staking Slot Counts
        // `getCurrentRoleNodeCounts` calculates the counts based on the current list
        // so we just store the result in storage after calculating
        Let slotCounts = FlowIDTableStaking.getCurrentRoleNodeCounts()
        stakingAccount.save(slotCounts, to: /storage/flowStakingRoleNodeCounts)

        // Approve List needs to be changed from [String] to {String: Bool}
        let approveList: [String] = stakingAccount.load<[String]>(from: /storage/idTableApproveList)
            ?? panic("Could not load the approve list from storage")

        // Create the approve list dictionary with the loaded value, then store the dictionary in the same path
        var approveDict: {String: Bool} = {}
        for nodeID in approveList {
            approveDict[nodeID] = true
        }

        // Participant Node list needs to be changed from [String] to {String: Bool}
        let participantList: [String] = stakingAccount.load<[String]>(from: /storage/idTableCurrentList)
            ?? panic("Could not load the participant list from storage")

        // Create the participant dictionary with the loaded value, then store the dictionary in the same path
        var participantDict: {String: Bool} = {}
        for nodeID in participantList {
            participantDict[nodeID] = true
        }

        acct.contracts.update__experimental(name: "FlowIDTableStaking", code: code)
    }
}

@joshuahannan joshuahannan marked this pull request as ready for review January 24, 2023 16:13
Copy link
Member

@jordanschalm jordanschalm left a comment

Choose a reason for hiding this comment

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

Reviewed new commit 81cfbf1

contracts/FlowIDTableStaking.cdc Show resolved Hide resolved
lib/go/test/test.go Outdated Show resolved Hide resolved
lib/go/test/test.go Outdated Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Show resolved Hide resolved
@joshuahannan joshuahannan mentioned this pull request Feb 2, 2023
@joshuahannan joshuahannan merged commit 276863c into master Feb 2, 2023
@joshuahannan joshuahannan deleted the node-lists branch February 2, 2023 16:58
bors bot added a commit to onflow/flow-go that referenced this pull request Feb 9, 2023
3837: Update `core-contracts` dependency (slot selection) r=jordanschalm a=jordanschalm

This PR updates the version of `flow-core-contracts` to include onflow/flow-core-contracts#321. 
- Updates Staking Table contract deployment and scripts to match new version
- Fixes issue where transaction errors in state bootstrapping were swallowed
  - This revealed some FVM tests which were configured in a way which failed state bootstrapping (see [comment](https://github.com/onflow/flow-go/pull/3837/files#r1083076101))

### Outstanding Issues
- [x] Update FVM tests which fail state bootstrapping https://github.com/onflow/flow-go/pull/3837/files#r1083076101
- [x] Fix `core-contracts` bug onflow/flow-core-contracts#339

Co-authored-by: Jordan Schalm <[email protected]>
Co-authored-by: Janez Podhostnik <[email protected]>
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.

6 participants