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

ranchalp · 2024-01-03T19:46:29Z

Resolves #16.

Incomplete, and so far mostly created just to allow for discussion of design decisions.

ranchalp · 2024-01-06T09:10:04Z

More refactoring and optimizations to do (e.g. sendersIndex not being recalculated for every new quorumStateExplicit), but explicit and implicit justifications both work by now.

anorth

This interface-based "generalization of quorumState" way of doing both at once here has introduced a lot of code, some copy paste, and plumbing of values around more than necessary. The message justificaition check should be performed at the same place the implicit justification is checked. Everything after that should just be able to assume justified messages. I don't think quorumState needs generalisation - it should know nothing about explicit validation and can instead be simplified if we go that route. State and justification will be completely separate, which will be great.

I'm still not that confident that supporting both at once in the codebase is reasonable. This method is not one that's just an easy removal of a single call and the block of code that it calls – it's had a large impact on the existing code structure.

This has identified the need for an improved PowerTable type, though, so let's break that out into a PR ahead of this one.

anorth · 2024-01-09T00:37:01Z

f3/api.go

@@ -51,11 +51,19 @@ type Signer interface {
 	Verify(sender ActorID, msg, sig []byte) bool
 }

+type Aggregator interface {
+	// Aggregates signatures from a participant to an existing signature.
+	Aggregate(sig []byte, senderID ActorID, aggSignature []byte, signers *BitSet, actor2Index map[ActorID]uint64) ([]byte, *BitSet)


Putting the bitset and lookup table as parameters here implies that the host has the responsibility of interpreting these to work out what order to put the signatures together. I don't think that's right – the F3 code should do that. The host is the one that knows how to actually aggregate the signatures, but I think this API should be simple to implement, rather than simple to call. For example, we shouldn't re-implement this association in the fake/testing host (only the actual signing should be fake).

anorth · 2024-01-09T00:40:19Z

f3/chain.go

+func ZeroECChain() ECChain {
+	return ECChain{}
+}
+


I think it's better to just use ECChain{} inline where needed. Here the canonical Go zero value is what we want. Having a method to construct the zero value implies that the canonical one is somehow not valid. (Note that nil is also valid, maybe everywhere, and perhaps we should switch to using that).

anorth · 2024-01-09T00:41:36Z

sim/network.go

+func (n *Network) Aggregate(sig []byte, actorID f3.ActorID, aggSignature []byte, signers *f3.BitSet, actor2Index map[f3.ActorID]uint64) ([]byte, *f3.BitSet) {
+	// Fake implementation.
+	// Just appends signature to aggregate signature.
+	// This fake aggregation is not commutative (order matters), unlike the real one.


This is a bit of an issue if it has any chance of constraining the calling code. Can you make it commute if the real one does? E.g. xor.

The problem with XOR is that the verifier would have to somehow get the individual signatures in order to verify the aggregate, changing the interface of the aggregator w.r.t. the non-fake implementation. I have instead ensured determinism of the aggregate and this determinism is checked in the verifyAggregate, which is essentially equivalent for the purposes of the code outside the aggregator (the resulting signature is the same for everyone from aggregating and the verify only is correct if the signature is the same).

Left a comment in the Aggregate function.

anorth · 2024-01-09T00:42:44Z

sim/network.go

+		panic("signers size is less than actor2Index")
+	}
+
+	signers.Set(actor2Index[actorID])


Following on my comment about this API, having it mutate the parameter is very unexpected. The caller should do this (and drop the parameter).

Done (in dedicated PR #35 )

anorth · 2024-01-09T22:22:00Z

f3/granite.go

+	// GossiPBFT instance number
+	Instance uint32
+	// GossiPBFT round
+	Round uint32


Put these in the same order as they are in GMessage. Remove the copy-pasta comments.

anorth · 2024-01-09T22:23:09Z

f3/granite.go

+
+func ZeroAggEvidence() AggEvidence {
+	return AggEvidence{}
+}


I don't think we need this method if the canonical Go zero value is what we want.

anorth · 2024-01-09T22:23:29Z

f3/granite.go

+}
+
+func (a AggEvidence) isZero() bool {
+	return a.Step == "" && a.Value.IsZero() && a.Instance == 0 && a.Round == 0 && a.Signers.Size() == 0 && len(a.Signature) == 0


Just check for a == AggEvidence{}. It's debatable whether you need this method, since the caller can just do that.

AggEvidence contains fields that make it not comparable with == such as Signers (bitfield.Bitfield | BitSet), Signature ([] byte) and Value (ECChain, which is []Tipset).

anorth · 2024-01-09T22:30:52Z

f3/granite.go

@@ -728,6 +1039,99 @@ func (c *convergeState) findMinTicketProposal() ECChain {
 	return minValue
 }

+// BitSet is a bitset implementation.
+type BitSet struct {


Please import and use https://github.com/filecoin-project/go-bitfield/blob/master/bitfield.go instead

anorth · 2024-01-09T22:32:36Z

f3/granite.go

+
+// SenderIndex maps ActorID to a unique index in the range [0, len(powerTable.Entries)).
+// This is used to index into a BitSet. index2Actor is the reverse mapping.
+type SenderIndex struct {


Rather than this we probably want to build a stronger PowerTable type that is an ordered map, so we can just look up entries by index. The host should build it for us. I think defining and implementing that should be a PR of its own that you can do before this one.

Just did this one: see here.

anorth · 2024-01-09T22:33:38Z

f3/granite.go

+	}
+
+	// Sort the keys
+	sort.Slice(keys, func(i, j int) bool { return keys[i] < keys[j] })


This probably isn't the ordering we will want - probably want descending by power instead.

ranchalp added 2 commits January 3, 2024 16:37

Implement AggregatedEvidence struct type

2bf0920

Implement fake aggregation and aggregated verification

56ed7d1

ranchalp force-pushed the explicit-justification branch from ab5b69d to 56ed7d1 Compare January 4, 2024 09:21

Generalize quorumState and justification

6ed3e3d

ranchalp force-pushed the explicit-justification branch from dd73283 to 6ed3e3d Compare January 4, 2024 18:02

ranchalp changed the title ~~Implement AggregatedEvidence struct type~~ Explicit message justification (Opt: generalization of quorumState) Jan 5, 2024

ranchalp force-pushed the explicit-justification branch 2 times, most recently from 641fbaf to 529793e Compare January 6, 2024 09:09

ranchalp force-pushed the explicit-justification branch 2 times, most recently from db678ed to 17421fa Compare January 9, 2024 17:09

anorth reviewed Jan 9, 2024

View reviewed changes

ranchalp force-pushed the explicit-justification branch from 17421fa to fabf199 Compare January 10, 2024 10:22

ranchalp mentioned this pull request Jan 10, 2024

Implement PowerTable as ordered map #32

Merged

ranchalp force-pushed the explicit-justification branch 2 times, most recently from d1f0d2d to e8c84db Compare January 11, 2024 15:52

Refactor & Implement explicit justification within modular framework

d5f3ee3

ranchalp force-pushed the explicit-justification branch from e8c84db to d5f3ee3 Compare January 11, 2024 15:55

ranchalp closed this Jan 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

ranchalp commented Jan 3, 2024

ranchalp commented Jan 6, 2024 •

edited

Loading

anorth left a comment •

edited

Loading

anorth Jan 9, 2024

anorth Jan 9, 2024

anorth Jan 9, 2024

ranchalp Jan 11, 2024

anorth Jan 9, 2024

ranchalp Jan 15, 2024

anorth Jan 9, 2024

anorth Jan 9, 2024

anorth Jan 9, 2024

ranchalp Jan 11, 2024 •

edited

Loading

anorth Jan 9, 2024

ranchalp Jan 15, 2024

anorth Jan 9, 2024

ranchalp Jan 10, 2024

anorth Jan 9, 2024

ranchalp Jan 15, 2024

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

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

Conversation

ranchalp commented Jan 3, 2024

ranchalp commented Jan 6, 2024 • edited Loading

anorth left a comment • 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

ranchalp Jan 11, 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

ranchalp commented Jan 6, 2024 •

edited

Loading

anorth left a comment •

edited

Loading

ranchalp Jan 11, 2024 •

edited

Loading