fix: general improvements in tm2/bft/consensus

[petar-dambovaliev]

These are general improvements to concurrency in tm2

• Don't acquire write lock when it is not necessary
• Handle ignored errors
• Implement a uniform comparison method (readability + type safety)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.48%. Comparing base (84ba9c9) to head (92abef4).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1495      +/-   ##
==========================================
- Coverage   47.49%   47.48%   -0.01%     
==========================================
  Files         388      389       +1     
  Lines       61305    61623     +318     
==========================================
+ Hits        29117    29264     +147     
- Misses      29750    29910     +160     
- Partials     2438     2449      +11     

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

[zivkovicmilos]

Thank you for the improvements 🙏

[kristovatlas]

I did not review business logic much since I'm not up to speed on consensus yet. This PR passes all of the following checks:

• This PR introduces new 3rd-party source code or binaries:
  • External code is from a reputable source
  • External code has underwent a documented security audit and high/severe findings have been mitigated
  • External code has associated vulnerability disclosure policy and contact information
  • External code has published previous vulnerabilities/fixes
  • Version of external code is latest
• Findings introduced by the first-order source code of this PR generated by GoSec have been reviewed
  • gnovm/pkg/gnomod/fetch.go - no findings
  • tm2/ordering/ordering.go - no findings
  • tm2/pkg/bft/consensus/common_test.go - no findings
  • tm2/pkg/bft/consensus/reactor.go - findings not in scope of PR
  • tm2/pkg/bft/consensus/state.go - findings not in scope of PR
  • tm2/pkg/bft/consensus/types/round_state.go - no findings
  • tm2/pkg/bft/consensus/types/round_state_test.go - no findings
  • tm2/pkg/bft/consensus/wal_generator.go - no findings
  • tm2/pkg/bft/types/priv_validator.go - no findings
  • tm2/pkg/bft/types/validator.go - no findings
  • tm2/pkg/bft/types/validator_set.go - no findings
  • tm2/pkg/bft/types/validator_set_test.go - no findings
  • tm2/pkg/bft/wal/wal.go - findings not in scope of PR
  • tm2/pkg/crypto/crypto.go - no findings
  • tm2/pkg/crypto/keys/client/add.go - no findings
  • tm2/pkg/store/cache/mergeiterator.go - no findings
• This PR has impacts the most security sensitive areas of the code such as entropy generation, secret storage, transaction/message signing, etc.

• Comment: Consensus code may be security sensitive

• This PR includes sufficient test coverage to demonstrate defensive programming, lack of surreptitious code, etc.
• PR does not contain undocumented “magic values”

[thehowl]

Disagree on the ordering package, rest looks pretty much OK.

[thehowl]

I'm not convinced by this package.

Go has recently introduced a utility function for comparison. Its return values are simple integers -1 0 1, same as bytes.Compare and all other similar functions. Do you have a compelling case as to why we should have this, as compared to using < 0 == 0 > 0 for comparison functions?

[petar-dambovaliev]

The only reason i have is it is more civilized...

[thehowl]

I still don't think that adding ordering is a good idea if no-one in Go does it. I'll add this to the review meeting so we can decide.

[thehowl]

From the review meeting: we agreed that returning structs instead of integers in this place, and going against the established go convention of returning -1 / 0 / 1 is not something we want to do. So PR good to go after we resolve that :)

[petar-dambovaliev]

From the review meeting: we agreed that returning structs instead of integers in this place, and going against the established go convention of returning -1 / 0 / 1 is not something we want to do. So PR good to go after we resolve that :)

If that is removed, there is pretty much nothing in the PR that is left. I am closing this PR.