Skip to content
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.

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

fees: exercise non-zero gas fees in testnet #4306

Closed
Tracked by #3367
TalDerei opened this issue May 2, 2024 · 15 comments
Closed
Tracked by #3367

fees: exercise non-zero gas fees in testnet #4306

TalDerei opened this issue May 2, 2024 · 15 comments
Assignees
Labels
A-fees Area: Relates to transaction fees _P-high High priority _P-V1 Priority: slated for V1 release
Milestone

Comments

@TalDerei
Copy link
Collaborator

TalDerei commented May 2, 2024

Perform a governance parameter change proposal to modify the genesis parameter for setting non-zero gas prices in the testnet. This seems like a natural step forward following #4154.

cc @conorsch

@TalDerei TalDerei added needs-refinement unclear, incomplete, or stub issue that needs work A-fees Area: Relates to transaction fees labels May 2, 2024
@github-project-automation github-project-automation bot moved this to Backlog in Penumbra May 2, 2024
@hdevalence
Copy link
Member

This can't happen without changes currently part of #4320, because there is a critical bug in the current fee component that makes it impossible to set the fees via governance like they are supposed to be.

@aubrika aubrika added _P-V1 Priority: slated for V1 release _P-high High priority and removed needs-refinement unclear, incomplete, or stub issue that needs work labels May 8, 2024
@conorsch
Copy link
Contributor

conorsch commented May 8, 2024

Blocked until we get out the next chain upgrade, tracked in #4325.

@erwanor
Copy link
Member

erwanor commented May 18, 2024

We discussed this synchronously, and the current plan is to do a parameter change proposal some time after we land #4402, but well ahead of a second upgrade that would include #4326.

@aubrika aubrika moved this from Backlog to Todo in Penumbra May 20, 2024
@TalDerei TalDerei self-assigned this May 20, 2024
@TalDerei
Copy link
Collaborator Author

what's the current status of this?

@erwanor
Copy link
Member

erwanor commented May 30, 2024

We haven't turned them on, but we could do that later today or tomorrow once 77 is deployed

@aubrika aubrika modified the milestones: Sprint 7, Sprint 8 Jun 3, 2024
@conorsch
Copy link
Contributor

conorsch commented Jun 3, 2024

We discussed this in sprint-planning today, and want to give it a go. I'm happy to pair with @TalDerei on submitting a gov proposal, so more folks on the team are familiar with the workflows involved. Still TBD is what the structure of the param change proposal needs to be. Generating a template, I see relevant entries as:

[[changes]]
component = "feeParams"
key = "fixedGasPrices"
value = "{}"

[[changes]]
component = "feeParams"
key = "fixedAltGasPrices"
value = "[{\"assetId\":{\"inner\":\"HW2Eq3UZVSBttoUwUi/MUtE7rr2UU7/UH500byp7OAc=\"}},{\"assetId\":{\"inner\":\"nwPDkQq3OvLnBwGTD+nmv1Ifb2GEmFCgNHrU++9BsRE=\"}}]"

So what should we set fixedGasPrices to, exactly? We need to specify the amount of upenumbra, and also figure out how to structure that request in the param change proposal. We'll test on preview and/or devnets to figure out the right shape.

@conorsch
Copy link
Contributor

conorsch commented Jun 3, 2024

To figure out the shape of the param change, as it needs to be specified in the TOML proposal file, I created a local devnet with fees enabled (via --gas-price-simple 5, same as we use in smoke tests), and then polled the grpc endpoint to inspect how the fee params were specified:

❯ grpcurl -plaintext localhost:8080  penumbra.core.app.v1.QueryService/AppParameters  |  jq -r .appParameters.feeParams
{
  "fixedGasPrices": {
    "blockSpacePrice": "5",
    "compactBlockSpacePrice": "5",
    "verificationPrice": "5",
    "executionPrice": "5"
  },
  "fixedAltGasPrices": [
    {
      "blockSpacePrice": "50",
      "compactBlockSpacePrice": "50",
      "verificationPrice": "50",
      "executionPrice": "50",
      "assetId": {
        "inner": "HW2Eq3UZVSBttoUwUi/MUtE7rr2UU7/UH500byp7OAc="
      }
    },
    {
      "blockSpacePrice": "50",
      "compactBlockSpacePrice": "50",
      "verificationPrice": "50",
      "executionPrice": "50",
      "assetId": {
        "inner": "nwPDkQq3OvLnBwGTD+nmv1Ifb2GEmFCgNHrU++9BsRE="
      }
    }
  ]
}

Compare that with how the current testnet declares those params:

❯ grpcurl grpc.testnet.penumbra.zone:443 penumbra.core.app.v1.QueryService/AppParameters  |  jq -r .appParameters.feeParams
{
  "fixedGasPrices": {},
  "fixedAltGasPrices": [
    {
      "assetId": {
        "inner": "HW2Eq3UZVSBttoUwUi/MUtE7rr2UU7/UH500byp7OAc="
      }
    },
    {
      "assetId": {
        "inner": "nwPDkQq3OvLnBwGTD+nmv1Ifb2GEmFCgNHrU++9BsRE="
      }
    }
  ]
}

So, the snippet we want to structure in the proposal is:

  "fixedGasPrices": {
    "blockSpacePrice": "5",
    "compactBlockSpacePrice": "5",
    "verificationPrice": "5",
    "executionPrice": "5"
  },

and maybe it'd be easier to convert if we use a compact output:

❯ grpcurl -plaintext localhost:8080  penumbra.core.app.v1.QueryService/AppParameters  |  jq  .appParameters.feeParams -c
{"fixedGasPrices":{"blockSpacePrice":"5","compactBlockSpacePrice":"5","verificationPrice":"5","executionPrice":"5"},"fixedAltGasPrices":[{"blockSpacePrice":"50","compactBlockSpacePrice":"50","verificationPrice":"50","executionPrice":"50","assetId":{"inner":"HW2Eq3UZVSBttoUwUi/MUtE7rr2UU7/UH500byp7OAc="}},{"blockSpacePrice":"50","compactBlockSpacePrice":"50","verificationPrice":"50","executionPrice":"50","assetId":{"inner":"nwPDkQq3OvLnBwGTD+nmv1Ifb2GEmFCgNHrU++9BsRE="}}]}

Let's try cobbling that together and submitting a proposal like that to a devnet., to validate that it works as expected.

@conorsch
Copy link
Contributor

conorsch commented Jun 3, 2024

Paired with @TalDerei, submitted a proposal:

❯ pcli q governance proposal 7 definition
id = "7"
title = "enable fees on testnet"
description = "switch to using non-zero fees for transactions on the testnet"

[[parameterChange.preconditions]]
component = "feeParams"
key = "fixedGasPrices"
value = "{}"

[[parameterChange.changes]]
component = "feeParams"
key = "fixedGasPrices"
value = "{\"blockSpacePrice\":\"5\",\"compactBlockSpacePrice\":\"5\",\"verificationPrice\":\"5\",\"executionPrice\":\"5\"}"

@conorsch
Copy link
Contributor

conorsch commented Jun 3, 2024

As the literal proposal above shows, we've tried to change only the fixedGasPrices. Also evident in the examples above is fixedAltGasPrices. We're not touching that, because we don't understand the implications of doing so. If someone wants to chime in with motivations about that, please do so here for posterity. Otherwise, feel free to cast your vote on 7.

@hdevalence
Copy link
Member

We're not touching that, because we don't understand the implications of doing so. If someone wants to chime in with motivations about that, please do so here for posterity

I'd recommend (in a follow on proposal) setting them to be 10x the current UM-denominated market price of the other token, it's not particularly essential to do at the same time so it can wait.

@hdevalence
Copy link
Member

value = "{\"blockSpacePrice\":\"5\",\"compactBlockSpacePrice\":\"5\",\"verificationPrice\":\"5\",\"executionPrice\":\"5\"}"

I don't think it matters much for the test, but we may want to change these as they may round to 0. Remember that they have an implicit denominator of 1,000. We'll also probably want the compactBlockSpacePrice to dominate the blockSpacePrice, though that doesn't matter at this stage for testing that the fees work.

@conorsch
Copy link
Contributor

conorsch commented Jun 4, 2024

Proposal 7 passed, so we now have active, but minimal, fees on the testnet. A naive pcli tx send of 1cube showed my penumbra balance decremented by 0.00004 penumbra, which looks right to me. One thing that definitely broke was validator voting:

❯ pcli validator vote cast yes --on 9
Scanning blocks from last sync height 375858 to latest height 375858
[0s] ██████████████████████████████████████████████████       0/0       0/s ETA: 0s
building transaction [1 actions, 0 proofs]...
finished proving in 0.002 seconds [1 actions, 0 proofs, 297 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: {} }

So it looks like the pcli-implementation of validator-voting isn't paying fees properly. No CLI args that I could find allowed me to override this behavior, so I think we need a point release.

@hdevalence
Copy link
Member

There’s no validator-specific fee handling code, so it’s just broken.

conorsch added a commit to penumbra-zone/hermes that referenced this issue Jun 4, 2024
After enabling fees on Testnet 77, we observed an error in Hermes when
trying to spend.

Refs penumbra-zone/penumbra#4306
@erwanor
Copy link
Member

erwanor commented Jun 4, 2024

We turned up another one: planning for a validator upload fails because the client fee calculation rounds down to zero (maybe because of fee tier scaling? unclear) but the fees are nonzero etc.

@TalDerei TalDerei moved this from Todo to In progress in Penumbra Jun 4, 2024
conorsch added a commit that referenced this issue Jun 4, 2024
We recently enabled fees on Testnet 77 (#4306), and in the process
broke a few things, such as Galileo. While working on adding FeeTier
support to Galileo, Galileo's clap setup wanted Display and FromStr.
Added those to satisfy the build, and then circled back to pcli to reuse
the same impls, which allowed us to snip out some duplicative code.
conorsch added a commit to penumbra-zone/galileo that referenced this issue Jun 4, 2024
During Testnet 77 we enabled fees [0] and in the process broke some
things like Galileo. Adding FeeTier support is rather straightforward,
although this changeset requires a patch to the `penumbra-fee` crate in
order to compile [1].

[0] penumbra-zone/penumbra#4306
[1] penumbra-zone/penumbra#4539
conorsch added a commit that referenced this issue Jun 4, 2024
We recently enabled fees on Testnet 77 (#4306), and in the process
broke a few things, such as Galileo. While working on adding FeeTier
support to Galileo, Galileo's clap setup wanted Display and FromStr.
Added those to satisfy the build, and then circled back to pcli to reuse
the same impls, which allowed us to snip out some duplicative code.
conorsch added a commit that referenced this issue Jun 4, 2024
We recently enabled fees on Testnet 77 (#4306), and in the process
broke a few things, such as Galileo. While working on adding FeeTier
support to Galileo, Galileo's clap setup wanted Display and FromStr.
Added those to satisfy the build, and then circled back to pcli to reuse
the same impls, which allowed us to snip out some duplicative code.

(cherry picked from commit 8035e29)
conorsch added a commit to penumbra-zone/galileo that referenced this issue Jun 4, 2024
During Testnet 77 we enabled fees [0] and in the process broke some
things like Galileo. Adding FeeTier support is rather straightforward,
although this changeset requires a patch to the `penumbra-fee` crate in
order to compile [1].

[0] penumbra-zone/penumbra#4306
[1] penumbra-zone/penumbra#4539
conorsch added a commit to penumbra-zone/galileo that referenced this issue Jun 4, 2024
During Testnet 77 we enabled fees [0] and in the process broke some
things like Galileo. Adding FeeTier support is rather straightforward,
although this changeset requires a patch to the `penumbra-fee` crate in
order to compile [1].

[0] penumbra-zone/penumbra#4306
[1] penumbra-zone/penumbra#4539
cratelyn pushed a commit to penumbra-zone/galileo that referenced this issue Jun 4, 2024
During Testnet 77 we enabled fees [0] and in the process broke some
things like Galileo. Adding FeeTier support is rather straightforward,
although this changeset requires a patch to the `penumbra-fee` crate in
order to compile [1].

[0] penumbra-zone/penumbra#4306
[1] penumbra-zone/penumbra#4539
conorsch added a commit that referenced this issue Jun 4, 2024
We recently enabled fees on Testnet 77 (#4306), and in the process
broke a few things, such as Galileo. While working on adding FeeTier
support to Galileo, Galileo's clap setup wanted Display and FromStr.
Added those to satisfy the build, and then circled back to pcli to reuse
the same impls, which allowed us to snip out some duplicative code.

(cherry picked from commit 8035e29)
@TalDerei
Copy link
Collaborator Author

TalDerei commented Jun 5, 2024

closing as completed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-fees Area: Relates to transaction fees _P-high High priority _P-V1 Priority: slated for V1 release
Projects
Archived in project
Development

No branches or pull requests

5 participants