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

Wrapup of multidimensional gas fees #3133

Merged
merged 11 commits into from
Oct 5, 2023
Merged

Wrapup of multidimensional gas fees #3133

merged 11 commits into from
Oct 5, 2023

Conversation

zbuc
Copy link
Member

@zbuc zbuc commented Oct 2, 2023

  • Add hardcoded gas prices as a fee component parameter
  • Add gas checks to the transaction-level fee logic.
  • Allow fetching fee_params in the AppParameters.
  • Include gas prices in the compact block.
  • Change the TransactionPlanner to account for fees.

Closes #2970

@zbuc zbuc temporarily deployed to smoke-test October 2, 2023 18:47 — with GitHub Actions Inactive
@zbuc zbuc marked this pull request as ready for review October 3, 2023 21:21
@hdevalence
Copy link
Member

Allow fetching fee_params including current gas prices in the AppParameters.

Two design questions here:

  1. I think we should have an RPC that fetches the current gas prices specifically, defined in the fee component.
  2. I'm not sure we should have the gas prices be part of the AppParameters.

The reasoning behind both of these is that if we do automatic gas pricing based on resource usage, we'll be adjusting the gas prices dynamically, and the chain may be frequently editing the prices. In that case, it would be a bit incongruous for the AppParameters to have some data that's chosen by humans (either in source code or governance) and changing infrequently, together with other data that's chosen automatically. And, in that case, suppose that we have parameters that control the auto-updater for the prices. If the gas prices themselves are part of the AppParameters, where do we put the higher-level parameters that control them? In the same structure? To the side? Also, if the gas prices change reasonably often, someone might be querying them frequently, in which case it'd be easier and more efficient to just get the data they need.

So, even though we're punting on actually implementing an auto-pricing mechanism, I think we should plan with it in mind, and I think it would be cleaner if we thought of the gas prices as being data, rather than parameters.

@zbuc
Copy link
Member Author

zbuc commented Oct 3, 2023

Sure, I implemented them as parameter based on this comment from the original issue:

Add hardcoded gas prices as a chain parameter
To start, we can hardcode the gas prices as a chain parameter, and have the default parameter choice be all-zero. This lets us experiment with nonzero fees on devnets (by editing the generated parameters in the genesis.json) prior to committing to the change and potentially breaking client software.

It sounds like we want to change the design and remove them from being parameters?

@zbuc
Copy link
Member Author

zbuc commented Oct 3, 2023

FWIW I don't have a strong opinion on whether they should be parameters or not -- my gut says it probably fine and even if they are managed automatically in the future that isn't necessarily a reason to exclude them from being parameters; we already have other parameters that aren't controllable by governance.

Re: "only querying for the data you need", this could be solved by adding additional RPCs to fetch per-component parameters instead of only being able to fetch all of AppParameters. This is probably something we want to do anyways, as many commands in pcli currently are fetching all of AppParameters just to access the chain ID.

@hdevalence
Copy link
Member

Sounds good. Let's leave them as parameters for now and iterate on it later.

Comment on lines 266 to 271
let params = app
.view
.as_mut()
.context("view service must be initialized")?
.app_params()
.await?;
Copy link
Member

Choose a reason for hiding this comment

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

This is pulling the gas prices from the AppParams stored in the view service. How does the view service have the current AppParams?

I believe the CompactBlock currently contains a way for the full node to signal changes in parameters by including a new AppParams in the compact block. But this design was a compromise in the case where parameter changes were rare and infrequent, and would be much more problematic if the chain were to frequently adjust the gas prices.

In that case, we'd end up filling the compact blocks with the complete history of changes to gas prices, increasing bandwidth costs for all clients, and supplying them with lots of information they don't care about -- the intermediate history of gas prices is useless, only the gas prices at the moment they submit the application are relevant.

So, I wonder if we should be fetching the latest gas prices from the full node, rather than the view service, and not baking in an assumption that the view server has synchronized the current gas prices.

Alternatively, we could decide that the bandwidth cost of gas price changes isn't a big deal, and decide that the view service should synchronize gas prices.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe the CompactBlock currently contains a way for the full node to signal changes in parameters by including a new AppParams in the compact block.

It did but this was broken (see #3107). I think you raise a valid issue in that updating the AppParams in the compact block requires storing the entire AppParams structure in the compact block. If only the gas prices are changing, and they are changing on a block-by-block basis, this is rather expensive.

Another potential alternative (that might be desirable when implementing #3107) is to have the param change updates in the compact block be at the component-level. That would mean we don't need to store a complete AppParams for a gas prices change. Or we poll the gas prices directly from the full node. 🤷

Copy link
Member

Choose a reason for hiding this comment

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

What if:

  1. We pull the gas prices out of the AppParameters, so that we can have a separation between "governance-controlled parameters that change infrequently and that clients can be updated on" and "auto-controlled prices that change frequently"
  2. We add the gas prices into the CompactBlock, and decide that the chain will stream them to the client when they change.

This way we don't have to do a bigger architectural change about per-component parameter change updates etc. I think that the bandwidth costs might be pretty minimal, we could do some back of the envelope estimates.

@hdevalence
Copy link
Member

Branching off of https://github.com/penumbra-zone/penumbra/pull/3133/files#r1344827368, here's a further rabbithole to go down: how do gas prices affect transaction linkability?

Historically, one of the reasons that Zcash was resistant to having a fee market was that the fee is necessarily revealed in the clear, and so it's a vector to leak information about the client. For instance, if different clients have different logic for fee selection, it might be possible to infer which client was used to create a specific transaction, or to link two transactions by the same user (if that user overrode the fee amount in a unique way shared only among those transactions).

We're exposed to this because of the ability to set a custom fee amount. However, we do need to price resources, to avoid the DoS problems later experienced with Zcash (and with the perspective that a fee market will exist, regardless of whether we'd like it to or not), and so we should take a harm reduction approach.

One way that we could reduce the anonymity risk of setting a custom fee amount would be to use the fact that we have static gas amounts, and to have a few preset values for "low" "medium" "high" that would have different multiples of the base price (e.g., low = 1.05x, medium = 1.3x, high = 2x), rather than an arbitrary setting.

However, while we can quantize the multiple into a few settings, this still allows an observer who sees the transaction to infer what prices were used by that user's client, and hence learn information about when they started planning the transaction. Consider, for instance, the "randomized TWAP" use case, where a user wants to perform a swap over some time period without revealing their complete flow, by splitting their trade into sub-trades with randomized sub-amounts. The TransactionPlan mechanism allows all sub-trades to be planned and authorized by the user at once, storing a collection of pre-authorized transaction plans that can be proved just before submission so that the time of authorization isn't revealed by the transaction contents. But auto-adjusting gas prices potentially messes this up, since the plan has to include what fee it will pay, and computing the fee requires using a specific choice of gas prices.

Perhaps this suggests that we should try to change prices less often than every block, to have longer chunks of time in which gas prices don't change. On the other hand, that messes up the resource pricing goal. Another solution could be to apply some kind of quantization after computnig the fees, so that if gas prices are slightly different the fee would still be the same, although this also seems potentially problematic to define in advance (how to we know how much to quantize without knowing what the prices will be?)

@hdevalence
Copy link
Member

This looks good, I think there might be some init logic happening, since the smoke test fails to query app parameters:

Error: status: Unavailable, message: "error getting fee parameters: Missing FeeParameters", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc", "date": "Tue, 03 Oct 2023 21:24:12 GMT", "content-length": "0", "access-control-allow-credentials": "true", "vary": "origin", "vary": "access-control-request-method", "vary": "access-control-request-headers", "access-control-expose-headers": "grpc-status,grpc-message,grpc-status-details-bin"} }
2023-10-03T21:24:12.650217Z  INFO abci:BeginBlock{height=111}: pd::consensus: beginning block time=Time(2023-10-03 21:24:12.100867336)
2023-10-03T21:24:12.737014Z  INFO abci:Commit: pd::consensus: committed block app_hash=AppHash("c1443b696ab9e40fa4be17aa96fbed67a98245377b3895865149edfc80ecdc23")
Error: pclientd exited early: ExitStatus(unix_wait_status(256))
FOct 03 21:24:12.933  INFO pclientd: starting pclientd opt.home="/tmp/.tmpaNGjYD" config.bind_addr=127.0.0.1:8081 config.grpc_url=http://127.0.0.1:8080/
Error: status: Unavailable, message: "error getting fee parameters: Missing FeeParameters", details: [], metadata: MetadataMap { headers: {"content-type": "application/grpc", "date": "Tue, 03 Oct 2023 21:24:12 GMT", "content-length": "0", "access-control-allow-credentials": "true", "vary": "origin", "vary": "access-control-request-method", "vary": "access-control-request-headers", "access-control-expose-headers": "grpc-status,grpc-message,grpc-status-details-bin"} }
2023-10-03T21:24:13.198481Z  INFO abci:BeginBlock{height=112}: pd::consensus: beginning block time=Time(2023-10-03 21:24:12.648688611)

@zbuc zbuc temporarily deployed to smoke-test October 4, 2023 16:24 — with GitHub Actions Inactive
@zbuc zbuc temporarily deployed to smoke-test October 4, 2023 17:50 — with GitHub Actions Inactive
@plaidfinch
Copy link
Collaborator

However, while we can quantize the multiple into a few settings, this still allows an observer who sees the transaction to infer what prices were used by that user's client, and hence learn information about when they started planning the transaction. Consider, for instance, the "randomized TWAP" use case, where a user wants to perform a swap over some time period without revealing their complete flow, by splitting their trade into sub-trades with randomized sub-amounts. The TransactionPlan mechanism allows all sub-trades to be planned and authorized by the user at once, storing a collection of pre-authorized transaction plans that can be proved just before submission so that the time of authorization isn't revealed by the transaction contents. But auto-adjusting gas prices potentially messes this up, since the plan has to include what fee it will pay, and computing the fee requires using a specific choice of gas prices.

Could this be solved by having the "randomized TWAP" client also add noise into the fees paid, to obscure the precise base gas price at time of transaction submission? This would be marginally more expensive, but maybe not so expensive, if the prices change relatively slowly and the duration of the TWAP is short?

@hdevalence
Copy link
Member

Unfortunately, adding noise only to the RTWAP transactions doesn't really help, because then they're distinguishable from other transactions that would use quantized fee amounts. We could make all transactions have noisy fee levels, but this is also tricky, because that opens up a large surface area for implementation fingerprinting (what if different clients add noise in different ways that turn out to be statistically distinguishable?).

One possibility that might be workable is to stick with preset tiers for gas adjustments (e.g., "low" = 1.05x base, "medium" = 1.3x base, "high" = 2x base), but instead of using fixed multipliers, define specific distributions for clients to draw from to choose a multiplier. So setting "medium" would cause your client to sample a random value near 1.3, using the same distribution as other people's clients.

The hope would then be that slight variation in the gas prices used to compute the fee amount would hide in the noise of the randomized adjustment. This wouldn't be the case if prices changed very rapidly, but if they change only a little bit, it might work.

For a first pass, though, I think we should stick to fixed tiers and figure out randomization later.

@zbuc
Copy link
Member Author

zbuc commented Oct 5, 2023

@hdevalence can you expand on this a bit further:

However, while we can quantize the multiple into a few settings, this still allows an observer who sees the transaction to infer what prices were used by that user's client, and hence learn information about when they started planning the transaction. Consider, for instance, the "randomized TWAP" use case, where a user wants to perform a swap over some time period without revealing their complete flow, by splitting their trade into sub-trades with randomized sub-amounts. The TransactionPlan mechanism allows all sub-trades to be planned and authorized by the user at once, storing a collection of pre-authorized transaction plans that can be proved just before submission so that the time of authorization isn't revealed by the transaction contents. But auto-adjusting gas prices potentially messes this up, since the plan has to include what fee it will pay, and computing the fee requires using a specific choice of gas prices.

Even in the case that the various transactions constituting the TWAP are linkable via the gas price, the assets and transaction contributions are still shielded. Is the concern revealing alpha re: the TWAP, or something else?

@hdevalence
Copy link
Member

Even in the case that the various transactions constituting the TWAP are linkable via the gas price, the assets and transaction contributions are still shielded. Is the concern revealing alpha re: the TWAP, or something else?

Yeah, exactly, the concern is that the individual transactions comprising the TWAP may be linkable to each other.

More generally, one of the goals of making transaction authorization bind only to the effecting data (which does not include the ZKPs or the proof anchor) was so that the final transaction would not reveal when it was authorized. This prevents distinguishing transactions made with a custody setup that has more latency. For instance, if we had the signatures sign over the proofs, the proofs would have to be made first, and then an observer could tell that a transaction took a long time to sign by noticing that it was submitted with a much older anchor than when it was included in a block.

The fact that the fee is part of the effecting data (necessarily! it does matter that users authorize the fees they pay, and it affects spends etc), together with the fact that fees now depend on gas prices, means we somewhat reintroduce this property. So it's somewhat desirable if the gas prices don't change super super rapidly. On the other hand, having accurate representation of the costs of resource use is also important, so there will be a balancing act.

@zbuc zbuc temporarily deployed to smoke-test October 5, 2023 18:05 — with GitHub Actions Inactive
@zbuc zbuc temporarily deployed to smoke-test October 5, 2023 19:13 — with GitHub Actions Inactive
@zbuc zbuc merged commit 77c078f into main Oct 5, 2023
7 of 8 checks passed
@zbuc zbuc deleted the 2970_wrapup branch October 5, 2023 19:46
@cratelyn cratelyn added the A-fees Area: Relates to transaction fees label Apr 18, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Groundwork for Multidimensional Gas Fees
4 participants