-
Notifications
You must be signed in to change notification settings - Fork 20
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
Move autopilots
into autopilot_state
#1657
base: dev
Are you sure you want to change the base?
Conversation
… into pj/autopilot-state
stores/sql/mysql/migrations/main/migration_00026_autopilot_state_1.sql
Outdated
Show resolved
Hide resolved
RenewWindow: 144 * 7 * 2, | ||
Download: 1e12, // 1 TB | ||
Upload: 1e12, // 1 TB | ||
Storage: 4e12, // 4 TB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar conversation: since we now have initial contract funding instead of a global allowance and refreshing a contract in v2 will be less painful, are these even necessary? Attempting to calculate this ahead of time adds unnecessary setup complexity and overhead. It may end up in more contracts overall, but storage utilization could be more efficient if the scoring didn't need to check for storage space available based on expected storage * redundancy / target contracts
.
If a host runs out of storage space, but is otherwise in working condition, form a contract with a new host instead of churning it out of the set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could get rid of the remaining storage score and simplify the collateral score and instead just check whether a host is already full / out of collateral before forming a contract without giving them a score.
Thanks @n8maninger for the review. I combined your suggestion into a separate issue and plan to F/U with that immediately after this one. Getting rid of the current period never crossed my mind, but since we got rid of allowance that's now a very logical next step indeed. I won't resolve the open comments to allow further discussion. |
5fa9d80
to
876aca9
Compare
876aca9
to
75de934
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good overall. Just need to make sure
- migrations were manually tested
- we make sure the changes here are consistent with refactor(renterd): remove contract sets web#810 and that we somewhat merge these together
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lgtm. @alexfreska any chance we can break out the changes of your big UI PR relevant to this PR and then tag a version? That way we could update the UI in this PR to make sure we don't break the UI when merging this.
This migrates the
autopilots
to a singleautopilot_state
table, effectively removing the concept of multiple autopilots. I think autopilot state can be defined as config + the current period, I opted to keep theAutopilotConfig
which can be updated in thebus
. Thecurrent_period
can be updated separately, which solves a race in the autopilot but that won't be an issue anyway since we're merging autopilot and bus. Tested on SQLite, not (yet) on MySQL, want to get a round of reviews in first.