Change Planner to use optional gas prices field and error if unset #4554

aubrika · 2024-06-04T20:45:11Z

Closes #4555 and fixes fee inclusion in validator definition upload, validator vote casting, and tx sweep.

This also introduces some protection for future planner invocations. The Planner's gas_prices field has been changed to an Option type and if it's not set it will return an error during planning:

$ cargo run --bin pcli --release -- --home ~/.local/share/pcli-preview/ validator definition upload --file validator.toml
...
Error: planner instances must call set_gas_prices prior to planning

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:

should be limited to addressing client-side failures on tx submission, so shouldn't change consensus on which

conorsch · 2024-06-04T20:52:19Z

We discussed this in IBC sync, because getting this work resolved is relevant to near-term IBC work. Looks like as written, this PR will fix #4541, but not #4540, which is required for unblocking IBC.

@zbuc has volunteered to tackle implementing a deeper API change here, discussed in Discord, making GasPrices optional, to avoid the hard-to-hold planner interface problem that was encountered as part of the fee enabling (#4306), as seen in Galileo today penumbra-zone/galileo#98.

conorsch · 2024-06-04T20:53:44Z

Refs #4555

conorsch

There are CI failures that appear to be related to an old base branch. This should be rebased (and preferably squashed) onto lastest main, in particular in order to include #4539, which will affect how the FeeTier usage is applied throughout the pcli cli opts.

crates/bin/pcli/src/command/validator.rs

conorsch · 2024-06-04T21:43:38Z

crates/view/src/planner.rs

-            &self.gas_prices,
+            &self
+                .gas_prices
+                .context("planner instances must call set_gas_prices prior to planning")?,


While I'm not a huge fan of forcing a separate call rather than doing it automatically, I love how descriptive this error message is!

We alternatively could just change the Planner constructor to require GasPrices or a ViewClient instance it can fetch them from

conorsch · 2024-06-04T21:49:52Z

Thanks for fixing, testing against local devnet with fees....

conorsch · 2024-06-04T22:03:28Z

Confirmed that validator definition uploads are working.

fails on main

❯ cargo run --release --bin pcli -- --home ~/.local/share/pcli-devnet validator  definition upload --file val.toml
    Finished release [optimized] target(s) in 0.28s
     Running `target/release/pcli --home /home/conor/.local/share/pcli-devnet validator definition upload --file val.toml`
Scanning blocks from last sync height 69111 to latest height 69111
[0s] ██████████████████████████████████████████████████       0/0       0/s ETA: 0s
building transaction [1 actions, 0 proofs]...
finished proving in 0.001 seconds [1 actions, 0 proofs, 491 bytes]
broadcasting transaction and awaiting confirmation...
Error: error broadcasting transaction

Caused by:
    status: Internal, message: "Error submitting transaction: code 1, log: failed to deliver transaction: check_stateful failed: fee must be greater
than or equal to the transaction base price (supplied: 0, base: 3)", details: [], metadata: MetadataMap { headers: {} }

passes on this branch

❯ cargo run --release --bin pcli -- --home ~/.local/share/pcli-devnet validator  definition upload --file ../penumbra/val.toml
   Compiling penumbra-dex v0.77.0 (/home/conor/src/penumbra2/crates/core/component/dex)
   Compiling penumbra-auction v0.77.0 (/home/conor/src/penumbra2/crates/core/component/auction)
   Compiling penumbra-compact-block v0.77.0 (/home/conor/src/penumbra2/crates/core/component/compact-block)
   Compiling penumbra-proof-setup v0.77.0 (/home/conor/src/penumbra2/crates/crypto/proof-setup)
   Compiling penumbra-transaction v0.77.0 (/home/conor/src/penumbra2/crates/core/transaction)
   Compiling penumbra-tendermint-proxy v0.77.0 (/home/conor/src/penumbra2/crates/util/tendermint-proxy)
   Compiling penumbra-custody v0.77.0 (/home/conor/src/penumbra2/crates/custody)
   Compiling penumbra-app v0.77.0 (/home/conor/src/penumbra2/crates/core/app)
   Compiling penumbra-view v0.77.0 (/home/conor/src/penumbra2/crates/view)
   Compiling penumbra-wallet v0.77.0 (/home/conor/src/penumbra2/crates/wallet)
   Compiling pcli v0.77.0 (/home/conor/src/penumbra2/crates/bin/pcli)
    Finished release [optimized] target(s) in 51.73s
     Running `target/release/pcli --home /home/conor/.local/share/pcli-devnet validator definition upload --file ../penumbra/val.toml`
Scanning blocks from last sync height 69124 to latest height 69124
[0s] ██████████████████████████████████████████████████       0/0       0/s ETA: 0s
building transaction [3 actions, 2 proofs]...
finished proving in 0.527 seconds [3 actions, 2 proofs, 2081 bytes]
broadcasting transaction and awaiting confirmation...
transaction broadcast successfully: 5d264f79d166bb8b90238b9be299465baaad6fd5c3a61575793ddff52a8ca80c
transaction confirmed and detected: 5d264f79d166bb8b90238b9be299465baaad6fd5c3a61575793ddff52a8ca80c @ height 69125
Uploaded validator definition

conorsch · 2024-06-04T22:05:30Z

However, validator voting appears not to work still:

failing, as expected, on main

❯ cargo -q run --release --bin pcli -- --home /tmp/devnet-val0/ validator vote cast yes --on 4
Scanning blocks from last sync height 69206 to latest height 69206
[0s] ██████████████████████████████████████████████████       0/0       0/s ETA: 0s
building transaction [1 actions, 0 proofs]...
finished proving in 0.001 seconds [1 actions, 0 proofs, 306 bytes]
broadcasting transaction and awaiting confirmation...
Error: error broadcasting transaction

Caused by:
    status: Internal, message: "Error submitting transaction: code 1, log: failed to deliver transaction: check_stateful failed: fee must be greater
than or equal to the transaction base price (supplied: 0, base: 1)", details: [], metadata: MetadataMap { headers: {} }

failing, differently, on this branch

❯ cargo -q run --release --bin pcli -- --home /tmp/devnet-val0/ validator vote cast yes --on 4
Scanning blocks from last sync height 69209 to latest height 69209
[0s] ██████████████████████████████████████████████████       0/0       0/s ETA: 0s
Error: ran out of notes to spend while planning transaction, need 1 of asset passet1984fctenw8m2fpl8a9wzguzp7j34d7vravryuhft808nyt9fdggqxmanqm

Not sure how to debug that passet denom. This PR is clearly an improvement, it looks like #4540 is not yet fixed by it though.

zbuc · 2024-06-04T22:10:35Z

@conorsch passet1984fctenw8m2fpl8a9wzguzp7j34d7vravryuhft808nyt9fdggqxmanqm is the penumbra asset id, do you have enough balance in the validator's wallet?

conorsch · 2024-06-04T22:17:37Z

@zbuc Oy, that was it!

❯ cargo -q run --release --bin pcli -- --home /tmp/devnet-val0/ validator vote cast yes --on 5
Scanning blocks from last sync height 69376 to latest height 69376
[0s] ██████████████████████████████████████████████████       0/0       0/s ETA: 0s
building transaction [3 actions, 2 proofs]...
finished proving in 0.559 seconds [3 actions, 2 proofs, 1896 bytes]
broadcasting transaction and awaiting confirmation...
transaction broadcast successfully: fdd05eb8fbdd4be7ee8d70a8adbc82366242d9b98f79e6fc6118c070a7bfbbcf
transaction confirmed and detected: fdd05eb8fbdd4be7ee8d70a8adbc82366242d9b98f79e6fc6118c070a7bfbbcf @ height 69377
Cast validator vote

Thanks, all set!

conorsch · 2024-06-04T22:22:59Z

Drat, this doesn't apply cleanly to 0.77.x. Might have to pull in #4510 as well....

aubrika requested a review from zbuc June 4, 2024 20:45

aubrika had a problem deploying to smoke-test June 4, 2024 20:45 — with GitHub Actions Failure

zbuc approved these changes Jun 4, 2024

View reviewed changes

conorsch marked this pull request as draft June 4, 2024 20:54

zbuc had a problem deploying to smoke-test June 4, 2024 21:35 — with GitHub Actions Failure

zbuc marked this pull request as ready for review June 4, 2024 21:35

zbuc changed the title ~~validator fees fix~~ Change Planner to use optional gas prices field and error if unset Jun 4, 2024

cronokirby approved these changes Jun 4, 2024

View reviewed changes

conorsch self-requested a review June 4, 2024 21:41

conorsch requested changes Jun 4, 2024

View reviewed changes

TalDerei and others added 2 commits June 4, 2024 17:45

scratch

cf2ab8c

Switch gas prices to an Option on the planner and error if not set

8f29a83

zbuc force-pushed the validator-def-gas branch from 3eac8c3 to 8f29a83 Compare June 4, 2024 21:45

zbuc had a problem deploying to smoke-test June 4, 2024 21:45 — with GitHub Actions Error

PR fixes

406cf56

zbuc temporarily deployed to smoke-test June 4, 2024 21:48 — with GitHub Actions Inactive

zbuc mentioned this pull request Jun 4, 2024

pcli should use friendlier denoms rather than asset IDs when printing error messages #4557

Closed

conorsch approved these changes Jun 4, 2024

View reviewed changes

conorsch merged commit 77f3f22 into main Jun 4, 2024
13 checks passed

conorsch deleted the validator-def-gas branch June 4, 2024 22:18

This was referenced Jun 4, 2024

backport fixes to planner gas prices & validator voting #4559

Merged

fees: validator voting broken #4540

Closed

conorsch mentioned this pull request Jun 4, 2024

fees: validator definition submission broken #4541

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change Planner to use optional gas prices field and error if unset #4554

Change Planner to use optional gas prices field and error if unset #4554

aubrika commented Jun 4, 2024 •

edited by zbuc

Loading

conorsch commented Jun 4, 2024

conorsch commented Jun 4, 2024

conorsch left a comment

conorsch Jun 4, 2024

zbuc Jun 4, 2024

conorsch commented Jun 4, 2024

conorsch commented Jun 4, 2024

conorsch commented Jun 4, 2024

zbuc commented Jun 4, 2024 •

edited

Loading

conorsch commented Jun 4, 2024

conorsch commented Jun 4, 2024

Change Planner to use optional gas prices field and error if unset #4554

Change Planner to use optional gas prices field and error if unset #4554

Conversation

aubrika commented Jun 4, 2024 • edited by zbuc Loading

conorsch commented Jun 4, 2024

conorsch commented Jun 4, 2024

conorsch left a comment

Choose a reason for hiding this comment

conorsch Jun 4, 2024

Choose a reason for hiding this comment

zbuc Jun 4, 2024

Choose a reason for hiding this comment

conorsch commented Jun 4, 2024

conorsch commented Jun 4, 2024

conorsch commented Jun 4, 2024

zbuc commented Jun 4, 2024 • edited Loading

conorsch commented Jun 4, 2024

conorsch commented Jun 4, 2024

aubrika commented Jun 4, 2024 •

edited by zbuc

Loading

zbuc commented Jun 4, 2024 •

edited

Loading