pmonitor: create client activity monitor #4844

redshiftzero · 2024-08-29T21:46:51Z

Describe your changes

This PR adds a tool pmonitor that will enable one to monitor the activity of a list of FullViewingKeys

Plan:

Make a new tool pmonitor (because we need to use pclientd or pcli which assumes a single view key)
Load a JSON file containing a array of FullViewingKeys
Build a directory tree that has pcli/pclientd home directories that have view databases and have the current state
One off scanning of the compact block with each FVK to determine the genesis allocation for that FVK
For each FVK, check the current UM-equivalent balance for the balance is greater than or equal to the genesis balance
Print each FVKs, their genesis balance, and their current UM-equivalent balance
Handle migrated accounts

Issue ticket number and link

Will close #4832

Test

Point to the desired RPC and provide a JSON array of FVKs: cargo run --bin pmonitor init --fvks test.json --grpc-url https://yourtestnetrpchere.example.com

Expected state: you should have in your zone.penumbra.pmonitor home directory (stored next to wherever your pcli default homedir is, so on my Macbook it's "/Users/redshiftzero/Library/Application Support/zone.penumbra.pmonitor") a:

config file
for each FVK, a wallet_{uuid} directory

Run cargo run --bin pmonitor audit:

Syncing wallet: /Users/redshiftzero/Library/Application Support/zone.penumbra.pmonitor/wallet_3
Scanning blocks from last sync height 286483 to latest height 286483
[0s] ██████████████████████████████████████████████████       0/0       0/s ETA: 0s
Wallet synced successfully
FVK: penumbrafullviewingkey1[...snip...]
Genesis UM-equivalent balance: 0
Current UM-equivalent balance: 2999999066
✅ Expected balance! Balance is greater than or equal to the genesis balance
...

This will first sync all the wallets, then check the balances. Subsequent executions of the audit command will start with the state from the previous run.

Note: To reset after testing:
cargo run --bin pmonitor reset

Checklist before requesting a review

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 client-side tool only

conorsch · 2024-09-10T21:32:20Z

We just shipped the pcli-migrate-balance functionality (#4831), which enables full-wallet transfers, based on FVK. The pmonitor tool should recognize a migrated balance, which can be identified by a hardcoded memo: https://github.com/penumbra-zone/penumbra/blob/v0.80.4/crates/bin/pcli/src/command/migrate.rs#L105

aubrika · 2024-09-17T23:07:27Z

Sounds like this will be wrapped up or close this week, and since this is a read-only op, whoever reviews this should be sure to test against both testnet and mainnet. I can help generate a list of FVKs and checking the implementation should be straightforward from there.

redshiftzero · 2024-09-20T17:10:59Z

status: this now has support for migrated accounts, but needs testing before marking ready for review

conorsch · 2024-09-23T20:14:14Z

I'll take a first pass at review here, confirming that the tool 1) correctly identifies when accounts have been spent out of; and 2) supports the pcli-migrate-balance flow.

conorsch

In terms of functionality, the tool appears to work well: in all the testing I've done, it correctly notates when balance is absent. Still more interactive testing to perform, but the implementation seems sound!

UX-wise, however, the tool obscures detection of misbehavior: any indication of a misbehavior is likely lost to scrollback in the default stdout, where there are more than, say, 5 FVKs to scan. Worse, the tool exits 0 regardless of whether misbehavior was detected. Suggestions to resolve:

emit a "summary" message at the end of the audit run, notating explicitly if any accounts were found to have funds missing;
designate a specific exit code, e.g. 10, for signaling that misbehavior was detected; that will allow the tool to be run in headless environments on a schedule, without resorting to string parsing;
(optional) convert the per-account messaging to tracing-subscriber statements, subject to RUST_LOG tweaking, to enable focusing on the summary message.

I left some in-line comments throughout, and also pushed up a script I've been using locally to test on a devnet. The biggest complaint I have about the technical bits are that the tool assumes it can cargo run --bin pcli, which I'd prefer to address before merge.

conorsch · 2024-09-26T22:51:03Z

crates/bin/pmonitor/src/main.rs

+        }
+
+        // Invoke pcli to initialize the wallet (hacky)
+        let output = ProcessCommand::new("cargo")


Let's not assume that we have a full rust env and the git repo relative to the cwd for the command. Once the pmonitor binary is built, it should have everything it needs to interact with external services and perform the check, regardless of where it's executed.

Looks like not everything we need is already pub in pcli, but could be. Also willing to default to calling pcli directly, rather than cargo run --bin pcli, because then we can at least error out if not found and provide a helpful error msg.

conorsch · 2024-09-26T22:57:53Z

crates/view/src/storage.rs

-                    height_created = excluded.height_created, 
-                    address_index = excluded.address_index, 
-                    source = excluded.source, 
-                    height_spent = excluded.height_spent, 


Note to self: there's whitespace trimming in these SQL statements, but also a substantive schema change below.

conorsch · 2024-09-26T23:01:33Z

crates/view/src/storage/schema.sql

@@ -54,7 +54,8 @@ CREATE TABLE tx (
    tx_hash                 BLOB PRIMARY KEY NOT NULL,
    tx_bytes                BLOB NOT NULL,
    block_height            BIGINT NOT NULL,
-    return_address          BLOB
+    return_address          BLOB,
+    memo_text               TEXT


Schema change! This breaks existing clients. Testing locally against testnet, I do indeed see the error message:

Error: can't load view database created by client version 0.80.5 using client version 0.80.5: they have different schemata, so you need to reset your view database and resynchronize by running pcli view reset

That's probably tolerable for a point-release, but we should be careful about downstream consequences. I'm thinking in particular of relayers like hermes, which will likely fail when their deps are upgraded to a more recent view crate. I see we already merged a schema change in #4801 so we'll need to accommodate for the change already anyway. Relevant for rebasing the hermes fork, cc @zbuc.

conorsch · 2024-09-26T23:03:18Z

crates/bin/pmonitor/src/main.rs

+                    if total_um_equivalent_amount < genesis_um_equivalent_amount {
+                        println!("✘ Unexpected balance! Balance is less than the genesis balance");
+                    } else {
+                        println!("✅ Expected balance! Balance is greater than or equal to the genesis balance");


Love the emojis, makes the wall of text a bit easier to scan visually. I only tested with 10 FVKs, though, and most of the relevant content was off the screen. Maybe tracing statements would be more appropriate here.

conorsch · 2024-10-07T18:46:14Z

Rebased on latest main, and added a handful of commits. Logging is now more verbose, but still TK are summary message and custom exit codes. During testing, I noticed that the tool will report an error if an account was migrated: we should refactor the audit logic to continue searching the migrated account, without reporting an error, or else explicitly state that a new migration was detected and therefore the tool should be rerun.

Previously if an FVK was in the monitoring list that did not have a genesis allocation, we'd bail out. Instead, let's just note that its genesis balance is 0, and continue on.

Prefers `tracing_subscriber` logging over standard `println` messages, so that verbosity of output is customizable. Sorts the deps in pmonitor manifest. refactor: remove unwraps

Pretty sure this is strictly necessary: in order to detect the substring message in a memo text, we must perform the sql query via `sqlx` by providing wildcard characters like `%`. Updated query calls and associated docstrings.

The mismatch between view db filenames was causing some confusing behavior. Updated for consistency between both pcli and the pmonitor view service.

Uses `colorize` dep for pretty-printing human-readable message.

Adds automated testing of common use cases for the `pmonitor` FVK audit tool. The cargo tests are adapted from a one-off bash script written to aid in review of the original pmonitor PR. ci: add pmonitor integration workflow ci: adjust smoke concurrency ci: load rust cache

conorsch · 2024-10-11T21:48:38Z

Looking great. I added some tests, and made some changes to implement the requested features, namely 1) summary message at end of run, and 2) exiting non-zero on failure.

Example output from passing `audit`

Example output from failing `audit`

Also tacked on some module docs to explain how it's intended to be used. Haven't added any content to the guide, but we can do that in a follow up if it's useful.

redshiftzero · 2024-10-16T17:33:58Z

thank you for adding these changes @conorsch, I've reviewed them (as of eec9d25) and they LGTM

redshiftzero temporarily deployed to smoke-test August 29, 2024 21:46 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test August 29, 2024 22:36 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 4, 2024 19:36 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 6, 2024 18:27 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 13, 2024 03:39 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 13, 2024 18:10 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 13, 2024 21:46 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 16, 2024 16:04 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 16, 2024 17:15 — with GitHub Actions Inactive

redshiftzero force-pushed the client-activity branch from 0af76e5 to 7316cda Compare September 16, 2024 20:04

redshiftzero temporarily deployed to smoke-test September 16, 2024 20:04 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 16, 2024 20:48 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 20, 2024 15:20 — with GitHub Actions Inactive

conorsch self-requested a review September 23, 2024 20:11

conorsch changed the title ~~wip: add client activity monitor~~ pmonitor: add client activity monitor Sep 23, 2024

conorsch changed the title ~~pmonitor: add client activity monitor~~ pmonitor: create client activity monitor Sep 23, 2024

redshiftzero temporarily deployed to smoke-test September 24, 2024 18:01 — with GitHub Actions Inactive

redshiftzero temporarily deployed to smoke-test September 24, 2024 20:38 — with GitHub Actions Inactive

conorsch temporarily deployed to smoke-test September 26, 2024 22:40 — with GitHub Actions Inactive

conorsch marked this pull request as ready for review September 26, 2024 22:46

conorsch requested changes Sep 26, 2024

View reviewed changes

conorsch mentioned this pull request Oct 4, 2024

Release v0.80.6, via point-release #4882

Closed

7 tasks

conorsch force-pushed the client-activity branch from cdcb279 to 45b91cd Compare October 7, 2024 18:44

conorsch temporarily deployed to smoke-test October 7, 2024 18:44 — with GitHub Actions Inactive

conorsch added the pr-needs-testing Before being mergeable, this PR should add tests to the code, or have a human perform tests label Oct 7, 2024

redshiftzero and others added 18 commits October 11, 2024 14:38

pmonitor: check if account has been migrated

db5bcb7

pmonitor: add PmonitorConfig so we can track migrations

e4200b9

pmonitor: refactor audit command to use PmonitorConfig

967bccb

pmonitor: refactor wallet creation into method

80c53ec

pmonitor: move genesis balance computation into init

99146de

pmonitor: support balance tracking for migrated accounts

5476994

pmonitor: allow FVKs to be included that do not have genesis allocs

8b4abae

Previously if an FVK was in the monitoring list that did not have a genesis allocation, we'd bail out. Instead, let's just note that its genesis balance is 0, and continue on.

pmonitor: more friendly messages during init

e507f14

feat: add tracing log messages for pmonitor

49e7b27

Prefers `tracing_subscriber` logging over standard `println` messages, so that verbosity of output is customizable. Sorts the deps in pmonitor manifest. refactor: remove unwraps

fix: sql memo query

b9ae61f

Pretty sure this is strictly necessary: in order to detect the substring message in a memo text, we must perform the sql query via `sqlx` by providing wildcard characters like `%`. Updated query calls and associated docstrings.

fix: use VIEW_FILE_NAME const in pmonitor

a4564fc

The mismatch between view db filenames was causing some confusing behavior. Updated for consistency between both pcli and the pmonitor view service.

fix: don't shell out to pcli for wallet init

2f773c2

fix: allow small deviation from genesis, for fees

cc53601

fix: reuse view client in pmonitor

953e481

feat: summary message for pmonitor

b456565

Uses `colorize` dep for pretty-printing human-readable message.

feat: better error handling on pmonitor config dir

a2dd5c4

docs: add module documentation for pmonitor

eec9d25

conorsch force-pushed the client-activity branch from 3fe1ed1 to eec9d25 Compare October 11, 2024 21:38

conorsch temporarily deployed to smoke-test October 11, 2024 21:39 — with GitHub Actions Inactive

conorsch self-requested a review October 11, 2024 21:48

conorsch approved these changes Oct 11, 2024

View reviewed changes

conorsch removed the pr-needs-testing Before being mergeable, this PR should add tests to the code, or have a human perform tests label Oct 11, 2024

fix: convert remaining println statements to logging

276a48e

conorsch temporarily deployed to smoke-test October 17, 2024 20:44 — with GitHub Actions Inactive

conorsch merged commit 954e777 into main Oct 17, 2024
14 checks passed

conorsch deleted the client-activity branch October 17, 2024 22:52

conorsch mentioned this pull request Oct 22, 2024

Release v0.80.7, via point-release #4901

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pmonitor: create client activity monitor #4844

pmonitor: create client activity monitor #4844

redshiftzero commented Aug 29, 2024 •

edited

Loading

conorsch commented Sep 10, 2024

aubrika commented Sep 17, 2024

redshiftzero commented Sep 20, 2024

conorsch commented Sep 23, 2024

conorsch left a comment

conorsch Sep 26, 2024

conorsch Sep 26, 2024

conorsch Sep 26, 2024

conorsch Sep 26, 2024

conorsch commented Oct 7, 2024

conorsch commented Oct 11, 2024

redshiftzero commented Oct 16, 2024

pmonitor: create client activity monitor #4844

pmonitor: create client activity monitor #4844

Conversation

redshiftzero commented Aug 29, 2024 • edited Loading

Describe your changes

Issue ticket number and link

Test

Checklist before requesting a review

conorsch commented Sep 10, 2024

aubrika commented Sep 17, 2024

redshiftzero commented Sep 20, 2024

conorsch commented Sep 23, 2024

conorsch left a comment

Choose a reason for hiding this comment

conorsch Sep 26, 2024

Choose a reason for hiding this comment

conorsch Sep 26, 2024

Choose a reason for hiding this comment

conorsch Sep 26, 2024

Choose a reason for hiding this comment

conorsch Sep 26, 2024

Choose a reason for hiding this comment

conorsch commented Oct 7, 2024

conorsch commented Oct 11, 2024

Example output from passing audit

Example output from failing audit

redshiftzero commented Oct 16, 2024

redshiftzero commented Aug 29, 2024 •

edited

Loading

Example output from passing `audit`

Example output from failing `audit`