Message templating, allowing multiple identities in one instance #188

Kubuxu · 2024-05-03T15:15:55Z

Removes ParitcipantID from gpbft; the signing is outside of gpbft, leading to internal messages being delivered "from the outside".

masih · 2024-05-08T10:15:33Z

@Kubuxu can you resolve conflicts before I review please?

Paritcipant.ID is only for testing/debug, need to rip it out Tests are incoming Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Kubuxu · 2024-05-08T15:13:39Z

@masih rebased

masih · 2024-05-08T17:24:50Z

sim/network.go

 	if n.participants[p.ID()] != nil {
 		panic("duplicate participant ID")
 	}
 	n.participantIDs = append(n.participantIDs, p.ID())
 	n.participants[p.ID()] = p
 }

+////// Network interface


Subjective: could we avoid these comments going forward, please? They seem redundant considering modern IDE capabilities.

Suggested change

////// Network interface

masih · 2024-05-08T17:27:56Z

sim/network.go

+	}
+}
+
+type NetworkFor struct {


Any reason for exporting this struct?

Is there any reason not to? It is a wrapper onto Network which is also exported.

My vote and the common convention is to not export unless needed. This is not a blocker. Please don't let me slow you down. We can refactor it later.

sim/sim.go

masih · 2024-05-08T17:33:17Z

sim/host.go

@@ -23,13 +23,20 @@ type simHost struct {
 	id  gpbft.ActorID
 }

+type SimNetwork interface {


Can we unify SimNetwork, simHost and NetworkFor into one struct(simHost)?

I am curious why we want all three as separate as they are.

gpbft/messagetemplate.go

masih · 2024-05-08T17:43:07Z

gpbft/messagetemplate.go

+	gmsg := &GMessage{
+		Sender:        st.ParticipantID,
+		Vote:          st.Payload,
+		Signature:     payloadSignature,
+		Ticket:        vrf,
+		Justification: st.Justification,
+	}
+	return gmsg


Suggested change

gmsg := &GMessage{

Sender: st.ParticipantID,

Vote: st.Payload,

Signature: payloadSignature,

Ticket: vrf,

Justification: st.Justification,

}

return gmsg

return &GMessage{

Sender: st.ParticipantID,

Vote: st.Payload,

Signature: payloadSignature,

Ticket: vrf,

Justification: st.Justification,

}

masih · 2024-05-08T17:44:14Z

gpbft/messagetemplate_test.go

+			Power:  big.NewInt(1),
+		},
+	}...)
+	assert.NoError(t, err)


This is a fatal error; use require.

Yeah, I had require and assert swapped around.

masih · 2024-05-08T17:46:17Z

gpbft/gpbft.go

+	//if err != nil {
+	//i.log("error while signing message: %v", err)
+	//return
+	//}


Clean up commented out code?

gpbft/api.go

masih · 2024-05-08T17:47:56Z

gpbft/messagetemplate.go

+	return signingTemplate, nil
+}
+
+// Sign creates signatures for the SigningTemplate, it could live across RPC boundry


Suggested change

// Sign creates signatures for the SigningTemplate, it could live across RPC boundry

// Sign creates signatures for the SigningTemplate, it could live across RPC boundary.

anorth

The big items:

[BLOCKING] Logs must have participant IDs
[BLOCKING] Now-redundant local message inbox should be deleted
There's lots of plumbing of values from outside through gpbft and then back to outside as part of the message template. The host already has these things, can we avoid plumbing them and just look up by instance ID?

The change to async delivery to self is a notable enough change that I would suggest doing it in an independent PR, before removing the participant ID. There's a bunch of other pre-factor work in the sim/network which could probably be pulled out ahead of the actual message templates, too.

anorth · 2024-05-06T21:36:30Z

sim/network.go

 func (n *Network) NetworkName() gpbft.NetworkName {
 	return n.networkName
 }

 func (n *Network) Broadcast(msg *gpbft.GMessage) {
 	n.log(TraceSent, "P%d ↗ %v", msg.Sender, msg)
 	for _, dest := range n.participantIDs {
+		// FIXME: something in gpbft assumes that we will immediately recieve message from ourselves


Yes, it needs that, so needs to be enabled somehow

anorth · 2024-05-09T20:50:08Z

gpbft/api.go

-	// Sends a message to all other participants.
-	// The message's sender must be one that the network interface can sign on behalf of.
-	Broadcast(msg *GMessage)
+	// Requests that the message is signed and broadcasted, it should also be delivered locally


I see you have removed the synchronous self-delivery in broadcast, because the participant can't attach its own ID to the message being sent.

This change from sync to async is probably fine, I don't think anything relies on the synchronicity. However, there's now a bunch of code about a local message queue that is redundant and should be removed.

anorth · 2024-05-09T20:52:37Z

gpbft/gpbft.go

-		Ticket:        ticket,
+	messageTemplate := MessageTemplate{
+		powerTable:    &i.powerTable,
+		NetworkName:   i.participant.host.NetworkName(),


This name is fetched from the host just to be passed straight back out. Remove NetworkName from the host API and just attach it when building messages from templates.

anorth · 2024-05-09T21:05:34Z

sim/network.go

+			})
+	}
+}
+
 func (n *Network) NetworkName() gpbft.NetworkName {
 	return n.networkName
 }

 func (n *Network) Broadcast(msg *gpbft.GMessage) {


Please move this now next to Broadcast as part of the adversary interface only.

anorth · 2024-05-09T21:06:38Z

sim/network.go

-					deliverAt: n.clock.Add(latencySample),
-				})
+		if dest == msg.Sender {
+			continue


As far as I can tell this is functionally equivalent. If that's right, I think the previous approach without unstructured control flow is marginally better. Non-blocking.

anorth · 2024-05-09T21:10:10Z

gpbft/participant_test.go

-
-	// Expect message is signed with expected payload.
-	pt.host.On("Sign", pt.pubKey, pt.matchMessageSigningPayload()).Return(wantSignature, nil)
-
 	// Expect a broadcast occurs with quality phase message, and the expected chain, signature.


Suggested change

// Expect a broadcast occurs with quality phase message, and the expected chain, signature.

// Expect a broadcast occurs with quality phase message, and the expected chain.

anorth · 2024-05-09T21:15:33Z

gpbft/messagetemplate.go

+)
+
+type MessageTemplate struct {
+	powerTable powerTableAccess


This feels a bit clumsy to me, though I see how it was reached from the current code. The power table came from the host in the first place, and we're just passing it back here. This field could instead be just an instance ID (if the host doesn't already know this), and then the host could pass the right power table to Build.

This is a symptom of another awkwardness, that the signing key is looked up by public key rather than by participant ID. Previously I thought this was not worth changing, but it may be contributing here.

The point is that the MessageTemplate is exposed to whoever integrates it (Lotus), which can create the signing payload that can cross RPC boundaries to be signed and given back.
I will document it more, does that change your CR?

I don't have a good enough view into how this will be used in the integration, so I won't hold you up on it. I do feel like we can do better, but don't know enough to say exactly how.

anorth · 2024-05-09T21:16:28Z

gpbft/messagetemplate.go

+type MessageTemplate struct {
+	powerTable powerTableAccess
+
+	NetworkName NetworkName


Ditto the host should know this, no need to plumb it through GPBFT instance.

anorth · 2024-05-09T21:17:23Z

gpbft/messagetemplate.go

+	NetworkName NetworkName
+	Payload     Payload
+
+	BeaconForTicket []byte


This came from the host too. Maybe we can avoid plumbing this through too. Should be able to look it up by instance. (The sim host probably doesn't have this mapping yet).

anorth · 2024-05-09T21:21:34Z

gpbft/gpbft.go

@@ -842,16 +835,11 @@ func (i *instance) buildJustification(quorum QuorumResult, round uint64, phase P
 func (i *instance) log(format string, args ...interface{}) {
 	if i.tracer != nil {
 		msg := fmt.Sprintf(format, args...)
-		i.tracer.Log("P%d{%d}: %s (round %d, step %s, proposal %s, value %s)", i.participant.id, i.instanceID, msg,
+		i.tracer.Log("{%d}: %s (round %d, step %s, proposal %s, value %s)", i.instanceID, msg,


Blocking: participant IDs in the logs are critical to development and debugging of the protocol itself. Please put prefix a participant ID inside the Log call.

Please enable tracing on one of the current unit tests, check out the logs, and make sure we can replicate something similar after this change.

It was done in the sim.Network:
https://github.com/filecoin-project/go-f3/pull/188/files#diff-4fa4e1c1a7b22e16444c976a3fecfaf2d47f9c48ca7d1ebcb955034e9daab511R76-R79
I will check if it is sufficient.

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Kubuxu force-pushed the feat/message-template branch from 919979d to d05a2dd Compare May 7, 2024 18:35

Kubuxu marked this pull request as ready for review May 7, 2024 18:55

Kubuxu requested a review from masih May 7, 2024 18:55

Kubuxu mentioned this pull request May 8, 2024

Panic when receiving message signed by self #201

Closed

Kubuxu added 6 commits May 8, 2024 17:11

Message templating, allowing multiple identities in one instance

09069e1

Paritcipant.ID is only for testing/debug, need to rip it out Tests are incoming Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Fix broadcast to fix repeat adversary

ad6ca4b

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Remove ID from Participant, add tests to message teamplate

92bdb52

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Supress lint, using function from assert there is cleanest

d17a9d1

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Move Broadcast to sim

c3f29bc

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Cleanup comment on Log

2843401

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

Kubuxu force-pushed the feat/message-template branch from 824ed1a to 2843401 Compare May 8, 2024 15:11

masih reviewed May 8, 2024

View reviewed changes

anorth requested changes May 9, 2024

View reviewed changes

anorth mentioned this pull request May 9, 2024

Implement storage power evolution in simulations for honest participants #202

Merged

jennijuju linked an issue May 16, 2024 that may be closed by this pull request

Location of participants in Lotus stack, multiple identities in one instance #103

Open

Kubuxu mentioned this pull request May 20, 2024

Location of participants in Lotus stack, multiple identities in one instance #103

Open

Rename MessageTemplate -> MessageBuilder

ee89891

Signed-off-by: Jakub Sztandera <oss@kubuxu.com>

adlrocha added a commit that referenced this pull request May 21, 2024

FFF-103: Split async deliver from PR #188

c71cb75

adlrocha mentioned this pull request May 21, 2024

FFF-103: Async delivery of local messages #259

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Message templating, allowing multiple identities in one instance #188

Message templating, allowing multiple identities in one instance #188

Kubuxu commented May 3, 2024 •

edited

masih commented May 8, 2024

Kubuxu commented May 8, 2024

masih May 8, 2024

masih May 8, 2024

Kubuxu May 10, 2024

masih May 10, 2024

masih May 8, 2024

masih May 8, 2024

masih May 8, 2024

Kubuxu May 9, 2024

masih May 8, 2024

masih May 8, 2024

anorth left a comment

anorth May 6, 2024

anorth May 9, 2024

anorth May 9, 2024

anorth May 9, 2024

anorth May 9, 2024

anorth May 9, 2024

anorth May 9, 2024

Kubuxu May 10, 2024

anorth May 13, 2024

anorth May 9, 2024

anorth May 9, 2024

anorth May 9, 2024

Kubuxu May 10, 2024 •

edited

	// Sign creates signatures for the SigningTemplate, it could live across RPC boundry
	// Sign creates signatures for the SigningTemplate, it could live across RPC boundary.

	// Expect a broadcast occurs with quality phase message, and the expected chain, signature.
	// Expect a broadcast occurs with quality phase message, and the expected chain.

Message templating, allowing multiple identities in one instance #188

Are you sure you want to change the base?

Message templating, allowing multiple identities in one instance #188

Conversation

Kubuxu commented May 3, 2024 • edited

masih commented May 8, 2024

Kubuxu commented May 8, 2024

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

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

Kubuxu May 10, 2024 • edited

Choose a reason for hiding this comment

Kubuxu commented May 3, 2024 •

edited

Kubuxu May 10, 2024 •

edited