Cappl 1076 - #328
Conversation
nolag
left a comment
nolag
left a comment
There was a problem hiding this comment.
This all feels over complicated.
I think it will simplify greatly depending on the answer to my question regards identical consensus. From what I understand we are saying when there are 2f+1 valid observations and still not f+1 matches identical consensus failed, even if with later arrived observations it would have met the f+1 criteria then it should mean minimal change to consensus_execution. The above being the case, would this argument also apply to slices and maps, seems like it would? So in essence any user of identical across primitives, slices and maps needs to ensure that the source of the observations across the nodes would reasonably be expected to match and not meaningfully volatile (use median for volatile). |
6afec93 to
e204ea3
Compare
cc65558 to
9216612
Compare
f15f62f to
49a7aa6
Compare
| mddBytes, err := proto.MarshalOptions{Deterministic: true}.Marshal(&oracletypes.RequestObservation{ | ||
| Metadata: obs.Metadata, | ||
| Input: &sdk.SimpleConsensusInputs{ | ||
| Descriptors: obs.Input.Descriptors, | ||
| Default: obs.Input.Default, | ||
| }, | ||
| }) | ||
| if err != nil { | ||
| r.lggr.Errorw("could not marshal RequestObservation", "error", err) | ||
| continue | ||
| } | ||
|
|
||
| existingOutcomeSize = newSize | ||
| // Wrapped here to allow reuse of the existing CalculateOutcomeForObservations function for identical aggregation | ||
| allObservationsMDDBytes = append(allObservationsMDDBytes, values.Proto(values.NewBytes(mddBytes))) |
There was a problem hiding this comment.
Can't you wrap the RequestObservation instead of marshalling it to bytes, then wrapping the bytes?
There was a problem hiding this comment.
possible, but looking at the logic that would be used in values.Wrap -> CreateMapFromStruct(v any) and then the map comparison logic in CalculateOutcomeForObservations, it's going to be significantly more efficient to compare the serialised proto bytes.
There was a problem hiding this comment.
I commented back on the new revision with a better way to do it. Also, unsure why you think it would be more efficient to serialize, they both use reflection.
There was a problem hiding this comment.
unsure why you think it would be more efficient to serialize
The map comparison is going to identical check each attribute of the map versus just a []byte comparison, why would we want to do that? I don't see any good reason to do it that way
There was a problem hiding this comment.
I commented back on the new revision with a better way to do it
Replied there
| cachedObsSize = newSize | ||
| hasCapacity := observationBatch.AddObservation(ctx, reqObs) | ||
| if !hasCapacity { | ||
| break |
There was a problem hiding this comment.
I left a comment in outcome about this, same feeling here.
There was a problem hiding this comment.
(responded to previous)
| } | ||
|
|
||
| message RequestOutcome { | ||
| message ConsensusFailedOutcome { |
There was a problem hiding this comment.
For future reader's context:
We are in private beta and will have a maintenance window that we can deploy this in. It's better than complicating the plugin to support both.
0edde8a to
5715ceb
Compare
| func (r *reportingPlugin) calculateConsensusMetadataDescriptorAndDefault(observations []*oracletypes.RequestObservation) (*oracletypes.RequestObservation, error) { | ||
| var allObservationsMDDBytes []*valuespb.Value | ||
| for _, obs := range observations { | ||
| mddBytes, err := proto.MarshalOptions{Deterministic: true}.Marshal(&oracletypes.RequestObservation{ |
There was a problem hiding this comment.
Don't serialize this then wrap they bytes. Simply wrap it with values.Wrap then use the proto as the observation.
Alternatively, to be more efficient, refactor handleIdenticalAggregation to have a helper that takes a slice of proto.Message, then serialize them there. That way, serialization logic is in one place, and you can use RequestObservation directly and the return value will be from the candidate so you won't need to deserialize again.
There was a problem hiding this comment.
Simply wrap it with values.Wrap
This will result in it being compared as a map, which will be obviously be less performant (but perhaps not in a way that matters), but it would be possible in that scenario to get a consensus mdd that does not contain values for some attributes as the map identical consensus returns the sub-set of attributes that match, so you would get a resultant consensus mdd with some attributes not set, when it should not be returning any consensus mdd at all.
Alternatively, to be more efficient, refactor handleIdenticalAggregation
This means modifying code designed to handle user supplied values to handle value types that would only be supplied by the plugin implementation. The sole advantage over the current approach is that it would avoid a single deserialization per request, given the outcome phase will typically deserialise ~10 times per request at least (depending on node count, and thats observations including the value, unlike here where its just mdd data) - I don't think this is justified, IMO the current approach is simple and avoids adding additional complication to the aggregation code.
There was a problem hiding this comment.
Ok, I'm unsure of the perf impact in that case, so maybe you're right. It should all be negligible, I won't hold this PR up longer.
| @@ -28,6 +28,7 @@ type ContractTransmitter struct { | |||
|
|
|||
| func (c *ContractTransmitter) Transmit(ctx context.Context, configDigest types.ConfigDigest, seqNr uint64, | |||
There was a problem hiding this comment.
With everything going on, it would be a lot easier if we didn't make the plugin return []byte for the info. Just pass the a strongly typed struct.
There was a problem hiding this comment.
(see reply to previous comment on this)
|
| return nil, fmt.Errorf("failed to serialise batch of request ids: %w", err) | ||
| } | ||
|
|
||
| qb.lggr.Debugw("serialised batch of request ids", "numRequests", len(qb.RequestIDs), |
There was a problem hiding this comment.
nit: it would be helpful to log a full list of IDs because then we can trace it throughout the plugin execution. Same for all other phases.
| message RequestOutcome { | ||
| message ConsensusOutcome { | ||
| oneof outcome { | ||
| ConsensusSuccessOutcome success = 1; |
There was a problem hiding this comment.
Can you add a comment clarifying what can cause failure here? For example if all nodes pass the same error message as observation, will consensus end with a failure or not? If it does, then how can we distinguish failures caused by the user from our own failures? In other words, which metric should we base our dashboards and alerts on?
| metrics metrics | ||
|
|
||
| outctx ocr3types.OutcomeContext | ||
|
|
There was a problem hiding this comment.
nit: why so many empty lines?
There was a problem hiding this comment.
I have a weakness for whitespace :) - I'll sort it
| err := proto.Unmarshal(query, requestsQuery) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, fmt.Errorf("failed to unmarshal query: %w", err) |
There was a problem hiding this comment.
Can't we continue and still send all observations? That would prevent any stalls by buggy/malicious leader.
There was a problem hiding this comment.
Assuming this comment was meant for the observation phase? In the outcome phase the only option would be to ignore it (effectively current behaviour) or fail all requests which would be a worse outcome. Plus, by the time you are the observation phase the query has been validated in the observation phase, so to that.....
In the observation phase: there is a reasonable chance that request id order would be different across the nodes meaning consensus on the observations would probably not be reached. Also, if a leader is buggy/malicious they could send a list of invalid IDs so this wouldn't address that issue.
|
|
||
| if len(errors) >= r.f+1 { | ||
| consensusFailedMsg := fmt.Sprintf( | ||
| "consensus calculation failed: received %d errors which is >= f+1 (%d) for requestID %s\nconsensus metadata, descriptor and default: %+v\nerrors received: %+v", |
There was a problem hiding this comment.
nit: probably better to avoid newline characters
5 participants
PR to fast fail consensus requests when it is determined that it is not possible for the request to succeed. This allows for improved UX of the capability, but importantly also prevents a DoS attack vector.
In summary the approach taken is as follows:
Observation Phase:
All observations from the Observations phase for each request are passed to the Outcome phase, regardless of whether or not the observation is 'valid'. This allows the outcome to determine when >= 2f+1 requests have been. It follows that there must be at least f+1 valid observations amongst the received set.
Outcome Phase:
Once >= 2f+1 observations have been received the outcome phase attempts to achieve consensus across the observations, an outcome is created in both the success and failure cases which allows more timely processing of requests in the failure case and improves the UX with more meaningful failure messages returned to the workflow.