Skip to content
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

Revert "abci: change client to use multi-reader mutexes (#6306)" #7106

Merged
merged 1 commit into from Oct 12, 2021
Merged

Revert "abci: change client to use multi-reader mutexes (#6306)" #7106

merged 1 commit into from Oct 12, 2021

Conversation

tychoish
Copy link
Contributor

@tychoish tychoish commented Oct 12, 2021 •
edited

This reverts commit 1c4dbe3.

After looking through the (few) recent commits to v0.34x (last two releases) it seems that this is the only possible cause of the observed increased P2P workload and connection churn observed by some chains following upgrade. Everything else that's changed in the v0.34 branch is either related to statesync, or the light client, or logging related, so this is really the only thing that could cause a problem.

My working theory is that by relaxing the mutex and allowing parallelism of some previously serialized operations, some applications may speed up in ways that causes more data to get gossiped, which can impact the network. I haven't found a smoking gun, but I have no other suspects.

@tychoish tychoish added S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x S:backport-to-v0.35.x labels Oct 12, 2021
@cmwaters
Copy link
Contributor

What's the reasoning behind the reversion? Do you mind adding a description

@tychoish
Copy link
Contributor Author

What's the reasoning behind the reversion? Do you mind adding a description

Done!

tac0turtle
tac0turtle approved these changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@tac0turtle tac0turtle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you! Looking through abci I am leaning towards query being the culprit due to peer filtering.

@williambanfield
Copy link
Contributor

I'm definitely fine reverting this but am a little sad we don't have better visibility into the problem. I think better metrics and improved testnets will hopefully help us be a bit more sure of problems like this in the future.

Do we have criteria to tell if this worked?

@tychoish
Copy link
Contributor Author

I'm definitely fine reverting this but am a little sad we don't have better visibility into the problem. I think better metrics and improved testnets will hopefully help us be a bit more sure of problems like this in the future.

Agreed. And I think we can (maybe?) delete a lot of the locking of the clients in the future around ABCI++ and work with the RPC/client protocls.

Do we have criteria to tell if this worked?

Not that I know of. I think it may be possible to reproduce the bug in a testnet.

@tychoish tychoish merged commit 34a3fcd into tendermint:master Oct 12, 2021
mergify bot pushed a commit that referenced this pull request Oct 12, 2021
@tychoish
Revert "abci: change client to use multi-reader mutexes (#6306)" (#7106)
b0f877e 
This reverts commit 1c4dbe3.

(cherry picked from commit 34a3fcd)

# Conflicts:
#	abci/client/creators.go
#	abci/client/local_client.go
#	consensus/common_test.go
#	internal/consensus/byzantine_test.go
#	internal/consensus/reactor_test.go
@mergify mergify bot mentioned this pull request Oct 12, 2021
Merged
mergify bot pushed a commit that referenced this pull request Oct 12, 2021
@tychoish
Revert "abci: change client to use multi-reader mutexes (#6306)" (#7106)
e2b9c0e 
This reverts commit 1c4dbe3.

(cherry picked from commit 34a3fcd)
@mergify mergify bot mentioned this pull request Oct 12, 2021
Merged
cmwaters pushed a commit that referenced this pull request Oct 12, 2021
@mergify
Revert "abci: change client to use multi-reader mutexes (#6306)" (bac…
a82cb7d 
…kport #7106) (#7109)
tychoish pushed a commit that referenced this pull request Oct 12, 2021
@mergify
Revert "abci: change client to use multi-reader mutexes (#6306)" (bac…
b7fe214 
…kport #7106) (#7110)

* Revert "abci: change client to use multi-reader mutexes (#6306)" (#7106)

This reverts commit 1c4dbe3.

(cherry picked from commit 34a3fcd)
tychoish added a commit to tychoish/tendermint that referenced this pull request Oct 13, 2021
@tychoish
Revert "abci: change client to use multi-reader mutexes (tendermint#6306
1ba713c 
)" (tendermint#7106)

This reverts commit 1c4dbe3.
@yun-yeo yun-yeo mentioned this pull request Nov 1, 2021
Merged
8 tasks
yun-yeo added a commit to terra-money/tendermint that referenced this pull request Dec 23, 2021
@yun-yeo
Revert "Revert "abci: change client to use multi-reader mutexes (tend…
a8a273b 
…ermint#6306)" (backport tendermint#7106) (tendermint#7109)"

This reverts commit a82cb7d.
@qianxiaofeng qianxiaofeng mentioned this pull request Jan 16, 2022
Closed
@cmwaters cmwaters mentioned this pull request Feb 22, 2022
Merged
@lavabyrd lavabyrd mentioned this pull request Apr 22, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@williambanfield williambanfield williambanfield approved these changes

@alexanderbez alexanderbez alexanderbez approved these changes

@tac0turtle tac0turtle tac0turtle approved these changes

@cmwaters cmwaters Awaiting requested review from cmwaters

@creachadair creachadair Awaiting requested review from creachadair

@ebuchman ebuchman Awaiting requested review from ebuchman

Assignees
No one assigned
Labels
S:backport-to-v0.34.x Tell mergify to backport the PR to v0.34.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@tychoish @cmwaters @williambanfield @alexanderbez @tac0turtle