ci(lint): golangci-lint and fieldalignment

[ramin]

At suggestion and request of @Wondertan, implemented govet field alignment linting for struct alignment/padding checks, and subsequently committed fixes for them, as we did in celestiaorg/celestia-node#2856

Needed to take a bit of a quest to get here, steps from volume 1:

• set up golangci-lint for the repo (using same .golangci.yml we use in celestia-node)
• set a go-version consistently throughout CI by setting in a .go-version file and reading that into GITHUB_OUTPUT in a setup step. Set to 1.21
• introduce a lint job that build is now dependent on

Now we have lint errors. Volume 2 was fixin'. Fixes for lint errors golangci-lint found that are corrected in this PR:

• unused parameters (mostly in _test.go files)
• spelling that was in the Queen's English vs the language of freedom
• long lines are looooooooooooong
• unnecessary conversions (lots of uint64 type casting where the return type is/was already uint64)

Finally, time for Return of the King. We then had "just" the fieldalignment optimizations, which have since been re-aligned. For example, here was the suggested output that was since altered

headertest/dummy_header.go:19:18: fieldalignment: struct with 80 pointer bytes could be 72 (govet)
type DummyHeader struct {
                 ^
p2p/exchange.go:36:35: fieldalignment: struct with 152 pointer bytes could be 144 (govet)
type Exchange[H header.Header[H]] struct {
                                  ^
p2p/exchange_metrics.go:29:22: fieldalignment: struct with 168 pointer bytes could be 144 (govet)
type exchangeMetrics struct {
                     ^
p2p/options.go:20:23: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type ServerParameters struct {
                      ^
p2p/options.go:125:23: fieldalignment: struct with 72 pointer bytes could be 40 (govet)
type ClientParameters struct {
                      ^
p2p/peer_stats.go:13:15: fieldalignment: struct with 72 pointer bytes could be 32 (govet)
type peerStat struct {
              ^
p2p/peer_stats.go:100:16: fieldalignment: struct with 72 pointer bytes could be 32 (govet)
type peerQueue struct {
               ^
p2p/peer_tracker.go:30:18: fieldalignment: struct with 120 pointer bytes could be 96 (govet)
type peerTracker struct {
                 ^
p2p/session.go:31:34: fieldalignment: struct with 112 pointer bytes could be 96 (govet)
type session[H header.Header[H]] struct {
                                 ^
p2p/subscriber.go:26:37: fieldalignment: struct with 48 pointer bytes could be 40 (govet)
type Subscriber[H header.Header[H]] struct {
                                    ^
p2p/subscriber_metrics.go:19:24: fieldalignment: struct with 96 pointer bytes could be 88 (govet)
type subscriberMetrics struct {
                       ^
store/batch.go:16:32: fieldalignment: struct with 40 pointer bytes could be 16 (govet)
type batch[H header.Header[H]] struct {
                               ^
store/heightsub.go:16:36: fieldalignment: struct with 24 pointer bytes could be 8 (govet)
type heightSub[H header.Header[H]] struct {
                                   ^
store/metrics.go:15:14: fieldalignment: struct with 88 pointer bytes could be 80 (govet)
type metrics struct {
             ^
store/options.go:14:17: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type Parameters struct {
                ^
sync/metrics.go:13:14: fieldalignment: struct with 24 pointer bytes could be 16 (govet)
type metrics struct {
             ^
sync/ranges.go:12:33: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type ranges[H header.Header[H]] struct {
                                ^
sync/ranges.go:89:38: fieldalignment: struct with 32 pointer bytes could be 8 (govet)
type headerRange[H header.Header[H]] struct {
                                     ^
sync/sync.go:33:33: fieldalignment: struct with 360 pointer bytes could be 312 (govet)
type Syncer[H header.Header[H]] struct {
                                ^
sync/sync.go:128:12: fieldalignment: struct with 136 pointer bytes could be 96 (govet)
type State struct {
           ^
sync/sync_getter.go:12:37: fieldalignment: struct with 48 pointer bytes could be 16 (govet)
type syncGetter[H header.Header[H]] struct {

[codecov-commenter]

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (28ff21c) 63.60% compared to head (5137209) 63.77%.

Files	Patch %	Lines
store/heightsub.go	0.00%	5 Missing ⚠️
p2p/options.go	33.33%	2 Missing ⚠️
headertest/subscriber.go	0.00%	1 Missing ⚠️
sync/ranges.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #132      +/-   ##
==========================================
+ Coverage   63.60%   63.77%   +0.16%     
==========================================
  Files          39       39              
  Lines        3366     3387      +21     
==========================================
+ Hits         2141     2160      +19     
- Misses       1060     1062       +2     
  Partials      165      165              

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

[Wondertan]

Queen's English vs the language of freedom

🤣

[Wondertan]

unnecessary conversions (lots of uint64 type casting where the return type is/was already uint64)

That's a relic from the time when int64 was used for heights. I thought we cleaned them up but seems like not

[Wondertan]

Hmm, so it seems like chasing perfect alignment can affect readability. Locks should be put on top of the field they synchronize.

---

The new field order does not make any sense to me, unfortunately. I think if we should disable lint for structs where performance gains are negligible, like in the case of singleton structs. Syncer, Exhange, Store, and so forth are meant to be instantiated once in application, meaning that the few bytes we get from realigning them are negligible., while readability and field grouping, in particular in such more complicated structs, are more important.

Perhaps we could use the ignore lint comment for such structs where we deliberately trade performance for readability

[ramin]

@Wondertan re-aligned some back to reflect idioms etc, have a look and point out any others that maybe want some re-jiggling, not too far from original in most cases

[cristaloleg]

IMO it will be better to not merge these changes 'cause Go will start doing this automatically, see accepted proposal golang/go#66408

[Wondertan]

Agree. Do you mean its fully automatic? My read is that it requires importing and embedding the layout struct

[cristaloleg]

Do you mean its fully automatic?

Of course not. Implicit field from structs package will reorder fields on a specific platform. By automatically I meant that we don't need to reorder fields (manually) when fields will be added/removed.