feat: shed: FIP0036 post poll result processing #9387

arajasek · 2022-09-28T01:12:14Z

Related Issues and Proposed Changes

This is a shed command that tabulates the results of FIP-0036. It is fed in votes from FilPoll, along with the state root and height (for this poll bafy2bzacebdnzh43hw66bmvguk65wiwr5ssaejlq44fpdei2ysfh3eefpdlqs and 2162760). See #9375 for more, this is a slight reworking of that.

Additional Info

lotus-shed fip36poll results --repo=/mnt/oldnet bafy2bzacebdnzh43hw66bmvguk65wiwr5ssaejlq44fpdei2ysfh3eefpdlqs 2162760 ~/Downloads/votes.json

Sample output:

Total acceptance token:  3641447073483215769868470
Total rejection token:  4546494321191196771637925
Total acceptance SP deal power:  20666424631525888
Total rejection SP deal power:  141508413655663360
Total acceptance SP rb power:  3486069098389962752
Total rejection SP rb power:  2454514779633483776
Total acceptance Client rb power:  15715869958078464
Total rejection Client rb power:  29557281734075904


Final results **drumroll**
token holders REJECT FIP-0036 :(
SPs by deal data stored VETO FIP-0036!!!
SPs by total raw byte power ACCEPT FIP-0036 :)
Storage Clients REJECT FIP-0036 :(

Checklist

Before you mark the PR ready for review, please make sure that:

All commits have a clear commit message.
The PR title is in the form of of <PR type>: <area>: <change being made>
- example: fix: mempool: Introduce a cache for valid signatures
- PR type: fix, feat, INTERFACE BREAKING CHANGE, CONSENSUS BREAKING, build, chore, ci, docs,perf, refactor, revert, style, test
- area: api, chain, state, vm, data transfer, market, mempool, message, block production, multisig, networking, paychan, proving, sealing, wallet, deps
This PR has tests for new functionality or change in behaviour
If new user-facing features are introduced, clear usage guidelines and / or documentation updates should be included in https://lotus.filecoin.io or Discussion Tutorials.
CI is green

jennijuju · 2022-09-28T01:34:38Z

Take out the emoji please 😜😜 let’s not make sentimental suggestion a😜😜

cmd/lotus-shed/fip-0036.go

jennijuju · 2022-09-28T03:39:11Z

cmd/lotus-shed/fip-0036.go

+			}
+
+			//process msigs
+			// TODO: Oh god, oh god, there's a possibility that a multisig has voted in both directions


lets add the ID from https://api.filpoll.io/api/polls/16/view-votes to Vote - and which can be used to indicate which option reaches threshold first

jennijuju · 2022-09-28T04:23:58Z

cmd/lotus-shed/fip-0036.go

+
+			if v == APPROVE {
+				approveBalance = types.BigAdd(approveBalance, accountActor.Balance)
+				votesIncludingMsigs[signerId] = APPROVE


why adding here?

oka after figuring out the intention of this map I think this should be removed

@jennijuju You're gonna have to say more than that :P

Why should it be removed?

(The purpose is just to have a map that includes votes from msigs for the purpose of checking owner addresses in the next loop)

maybe answer to my q is here: #9387 (comment)?

jennijuju · 2022-09-28T04:24:09Z

cmd/lotus-shed/fip-0036.go

+				clientApproveBytes = big.Add(clientApproveBytes, clientBytes)
+			} else {
+				rejectionBalance = types.BigAdd(rejectionBalance, accountActor.Balance)
+				votesIncludingMsigs[signerId] = Reject


why adding here?

jennijuju · 2022-09-28T05:10:07Z

cmd/lotus-shed/fip-0036.go

+		minerVotes := make(map[address.Address]uint64)
+		fmt.Println("counting miner votes")
+		for s, v := range votes {
+			if minerInfos, found := ownerMap[s]; found {


for s is msig - this is not checking against whether the music mets the threshold, add a voteMsig found check?

oops! that's supposed to be iterating over votesIncludingMsigs -- are you proposing just doing this loop twice, once for votes and once for votedMsigs? I do like that more.

im confused - then where is the miner balance when the owner is not msig accounted?

oh here

lotus/cmd/lotus-shed/fip-0036.go

Line 353 in 9928c1a

if minerInfos, found := ownerMap[signerId]; found {

..

cmd/lotus-shed/fip-0036.go

Stebalien

Initial pass. Mostly nits but it would be easier to review if we reduced code duplication and added some comments on the vote tallying strategy.

cmd/lotus-shed/fip-0036.go

Stebalien · 2022-09-28T07:30:50Z

cmd/lotus-shed/fip-0036.go

+				return idAddr
+			}
+
+			panic("didn't find addr")


Can someone not vote with a bogus ID? (I assume we should just skip here)

I don't believe FilPoll allows that (with the possible exception of certain Core Devs who submitted newly created IDs for the core devs portion of the vote).

You already have an instance:

2022/09/28 22:01:40 ignoring ballot {49 t1giru42mia2x27svolr3n7vth7byb77eshpc77iq 2022-09-28 18:12:09.179 +0000 UTC}: unknown robust address

This one was ID-ed way after 2162760

Yeah i wound up having to handle it lol -- only for vote signer resolution, though, the rest are panics.

cmd/lotus-shed/fip-0036.go

Stebalien · 2022-09-28T07:31:40Z

cmd/lotus-shed/fip-0036.go

+		workerMap := make(map[address.Address][]address.Address)
+		// a map of miners to some info aboout them for quick lookup
+		minerActorsInfo := make(map[address.Address]minerBriefInfo)
+		// a map of client addresses to deal data stored in proposals


For which one? I don't think so? Workers are BLS addresses, clients are either secp or bls?

Ah, sorry, I forgot that these needed to be key addresses.

Technically, we also need to handle the case where a multisig owns a multisig, which requires either multiple passes or a dependency graph. But I'm not sure how important that is.

cmd/lotus-shed/fip-0036.go

Stebalien · 2022-09-28T07:40:54Z

cmd/lotus-shed/fip-0036.go

+			//process msigs
+			// TODO: Oh god, oh god, there's a possibility that a multisig has voted in both directions
+			// and we'll pick a winner non-deterministically as we iterate...
+			// We need to factor in vote time if that happens and pick whoever went first


(IMO, just pick the direction with more votes and drop the vote on tie)

I do think it's correcter to pick the first vote that crossed the threshold -- that's how msigs operate on chain.

(gonna do that for now, and log how many instances of this we have... @ribasushi already has this info too).

@arajasek on my side I am not coming up with any instances of this, aside from the mega-msig

I don't agree that it's correcter to pick the first vote that crosses the threshold. That incentivizes a race as soon as the poll opens. We should want people to take as much time as they need before the poll closes to gather and weigh information.

I agree with @Stebalien's idea as the most straightforward solution.

Stebalien · 2022-09-28T07:43:26Z

cmd/lotus-shed/fip-0036.go

+						// msig has already voted, skip
+						continue
+					}
+					if mpv, found := msigPendingVotes[ms]; found { //other signers of the multisig have voted, yet the threshold has not met


So... we can simplify this code by always incrementing, then checking. I.e., don't special-case a threshold of one, etc.

You're right, i'll maybe get to that ;P

NOT YOUR PRIO

It'll make it a lot easier to reason about and review.

cmd/lotus-shed/fip-0036.go

ribasushi · 2022-09-28T17:33:38Z

cmd/lotus-shed/fip-0036.go

+
+		fmt.Println("iterating over proposals")
+		dps, _ := marketState.Proposals()
+		if err := dps.ForEach(func(dealID abi.DealID, d market.DealProposal) error {


@arajasek considering every deal as "legit" is not accurate. This is the filtering I do on my end:
... d.sector_activation_epoch IS NOT NULL AND d.deal_slash_epoch IS NULL and d.end_epoch > 2162760

It doesn't affect the result much at all, but still...

Yeah, there's some nitpicking to do here. I'm not convinced that your approach is correcter (we should clarify all of this for future polls).

A Deal in the Proposals already has is "accepted" by both parties and has collateral locked, so I'm not sure we should check d.sector_activation_epoch IS NOT NULL?

A slashed deal is still a deal that a client made (and SP accepted) and both put up collateral for, so should it not be counted?

d.end_epoch > 2162760 excludes deals that have expired but cron hasn't GCed yet...that's probably a better definition of "active deals" but it's a narrow boundary?

I definitely don't think any of your suggestions are "wrong", just...not clearly correcter?

(would be easy to adopt any subset of your suggestions, though)

@arajasek answering in the same order

The reason I always do this when parsing the state, is because we have deal Proposals that are slated to start far in the future, some in the next millennium

Expand for urgh

deal_id | status | deal_start_time | start_epoch ----------+-----------+------------------------+------------- 311683 | published | 3534-01-31 14:40:00+00 | 1591979600 219097 | published | 3390-03-05 22:00:00+00 | 1440604800 218449 | published | 3389-10-06 22:00:00+00 | 1440172800 166347 | published | 3279-01-31 22:00:00+00 | 1323748800 165999 | published | 3272-04-27 22:00:00+00 | 1316635200 165922 | published | 3270-07-02 22:00:00+00 | 1314720000 165919 | published | 3270-06-27 22:00:00+00 | 1314705600 165917 | published | 3270-06-27 22:00:00+00 | 1314705600 165921 | published | 3270-06-27 22:00:00+00 | 1314705600 165918 | published | 3270-06-27 22:00:00+00 | 1314705600 165912 | published | 3270-06-22 22:00:00+00 | 1314691200 165916 | published | 3270-06-22 22:00:00+00 | 1314691200 165911 | published | 3270-06-22 22:00:00+00 | 1314691200 165915 | published | 3270-06-22 22:00:00+00 | 1314691200 165914 | published | 3270-06-22 22:00:00+00 | 1314691200 165908 | published | 3270-06-17 22:00:00+00 | 1314676800 165909 | published | 3270-06-17 22:00:00+00 | 1314676800 165905 | published | 3270-06-17 22:00:00+00 | 1314676800 165910 | published | 3270-06-17 22:00:00+00 | 1314676800 165907 | published | 3270-06-17 22:00:00+00 | 1314676800 165904 | published | 3270-06-12 22:00:00+00 | 1314662400 165903 | published | 3270-06-12 22:00:00+00 | 1314662400 165902 | published | 3270-06-12 22:00:00+00 | 1314662400 159205 | published | 3240-01-31 22:00:00+00 | 1282723200 159203 | published | 3240-01-26 22:00:00+00 | 1282708800 159200 | published | 3240-01-26 22:00:00+00 | 1282708800 159204 | published | 3240-01-26 22:00:00+00 | 1282708800 159202 | published | 3240-01-26 22:00:00+00 | 1282708800 159201 | published | 3240-01-26 22:00:00+00 | 1282708800 159197 | published | 3240-01-21 22:00:00+00 | 1282694400 159195 | published | 3240-01-21 22:00:00+00 | 1282694400 159196 | published | 3240-01-21 22:00:00+00 | 1282694400 159193 | published | 3240-01-21 22:00:00+00 | 1282694400 159194 | published | 3240-01-21 22:00:00+00 | 1282694400 159190 | published | 3240-01-16 22:00:00+00 | 1282680000 159191 | published | 3240-01-16 22:00:00+00 | 1282680000 159192 | published | 3240-01-16 22:00:00+00 | 1282680000 159188 | published | 3240-01-16 22:00:00+00 | 1282680000 159189 | published | 3240-01-16 22:00:00+00 | 1282680000 159183 | published | 3239-12-22 22:00:00+00 | 1282608000 159181 | published | 3239-12-22 22:00:00+00 | 1282608000 159170 | published | 3239-06-10 22:00:00+00 | 1282046400 159155 | published | 3239-01-21 22:00:00+00 | 1281643200 192728 | published | 2959-10-11 20:08:30+00 | 987868577 2768016 | published | 2033-01-30 16:31:30+00 | 13080303 2768015 | published | 2033-01-30 16:31:00+00 | 13080302 2768014 | published | 2033-01-30 16:30:30+00 | 13080301 11512152 | published | 2022-10-16 09:14:00+00 | 2253508 11458042 | published | 2022-10-16 09:13:30+00 | 2253507 11464093 | published | 2022-10-16 09:13:30+00 | 2253507 11512158 | published | 2022-10-16 09:13:00+00 | 2253506 ...

"Slashed deal" is a misnomer: it means the underlying sector is gone for good and the deal has entered cron cleanup. I did confirm this with @ZenGround0 at multiple points.

No strong feelings about this one, though note that cron cleanup is sometimes up to 24h

The reason I always do this when parsing the state, is because we have deal Proposals that are slated to start far in the future, some in the next millennium

Persuaded, adding that check.

"Slashed deal" is a misnomer: it means the underlying sector is gone for good and the deal has entered cron cleanup. I did confirm this with @ZenGround0 at multiple points.

Yeah, you're right, but it's another 24 hour issue...I don't care enough for that ;)

jennijuju and others added 6 commits September 27, 2022 16:52

add token holder group

e88c206

add client

0addc72

get deal proposal

5f5491a

wip

3d5ecf2

refactors refactors refactors

9dfd604

fix: singer -> signer

9535009

arajasek requested a review from a team as a code owner September 28, 2022 01:12

arajasek changed the title ~~Asr/fip0036~~ feat: shed: FIP0036 post poll result processing Sep 28, 2022

arajasek force-pushed the asr/fip0036 branch 2 times, most recently from e303661 to 7f1302a Compare September 28, 2022 01:24

unify fip0036 shed commands for speed

87597cb

arajasek force-pushed the asr/fip0036 branch from 7f1302a to 87597cb Compare September 28, 2022 01:25

jennijuju reviewed Sep 28, 2022

View reviewed changes

cmd/lotus-shed/fip-0036.go Outdated Show resolved Hide resolved

jennijuju reviewed Sep 28, 2022

View reviewed changes

cmd/lotus-shed/fip-0036.go Outdated Show resolved Hide resolved

jennijuju reviewed Sep 28, 2022

View reviewed changes

cmd/lotus-shed/fip-0036.go Show resolved Hide resolved

jennijuju reviewed Sep 28, 2022

View reviewed changes

jennijuju requested changes Sep 28, 2022

View reviewed changes

Stebalien reviewed Sep 28, 2022

View reviewed changes

cmd/lotus-shed/fip-0036.go Outdated Show resolved Hide resolved

Stebalien reviewed Sep 28, 2022

View reviewed changes

ribasushi reviewed Sep 28, 2022

View reviewed changes

cmd/lotus-shed/fip-0036.go Outdated Show resolved Hide resolved

cmd/lotus-shed/fip-0036.go Show resolved Hide resolved

cmd/lotus-shed/fip-0036.go Outdated Show resolved Hide resolved

magik6k reviewed Sep 28, 2022

View reviewed changes

cmd/lotus-shed/fip-0036.go Outdated Show resolved Hide resolved

ribasushi reviewed Sep 28, 2022

View reviewed changes

arajasek added 2 commits September 28, 2022 16:39

address review

9928c1a

address review

2b08ed9

arajasek force-pushed the asr/fip0036 branch from d6e47f8 to 2b08ed9 Compare September 29, 2022 17:27

arajasek merged commit 4d28bde into master Oct 13, 2022

arajasek deleted the asr/fip0036 branch October 13, 2022 19:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: shed: FIP0036 post poll result processing #9387

feat: shed: FIP0036 post poll result processing #9387

arajasek commented Sep 28, 2022 •

edited

Loading

jennijuju commented Sep 28, 2022 •

edited

Loading

jennijuju Sep 28, 2022

jennijuju Sep 28, 2022

jennijuju Sep 28, 2022

arajasek Sep 28, 2022

arajasek Sep 28, 2022

jennijuju Sep 28, 2022

jennijuju Sep 28, 2022

arajasek Sep 28, 2022

jennijuju Sep 29, 2022

jennijuju Sep 29, 2022

Stebalien left a comment

Stebalien Sep 28, 2022

arajasek Sep 28, 2022

ribasushi Sep 28, 2022

arajasek Sep 29, 2022

Stebalien Sep 28, 2022

arajasek Sep 28, 2022

Stebalien Sep 29, 2022

Stebalien Sep 28, 2022

arajasek Sep 28, 2022

ribasushi Sep 28, 2022

geoff-vball Sep 28, 2022

Stebalien Sep 28, 2022

arajasek Sep 28, 2022

jennijuju Sep 28, 2022

Stebalien Sep 29, 2022

ribasushi Sep 28, 2022

arajasek Sep 28, 2022

arajasek Sep 28, 2022

ribasushi Sep 28, 2022

arajasek Sep 29, 2022

feat: shed: FIP0036 post poll result processing #9387

feat: shed: FIP0036 post poll result processing #9387

Conversation

arajasek commented Sep 28, 2022 • edited Loading

Related Issues and Proposed Changes

Additional Info

Checklist

jennijuju commented Sep 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Stebalien left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arajasek commented Sep 28, 2022 •

edited

Loading

jennijuju commented Sep 28, 2022 •

edited

Loading