Design a way to use the Rust view server with mock consensus integration tests #3913
Comments
cratelyn
added a commit
that referenced
this issue
Mar 5, 2024
see #3913. this is a speculative hack at providing a `ViewClient` implementation for the mock client. for now, only `todo!()` stubs are provided.
|
@hdevalence i prepared a scaffold in #3958, taking a small preparatory step towards using |
|
#3896 is loosely related to this work. |
This was referenced Mar 7, 2024
cratelyn
added a commit
that referenced
this issue
Mar 8, 2024
code motion, part 1 of 2. see #3913. we want to run a view client in application tests, so that we can use the `Planner` to build transactions. in order to create a view client, we need to run a grpc server that it can talk to. NB: we leave the "expensive" simulation service and the tendermint proxy out of this function for now.
This was referenced Mar 8, 2024
cratelyn
added a commit
that referenced
this issue
Mar 12, 2024
see #3913, #3973 and #3588. this is a second attempt, following up on #3980. #### 🔭 background NB: the difference between this and #3679 is that the latter (_which ran afoul of a regression_) would have `penumbra-app` create a `Routes`, that we would [add](https://github.com/penumbra-zone/penumbra/pull/3679/files#diff-fbc4204ceb976c8cb30ed06168e2476700bae21bfd803e26281b2d026194d430R204) to the builder (_which stays in `pd`_). here, i'm not trying to make that cut between `Router` and `Routes`, and am attempting to hoist the whole thing out of `pd`, without making changes to how we interact with `tonic`. my aim is for us to be able to move this, without running into that bug (#3697) again. NB: after running into problems in #3980, i found a way to easily reproduce the issue locally. my belief was that something related to our dependencies' cargo features was at play. rather than isolate the issue, it was easier to rewrite this (_it's just code motion, after all_) while running some of the network integration tests in a loop. unlike #3980, this moves the rpc server into `penumbra-app`, per #3980 (comment) #### 👁️ overview we would like to use the rust view server in mock consensus tests. in order to run the `penumbra_view::ViewServer` however, we need to spin up the corresponding grpc endpoint for it to connect to. this branch performs a bit of code motion, moving the `grpc_server` out of `pd` and into `penumbra-app`. there will likely be other functional changes to the code in question before we can use it in those tests, but this PR is interested in moving that code into a place where our tests can rely upon it.
1 task
cratelyn
added a commit
that referenced
this issue
May 8, 2024
## describe your changes this introduces some additional `tracing` instrumentation, used while debugging integration with the view server in mock consensus tests. because these events may be useful more broadly, and aren't strictly tied to testing, this is cherry-picked out of that work for separate consideration. ## issue ticket number and link * #3913 ## checklist before requesting a review - [x] if this code contains consensus-breaking changes, i have added the "consensus-breaking" label. otherwise, i declare my belief that there are not consensus-breaking changes, for the following reason: this branch only makes changes to tracing spans and events. as this is only altering telemetry, this is not consensus-breaking. --------- Co-authored-by: Henry de Valence <[email protected]>
cratelyn
added a commit
that referenced
this issue
May 22, 2024
`check_worker()` is called by almost every public interface in the view server, as a preliminary check that the worker task running in the background has not recorded an error of some kind. this commit makes a change to `check_worker()`, so that it does not need to frivolously acquire, release, and subsequently reacquire the mutex in the event of an error being stored in the slot. tracing events and documentation are also added to this function. x-ref: #3913
1 task
cratelyn
added a commit
that referenced
this issue
May 22, 2024
these are useful for the sake of following the sequence of function calls when debugging issues related to the view server, the view server worker, and integration tests making use of the `Planner`. x-ref: #3913
1 task
cratelyn
added a commit
that referenced
this issue
May 22, 2024
`check_worker()` is called by almost every public interface in the view server, as a preliminary check that the worker task running in the background has not recorded an error of some kind. this commit makes a change to `check_worker()`, so that it does not need to frivolously acquire, release, and subsequently reacquire the mutex in the event of an error being stored in the slot. tracing events and documentation are also added to this function. x-ref: #3913
cratelyn
added a commit
that referenced
this issue
May 22, 2024
these are useful for the sake of following the sequence of function calls when debugging issues related to the view server, the view server worker, and integration tests making use of the `Planner`. x-ref: #3913
cratelyn
added a commit
that referenced
this issue
May 22, 2024
`check_worker()` is called by almost every public interface in the view server, as a preliminary check that the worker task running in the background has not recorded an error of some kind. this commit makes a change to `check_worker()`, so that it does not need to frivolously acquire, release, and subsequently reacquire the mutex in the event of an error being stored in the slot. tracing events and documentation are also added to this function. x-ref: #3913
cratelyn
added a commit
that referenced
this issue
May 22, 2024
these are useful for the sake of following the sequence of function calls when debugging issues related to the view server, the view server worker, and integration tests making use of the `Planner`. x-ref: #3913
1 task
cratelyn
added a commit
that referenced
this issue
May 24, 2024
#### 💭 describe your changes instead of accepting a `url::Url` and constructing specifically the "real" `TendermintProxy`, tweak the signature of the router function so that it accepts a `T: TendermintProxyService`. in the future, this will allow mock consensus test cases to run an rpc endpoint for the view server, without running a Tendermint/CometBFT process. #### 🔖 issue ticket number and link see #3913. #### ✅ checklist before requesting a review - [x] if this code contains consensus-breaking changes, i have added the "consensus-breaking" label. otherwise, i declare my belief that there are not consensus-breaking changes, for the following reason: > this is a like-for-like refactoring, changing a function > signature.
1 task
cratelyn
added a commit
that referenced
this issue
May 24, 2024
#### 💭 describe your changes instead of accepting a `url::Url` and constructing specifically the "real" `TendermintProxy`, tweak the signature of the router function so that it accepts a `T: TendermintProxyService`. in the future, this will allow mock consensus test cases to run an rpc endpoint for the view server, without running a Tendermint/CometBFT process. #### 🔖 issue ticket number and link see #3913. #### ✅ checklist before requesting a review - [x] if this code contains consensus-breaking changes, i have added the "consensus-breaking" label. otherwise, i declare my belief that there are not consensus-breaking changes, for the following reason: > this is a like-for-like refactoring, changing a function > signature.
1 task
1 task
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Is your feature request related to a problem? Please describe.
Currently we are trying to expand the capability of our state machine tests, going depth-first: writing a few, targeted tests that incrementally expand the capability of the test harness and allow us to discover requirements.
These tests are intended to exercise functionality end to end, including client logic. Currently we do this with the MockClient, a minimal reference implementation of key Penumrba client logic.
However, a benefit and limitation of this approach is that we cannot automatically generate transaction plans, because the MovkClient cannot be used with a Planner. This is a benefit because we can write tests that are very precise about their state changes and do not rely on a much larger mass of code, but a limitation because more complex test logic would prefer to use the planner.
Additional context here: #3891 (comment)
Describe the solution you'd like
There are a lot of moving pieces and a lot of context to consider to figure out the right design. @cratelyn and I should pair to explore the best path forward.
🔗 related work
ViewClient(work in progress) #3958Option::expectcall #3971penumbra-app#3996error_slotmutex #4434ViewServicemethods #4435tendermint-*compatibility #4447rpc::router(..)accepts aTendermintProxyService#4460TransactionPlan::with_populated_detection_data()#4469TestNode<C>isSend + Sync#4470Plannercan spend notes via mock consensus #4471The text was updated successfully, but these errors were encountered: