penumbra-zone / web

Apache License 2.0
10 stars 7 forks source link

On hold: fix validator exchange rate #1272

Closed Valentine1898 closed 2 weeks ago

Valentine1898 commented 2 weeks ago

close #1239

context:

I think the problem is that we are taking validatorExchangeRate to create delegate transaction plan from validatorInfo instead of using CurrentValidatorRate rpc

There is a potential problem with validatorInfo, which is that we update the validator data in indexed-db when transitioning epochs, but we do it in the last block of the epoch that ends, not in the first block of the new epoch. see: https://github.com/penumbra-zone/web/blob/fe0d7ae1cdaa7a35bc833971d862c3ec4875ca0e/packages/query/src/block-processor.ts#L353-L356

I can confirm that validatorExchangeRate obtained from validatorInfo and validatorExchangeRate obtained by CurrentValidatorRate rpc have different values for the same validator

image

I also noticed that pcli uses CurrentValidatorRate rpc instead of using validatorExchangeRate from validatorInfo

https://github.com/penumbra-zone/penumbra/blob/da3ef0a312c193d74c55bfd0282dad2affdb93d0/crates/bin/pcli/src/command/tx.rs#L503-L508

changeset-bot[bot] commented 2 weeks ago

⚠️ No Changeset found

Latest commit: 07a935e5913001d86ab8936446c990f884a7ccc2

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Valentine1898 commented 2 weeks ago

should updatePriceForValidatorDelegationToken also be updated to reflect using the CurrentValidatorRate rpc?

Good question, we probably don't want to run a couple hundred extra queries while scanning blocks Actually I wonder why validatorInfo might contain validatorExchangeRate values that are not up to date. Maybe you know who from the core team can help us with this?

can we get ride of the getRateData getter?

I'm in no rush to remove getters that aren't being used, because in theory it could be useful to other consumers of the getters package

erwanor commented 2 weeks ago

Wait, shouldn't the validator exchange rates be updated by the block processor? if they're not, what if made it do that instead of doing a leaky RPC on every delegation/undelegation request

Valentine1898 commented 2 weeks ago

Wait, shouldn't the validator exchange rates be updated by the block processor? if they're not, what if made it do that instead of doing a leaky RPC on every delegation/undelegation request

block processor updates validatorInfo, and validatorInfo contains rate data. But the problem is that rate data retrieved from validatorInfo sometimes does not equal to rate data retrieved from CurrentValidatorRate rpc

Can rate data change during an epoch?

erwanor commented 2 weeks ago

Generally speaking, rate data only ever change at epoch boundaries and any deviation from this behavior is a serious bug. I think what's more likely is that the calls happened in different epoch, and that the bug we're observing is due to lossy conversions when storing to IDB.

IMO it would be best to investigate this conjecture ("what happens when we record the exchange rate to IDB") before considering doing those RPC calls on each staking actions

Valentine1898 commented 2 weeks ago

Generally speaking, rate data only ever change at epoch boundaries and any deviation from this behavior is a serious bug. I think what's more likely is that the calls happened in different epoch, and that the bug we're observing is due to lossy conversions when storing to IDB.

IMO it would be best to investigate this conjecture ("what happens when we record the exchange rate to IDB") before considering doing those RPC calls on each staking actions

Okay, that makes sense.

I'll hold this PR until I investigate why rate data in indexed-db is outdated

Valentine1898 commented 2 weeks ago

closed as there is a better solution to this issue which will be fixed in core repo

https://discord.com/channels/824484045370818580/1044457038886998137/1250096780880908298