-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add funds that have left FilReserve to circ supply #4160
Conversation
8761e24
to
caf7c18
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.
Looks good, but @zixuanzh should approve too
build/params_testground.go
Outdated
@@ -55,6 +55,7 @@ var ( | |||
|
|||
FilBase uint64 = 2_000_000_000 | |||
FilAllocStorageMining uint64 = 1_400_000_000 | |||
FilReserved uint64 = 1_400_000_000 |
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.
Lets use 300_000_000 for testing too?
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.
Done
// continue to use preIgnitionGenInfos, nothing changed at the Ignition epoch | ||
vf = big.Add(vf, sm.preIgnitionGenInfos.genesisMarketFunds) | ||
if height <= build.UpgradeActorsV2Height { | ||
// continue to use preIgnitionGenInfos, nothing changed at the Ignition epoch |
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.
Why do we not need to add genesisPledge after Ignition epoch?
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.
I might be wrong, but I assumed that these funds will now be accounted for in "what's missing from FilReserve", since that's where they came from originally? Is that not the case?
chain/stmgr/stmgr.go
Outdated
} | ||
|
||
// If money enters the reserve actor, this could lead to a negative term | ||
vf = big.Add(vf, big.Sub(big.NewFromGo(build.InitialFilReserved), ract.Balance)) |
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.
Let's call this ReserveDisbursed
as a separate term.
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.
Done
caf7c18
to
f55b18e
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.
one last minor comment, lgtm
} | ||
|
||
// If money enters the reserve actor, this could lead to a negative term | ||
return big.Sub(big.NewFromGo(build.InitialFilReserved), ract.Balance), nil |
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.
It is unclear which variable (there are two of the same name) InitialFilReserved
refers to in build, lets make sure we are using the right precision.
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.
Yeah, it's using the right one (the other one will be used for testground).
Good to be merged into next after #3936 lands |
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.
Still looks good
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.
Breaks consensus on master, rt.TotalFilCircSupply() returns >0 where it used to return 0
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.
(fixed with the last commit)
Yields the following numbers at height 130000 (after the v2 actors upgrade)
FilVested 13429543.811411839397160646 FIL
FilCirculating 5961737.242508608156582553