* Fix lock sequencing in socket client request tracking.
It is not safe to check base service state (IsRunning) while holding the lock
for the client state. If we do, then during shutdown we may deadlock with the
invocation of the OnStop handler, which the base service executes while holding
the service lock.
* Enqueue pending requests before sending them to the server.
If we don't do this, the server can reply before the request lands in the
queue. That will cause the receiver to terminate early for an unsolicited
response. So enqueue first: This is safe because we're doing it in the same
routine as services the channel, so we won't take another message till we are
safely past that point.
* Document what we did.
* Fix socket paths in tests.
Prior to this change, it was possible that two client calls could enqueue their
requests in the response queue in a different order than they were processed
by the sender goroutine. This violates the requirement that responses must be
delivered in the same order they were enqueued.
To avert this, make the sender goroutine responsible for enqueuing.
Also, remove an unnecessary channel buffer.
It seems to me that by adding requests to the clients tracker (the
`reqSent` linked list), then there's no need to actually drain the
channel, becuase we will mark all of these requests as done/errored
(which propogates to users, as users never get future objects any
more), and then the GC can reap all of the request objects and the
channel accordingly.
Bumps [github.com/vektra/mockery/v2](https://github.com/vektra/mockery) from 2.10.2 to 2.10.4.
<details>
<summary>Release notes</summary>
<p><em>Sourced from <a href="https://github.com/vektra/mockery/releases">github.com/vektra/mockery/v2's releases</a>.</em></p>
<blockquote>
<h2>v2.10.4</h2>
<h2>Changelog</h2>
<ul>
<li>c943e69 Merge pull request <a href="https://github-redirect.dependabot.com/vektra/mockery/issues/441">#441</a> from cfstras/fix/support-more-env-keys</li>
<li>ed87cf6 fix: allow configuring flags with "-" as Env var</li>
<li>17abd96 fix: unused config field <code>Tags</code></li>
<li>53114cf test: add test for env var configurations</li>
</ul>
<h2>v2.10.3</h2>
<h2>Changelog</h2>
<ul>
<li>ee25bcf Add/update mocks</li>
<li>4703d1a Merge pull request <a href="https://github-redirect.dependabot.com/vektra/mockery/issues/444">#444</a> from vektra/remove_need_deps</li>
<li>ba1f213 Remove packages.NeedDeps</li>
<li>ed38b20 Update go.sum</li>
</ul>
</blockquote>
</details>
<details>
<summary>Commits</summary>
<ul>
<li><a href="c943e696a0"><code>c943e69</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/vektra/mockery/issues/441">#441</a> from cfstras/fix/support-more-env-keys</li>
<li><a href="4703d1a42f"><code>4703d1a</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/vektra/mockery/issues/444">#444</a> from vektra/remove_need_deps</li>
<li><a href="ed38b20a49"><code>ed38b20</code></a> Update go.sum</li>
<li><a href="ee25bcfcb5"><code>ee25bcf</code></a> Add/update mocks</li>
<li><a href="ba1f213e17"><code>ba1f213</code></a> Remove packages.NeedDeps</li>
<li><a href="17abd96c57"><code>17abd96</code></a> fix: unused config field <code>Tags</code></li>
<li><a href="53114cfce4"><code>53114cf</code></a> test: add test for env var configurations</li>
<li><a href="ed87cf671a"><code>ed87cf6</code></a> fix: allow configuring flags with "-" as Env var</li>
<li>See full diff in <a href="https://github.com/vektra/mockery/compare/v2.10.2...v2.10.4">compare view</a></li>
</ul>
</details>
<br />
[](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores)
Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`.
[//]: # (dependabot-automerge-start)
[//]: # (dependabot-automerge-end)
---
<details>
<summary>Dependabot commands and options</summary>
<br />
You can trigger Dependabot actions by commenting on this PR:
- `@dependabot rebase` will rebase this PR
- `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it
- `@dependabot merge` will merge this PR after your CI passes on it
- `@dependabot squash and merge` will squash and merge this PR after your CI passes on it
- `@dependabot cancel merge` will cancel a previously requested merge and block automerging
- `@dependabot reopen` will reopen this PR if it is closed
- `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually
- `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself)
- `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself)
</details>
* Outstanding abci-gen changes to 'pb.go' files
* Removed modified_tx_status from spec and protobufs
* Fix sed for OSX
* Regenerated abci protobufs with 'abci-proto-gen'
* Code changes. UTs e2e tests passing
* Recovered UT: TestPrepareProposalModifiedTxStatusFalse
* Adapted UT
* Fixed UT
* Revert "Fix sed for OSX"
This reverts commit e576708c61.
* Update internal/state/execution_test.go
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
* Update abci/example/kvstore/kvstore.go
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
* Update internal/state/execution_test.go
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
* Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
* Addressed some comments
* Added one test that tests error at the ABCI client + Fixed some mock calls
* Addressed remaining comments
* Update abci/example/kvstore/kvstore.go
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
* Update abci/example/kvstore/kvstore.go
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
* Update abci/example/kvstore/kvstore.go
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
* Update spec/abci++/abci++_tmint_expected_behavior_002_draft.md
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
* Addressed William's latest comments
* Adressed Michael's comment
* Fixed UT
* Some md fixes
* More md fixes
* gofmt
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
Co-authored-by: M. J. Fromberger <fromberger@interchain.io>
This change set implements the most recent version of `FinalizeBlock`.
# What does this change actually contain?
* This change set is rather large but fear not! The majority of the files touched and changes are renaming `ResponseDeliverTx` to `ExecTxResult`. This should be a pretty inoffensive change since they're effectively the same type but with a different name.
* The `execBlockOnProxyApp` was totally removed since it served as just a wrapper around the logic that is now mostly encapsulated within `FinalizeBlock`
* The `updateState` helper function has been made a public method on `State`. It was being exposed as a shim through the testing infrastructure, so this seemed innocuous.
* Tests already existed to ensure that the application received the `ByzantineValidators` and the `ValidatorUpdates`, but one was fixed up to ensure that `LastCommitInfo` was being sent across.
* Tests were removed from the `psql` indexer that seemed to search for an event in the indexer that was not being created.
# Questions for reviewers
* We store this [ABCIResponses](5721a13ab1/proto/tendermint/state/types.pb.go (L37)) type in the data base as the block results. This type has changed since v0.35 to contain the `FinalizeBlock` response. I'm wondering if we need to do any shimming to keep the old data retrieveable?
* Similarly, this change is exposed via the RPC through [ResultBlockResults](5721a13ab1/rpc/coretypes/responses.go (L69)) changing. Should we somehow shim or notify for this change?
closes: #7658
This change implements the spec for `ProcessProposal`. It first calls the Tendermint block validation logic to check that all of the proposed block fields are well formed and do not violate any of the rules for Tendermint to consider the block valid and then passes the validated block the `ProcessProposal`.
This change also adds additional fixtures to test the change. It adds the `baseMock` types that holds a mock as well as a reference to `BaseApplication`. If the function was not setup by the test on the contained mock Application, the type delegates to the `BaseApplication` and returns what `BaseApplication` returns.
The change also switches the `makeState` helper to take an arg struct so that an ABCI application can be plumbed through when needed.
closes: #7656
Closes#7073
As part of the 0.36 cycle we've discussed and decided to remove the mutex in tendermint that protects the ABCI application. First, applications should be able to be responsible for their own concurrency control, and can make more fine-grained decisions about concurrent use than tendermint ever could. Second, I've observed in recent weeks as we've been making this change that the mutex wasn't applied particularly consistently in many cases (e.g. multiple "local" connections to the application had multiple locks, etc.) so this will give more consistent experiences across ABCI execution environments, and simplifies the tendermint ABCI handling code.
While I'd hoped to be able to make the socket client less weird, I
think that this is a nice middle ground in terms of improving
readability and removing the vestigal components without breaking
anything or radically changing the underlying assumptions.
In the future we'd want to have requests be identified by a request
ID, and then we could drop the request tracking logic in the client
entirely, and this is protocol breaking. The alternatives aren't
substantively different than the current implementation.
This change changes the ABCI socket client to allow goroutines to block writing to the internal queue. This has the effect ensuring that callers of the ABCI methods do not error on a full internal queue at the expense of allowing the number of goroutines waiting on this internal queue to grow in an unbounded fashion. This tradeoff seems preferable since it allows callers of the ABCI methods to be certain that a request that was made will reach the application if it is available.
Closes: #7827
This change was initially implemented here: e13b4386ff and never landed on v0.34, only v0.35+
This follows along in the spirit of #7845 but is orthogonal to
removing `CheckTxAsync` (which will come after the previous commit
lands,) so I thought I'd get it out there earlier.
* Rebased and git-squashed the commits in PR #6546
migrate abci to finalizeBlock
work on abci, proxy and mempool
abciresponse, blok events, indexer, some tests
fix some tests
fix errors
fix errors in abci
fix tests amd errors
* Fixes after rebasing PR#6546
* Restored height to RequestFinalizeBlock & other
* Fixed more UTs
* Fixed kvstore
* More UT fixes
* last TC fixed
* make format
* Update internal/consensus/mempool_test.go
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
* Addressed @williambanfield's comments
* Fixed UTs
* Addressed last comments from @williambanfield
* make format
Co-authored-by: marbar3778 <marbar3778@yahoo.com>
Co-authored-by: William Banfield <4561443+williambanfield@users.noreply.github.com>
This change has two main effects:
1. Remove most of the Async methods from the abci.Client interface.
Remaining are FlushAsync, CommitTxAsync, and DeliverTxAsync.
2. Rename the synchronous methods to remove the "Sync" suffix.
The rest of the change is updating the implementations, subsets, and mocks of
the interface, along with the call sites that point to them.
* Fix stringly-typed mock stubs.
* Rename helper method.
This averts a log-after-close issue. We should probably also chase the shutdown
issues, but since ABCI clients should generally only shut down once per process
I don't think this is a real priority, and the trace is hairy.
This continues the push of plumbing contexts through tendermint. I
attempted to find all goroutines in the production code (non-test) and
made sure that these threads would exit when their contexts were
canceled, and I believe this PR does that.
Rework the implementation of event query parsing and execution to
improve performance and reduce memory usage.
Previous memory and CPU profiles of the pubsub service showed query
processing as a significant hotspot. While we don't have evidence that
this is visibly hurting users, fixing it is fairly easy and self-contained.
Updates #6439.
Typical benchmark results comparing the original implementation (PEG) with the reworked implementation (Custom):
```
TEST TIME/OP BYTES/OP ALLOCS/OP SPEEDUP MEM SAVING
BenchmarkParsePEG-12 51716 ns 526832 27
BenchmarkParseCustom-12 2167 ns 4616 17 23.8x 99.1%
BenchmarkMatchPEG-12 3086 ns 1097 22
BenchmarkMatchCustom-12 294.2 ns 64 3 10.5x 94.1%
```
Components:
* Add a basic parsing benchmark.
* Move the original query implementation to a subdirectory.
* Add lexical scanner for Query expressions.
* Add a parser for Query expressions.
* Implement query compiler.
* Add test cases based on OpenAPI examples.
* Add MustCompile to replace the original MustParse, and update usage.
This is a very small change, but removes a method from the
`service.Service` interface (a win!) and forces callers to explicitly
pass loggers in to objects during construction rather than (later)
injecting them. There's not a real need for this kind of lazy
construction of loggers, and I think a decent potential for confusion
for mutable loggers.
The main concern I have is that this changes the constructor API for
ABCI clients. I think this is fine, and I suspect that as we plumb
contexts through, and make changes to the RPC services there'll be a
number of similar sorts of changes to various (quasi) public
interfaces, which I think we should welcome.
The main effect of this change is to flush the socket client and server message
encoding buffers immediately once the message is fully and correctly encoded.
This allows us to remove the timer and some other special cases, without
changing the observed behaviour of the system.
-- Background
The socket protocol client and server each use a buffered writer to encode
request and response messages onto the underlying connection. This reduces the
possibility of a single message being split across multiple writes, but has the
side-effect that a request may remain buffered for some time.
The implementation worked around this by keeping a ticker that occasionally
triggers a flush, and by flushing the writer in response to an explicit request
baked into the client/server protocol (see also #6994).
These workarounds are both unnecessary: Once a message has been dequeued for
sending and fully encoded in wire format, there is no real use keeping all or
part of it buffered locally. Moreover, using an asynchronous process to flush
the buffer makes the round-trip performance of the request unpredictable.
-- Benchmarks
Code: https://play.golang.org/p/0ChUOxJOiHt
I found no pre-existing performance benchmarks to justify the flush pattern,
but a natural question is whether this will significantly harm client/server
performance. To test this, I implemented a simple benchmark that transfers
randomly-sized byte buffers from a no-op "client" to a no-op "server" over a
Unix-domain socket, using a buffered writer, both with and without explicit
flushes after each write.
As the following data show, flushing every time (FLUSH=true) does reduce raw
throughput, but not by a significant amount except for very small request
sizes, where the transfer time is already trivial (1.9μs). Given that the
client is calibrated for 1MiB transactions, the overhead is not meaningful.
The percentage in each section is the speedup for flushing only when the buffer
is full, relative to flushing every block. The benchmark uses the default
buffer size (4096 bytes), which is the same value used by the socket client and
server implementation:
FLUSH NBLOCKS MAX AVG TOTAL ELAPSED TIME/BLOCK
false 3957471 512 255 1011165416 2.00018873s 505ns
true 1068568 512 255 273064368 2.000217051s 1.871µs
(73%)
false 536096 4096 2048 1098066401 2.000229108s 3.731µs
true 477911 4096 2047 978746731 2.000177825s 4.185µs
(10.8%)
false 124595 16384 8181 1019340160 2.000235086s 16.053µs
true 120995 16384 8179 989703064 2.000329349s 16.532µs
(2.9%)
false 2114 1048576 525693 1111316541 2.000479928s 946.3µs
true 2083 1048576 526379 1096449173 2.001817137s 961.025µs
(1.5%)
Note also that the FLUSH=false baseline is actually faster than the production
code, which flushes more often than is required by the buffer filling up.
Moreover, the timer slows down the overall transaction rate of the client and
server, indepenedent of how fast the socket transfer is, so the loss on a real
workload is probably much less.
This change aims to keep versions of mockery consistent across developer laptops.
This change adds mockery to the `tools.go` file so that its version can be managed consistently in the `go.mod` file.
Additionally, this change temporarily disables adding mockery's version number to generated files. There is an outstanding issue against the mockery project related to the version string behavior when running from `go get`. I have created a pull request to fix this issue in the mockery project.
see: https://github.com/vektra/mockery/issues/397
There are many `//go:generate mockery` lines in the source code.
This change adds a make target to invoke these mock generations.
This change also invokes the mock invocations and adds the resulting mocks to the repo.
Related to #5274
## Description
Internalize some libs. This reduces the amount ot public API tendermint is supporting. The moved libraries are mainly ones that are used within Tendermint-core.
