[Dynamic Protocol State] Sync master to feature/dynamic-protocol-state

[durkmurder]

Context

This PR syncs feature branch with latest master, main reason for it is to fix CI and get latest crypto related changes.

Merge had some conflicts due to usage of Identity where we use IdentitySkeleton.

List of conflicted files:

#       Makefile
#       engine/access/apiproxy/access_api_proxy.go
#       engine/access/apiproxy/access_api_proxy_test.go
#       engine/access/rest/apiproxy/rest_proxy_handler.go
#       engine/common/grpc/forwarder/forwarder.go
#       go.mod
#       go.sum
#       insecure/go.mod
#       insecure/go.sum
#       integration/go.mod
#       integration/go.sum
#       model/flow/identity.go
#       model/flow/identity_test.go
#       network/p2p/scoring/utils_test.go
#       state/protocol/badger/validity_test.go

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a7b22f6) 56.16% compared to head (c585750) 61.45%.

Additional details and impacted files

@@                        Coverage Diff                         @@
##           feature/dynamic-protocol-state    #5117      +/-   ##
==================================================================
+ Coverage                           56.16%   61.45%   +5.29%     
==================================================================
  Files                                 976      579     -397     
  Lines                               90630    57882   -32748     
==================================================================
- Hits                                50899    35573   -15326     
+ Misses                              35943    19611   -16332     
+ Partials                             3788     2698    -1090     

Flag	Coverage Δ
unittests	61.45% <ø> (+5.29%)	⬆️

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.

[jordanschalm]

I assume this is coming in from master, but I'm curious why this was added. What if both iy and other are nil?

[durkmurder]

not sure, I thought maybe some cases due to new crypto, but I didn't check, seemed not harmful so I've left it. I will also try if something breaks without it. @tarakby have you added this by any chance?

[AlexHentschel]

I think we really need @tarakby's input here.

• Reviewing flow.Identity, there are two methods for checking equality: Equals and EqualTo, where it seems @tarakby added the former in May
• Neither method has documentation and their naming indicates that both do very similar things. This lack of documentation should be fixed (happy to roll these few lines into our PR)
  • based on the implementation, Equals seems to handle nil inputs (but not a nil receiver), while EqualTo does not handle a nil input. Is that correct?
  • Equals calls the PublicKey.Equals method for the keys. However, the PublicKey API does not specify behaviour for nil inputs nor nil receivers. I think we need to add documentation. Does the method handle nil receivers, because EqualTo tolerates nil keys?
  • To me, the most consistent convention would be to consider two nil objects identical (principle of Vacuous truth), but I think neither implementation Equals or EqualTo adheres to this principle.

[AlexHentschel]

Just checked and at least in master, Equals is only used in tests. All test still pass, when replacing Equals -> EqualTo

[tarakby]

Yes I added it in this commit a4fb435 to fix tests (see commit code) that have been testing equality of identities (and their public key fields) based on struct equalities. That was not right because comparing public keys should be based on the Equals method (two public keys may be equal but have different structs in memory).

I added the new function only to fix the tests and I didn't pay attention to the existing EqualTo that seems to compare public keys properly. One of the two functions should be enough, the new one I added is redundant.

Regarding the nil cases, the function I added makes the assumption that iy isn't nil, though this is implicit and not documented (not great my bad). Feel free to clean up the functions and the nil case treatment.
Another poor documented function is the public key comparison within the crypto module, it assumes the receiver and argument are both non nil. I will update the crypto module to either address the nil cases or document the assumptions clearly.

[tarakby]

( sorry @AlexHentschel I just noticed that I replied without seeing your comments as my opened the conversation early and didn't refresh it before sending my reply )

[tarakby]

Just checked and at least in master, Equals is only used in tests

That's correct, as you see on the commit a4fb435, I only used that function to fix the tests. I should have even added the new function as a test utility.

[tarakby]

opened this issue: onflow/crypto#4

[durkmurder]

when merging PR I have removed Equals and left only EqualTo but moved the check for nil pointer to it. After this discussion I have removed the nil pointer semantics all together, I don't think we want to support a case where we compare with nil, additionally if rhs can be nil then we need to think about a case where lhs is nil which requires introducing weird rules for comparing nil == nil. I would like to avoid it by relying on safety-by-default of pointers, meaning if we accidentally compare with nil(which we shouldn't) then the software will crash.

[jordanschalm]

Agree with not supporting nil *Identitys 👍

[AlexHentschel]

this is a net new addition, right? I don't think it exists that way on master.