fix: patch data race

[evan-forbes]

Short term fix for celestiaorg/celestia-node#1252
Long term fix tracked in #882

Description

tendermint v0.34.x makes frequent use of a global var (env) for the RPC requests. If running multiple nodes, this can cause a data race. This fix is a tad hacky, since we're just adding a mutex and not fixing the source of the non-determinism, but it makes the race detector happy(ier) while also allowing for tests to pass in celestia-node.

[evan-forbes]

created the temporary v1.3.3-tm-v0.34.20-celestia-node-patch tag with this fix added onto v1.3.3

[rach-id]

The change itself LGTM. However, I added a question to understand more as I am not familiar with this part of the codebase 🐱

[Wondertan]

🫡

[Wondertan]

So I did another clear-mind review of this, and the reason why this fix does not fix anything is that all the reads are not protected by the mutex. That is, we only lock the mutex when we SetEnvironment, but not when we access it

[rootulp]

Thanks for flagging this @Wondertan ! I'll tackle locking / unlocking the mutex whenever we get the environment. I expect this may involve introducing a helper like

func GetEnvironment() *Environment {
	mut.Lock()
	defer mut.Unlock()
	return env
}

[rootulp]

Another instance of #836 in https://github.com/celestiaorg/celestia-core/actions/runs/3250400572/jobs/5334043979#step:6:111

[evan-forbes]

are we still wanting to fix this @rootulp @Wondertan

[rootulp]

I'm not actively working on it. Defer to @Wondertan if we still want to fix this. I assumed it's still happening in celestia-node but I just tried go test ./... -race which didn't report any data races

[Wondertan]

It might not happen instantly. It still happens on CI from time to time. The issue is still in the code, so we should fix this, IMO. Any long/short-term solutions work for me, and I think it would be better for you to decide which works better.

[rootulp]

Since this is still happening in celestia-node, I'll address #869 (comment) today as a short-term fix and create an issue to explore a long-term fix. Any objections @evan-forbes @Wondertan ?

[Wondertan]

No, from my side

[rootulp]

Tagging @evan-forbes for review b/c I can't explicitly add you as a reviewer

[evan-forbes]

I can't approve cause I originally opened this PR, but this is me approving

🤞 hoping this helps the race detector

[evan-forbes]

oh no