8000 consensus: rename (*PeerState).ToJSON to MarshalJSON (backport #524) by mergify[bot] · Pull Request #532 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Mar 15, 2023
Merged

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

merged 2 commits into from
Mar 15, 2023

Conversation

mergify[bot]
Copy link
Contributor
@mergify mergify bot commented Mar 14, 2023

This is an automatic backport of pull request #524 done by Mergify.
Cherry-pick of 587bc0b has failed:

On branch mergify/bp/v0.37.x/pr-524
Your branch is up to date with 'origin/v0.37.x'.

You are currently cherry-picking commit 587bc0b18.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   consensus/reactor.go
	modified:   rpc/core/consensus.go

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	added by them:   .changelog/v0.37.0/bug-fixes/524-rename-peerstate-tojson.md

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally

Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

@mark-rushakoff @mergify
consensus: rename (*PeerState).ToJSON to MarshalJSON (#524)
592a6f3 
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

(cherry picked from commit 587bc0b)

# Conflicts:
#	.changelog/v0.37.0/bug-fixes/524-rename-peerstate-tojson.md
@mergify mergify bot requested a review from a team as a code owner March 14, 2023 15:55
@mergify mergify bot added the conflicts label Mar 14, 2023
@thanethomson
Move changelog entry to correct folder
25919ed 
Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson thanethomson merged commit 39eeb8b into v0.37.x Mar 15, 2023
@thanethomson thanethomson deleted the mergify/bp/v0.37.x/pr-524 branch March 15, 2023 13:09
mark-rushakoff added a commit to cosmos/cosmos-sdk that referenced this pull request Apr 11, 2023
@mark-rushakoff
build(deps): use latest commit of CometBFT v0.37.x branch
9229042 
This includes two fixes for two data races that occasionally happen in
Cosmos SDK tests: cometbft/cometbft#690 and
cometbft/cometbft#532.

We will eventually be switching to CometBFT v0.38 (see #15519), which
also contains those fixes.
@mark-rushakoff mark-rushakoff mentioned this pull request Apr 11, 2023
Merged
8 tasks
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jul 11, 2023
@mergify @mark-rushakoff
@thanethomson @roy-dydx
consensus: rename (*PeerState).ToJSON to MarshalJSON (backport cometb…
246ed98 
…ft#524) (cometbft#532)

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

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

(cherry picked from commit 587bc0b)

# Conflicts:
#	.changelog/v0.37.0/bug-fixes/524-rename-peerstate-tojson.md

* Move changelog entry to correct folder

Signed-off-by: Thane Thomson <connect@thanethomson.com>

---------

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Co-authored-by: Mark Rushakoff <mark.rushakoff@gmail.com>
Co-authored-by: Thane Thomson <connect@thanethomson.com>
@faddat faddat mentioned this pull request Jan 5, 2024
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thanethomson thanethomson thanethomson approved these changes

@cason cason cason approved these changes

Assignees
No one assigned
Labels
conflicts
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thanethomson @cason @mark-rushakoff
0