Implement LLO (data streams/mercury v1.0) #5
Conversation
samsondav
changed the title
samsondav
force-pushed
the
MERC-1516-v-1-0-multichain-mercury-parallel-composition
branch
5 times, most recently
from
50663d4
to
da9c0a0
Compare
samsondav
force-pushed
the
MERC-1516-v-1-0-multichain-mercury-parallel-composition
branch
from
da9c0a0
to
35308d3
Compare
|
|
||
| // Indicates whether a report can be generated for the given channel. | ||
| // Returns nil if channel is reportable | ||
| func (out *Outcome) IsReportable(channelID commontypes.ChannelID) error { |
There was a problem hiding this comment.
is it necessary to have this at the channel level and rather than the stream level?
I see this:
for _, streamID := range channelDefinition.StreamIDs {
if out.StreamMedians[streamID] == nil {
return errors.New("IsReportable=false; median was nil")
}
}
does that mean if a single stream is failing - the entire channel fails to report? Is there a reason why that must happen?
There was a problem hiding this comment.
Hmm, good question. It think it probably does need to be at the channel level because otherwise what would be the outcome?
Imagine you try to create a report and one of the streams is bad. The datatype has no way to mark this. Do you just write zeroes? That could lead to some really unexpected and bad results. I think we cannot bake a report at all if any one of the streams is bad.
| timestampsNanoseconds = append(timestampsNanoseconds, observation.UnixTimestampNanoseconds) | ||
|
|
||
| for channelID := range observation.RemoveChannelIDs { | ||
| removeChannelVotesByID[channelID]++ |
There was a problem hiding this comment.
should we verify the channel-id exists in removeChannelVotesByID - idk if it's possible for a faulty/malicious observation to come in and break this flow by adding a non-existent channel id
There was a problem hiding this comment.
We don't need to verify it exists, in fact this map is empty so none of them will exist.
We need to handle the scenario that the node executing this code is:
- Honest
- May have an outdated/incorrect view of channel definitions
We come to consensus on which channels should be added/removed. It requires that at least 2f+1 (I think) nodes are honest and have a proper view on channel definitions. So even if you have a malicious node put whatever garbage it wants here, unless 2f+1 are in agreement, it won't be applied.
| ///////////////////////////////// | ||
| // outcome.LifeCycleStage | ||
| ///////////////////////////////// | ||
| if previousOutcome.LifeCycleStage == LifeCycleStageStaging && validPredecessorRetirementReport != nil { |
There was a problem hiding this comment.
what if we don't want to promote to LifeCylceStageProduction? is there a way to cancel - is that just canceling the job and creating new configs on-chain?
There was a problem hiding this comment.
The LifeCycleStage code currently does nothing since it is not even implemented on the contract. It was copied direct from the sketch and I didn't touch any of it. It will be implemented in https://smartcontract-it.atlassian.net/browse/MERC-3386
| for streamID, observations := range streamObservations { | ||
| sort.Slice(observations, func(i, j int) bool { return observations[i].Cmp(observations[j]) < 0 }) | ||
| if len(observations) <= p.F { | ||
| continue |
There was a problem hiding this comment.
should we be checking for 2f+1 observations?
There was a problem hiding this comment.
It's a good question. This probably needs to be refactored into an aggregate function like we do for mercury here: https://github.com/smartcontractkit/chainlink-data-streams/blob/master/mercury/aggregate_functions.go#L23
We have a ticket open for refactoring this entire file and adding more tests: https://smartcontract-it.atlassian.net/browse/MERC-3524
I have pushed a change with a comment that explains this check.
There was a problem hiding this comment.
Nice - thanks for opening that thread with research clarified the check here
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
No description provided.