skip-mev / cometbft

CometBFT (fork of Tendermint Core): A distributed, Byzantine fault-tolerant, deterministic state machine replication engine
https://docs.cometbft.com
Apache License 2.0
0 stars 0 forks source link

improve consensus state concurrency mutex handling patterns #2

Closed wesl-ee closed 3 weeks ago

wesl-ee commented 1 month ago

Presently a mutex is used to prevent data hazards on the consensus State struct.

https://github.com/skip-mev/cometbft/blob/48db7ac0037db4e16eab7536a05966f928a4b70b/internal/consensus/state.go#L93

This state mutex is sometimes accessed and locked from outside its handlers creating confusing access patterns. Mostly during catchup.

https://github.com/skip-mev/cometbft/blob/48db7ac0037db4e16eab7536a05966f928a4b70b/internal/consensus/reactor.go#L122-L123

This access pattern necessitates seemingly random unlock logic within the State mutators themselves to permit this access.

https://github.com/skip-mev/cometbft/blob/48db7ac0037db4e16eab7536a05966f928a4b70b/internal/consensus/state.go#L913-L919

These access patterns could be improved by internalizing all mutation and access of the consensus State struct into state.go. The goal is to eliminate needing to access and lock the mutex outside of the getters and setters defined for the consensus state struct. An alternative approach is to formalize consensus state into its own package, but I don't believe this complexity is needed. Tidying up mutex semantics should be good enough, and further abstraction has no performance benefit.

wesl-ee commented 1 month ago

not using channels anymore, I started writing this way and it looked way uglier. I'll keep the mutex and clean up the mutex hygiene to only permit thread-safe access to data within the getters and setters defined on the struct.