[Malleability] Automatic malleability checker

[durkmurder]

Context

This PR implements a checker that can be used on any type that implements flow.IDEntity to ensure that the implementation of ID() is correct and the structure is not malleable.

The idea behind the checker is to rely on reflection to traverse the data structure and ensure that after modifying each field of the structure we will end up with a different hash comparing to the one taken before the modification.

Please be aware, that it's highly experimental stuff, if you see any area for improvement or any potential issues don't hesitate to reach out to me or flag them.

TODO:

• add more tests for panic cases

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 48.93617% with 96 lines in your changes missing coverage. Please review.

Project coverage is 41.28%. Comparing base (2a9eb10) to head (807cb8d).

Files with missing lines	Patch %	Lines
utils/unittest/entity.go	48.93%	86 Missing and 10 partials ⚠️

Additional details and impacted files

@@                   Coverage Diff                    @@
##           feature/malleability    #7069      +/-   ##
========================================================
+ Coverage                 41.23%   41.28%   +0.04%     
========================================================
  Files                      2138     2138              
  Lines                    188568   188735     +167     
========================================================
+ Hits                      77761    77914     +153     
+ Misses                   104312   104307       -5     
- Partials                   6495     6514      +19     

Flag	Coverage Δ
unittests	41.28% <48.93%> (+0.04%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[tim-barry]

I ran into an issue with reflection when trying to run the malleability check on Header: it failed because the ID() function on the struct had a value receiver instead of a pointer receiver, AND I was passing in the struct directly instead of a pointer/reference to it.
The error message I received was confusing and unintuitive (that one of the structure's fields was unaddressable, when in fact the issue was that the struct as a whole was unaddressable). Therefore I think we should at least improve the error message for this situation, if not fix it entirely.

I think there are three potential ways to address this:

1. No change here, but enforce that all implementations of ID() must take a pointer receiver (How? Is this otherwise desirable?)
2. Require that a pointer type be passed here, and add an error message if the entity passed in was not a reference. Tests would need to ensure they are passing a reference to the underlying struct to avoid failing this requirement. For example, with this change the current TestRequireEntityNonMalleable/type_alias would fail, and would need to replace IntList{1, 2, 3} with &IntList{1, 2, 3} to pass.

Suggested change

	}
	} else {
	// If it is not a pointer type, we may not be able to set fields to test malleability, since the entity may not be addressable
	require.Failf(t, "entity is not a pointer type (try taking a reference to it)", "entity: %v %v", v.Kind(), v.Type())
	}

3. Automatically create an addressable copy of any non-addressable values: this is a fully general solution that should guarantee that all implementations of IDEntity can be checked.

Suggested change

	}
	} else {
	// Create a copy to guarantee value is settable
	tmp := reflect.New(v.Type())
	tmp.Elem().Set(v)
	entity = tmp.Interface().(flow.IDEntity)
	v = tmp.Elem()
	}

(adapted from stackoverflow)

[durkmurder]

Thanks for spotting this. I am leaning towards your suggestion 2), I am a bit afraid of adding more shenanigans with reflection as it's already complicated so I would require that we pass a pointer since we most of the time operate on pointers either way.

I have also found another problem that the checker fails on processing time.Time because it tries to recursively check it but it has private fields so I have updated the logic to first try generate a field, event if it's a struct and only if we have failed then we apply a field-by-field recursive check. 1534144 (#7069)

[tarakby]

I like the idea, great work 👏🏼
I did not fully understand the reflection part yet, but I left some comments about the rest.

[jordanschalm]

Have not finished reviewing yet. Just some preliminary comments.

[jordanschalm]

This seems identical to EntityListFixture - can we remove one?

[durkmurder]

sure, this is just old code moved from other file

[jordanschalm]

I don't understand the reasoning behind this constraint. (As I understand the documentation, the checker will not modify fields which are set to zero/nil in the input.) However, it also seems like this constraint only applies to map/slice-typed fields.

A big part of the malleability work is to enforce that if a model field changes, the ID must also change. But this constraint introduces significant surface area for our tests to fail to check these cases, if the test accidentally passes in a model fixture with any of its fields nil (those fields won't be checked for malleability!).

It's very easy to accidentally not initialize a struct field in Go, generally speaking. I think there are quite a few instances where our fixtures do not initialize every field. I found a couple of examples:

• ClusterPayloadFixture sets ReferenceBlockID to zero
• PayloadFixture sets all fields to nil

Most of the existing usages of the Malleability checker are just passing in the fixture for the type being checked. So I'm worried that we can easily be creating tests that are not exhaustively checking malleability, because they do not modify certain fields.

Suggestions

• Clarify in the documentation exactly what zero value inputs will result in the Checker not checking the field for malleability, if this behaviour is necessary
• Change the default behaviour: if an entity is passed in with a field which we will not be able to check for any reason (currently: a nil map/slice) then throw an error.

[durkmurder]

Thanks for the observation. I am well aware of this and specifically made this decision to support cases where we actually allow nil fields and expect them. Lets first understand how checker behaves.
If it encounters a field which is of kind:

1. empty map
2. empty slice
3. nil pointer
   It will omit them from malleability check and assume that field doesn't contribute to the malleability of the data structure.
   Why? Because we have structures that allow nil fields, for example EpochCommit with empty DKGIndexMap. If we always modify the field even if it's nil we are suddenly checking another structure which potentially(and in this case definitely) invokes different code path in ID calculation. That is why I did it this way. Originally I thought about the structure that we pass as a blueprint for checking, we specifically state what fields to check, mind that this doesn't mean ZeroID is omitted.

While I was writing this response it crossed my mind that we can use struct tag like "malleability:"optional"" to mark a field that can be omitted, otherwise we always try to modify the field.

I would be interested to discuss it, if you have any ideas how can we address this problem.

[jordanschalm]

Thanks for the explanation. I agree we need a way to handle cases where a nil field changes the way the ID is computed.

In my opinion, EpochCommit.DKGIndexMap is an atypical case and we should avoid designing the general tool around it. We handle nil instances of the field differently to support backward-compatibility, but:

• most of our models don't do this
• we intend for this to be temporary (code comment)

I'd rather have the MalleabilityChecker be bulletproof for the 90+% of model types than try to make it fit every model, but be easier to misuse in the common case. So I do still feel that the default behaviour should be to always check malleability on every field, and return an error if we can't for some reason.

For unusual models, like the current EpochCommit we could do the malleability check manually (I'm guessing this kind of ID computation logic bifurcation will remain uncommon). Or we could pass in some config to MalleabilityChecker to allow it to skip certain fields. (I like you idea of using struct tags. We could also potentially use the WithCustomType option.)

[jordanschalm]

Here is an example I noticed in one of the current PRs using MalleabilityChecker, which would have failed to detect a malleable ID implementation:

• The unit test uses ExecutionResultFixture, which has an empty ServiceEvents field
• If ID were malleable the test wouldn't have caught it

[durkmurder]

Agreed to implement next behavior:

• if slice, map is empty or nil - return an error, unless it's marked with struct tag as optional
• if field is nil - return an error, unless it's marked with struct tag as optional

This implements safety by default and protects from development errors and in rare cases where we need an optional field we use a struct tag.

[jordanschalm]

I think we can remove the generated return value now that we are checking for the struct tag here.

[durkmurder]

1cd51ed (#7069)

[jordanschalm]

I think we could add one more struct type and test here to test and document usage of the malleability:"optional" tag

[durkmurder]

27718b3 (#7069)