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

Epoch phase performance improvements, access node open slot limits, and EpochStart event #379

Merged
merged 39 commits into from
Nov 21, 2023

Conversation

joshuahannan
Copy link
Member

@joshuahannan joshuahannan commented Aug 9, 2023

Makes three changes to improve the performance of the epoch transition:

  1. Moves FlowEpoch.calculateAndSetRewards to the end of the epoch setup phase/beginning of epoch commit, which currently does not have much complexity. This removes calculating rewards from the end of the epoch, which when combined with moveTokens, was causing the execution to take a long time.

  2. Changes FlowIDTableStaking.setNewMovesPending() to be able to add a new node or delegator to the moves pending list without loading and storing to and from storage. If many nodes and delegators were unstaking tokens, this was very costly at the end of each epoch. The change uses a reference to allow the moves pending list to be modified in memory and not loaded or stored during node registration transactions.

  3. Updates the methods that are run at the end of the service auction to construct the proposed Node ID list as they go instead of the epoch contract having to query it fully at the end, which requires iterating through all nodes in the identity table to construct it.

Adds some new features to the epoch and staking contracts

  1. Adds a FlowEpoch.EpochStart event that is emitted at the start of every epoch and includes important information related to FlowToken and epoch information.

  2. Adds a feature to open up new access node slots each epoch instead of the service account committee having to do it manually

Also updates some comments and error messages for some transactions

No tests are needed for the first two improvements because they don't affect the behavior of the contracts, just the performance

@joshuahannan joshuahannan changed the title Adds performance improvements to end of epoch operations Adds performance improvements to epoch phase transition operations Aug 9, 2023
Copy link
Collaborator

@janezpodhostnik janezpodhostnik left a comment

Choose a reason for hiding this comment

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

Can you explain in more detail why we can do this? I'm not sure I understand.

If the rewards are calculated earlier is there not a chance they would change?

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

I'm moving the rewards calculation to earlier so that it doesn't have to happen in the same transaction where the epoch transitions and tokens are moved. We currently don't have any logic to change rewards once they have been calculated anyway, so it doesn't really matter if they are calculated earlier.

@joshuahannan joshuahannan changed the title Adds performance improvements to epoch phase transition operations Adds performance improvements to epoch phase transition operations and more access node slot limits Aug 24, 2023
@joshuahannan joshuahannan changed the title Adds performance improvements to epoch phase transition operations and more access node slot limits Epoch phase performance improvements, access node open slot limits, and EpochStart event Aug 28, 2023
@joshuahannan
Copy link
Member Author

@jordanschalm No rush, but I could use another review on this when you get the chance. We could also schedule a call so I can walk you through the changes if you need help

contracts/epochs/FlowEpoch.cdc Outdated Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Outdated Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Outdated Show resolved Hide resolved
contracts/epochs/FlowEpoch.cdc Show resolved Hide resolved
contracts/epochs/FlowEpoch.cdc Show resolved Hide resolved
lib/go/test/flow_idtable_staking_test.go Outdated Show resolved Hide resolved
lib/go/test/staking_test_helpers.go Outdated Show resolved Hide resolved
transactions/idTableStaking/admin/set_open_node_slots.cdc Outdated Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Show resolved Hide resolved
contracts/FlowIDTableStaking.cdc Outdated Show resolved Hide resolved
@joshuahannan
Copy link
Member Author

@jordanschalm Can you take a look at my last two commits? I've added in all the performance improvements we discussed this week

@bjartek
Copy link
Contributor

bjartek commented Nov 15, 2023

I think it is very important that every set of service chunk events that transitions something contains the counter field. Atm NewEpoch and Payout event from FlowIDTableStaking does not contain this. This makes it very hard to identify what epoch this payout was for. In some cases these events happen before NewEpoch and in others it hapends after. If we have to "fix stuff"

@joshuahannan
Copy link
Member Author

@bjartek That would be nice to have, but that might require the staking contract to import the FlowEpoch contract, which would be a circular dependency, and we want to avoid that. I guess we could pass the epoch counter as an argument to the payRewards() and moveTokens() functions so we can use it in the events, but that still feels a little weird. It isn't enough that you can infer the epoch from when the event was emitted? We could potentially include it if it is that important to you

@bjartek
Copy link
Contributor

bjartek commented Nov 15, 2023

Is it possible to have another event emitted when rewards are paid from FlowEpoch that has the counter?

@jordanschalm
Copy link
Member

I guess we could pass the epoch counter as an argument to the payRewards() and moveTokens() functions so we can use it in the events

Assuming it's relatively straightforward, I think we should just do this and include the epoch counter in all events where it could be useful. Inferring auxiliary data about events based on the current state when they are emitted, or based on other events, is error-prone and more complex.

@joshuahannan
Copy link
Member Author

Okay. I'll add those to the events in the staking contract then

jordanschalm and others added 3 commits November 20, 2023 13:26
* add duration to EpochSetup

This is needed to compute tau, the steady-state optimal time/view,
in the cruise control system

* update generated files

* update tests

* tidy

* tidy
@joshuahannan
Copy link
Member Author

I did the upgrade on canary and everything seems to be working properly. We'll upgrade testnet on monday the 27th and stick with the dec 6th mainnet upgrade. I'm going to go ahead and merge this though unless anyone has any objections

@joshuahannan joshuahannan merged commit e05552a into master Nov 21, 2023
2 checks passed
@joshuahannan joshuahannan deleted the access-slots branch February 7, 2024 16:38
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.

5 participants