Implement PowerTable as ordered map #32

ranchalp · 2024-01-10T11:19:25Z

EDIT: builds upon #34 , so merge that one first.

Implements PowerTable as an ordered map. To be used by explicit justification to create/verify bitsets (see here) and, even if using implicit justification, by the creation/verification of PoFs as part of the catch-up mechanism.

anorth

We really need two things here:

A data structure that can be serialized as the canonical power table, giving a CID
A runtime structure for looking up power values and keys

This is good for (2), but we won't want to serialise the lookup table. For now lets ignore that problem. We can chose later to add a new type for the serializable form, or manually implement serialisation here, including only the wanted data.

anorth · 2024-01-15T23:42:10Z

f3/granite.go

@@ -44,6 +45,24 @@ type GMessage struct {
 	Ticket Ticket
 	// Signature by the sender's public key over Instance || Round || Step || Value.
 	Signature []byte
+
+	Evidence AggEvidence


This is unused in this PR too - can we keep it out until it will be used?

Done by rebasing from #34

anorth · 2024-01-15T23:49:25Z

f3/powertable.go

+// It includes an ActorID and its Weight
+type PowerEntry struct {
+	ID    ActorID
+	Power uint


In the main network, power is always represented as a big integer. It's tempting to argue that an int64 should be enough for one miner, but that only(!) 16 EiB or 170x the current largest miner. 170x can disappear pretty quickly given an expected annual doubling. So I think we need a bigint here.

In types.go add type StoragePower big.Int and use that (this mimics Lotus)

f3/powertable.go

anorth · 2024-01-15T23:53:10Z

f3/powertable.go

 type PowerTable struct {
-	Entries map[ActorID]uint
+	Entries []PowerEntry    // Slice to maintain the order


Please document that this is to be maintained in order by (ID descending, Power ascending).

It's actually in order by (Power descending, ID ascending), but done otherwise.

anorth · 2024-01-15T23:58:54Z

f3/powertable.go

 	}
 }

+// NewPowerTableFromMap creates a new PowerTable from a map of ActorID to weight.
+// It is more efficient than Add, as it only needs to sort the entries once.
+func NewPowerTableFromMap(entriesMap map[ActorID]uint) PowerTable {


Are there any callers of this? I don't think this is how Lotus will call it to construct the table - it doesn't have a map like this.

I think the canonical constructor will be NewPowerTable(entries []Entry) *PowerTable, which should sort the entries and construct the lookup table and total.

I changed it to []PowerEntry.

anorth · 2024-01-16T00:01:49Z

f3/powertable.go

+	if _, ok := p.Lookup[id]; ok {
 		panic("duplicate power entry")
 	}


You can remove this, because you can check for duplication after doing the sort.Search.

anorth · 2024-01-16T00:04:22Z

f3/powertable.go

 type PowerTable struct {
-	Entries map[ActorID]uint
+	Entries []PowerEntry    // Slice to maintain the order
+	Lookup  map[ActorID]int // Map for quick index lookups


Please be more clear about what this is indexing. "Maps actor ID to the index of the associated entry in Entries"

f3/powertable.go

anorth · 2024-01-16T00:08:19Z

f3/powertable.go

 	p.Total += power
 }
+
+func (p *PowerTable) GetPower(id ActorID) uint {


Suggest making this Get(id ActorID) (Power, []byte) and return both the power and private key, when you add that. And maybe even add the ok bool return here too (otherwise add a Has() method and panic here if !ok)

ranchalp · 2024-01-16T10:06:42Z

Thanks! addressed the comments :)

anorth · 2024-01-16T19:37:03Z

f3/granite.go

-	weakThreshold := q.powerTable.Total * 1 / 3
-	if candidate.power > weakThreshold {
+	one := NewStoragePower(1)
+	weakThreshold := new(StoragePower).Mul(q.powerTable.Total, one)


No need to multiply by one

Oops, AI help was too obvious here 😅 . Done.

anorth · 2024-01-16T19:39:48Z

f3/granite.go

+	two := NewStoragePower(2)
+	three := NewStoragePower(3)
+
+	threshold := new(StoragePower).Mul(q.powerTable.Total, two)
+	threshold.Div(threshold, three)
+
+	return q.sendersTotalPower.Cmp(threshold) > 0


Can you factor out a hasStrongQuorum(part, total *StoragePower) bool pure function to use here and above? And maybe do a hasWeakQuorum too for symmetry.

Remember to replace the code here with a call to your new method

Good catch! Done (upcoming commit)

anorth · 2024-01-16T19:45:32Z

f3/granite.go

@@ -608,21 +520,21 @@ type quorumState struct {
 	// The power supporting each chain so far.
 	chainPower map[TipSetID]chainPower
 	// Total power of all distinct senders from which some chain has been received so far.
-	sendersTotalPower uint
+	sendersTotalPower StoragePower


It's tricky to know how to reference Go big.Ints properly. Is there a good reason this one is not a pointer but the others are? Using a pointer seems to be canonical so I'd suggest doing that here too, unless there's a good reason not to.

You are absolutely right. I started with no pointers but missed this one when switching to pointers.

anorth · 2024-01-16T19:49:13Z

f3/powertable.go

-		Total:   0,
+// NewPowerTable creates a new PowerTable from a slice of PowerEntry .
+// It is more efficient than Add, as it only needs to sort the entries once.
+func NewPowerTable(entries []PowerEntry) *PowerTable {


Please note the function takes ownership of the slice - it must not be modified afterwards.

Correct, but not yet an issue. I reckon you are noticing this but suggesting we leave it as is?

Sorry, by note I meant "write a comment"

f3/powertable.go

anorth · 2024-01-16T19:52:34Z

f3/powertable.go

+	})
+
+	// Check for duplication at the found index or adjacent entries
+	if index < len(p.Entries) && (p.Entries[index].ID == id || (index > 0 && p.Entries[index-1].ID == id)) {


You only need to check the exact entry pointed to.

Search uses binary search to find and return the smallest index i in [0, n) at which f(i) is true

f3/powertable.go

anorth · 2024-01-16T19:55:56Z

f3/powertable.go

+	p.Total.Add(p.Total, power)
+}
+
+func (p *PowerTable) Get(id ActorID) (*StoragePower, []byte, bool) {


Having seen this in use (especially ignoring the bool return), I have a further suggestion.

The existing code used the zero value map lookup to silently accept messages from participants with no power. Now is the time to change that. I recommend:

Add a Has(id) method here, and call it in isValid to reject messages from participants absent or with zero power

Remove the bool return here and just panic if not found. All the call sites should be able to rely on the prior filtering of valid messages.

anorth · 2024-01-16T19:57:56Z

f3/types.go

 	"strings"
 )

 type ActorID uint64

+type StoragePower big.Int


Make this a type alias and then you can remove all the wrapper methods.

type StoragePower = big.Int

This is slightly less "safe" in that it doesn't distinguish between different things a big.Int might be representing, but I think it's fine here (and Lotus does too).

Ah ok, I thought about this too but I did not do it precisely to distinguish between different things. But agreed that helps remove boilerplate. And specially for consistency with lotus it makes sense. Done.

anorth · 2024-01-16T19:59:05Z

sim/sim.go

 	participants := make([]*f3.Participant, simConfig.HonestCount)
 	for i := 0; i < len(participants); i++ {
 		participants[i] = f3.NewParticipant(f3.ActorID(i), graniteConfig, ntwk, vrf)
 		ntwk.AddParticipant(participants[i])
-		genesisPower.Add(participants[i].ID(), 1)
+		genesisPower.Add(participants[i].ID(), f3.NewStoragePower(1), make([]byte, 0))


Let's put a non-empty key in here so that the fake signature verification at least checks it's the right signature. Serialize the participant ID into a byte array, and maybe add some magic bytes to distinguish it from other serialized integers.

Co-authored-by: Alex North <[email protected]>

ranchalp · 2024-01-17T18:12:34Z

Just addressed the latest comments! Thanks!!

Kubuxu · 2024-01-17T18:36:39Z

f3/powertable.go

 type PowerTable struct {
-	Entries map[ActorID]uint
-	Total   uint
+	Entries []PowerEntry    // Slice to maintain the order. Meant to be maintained in order in order by (Power descending, ID ascending)


Q: Why do we build the power table sorted power?
Usage-wise, we will almost always query it by ActorID; events SPs appearing and disappearing are also rarer than an order of the power changing (especially in the long tail).

In general, it seems to me that having the power table have a different representation for granite format and different in the wire format (finality certificates) makes sense. Especially since we will need to optimize the wire format for usage in smart contracts.

I saw it was also part of granite as well. It needs to communicate which participants signed each step.
One option could be communicating that in the form of RLE+ bitfield of ActorIDs, keeping the power table format specifics out of the granite itself.
https://github.com/filecoin-project/go-bitfield/

I see what you're saying. Note that the TipSet structure here will have a PowerTable CID added to it, which must match the serialized format used in certificates. However that CID will come from outside, attached to the tipsets pushed in.

The signers will certainly be an RLE+ bitfield (it's present in subsequent PRs). A bitfield over powertable indices will be more dense than over ActorIDs, and I think the density does matter here, as bytes in the messages (of which there are quadratic in the participant count). The sorting by power is also a heuristic to increase density.

These are good questions, lets defer to an issue if you still think it's worth consideration. We can move on here with this, and come back to revise this structure if warranted.

We have not gotten there yet (See TODOs in #37's description), but having them sorted by power makes it easy for the creator of a message to optimize message size when creating a justification (i.e. the creator needs to just iterate until the strong quorum is reached when creating the justification). Not visible at the moment even in #37 because all received values that justify the same message are added to the justification (so in many cases this will be more added signatures than required for a strong quorum).

Also the cost of having a sorted power table is negligible compared to running Granite.

but having them sorted by power makes it easy for the creator of a message to optimize message size when creating a justification

sure, but this happens on the granite side, which can easily just form a temporary datastructure for the sort, I'm thinking about the 3rd party users who will have to replicate the semantics of whatever power table format we decide on using

Not delaying this PR due to this, as @anorth let's defer this discussion to when we get to finality certificate

anorth

LGTM after nits

anorth · 2024-01-17T19:22:10Z

f3/granite.go

+	two := NewStoragePower(2)
+	three := NewStoragePower(3)
+
+	threshold := new(StoragePower).Mul(q.powerTable.Total, two)
+	threshold.Div(threshold, three)
+
+	return q.sendersTotalPower.Cmp(threshold) > 0


Remember to replace the code here with a call to your new method

anorth · 2024-01-17T19:29:06Z

f3/powertable.go

 type PowerTable struct {
-	Entries map[ActorID]uint
-	Total   uint
+	Entries []PowerEntry    // Slice to maintain the order. Meant to be maintained in order in order by (Power descending, ID ascending)


I see what you're saying. Note that the TipSet structure here will have a PowerTable CID added to it, which must match the serialized format used in certificates. However that CID will come from outside, attached to the tipsets pushed in.

The signers will certainly be an RLE+ bitfield (it's present in subsequent PRs). A bitfield over powertable indices will be more dense than over ActorIDs, and I think the density does matter here, as bytes in the messages (of which there are quadratic in the participant count). The sorting by power is also a heuristic to increase density.

These are good questions, lets defer to an issue if you still think it's worth consideration. We can move on here with this, and come back to revise this structure if warranted.

anorth · 2024-01-17T19:29:32Z

f3/powertable.go

-		Total:   0,
+// NewPowerTable creates a new PowerTable from a slice of PowerEntry .
+// It is more efficient than Add, as it only needs to sort the entries once.
+func NewPowerTable(entries []PowerEntry) *PowerTable {


Sorry, by note I meant "write a comment"

anorth · 2024-01-17T19:31:45Z

go.mod

+	github.com/multiformats/go-base32 v0.0.3 // indirect
+	github.com/multiformats/go-multibase v0.0.1 // indirect
+	github.com/multiformats/go-multihash v0.0.13 // indirect
+	github.com/multiformats/go-varint v0.0.5 // indirect


It looks like there's some noise crept in here, please normalise before merging.

ranchalp · 2024-01-17T19:43:23Z

Addressed comments from today :)

ranchalp requested a review from anorth January 10, 2024 11:20

ranchalp mentioned this pull request Jan 10, 2024

Explicit message justification (Opt: generalization of quorumState) #31

Closed

ranchalp force-pushed the ordered-table branch 4 times, most recently from e7b007c to 242b38e Compare January 12, 2024 16:17

ranchalp mentioned this pull request Jan 12, 2024

Add Aggregator interface and implement fake aggregator #35

Merged

anorth reviewed Jan 16, 2024

View reviewed changes

ranchalp and others added 6 commits January 17, 2024 14:25

Add AggEvidence type

ed956c1

Require AggEvidence when broadcasting GMessage

18695b3

Make lint happy

6a4f91b

Implement PowerTable as ordered map

2d0ac4d

Update f3/powertable.go

cb6f043

Co-authored-by: Alex North <[email protected]>

Address comments

a2f4c1b

ranchalp force-pushed the ordered-table branch from 2201dad to a2f4c1b Compare January 17, 2024 17:26

Address comments II

0ed53b8

ranchalp force-pushed the ordered-table branch from 732f456 to 0ed53b8 Compare January 17, 2024 18:12

Kubuxu reviewed Jan 17, 2024

View reviewed changes

anorth approved these changes Jan 17, 2024

View reviewed changes

Address comments III

de51f34

Merge branch 'main' into ordered-table

9c1e3a5

ranchalp merged commit 2efdddf into filecoin-project:main Jan 18, 2024
2 checks passed

Implement PowerTable as ordered map #32

Implement PowerTable as ordered map #32

Conversation

ranchalp commented Jan 10, 2024 • edited Loading

anorth 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

ranchalp commented Jan 16, 2024

Choose a reason for hiding this comment

ranchalp Jan 17, 2024 • 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

ranchalp Jan 17, 2024 • 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

ranchalp Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranchalp commented Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Kubuxu Jan 17, 2024 • 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

anorth 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

ranchalp commented Jan 17, 2024

ranchalp commented Jan 10, 2024 •

edited

Loading

ranchalp Jan 17, 2024 •

edited

Loading

ranchalp Jan 17, 2024 •

edited

Loading

ranchalp Jan 17, 2024 •

edited

Loading

ranchalp commented Jan 17, 2024 •

edited

Loading

Kubuxu Jan 17, 2024 •

edited

Loading