Messages that fail validation should be rejected #956
Conversation
There was some confusion between "message is invalid" and "message could not yet be dispatched" in the event aggregator. This attempts to separate those two situations, as the former should be an immediate rejection. Fixes hyperledger#952 Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
awrichar
requested review from
peterbroadhurst,
nguyer and
shorsher
as code owners
| @@ -476,14 +476,10 @@ func (ag *aggregator) processMessage(ctx context.Context, manifest *core.BatchMa | |||
|
|
|||
| } | |||
|
|
|||
| dispatched := false | |||
| var newState core.MessageState | |||
| if dataAvailable { | |||
There was a problem hiding this comment.
I removed this check because there's no way to get this far if data was missing.
There was a problem hiding this comment.
@awrichar - why do we not need to call state.SetContextBlockedBy(ctx, *unmaskedContext, pin.Sequence) in the case of !dataAvailable
That change wasn't made in this PR, and I'm not paged back in enough to answer this myself. But I read the comments on the if dispatched { block below and it strikes me it applies on face value:
// - dispatched=false: we need to prevent dispatch of any subsequent messages on the same topic in the batch
There was a problem hiding this comment.
That question also occurred to me, and I was somewhat hoping you knew the answer 😅 sounds like one or both of us needs to page back in more fully on the difference between waiting, rejecting, and blocking.
There was a problem hiding this comment.
After looking again, it does feel like we should mark the context blocked when the message/data is unavailable. I rearranged the code blocks so it will fall through to that path now.
| @@ -1634,20 +1669,38 @@ func TestDispatchBroadcastQueuesLaterDispatch(t *testing.T) { | |||
| mim.On("FindIdentityForVerifier", ag.ctx, mock.Anything, mock.Anything).Return(org1, nil) | |||
|
|
|||
| mdm := ag.data.(*datamocks.Manager) | |||
| mdm.On("GetMessageWithDataCached", ag.ctx, msg1.Header.ID, data.CRORequirePublicBlobRefs).Return(msg1, core.DataArray{}, true, nil).Once() | |||
| mdm.On("GetMessageWithDataCached", ag.ctx, msg2.Header.ID, data.CRORequirePublicBlobRefs).Return(msg2, core.DataArray{}, true, nil).Once() | |||
| data1 := core.DataArray{} | |||
There was a problem hiding this comment.
Honestly the aggregator tests are a bit rusty. When I started debugging, they were following paths that I did not expect based on the test names. I suspect they've never been fully overhauled in the wake of lots of identity changes and lots of batch state changes.
I had to touch a few tests to fix them after my changes, so I also tried to make those more consistent with what I think they're supposed to be testing - but just wanted to note that there are quite a few others that are stale and might really need to do extra steps to validate things that end up in batchState.
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
There was some confusion between "message is invalid" and "message could not yet
be dispatched" in the event aggregator. This attempts to separate those two
situations, as the former should be an immediate rejection.
Fixes #952