consensus: rename (*PeerState).ToJSON to MarshalJSON (celestiaorg#524)

mark-rushakoff · web-flow · commit 587bc0b18d28 · 2023-03-14T15:55:08.000Z
In (*PeerState).PickSendVote, there is a Debug-level log that includes the PeerState value as a logging field. By default, zerolog json-encodes a struct passed as a log field (when the struct doesn't implement zerolog.LogObjectMarshaler). Because PeerState didn't have a MarshalJSON method, the JSON encoder fell back to reflection to encode the PeerState value. Reflection did not acquire the lock, and there were data races resulting from an unsynchronized read while logging the PeerState, and concurrent (locked) writes at least during (*PeerState).SetHasProposal and (*PeerState).SetHasVote. Given that there was only one call to (*PeerState).ToJSON in the cometbft repo, it seemed appropriate to just rename ToJSON to MarshalJSON, as opposed to leaving ToJSON for backwards compatibility. Any third party calls to ToJSON should be able to easily change the method being called. Example data race (which is no longer reproducible with this change): ``` ================== WARNING: DATA RACE Read at 0x00c0004a4870 by goroutine 131: reflect.Value.Bool() /opt/homebrew/Cellar/go/1.20.1/libexec/src/reflect/value.go:288 +0x7c encoding/json.boolEncoder() /opt/homebrew/Cellar/go/1.20.1/libexec/src/encoding/json/encode.go:539 +0x88 encoding/json.structEncoder.encode() /opt/homebrew/Cellar/go/1.20.1/libexec/src/encoding/json/encode.go:759 +0x1bc encoding/json.structEncoder.encode-fm() <autogenerated>:1 +0x94 encoding/json.structEncoder.encode() /opt/homebrew/Cellar/go/1.20.1/libexec/src/encoding/json/encode.go:759 +0x1bc encoding/json.structEncoder.encode-fm() <autogenerated>:1 +0x94 encoding/json.ptrEncoder.encode() /opt/homebrew/Cellar/go/1.20.1/libexec/src/encoding/json/encode.go:943 +0x2a4 encoding/json.ptrEncoder.encode-fm() <autogenerated>:1 +0x6c encoding/json.(*encodeState).reflectValue() /opt/homebrew/Cellar/go/1.20.1/libexec/src/encoding/json/encode.go:358 +0x74 encoding/json.(*encodeState).marshal() /opt/homebrew/Cellar/go/1.20.1/libexec/src/encoding/json/encode.go:330 +0x1a0 encoding/json.Marshal() /opt/homebrew/Cellar/go/1.20.1/libexec/src/encoding/json/encode.go:161 +0xa0 github.com/rs/zerolog.init.1.func1() /Users/hh/go/pkg/mod/github.com/rs/zerolog@v1.29.0/encoder_json.go:21 +0x4c github.com/rs/zerolog/internal/json.Encoder.AppendInterface() /Users/hh/go/pkg/mod/github.com/rs/zerolog@v1.29.0/internal/json/types.go:366 +0x5c github.com/rs/zerolog.appendFieldList() /Users/hh/go/pkg/mod/github.com/rs/zerolog@v1.29.0/fields.go:273 +0x2b8c github.com/rs/zerolog.appendFields() /Users/hh/go/pkg/mod/github.com/rs/zerolog@v1.29.0/fields.go:21 +0x160 github.com/rs/zerolog.(*Event).Fields() /Users/hh/go/pkg/mod/github.com/rs/zerolog@v1.29.0/event.go:165 +0x90 cosmossdk.io/log.zeroLogWrapper.Debug() /Users/hh/go/pkg/mod/cosmossdk.io/log@v0.0.0-20230313123454-0fe816b71a62/logger.go:89 +0x18 github.com/cosmos/cosmos-sdk/server/log.(*CometZeroLogWrapper).Debug() <autogenerated>:1 +0x74 github.com/cometbft/cometbft/consensus.(*PeerState).PickSendVote() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:1138 +0x1bc github.com/cometbft/cometbft/consensus.(*Reactor).gossipVotesForHeight() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:794 +0x260 github.com/cometbft/cometbft/consensus.(*Reactor).gossipVotesRoutine() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:724 +0x2cc github.com/cometbft/cometbft/consensus.(*Reactor).AddPeer.func2() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:199 +0x58 Previous write at 0x00c0004a4870 by goroutine 130: github.com/cometbft/cometbft/consensus.(*PeerState).SetHasProposal() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:1096 +0x118 github.com/cometbft/cometbft/consensus.(*Reactor).gossipDataRoutine() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:617 +0xab8 github.com/cometbft/cometbft/consensus.(*Reactor).AddPeer.func1() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:198 +0x58 Goroutine 131 (running) created at: github.com/cometbft/cometbft/consensus.(*Reactor).AddPeer() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:199 +0x240 github.com/cometbft/cometbft/p2p.(*Switch).addPeer() /Users/hh/go/src/github.com/cometbft/cometbft/p2p/switch.go:855 +0x7b4 github.com/cometbft/cometbft/p2p.(*Switch).acceptRoutine() /Users/hh/go/src/github.com/cometbft/cometbft/p2p/switch.go:707 +0x704 github.com/cometbft/cometbft/p2p.(*Switch).OnStart.func1() /Users/hh/go/src/github.com/cometbft/cometbft/p2p/switch.go:241 +0x34 Goroutine 130 (running) created at: github.com/cometbft/cometbft/consensus.(*Reactor).AddPeer() /Users/hh/go/src/github.com/cometbft/cometbft/consensus/reactor.go:198 +0x164 github.com/cometbft/cometbft/p2p.(*Switch).addPeer() /Users/hh/go/src/github.com/cometbft/cometbft/p2p/switch.go:855 +0x7b4 github.com/cometbft/cometbft/p2p.(*Switch).acceptRoutine() /Users/hh/go/src/github.com/cometbft/cometbft/p2p/switch.go:707 +0x704 github.com/cometbft/cometbft/p2p.(*Switch).OnStart.func1() /Users/hh/go/src/github.com/cometbft/cometbft/p2p/switch.go:241 +0x34 ================== ``` --- #### PR checklist - [ ] Tests written/updated - [x] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments
diff --git a/.changelog/unreleased/bug-fixes/524-rename-peerstate-tojson.md b/.changelog/unreleased/bug-fixes/524-rename-peerstate-tojson.md
@@ -0,0 +1,2 @@
+- `[consensus]` Rename `(*PeerState).ToJSON` to `MarshalJSON` to fix a logging data race
+  ([\#524](https://github.com/cometbft/cometbft/pull/524))
diff --git a/consensus/reactor.go b/consensus/reactor.go
@@ -1055,8 +1055,8 @@ func (ps *PeerState) GetRoundState() *cstypes.PeerRoundState {
 	return &prs
 }
 
-// ToJSON returns a json of PeerState.
-func (ps *PeerState) ToJSON() ([]byte, error) {
+// MarshalJSON implements the json.Marshaler interface.
+func (ps *PeerState) MarshalJSON() ([]byte, error) {
 	ps.mtx.Lock()
 	defer ps.mtx.Unlock()
 
diff --git a/rpc/core/consensus.go b/rpc/core/consensus.go
@@ -61,7 +61,7 @@ func (env *Environment) DumpConsensusState(ctx *rpctypes.Context) (*ctypes.Resul
 		if !ok { // peer does not have a state yet
 			continue
 		}
-		peerStateJSON, err := peerState.ToJSON()
+		peerStateJSON, err := peerState.MarshalJSON()
 		if err != nil {
 			return nil, err
 		}

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,2 @@`
	`1`	+- `[consensus]` Rename `(*PeerState).ToJSON` to `MarshalJSON` to fix a logging data race
	`2`	`+ ([\#524](https://github.com/cometbft/cometbft/pull/524))`
Original file line number	Diff line number	Diff line change
`@@ -1055,8 +1055,8 @@ func (ps PeerState) GetRoundState() cstypes.PeerRoundState {`
`1055`	`1055`	`return &prs`
`1056`	`1056`	`}`
`1057`	`1057`
`1058`		`-// ToJSON returns a json of PeerState.`
`1059`		`-func (ps *PeerState) ToJSON() ([]byte, error) {`
	`1058`	`+// MarshalJSON implements the json.Marshaler interface.`
	`1059`	`+func (ps *PeerState) MarshalJSON() ([]byte, error) {`
`1060`	`1060`	`ps.mtx.Lock()`
`1061`	`1061`	`defer ps.mtx.Unlock()`
`1062`	`1062`
Original file line number	Diff line number	Diff line change
`@@ -61,7 +61,7 @@ func (env Environment) DumpConsensusState(ctx rpctypes.Context) (*ctypes.Resul`
`61`	`61`	`if !ok { // peer does not have a state yet`
`62`	`62`	`continue`
`63`	`63`	`}`
`64`		`- peerStateJSON, err := peerState.ToJSON()`
	`64`	`+ peerStateJSON, err := peerState.MarshalJSON()`
`65`	`65`	`if err != nil {`
`66`	`66`	`return nil, err`
`67`	`67`	`}`