`TestValidatorSetHandling` is flaky

[tzdybal]

TestValidatorSetHandling is flaky - it sometimes fails like this:

    client_test.go:709: 
        	Error Trace:	client_test.go:709
        	Error:      	Received unexpected error:
        	            	failed to load validators for height 13: key not found
        	Test:       	TestValidatorSetHandling
I[2022-07-01|21:21:07.297] indexed block                                module=txindex height=13
    client_test.go:710: 
        	Error Trace:	client_test.go:710
        	Error:      	Expected value not to be nil.
        	Test:       	TestValidatorSetHandling
--- FAIL: TestValidatorSetHandling (0.13s)
panic: runtime error: invalid memory address or nil pointer dereference [recovered]
	panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x28 pc=0x11fa485]

goroutine 1972 [running]:
testing.tRunner.func1.2({0x134e020, 0x23f2590})
	/usr/lib/go/src/testing/testing.go:1389 +0x24e
testing.tRunner.func1()
	/usr/lib/go/src/testing/testing.go:1392 +0x39f
panic({0x134e020, 0x23f2590})
	/usr/lib/go/src/runtime/panic.go:838 +0x207
github.com/celestiaorg/optimint/rpc/client.TestValidatorSetHandling(0xc05f6a7380?)
	/home/tomek/devel/celestia/optimint/rpc/client/client_test.go:711 +0x1625
testing.tRunner(0xc05f6a7380, 0x183fa70)
	/usr/lib/go/src/testing/testing.go:1439 +0x102
created by testing.(*T).Run
	/usr/lib/go/src/testing/testing.go:1486 +0x35f

[distractedm1nd]

This error occurs sometimes because the store's current height is incremented before the validators are saved. So we read before we write:

https://github.com/celestiaorg/optimint/blob/b1df4346508c124d59504555f34210773a4e5b50/block/manager.go#L405-L430

Would there be any issue with waiting to set the height after we commit everything to the db?

[adlerjohn]

@distractedm1nd can you clarify a bit on this? From your description it seems to be a race condition. Is there some sort of linter or something we can use to detect potential race conditions at compile time?

[distractedm1nd]

Yes - it is a race condition, and I don't know if any others might emerge from here. I don't know the codebase well enough yet to infer anything about it without investigating more specific cases. Good question about the linter, I don't think a linter is capable of that, just mock testing maybe?