Skip to content

mock-consensus: 🪟 use view server in a test #3973

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
Closed
cratelyn wants to merge 10 commits into from
Closed

mock-consensus: 🪟 use view server in a test #3973

cratelyn wants to merge 10 commits into from

Conversation

@cratelyn
Copy link
Contributor

@cratelyn cratelyn commented Mar 7, 2024 •
edited
Loading

@cratelyn cratelyn added the A-mock-consensus Area: Relates to the mock consensus engine label Mar 7, 2024
@cratelyn cratelyn self-assigned this Mar 7, 2024
cratelyn
cratelyn commented Mar 7, 2024
View reviewed changes
crates/core/app/tests/mock_consensus.rs Outdated
Comment on lines 221 to 229
const VIEW_FILE_NAME: &str = "pcli-view.sqlite"; /*copied from pcli*/
let url = "http:127.0.0.1:8080".parse::<url::Url>()?;
let sqlite_path = tempdir.path().join(VIEW_FILE_NAME);
let view_service = penumbra_view::ViewServer::load_or_initialize(
Some(Utf8PathBuf::from_path_buf(sqlite_path).unwrap()),
&*test_keys::FULL_VIEWING_KEY,
url,
)
.await?;
Copy link
Contributor Author

@cratelyn cratelyn Mar 7, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

(hacky code disclaimer) 🔨

this currently fails. to my understanding this is because internally, this calls into Storage::load_or_initialize, which itself invokes QueryServiceClient::connect, here:

penumbra/crates/view/src/storage.rs

Lines 89 to 94 in a598c12

let mut client = AppQueryServiceClient::connect(node.to_string()).await?;
let params = client
.app_parameters(tonic::Request::new(AppParametersRequest {}))
.await?
.into_inner()
.try_into()?;

...which wants to connect to a AppQueryServiceServer. currently, that lives in pd, here:

penumbra/crates/bin/pd/src/main.rs

Lines 196 to 198 in a598c12

.add_service(we(AppQueryServiceServer::new(AppServer::new(
storage.clone(),
))))

i believe the correct choice forward will involve hoisting the grpc_server creation out of pd to a crate we can invoke here, does that sound correct to you as well @hdevalence?

i think i have answered my question. i should be able to keep making progress on this myself. 👍

Copy link
Contributor Author

@cratelyn cratelyn Mar 8, 2024

Choose a reason for hiding this comment

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

this led to #3980 🍒

@cratelyn cratelyn mentioned this pull request Mar 7, 2024
Closed
@cratelyn cratelyn force-pushed the kate/mock-consensus-view-server-test branch from 6e8887a to 74f7342 Compare March 8, 2024 16:02
cratelyn added 9 commits March 8, 2024 15:47
@cratelyn
refactor(pd): ⭐ hoist grpc server out of main
12ae02f 
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.
@cratelyn
grpc-server: 🤠 create empty library crate
3d1fad7
@cratelyn
grpc-server: 🍣 move constructor out to crate
f918a81 
and lo, we move the server out to the crate we made!

this will let us spin up the grpc server for use in penumbra-app tests,
so that we may run the view service and work with the Planner API.

this has another pleasant side effect, in slimming down the dependency
tree of `pd` rather substantively.
@cratelyn
pd: 👻 add a little whitespace
df5bde6 
subjective whitespace, between crate attributes and imports.
@cratelyn
app: 📦 add penumbra-view as a test dependency
5d0d16c
@cratelyn
view: 👀 tracing instrumentation for load_or_initialize
c630458
@cratelyn
tests(app): 🧪 add camino and url as test dependencies
e616d9a
@cratelyn
tests: 🔨 add mock consensus test with view server
6aa36dd
@cratelyn
test fixup blah
7f3f621
@cratelyn cratelyn force-pushed the kate/mock-consensus-view-server-test branch from 74f7342 to 7f3f621 Compare March 8, 2024 20:51
@cratelyn cratelyn mentioned this pull request Mar 8, 2024
Closed
@cratelyn cratelyn mentioned this pull request Mar 11, 2024
Merged
@cratelyn
Copy link
Contributor Author

cratelyn commented Mar 11, 2024

once i land #3996, i'll be putting this work down for a few days to focus on #3995.

cratelyn added a commit that referenced this pull request Mar 12, 2024
@cratelyn
refactor(pd): 🎬 pull grpc endpoint into penumbra-app (#3996)
86b53a5 
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.
@cratelyn cratelyn modified the milestones: Sprint 2, Sprint 3 Mar 14, 2024
@cratelyn cratelyn mentioned this pull request Mar 24, 2024
Merged
@aubrika aubrika modified the milestones: Sprint 2, Sprint 3 Mar 25, 2024
@cratelyn
Copy link
Contributor Author

cratelyn commented Apr 1, 2024

marking as blocked until work restructuring the planner has landed. (see #4141, #4081)

@TalDerei TalDerei mentioned this pull request Apr 4, 2024
Closed
@cratelyn cratelyn modified the milestones: Sprint 3, Sprint 4 Apr 8, 2024
@cratelyn cratelyn modified the milestones: Sprint 4, Sprint 5 Apr 22, 2024
@TalDerei
Copy link
Collaborator

TalDerei commented May 4, 2024

unblocked by #4312

@cratelyn cratelyn modified the milestones: Sprint 5, Sprint 6 May 7, 2024
@cratelyn
Copy link
Contributor Author

cratelyn commented May 7, 2024

this branch has been dormant for a few months. in that time, #3996 front-ran the code motion related to the grpc endpoint, and the mock_consensus.rs file was decomposed into distinct files in #4103.

i'm going to close this out, and carry this testing work forward in a new branch.

@cratelyn cratelyn closed this May 7, 2024
@cratelyn cratelyn mentioned this pull request May 8, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@cratelyn cratelyn

Labels

A-mock-consensus Area: Relates to the mock consensus engine

Projects

Penumbra
Archived in project

Milestone

Sprint 6

Development

Successfully merging this pull request may close these issues.

4 participants

@cratelyn @TalDerei @aubrika