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

fip-0100: initial pass at daily_fee accounting #1622

Merged
merged 15 commits into from
Feb 19, 2025

Conversation

rvagg
Copy link
Member

@rvagg rvagg commented Feb 14, 2025

Draft. Doesn't even pass all the existing tests yet, but I wanted to try and bang this out to at least get started to see what it might look like. I need to put a bit more thought into precisely where we're handling it and what we're doing.

@Stebalien Stebalien force-pushed the rvagg/fip-0100/fee-calc branch from cccfdfb to d014397 Compare February 14, 2025 19:46
Base automatically changed from rvagg/fip-0100/fee-calc to feat/fip-0100 February 14, 2025 20:49
@Stebalien Stebalien force-pushed the rvagg/fip-0100/fee-accounting branch from 1570235 to ad9059e Compare February 14, 2025 20:50
actors/miner/src/expiration_queue.rs Outdated Show resolved Hide resolved
actors/miner/src/expiration_queue.rs Show resolved Hide resolved
actors/miner/src/lib.rs Outdated Show resolved Hide resolved
1. We need to add to the total power when adding sectors, and deduct
when deleting partitions.
2. We're going to need to figure out some way to handle fees when we
compact partitions.
@Stebalien Stebalien force-pushed the rvagg/fip-0100/fee-accounting branch from aeb15d4 to 607b9b8 Compare February 15, 2025 00:13
@Stebalien

This comment was marked as resolved.

I'm using a negative delta in the recover case because that's what we do
elsewhere in that function, but I'm not super happy about it.
@Stebalien
Copy link
Member

I'm fixing multiple issues while trying to track down this bug. To say the least, I'm not feeling great about this FIP.

@Stebalien Stebalien force-pushed the rvagg/fip-0100/fee-accounting branch from 44b268f to bb764b3 Compare February 15, 2025 01:00
@rvagg
Copy link
Member Author

rvagg commented Feb 17, 2025

Test failure problem turned out to be a bad argument order specified in ExpirationQueue#remove(), it was removing pledge as if it was fee. But also the fee is zero all the way through that test so it's not exercising it and probably should be.

@rvagg rvagg force-pushed the rvagg/fip-0100/fee-accounting branch from 17cf92d to d2fc3de Compare February 17, 2025 04:41
@rvagg rvagg force-pushed the rvagg/fip-0100/fee-accounting branch from 47364f4 to 5a5b285 Compare February 17, 2025 06:44
@rvagg rvagg marked this pull request as ready for review February 17, 2025 10:54
@rvagg
Copy link
Member Author

rvagg commented Feb 17, 2025

Let me know if you can see opportunity for more invariant checks in here. I've got deadline total checks and expiration queue checks. Didn't see a point in adding anything at the miner level since it'd need to figure out live sectors from the deadlines anyway and then be comparing to the fees in the deadlines.

The most awkward bit of this I think is partition compaction. There's a big note in compact_partitions() explaining the awkwardness, but you can also see it evident in deadline_state_test.rs.

@rvagg
Copy link
Member Author

rvagg commented Feb 17, 2025

Just realising your TODO in Deadline::remove_partitions documented the problem as well but you proposed two other possible solutions. Mine is a bit more of a hack but I'd go for the Deadline::add_sectors flag option too. Adding the fee to the partitions seems like adding too much state and even more complexity in keeping everything in sync so I'd prefer to avoid that.

@Stebalien Stebalien force-pushed the rvagg/fip-0100/fee-accounting branch from a1507de to 4431ba3 Compare February 19, 2025 01:37
@rvagg rvagg force-pushed the rvagg/fip-0100/fee-accounting branch from 6080f1d to 4431ba3 Compare February 19, 2025 02:00
actors/miner/src/testing.rs Outdated Show resolved Hide resolved
actors/miner/src/testing.rs Outdated Show resolved Hide resolved
actors/miner/src/testing.rs Outdated Show resolved Hide resolved
actors/miner/src/lib.rs Outdated Show resolved Hide resolved
actors/miner/src/deadline_state.rs Outdated Show resolved Hide resolved
@rvagg rvagg merged commit 6e4141f into feat/fip-0100 Feb 19, 2025
11 checks passed
@rvagg rvagg deleted the rvagg/fip-0100/fee-accounting branch February 19, 2025 02:26
rvagg added a commit that referenced this pull request Feb 19, 2025
* fip-0100: initial pass at daily_fee accounting

* fip-0100: handle sector termination and expiration

* fip-0100: update total power and record a TODO

1. We need to add to the total power when adding sectors, and deduct
when deleting partitions.
2. We're going to need to figure out some way to handle fees when we
compact partitions.

* update the daily fee when faulting/recovering

I'm using a negative delta in the recover case because that's what we do
elsewhere in that function, but I'm not super happy about it.

* fip-0100: remove fees when removing faulty sectors from the expiration queue

* fip-0100: make clippy happy, fix bad arg order in ExpirationQueue#remove

* fip-0100: account for fee adjustment for fault reschedules

* fip-0100: add invariant for fees

* fip-0100: more invariants, handle partition removal/compaction cases

* fip-0100: document return values

* fip-0100: account for power changes when extending/snapping sectors

* fip-0100: add state invariant for live deadline power

* fip-0100: handle unproven power on termination

* fip-0100: handle remaining todos

* fip-0100: rename total_power to live_power

To match our other power variable names.

---------

Co-authored-by: Steven Allen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🎉 Done
Development

Successfully merging this pull request may close these issues.

2 participants