-
Notifications
You must be signed in to change notification settings - Fork 22
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
Worker executor response signature support #154
Conversation
Overall I like this. I think it works well with having a signer configurable to a node. I think we will need to shift this into support multiple signing options, but at face value I see now problem with this approach. |
@dmikey Additionally added support for providing metadata from workers. |
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.
Code-wise I think this is great! However I feel this is a little confused when it comes to some concepts, would love to discuss some things in more detail..
-
I don't really think the executor should know anything about signing. Executor should be dumb and only handle OS-level stuff and not be cognizant of anything else. It takes care of setting up the working directory and cleans up afterwards, sets the process resource limits and executes the process (OS-level
exec()
type stuff). I don't think the concept of signing fits well here and I think it's mixing up concerns. I think this would be more suited in thenode
package and thenode
signs the execution result the executor returns. Basically, I would just move this code to a different place. -
I'm not sure the aggregation part is working correctly. Commented in more detail on the relevant part of the code.
-
If the workers can choose their own signing scheme, we need to pass along the information on what scheme was used, no? It cannot be unlabeled else we don't know how to verify the signature. Also ties into discussion point 4.
-
Does it make sense for each worker to have the freedom to choose their signing schemes? IMO, they can choose which ones they support, but perhaps it's the client that can request a specific one in the execution request. If a client didn't set one, a head node can request a default scheme we choose, or not use any by default. The workers can then self-select - "if I don't support this signing scheme I won't reply to a roll call". This is super close to what we have with b7s attributes too. Though, at the moment, the attributes are only a strict match, and we don't support a syntax like
B7S_SignatureAlgorithms=A,B,C,D
to match "C". This is relevant if the node supports multiple signing schemes.
If we do move the signing code to the node
package, we need to handle three main paths
- simple execution (no consensus cluster) - https://github.com/blocklessnetwork/b7s/blob/main/node/worker_execute.go#L86
- raft cluster - https://github.com/blocklessnetwork/b7s/blob/main/node/consensus.go#L36 . Perhaps we modify the callback signature so it can modify the original request so it can add more data to it
- pbft cluster - https://github.com/blocklessnetwork/b7s/blob/main/consensus/pbft/execute.go#L73 (Note that PBFT mandates signatures - so workers cannot opt out of signing these)
I may be wrong on any one of these points or I might have misunderstood something, so feel free to correct me!
} | ||
|
||
type MetaProvider interface { | ||
WithMetadata(execute.Request, execute.RuntimeOutput) (interface{}, error) |
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.
WithMetadata
- This function name is IMO more common for specifying functional options - e.g. server.New(server.WithPort(8888))
. I think the name of a function is a bit misleading and is in fact more CreateMetadata
or GetMetadata
(this one less so because "get" implies extracting something from the input params).
I think just Metadata(req, runtimeOutput) (any, error)
is just fine.
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.
Nitpick - I prefer using any
instead of interface{}
. They mean the same, but I think any
reads a little nicer.
@@ -66,10 +67,28 @@ func (e *Executor) executeFunction(requestID string, req execute.Request) (execu | |||
|
|||
out, usage, err := e.executeCommand(cmd) | |||
if err != nil { | |||
return out, execute.Usage{}, fmt.Errorf("command execution failed: %w", err) | |||
return out, execute.Usage{}, []byte{}, nil, fmt.Errorf("command execution failed: %w", err) |
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.
And other places too - why not return nil
instead of []byte{}
?
stats[output] = resultStats{ | ||
seen: 0, | ||
peers: make([]peer.ID, 0), | ||
seen: 0, | ||
peers: make([]peer.ID, 0), | ||
signature: res.Signature, | ||
metadata: res.Metadata, | ||
} | ||
} | ||
|
||
stat.seen++ | ||
stat.peers = append(stat.peers, executingPeer) | ||
stat.signature = res.Signature | ||
stat.metadata = res.Metadata |
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 don't think this will work as intended..
Is this signature like a hash that can be recalculated by anyone or is it a signature that can be tied to the signer (can be verified by a public key)? I assume it's the latter.
Let's say we have 5 execution responses from 5 worker nodes. We will have e.g. the same execution result (stdout of the blockless runtime) but 5 different signatures, and I believe 5 different metadata objects. (I'm not sure if metadata is unique or not, but since it's configurable, it can be anything.)
When aggregating, we iterate through the results, and for the seen results, we increment the number of times it was seen, add the worker ID to the list of peers that have this exact result, and set the signature and metadata to that of the result we just processed. Meaning if we iterate through execution results of worker1 through worker5, we will save the signature of worker5 (if we process that one last, which also isn't guaranteed in a map). In each iteration we override the previous signature/metadata.
I think we will need to have a map that will map worker IDs to signatures and metadata.
For example something like:
{
"result": "whatever",
"peers": [
"worker1",
"worker2",
"worker3",
"worker4",
"worker5"
],
"frequency": 100,
"signatures": {
"worker1": {
"scheme": "abc", // without specifying the scheme we don't know how to verify the signature, no?
"sig": "0x1234567890abcdef1234567890abcdef"
},
// other workers
},
"metadata": {
"worker1": {
// metadata object
},
// other workers
}
}
@@ -98,6 +99,7 @@ func (r *Replica) execute(view uint, sequence uint, digest string) error { | |||
RequestTimestamp: request.Timestamp, | |||
Replica: r.id, | |||
}, | |||
Signature: hex.EncodeToString(res.Signature), |
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.
PBFT already has and mandates signatures - note the msg.Sign()
invocation below that will overwrite this value.
Closing this PR in favour of #156 |
Context
This PR adds support for b7s workers to optionally support signing responses (using any arbitrarily provided signature scheme/function as an executor option).
Additionally, support is added for providing additional relevant metadata by workers - as a worker executor option.