From 26c38e770ebdac15556922f95de1cf9eb4e70b93 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 10 Apr 2018 11:15:16 +0200 Subject: [PATCH 1/2] fix data race Closes #1442 ``` WARNING: DATA RACE Write at 0x00c4209de7c8 by goroutine 23: github.com/tendermint/tendermint/types.(*Block).fillHeader() /home/vagrant/go/src/github.com/tendermint/tendermint/types/block.go:88 +0x157 github.com/tendermint/tendermint/types.(*Block).Hash() /home/vagrant/go/src/github.com/tendermint/tendermint/types/block.go:104 +0x121 github.com/tendermint/tendermint/types.(*Block).HashesTo() /home/vagrant/go/src/github.com/tendermint/tendermint/types/block.go:135 +0x4f github.com/tendermint/tendermint/consensus.(*ConsensusState).enterPrecommit() /home/vagrant/go/src/github.com/tendermint/tendermint/consensus/state.go:1037 +0x182d github.com/tendermint/tendermint/consensus.(*ConsensusState).addVote() /home/vagrant/go/src/github.com/tendermint/tendermint/consensus/state.go:1425 +0x1a6c github.com/tendermint/tendermint/consensus.(*ConsensusState).tryAddVote() /home/vagrant/go/src/github.com/tendermint/tendermint/consensus/state.go:1318 +0x77 github.com/tendermint/tendermint/consensus.(*ConsensusState).handleMsg() /home/vagrant/go/src/github.com/tendermint/tendermint/consensus/state.go:581 +0x7a9 github.com/tendermint/tendermint/consensus.(*ConsensusState).receiveRoutine() /home/vagrant/go/src/github.com/tendermint/tendermint/consensus/state.go:539 +0x6c3 Previous read at 0x00c4209de7c8 by goroutine 47: github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common.(*HexBytes).MarshalJSON() :1 +0x52 github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.invokeMarshalJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:433 +0x88 github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec)._encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:82 +0x8d2 github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:50 +0x10e github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSONStruct() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:348 +0x539 github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec)._encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:119 +0x83f github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:50 +0x10e github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSONStruct() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:348 +0x539 github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec)._encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:119 +0x83f github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:50 +0x10e github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSONStruct() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:348 +0x539 github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec)._encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:119 +0x83f github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:50 +0x10e github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSONStruct() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:348 +0x539 github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec)._encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:119 +0x83f github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).encodeReflectJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/json-encode.go:50 +0x10e github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino.(*Codec).MarshalJSON() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/go-amino/amino.go:296 +0x182 github.com/tendermint/tendermint/rpc/lib/types.NewRPCSuccessResponse() /home/vagrant/go/src/github.com/tendermint/tendermint/rpc/lib/types/types.go:100 +0x12c github.com/tendermint/tendermint/rpc/lib/server.makeJSONRPCHandler.func1() /home/vagrant/go/src/github.com/tendermint/tendermint/rpc/lib/server/handlers.go:152 +0xab7 net/http.HandlerFunc.ServeHTTP() /usr/lib/go-1.9/src/net/http/server.go:1918 +0x51 net/http.(*ServeMux).ServeHTTP() /usr/lib/go-1.9/src/net/http/server.go:2254 +0xa2 github.com/tendermint/tendermint/rpc/lib/server.RecoverAndLogHandler.func1() /home/vagrant/go/src/github.com/tendermint/tendermint/rpc/lib/server/http_server.go:138 +0x4fa net/http.HandlerFunc.ServeHTTP() /usr/lib/go-1.9/src/net/http/server.go:1918 +0x51 net/http.serverHandler.ServeHTTP() /usr/lib/go-1.9/src/net/http/server.go:2619 +0xbc net/http.(*conn).serve() /usr/lib/go-1.9/src/net/http/server.go:1801 +0x83b Goroutine 23 (running) created at: github.com/tendermint/tendermint/consensus.(*ConsensusState).OnStart() /home/vagrant/go/src/github.com/tendermint/tendermint/consensus/state.go:250 +0x35b github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common.(*BaseService).Start() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common/service.go:130 +0x5fc github.com/tendermint/tendermint/consensus.(*ConsensusReactor).OnStart() /home/vagrant/go/src/github.com/tendermint/tendermint/consensus/reactor.go:69 +0x1b4 github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common.(*BaseService).Start() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common/service.go:130 +0x5fc github.com/tendermint/tendermint/consensus.(*ConsensusReactor).Start() :1 +0x43 github.com/tendermint/tendermint/p2p.(*Switch).OnStart() /home/vagrant/go/src/github.com/tendermint/tendermint/p2p/switch.go:177 +0x124 github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common.(*BaseService).Start() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common/service.go:130 +0x5fc github.com/tendermint/tendermint/node.(*Node).OnStart() /home/vagrant/go/src/github.com/tendermint/tendermint/node/node.go:416 +0xa1b github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common.(*BaseService).Start() /home/vagrant/go/src/github.com/tendermint/tendermint/vendor/github.com/tendermint/tmlibs/common/service.go:130 +0x5fc github.com/tendermint/tendermint/rpc/test.StartTendermint() /home/vagrant/go/src/github.com/tendermint/tendermint/rpc/test/helpers.go:100 +0x5b github.com/tendermint/tendermint/rpc/client_test.TestMain() /home/vagrant/go/src/github.com/tendermint/tendermint/rpc/client/main_test.go:17 +0x4c main.main() github.com/tendermint/tendermint/rpc/client/_test/_testmain.go:76 +0x1cd Goroutine 47 (running) created at: net/http.(*Server).Serve() /usr/lib/go-1.9/src/net/http/server.go:2720 +0x37c net/http.Serve() /usr/lib/go-1.9/src/net/http/server.go:2323 +0xe2 github.com/tendermint/tendermint/rpc/lib/server.StartHTTPServer.func1() /home/vagrant/go/src/github.com/tendermint/tendermint/rpc/lib/server/http_server.go:35 +0xb3 ``` --- consensus/reactor.go | 20 +++++++++++++++----- consensus/state.go | 13 +++++++++---- consensus/types/state.go | 2 +- rpc/core/consensus.go | 18 +++++++++++++----- rpc/core/pipe.go | 5 ++--- rpc/core/types/responses.go | 6 +++--- 6 files changed, 43 insertions(+), 21 deletions(-) diff --git a/consensus/reactor.go b/consensus/reactor.go index 8f341184e..385e8d867 100644 --- a/consensus/reactor.go +++ b/consensus/reactor.go @@ -9,7 +9,7 @@ import ( "github.com/pkg/errors" - "github.com/tendermint/go-amino" + amino "github.com/tendermint/go-amino" cmn "github.com/tendermint/tmlibs/common" "github.com/tendermint/tmlibs/log" @@ -838,8 +838,8 @@ var ( ErrPeerStateInvalidStartTime = errors.New("Error peer state invalid startTime") ) -// PeerState contains the known state of a peer, including its connection -// and threadsafe access to its PeerRoundState. +// PeerState contains the known state of a peer, including its connection and +// threadsafe access to its PeerRoundState. type PeerState struct { Peer p2p.Peer logger log.Logger @@ -878,12 +878,14 @@ func NewPeerState(peer p2p.Peer) *PeerState { } } +// SetLogger allows to set a logger on the peer state. Returns the peer state +// itself. func (ps *PeerState) SetLogger(logger log.Logger) *PeerState { ps.logger = logger return ps } -// GetRoundState returns an atomic snapshot of the PeerRoundState. +// GetRoundState returns an shallow copy of the PeerRoundState. // There's no point in mutating it since it won't change PeerState. func (ps *PeerState) GetRoundState() *cstypes.PeerRoundState { ps.mtx.Lock() @@ -893,6 +895,14 @@ func (ps *PeerState) GetRoundState() *cstypes.PeerRoundState { return &prs } +// GetRoundStateJSON returns a json of PeerRoundState, marshalled using go-amino. +func (ps *PeerState) GetRoundStateJSON() ([]byte, error) { + ps.mtx.Lock() + defer ps.mtx.Unlock() + + return cdc.MarshalJSON(ps.PeerRoundState) +} + // GetHeight returns an atomic snapshot of the PeerRoundState's height // used by the mempool to ensure peers are caught up before broadcasting new txs func (ps *PeerState) GetHeight() int64 { @@ -1055,7 +1065,7 @@ func (ps *PeerState) ensureCatchupCommitRound(height int64, round int, numValida } } -// EnsureVoteVitArrays ensures the bit-arrays have been allocated for tracking +// EnsureVoteBitArrays ensures the bit-arrays have been allocated for tracking // what votes this peer has received. // NOTE: It's important to make sure that numValidators actually matches // what the node sees as the number of validators for height. diff --git a/consensus/state.go b/consensus/state.go index c6532e369..2e97c4953 100644 --- a/consensus/state.go +++ b/consensus/state.go @@ -168,18 +168,23 @@ func (cs *ConsensusState) GetState() sm.State { return cs.state.Copy() } -// GetRoundState returns a copy of the internal consensus state. +// GetRoundState returns a shallow copy of the internal consensus state. func (cs *ConsensusState) GetRoundState() *cstypes.RoundState { cs.mtx.Lock() defer cs.mtx.Unlock() - return cs.getRoundState() -} -func (cs *ConsensusState) getRoundState() *cstypes.RoundState { rs := cs.RoundState // copy return &rs } +// GetRoundStateJSON returns a json of RoundState, marshalled using go-amino. +func (cs *ConsensusState) GetRoundStateJSON() ([]byte, error) { + cs.mtx.Lock() + defer cs.mtx.Unlock() + + return cdc.MarshalJSON(cs.RoundState) +} + // GetValidators returns a copy of the current validators. func (cs *ConsensusState) GetValidators() (int64, []*types.Validator) { cs.mtx.Lock() diff --git a/consensus/types/state.go b/consensus/types/state.go index 8e79f10d2..b18fbd7c0 100644 --- a/consensus/types/state.go +++ b/consensus/types/state.go @@ -52,7 +52,7 @@ func (rs RoundStepType) String() string { //----------------------------------------------------------------------------- // RoundState defines the internal consensus state. -// It is Immutable when returned from ConsensusState.GetRoundState() +// It should be immutable when returned from ConsensusState.GetRoundState(), but it's not. // TODO: Actually, only the top pointer is copied, // so access to field pointers is still racey // NOTE: Not thread safe. Should only be manipulated by functions downstream diff --git a/rpc/core/consensus.go b/rpc/core/consensus.go index 25b67925c..7647aef7d 100644 --- a/rpc/core/consensus.go +++ b/rpc/core/consensus.go @@ -1,8 +1,9 @@ package core import ( + "encoding/json" + cm "github.com/tendermint/tendermint/consensus" - cstypes "github.com/tendermint/tendermint/consensus/types" p2p "github.com/tendermint/tendermint/p2p" ctypes "github.com/tendermint/tendermint/rpc/core/types" sm "github.com/tendermint/tendermint/state" @@ -58,7 +59,7 @@ func Validators(heightPtr *int64) (*ctypes.ResultValidators, error) { return &ctypes.ResultValidators{height, validators.Validators}, nil } -// Dump consensus state. +// DumpConsensusState dumps consensus state. // // ```shell // curl 'localhost:46657/dump_consensus_state' @@ -83,11 +84,18 @@ func Validators(heightPtr *int64) (*ctypes.ResultValidators, error) { // } // ``` func DumpConsensusState() (*ctypes.ResultDumpConsensusState, error) { - peerRoundStates := make(map[p2p.ID]*cstypes.PeerRoundState) + peerRoundStates := make(map[p2p.ID]json.RawMessage) for _, peer := range p2pSwitch.Peers().List() { peerState := peer.Get(types.PeerStateKey).(*cm.PeerState) - peerRoundState := peerState.GetRoundState() + peerRoundState, err := peerState.GetRoundStateJSON() + if err != nil { + return nil, err + } peerRoundStates[peer.ID()] = peerRoundState } - return &ctypes.ResultDumpConsensusState{consensusState.GetRoundState(), peerRoundStates}, nil + roundState, err := consensusState.GetRoundStateJSON() + if err != nil { + return nil, err + } + return &ctypes.ResultDumpConsensusState{roundState, peerRoundStates}, nil } diff --git a/rpc/core/pipe.go b/rpc/core/pipe.go index 1eb00ceeb..0bbead943 100644 --- a/rpc/core/pipe.go +++ b/rpc/core/pipe.go @@ -3,9 +3,8 @@ package core import ( "time" - "github.com/tendermint/go-crypto" + crypto "github.com/tendermint/go-crypto" "github.com/tendermint/tendermint/consensus" - cstypes "github.com/tendermint/tendermint/consensus/types" "github.com/tendermint/tendermint/p2p" "github.com/tendermint/tendermint/proxy" sm "github.com/tendermint/tendermint/state" @@ -23,7 +22,7 @@ var subscribeTimeout = 5 * time.Second type Consensus interface { GetState() sm.State GetValidators() (int64, []*types.Validator) - GetRoundState() *cstypes.RoundState + GetRoundStateJSON() ([]byte, error) } type P2P interface { diff --git a/rpc/core/types/responses.go b/rpc/core/types/responses.go index 8a6fff63c..7a31ec5d8 100644 --- a/rpc/core/types/responses.go +++ b/rpc/core/types/responses.go @@ -1,6 +1,7 @@ package core_types import ( + "encoding/json" "strings" "time" @@ -8,7 +9,6 @@ import ( crypto "github.com/tendermint/go-crypto" cmn "github.com/tendermint/tmlibs/common" - cstypes "github.com/tendermint/tendermint/consensus/types" "github.com/tendermint/tendermint/p2p" "github.com/tendermint/tendermint/state" "github.com/tendermint/tendermint/types" @@ -109,8 +109,8 @@ type ResultValidators struct { } type ResultDumpConsensusState struct { - RoundState *cstypes.RoundState `json:"round_state"` - PeerRoundStates map[p2p.ID]*cstypes.PeerRoundState `json:"peer_round_states"` + RoundState json.RawMessage `json:"round_state"` + PeerRoundStates map[p2p.ID]json.RawMessage `json:"peer_round_states"` } type ResultBroadcastTx struct { From cca1dd8e3ec50024d4912f655960f811123e0cd0 Mon Sep 17 00:00:00 2001 From: Anton Kaliaev Date: Tue, 10 Apr 2018 11:36:31 +0200 Subject: [PATCH 2/2] removed excessive comment Refs https://github.com/tendermint/tendermint/pull/1446#discussion_r180353446 --- consensus/types/state.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/consensus/types/state.go b/consensus/types/state.go index b18fbd7c0..0c3626cc7 100644 --- a/consensus/types/state.go +++ b/consensus/types/state.go @@ -52,9 +52,6 @@ func (rs RoundStepType) String() string { //----------------------------------------------------------------------------- // RoundState defines the internal consensus state. -// It should be immutable when returned from ConsensusState.GetRoundState(), but it's not. -// TODO: Actually, only the top pointer is copied, -// so access to field pointers is still racey // NOTE: Not thread safe. Should only be manipulated by functions downstream // of the cs.receiveRoutine type RoundState struct {