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.

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

abci: Adapt unsynchronized local client to replicate remote client concurrency #9830

Merged
merged 14 commits into from
Dec 16, 2022

Conversation

thanethomson
Copy link
Contributor

After a prior discussion with @cmwaters, we decided that it's probably better in the short-term to offer a less flexible concurrency model than what I introduced in #9660, instead effectively replicating the concurrency model of the remote client (i.e. the socket client) for local clients.

This therefore reverts #9660 and simply creates a new ABCI client creator that does not hold a single lock over all created ABCI client interfaces.

This still, however, extends the E2E tests to randomly select either the local client creator with a global lock, or the new unsynchronized local client creator. This selection only applies to generated E2E tests that use the builtin app.

This is also introduced in such a way that we can still backport it to v0.37 and v0.34 in a non-breaking way.


PR checklist

  • Tests written/updated, or no tests needed
  • CHANGELOG_PENDING.md updated, or no changelog entry needed
  • Updated relevant documentation (docs/) and code comments, or no
    documentation updates needed

@thanethomson thanethomson added the C:abci Component: Application Blockchain Interface label Dec 3, 2022
@thanethomson thanethomson requested a review from a team December 3, 2022 13:49
@thanethomson
Copy link
Contributor Author

thanethomson commented Dec 4, 2022

This toy program attempts to demonstrate the problem with the current local client creator (queries can block core consensus functions), as well as how this solution changes the concurrency model: https://go.dev/play/p/umBdTfA5ohF

Copy link
Contributor

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

I like this "unsyc flavor" more as well, since it's somehow consistent with the socket protocol.

test/e2e/pkg/manifest.go Outdated Show resolved Hide resolved
test/e2e/pkg/manifest.go Outdated Show resolved Hide resolved
This reverts commit 45071d1.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Extend the E2E tests to randomly choose between the sync (default) and
unsync (new) local client creator.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
Signed-off-by: Thane Thomson <connect@thanethomson.com>
The approach of randomly generating the proxy mode across testnets
resulted in a totally uneven ratio of sync to unsync modes for all
testnets that happened to have a protocol of "builtin".

This commit adapts the E2E tests to have a new ABCI protocol option:
"builtin_unsync". This results in a better spread of sync/unsync choices
for generated testnets.

Signed-off-by: Thane Thomson <connect@thanethomson.com>
@thanethomson
Copy link
Contributor Author

thanethomson commented Dec 14, 2022

I've changed the approach here to one where, instead of having a separate manifest option for the builtin proxy mode, we just have an additional ABCIProtocol option of builtin_unsync (in addition to builtin, tcp, etc.).

The first approach I chose unfortunately resulted in all generated testnets whose ABCIProtocol was builtin also happening to have a BuiltinProxyMode of unsync, effectively making all of our nightly E2E tests exclusively run with the unsync option (seems like it was just how the dice rolled unfortunately).

This new approach better balances out the ratio of sync:unsync variations when running the generator:

> grep 'builtin' gen-group0*.toml
gen-group00-0000.toml:abci_protocol = "builtin"
gen-group00-0001.toml:abci_protocol = "builtin_unsync"
gen-group01-0005.toml:abci_protocol = "builtin"
gen-group02-0000.toml:abci_protocol = "builtin"
gen-group02-0001.toml:abci_protocol = "builtin"
gen-group02-0003.toml:abci_protocol = "builtin_unsync"
gen-group02-0004.toml:abci_protocol = "builtin"
gen-group03-0000.toml:abci_protocol = "builtin"
gen-group03-0001.toml:abci_protocol = "builtin"
gen-group04-0000.toml:abci_protocol = "builtin_unsync"
gen-group04-0002.toml:abci_protocol = "builtin_unsync"

I've triggered a manual E2E run here for the newest code in this PR.

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

@sergio-mena sergio-mena left a comment

Choose a reason for hiding this comment

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

Love the new approach

@thanethomson thanethomson merged commit 6878b38 into main Dec 16, 2022
@thanethomson thanethomson deleted the thane/abci-unsync-client branch December 16, 2022 15:19
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Apr 25, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Apr 25, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jun 30, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jul 2, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jul 2, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jul 2, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jul 3, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Jul 11, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 30, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 30, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 30, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 30, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 30, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 30, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 30, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 31, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 31, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 31, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Aug 31, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
matthewdowney pushed a commit to matthewdowney/cometbft that referenced this pull request Aug 31, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Dec 4, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Dec 4, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
lcwik added a commit to dydxprotocol/cometbft that referenced this pull request Dec 4, 2023
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Feb 20, 2024
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Apr 9, 2024
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Apr 9, 2024
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Apr 14, 2024
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Apr 16, 2024
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Apr 16, 2024
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Apr 16, 2024
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
roy-dydx pushed a commit to dydxprotocol/cometbft that referenced this pull request Apr 22, 2024
This is effectively a copy of the fully unsynchronized local client found in tendermint/tendermint#9660. Note that this was reverted and replaced by a version that has a mutex per instance of the client in tendermint/tendermint#9830. This change can be removed once a fully unsynchronized client is added back.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:abci Component: Application Blockchain Interface
Projects
Status: Done/Merged
Development

Successfully merging this pull request may close these issues.

None yet

2 participants