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

fix: correctly process compact block gas prices #1637

Merged
merged 2 commits into from
Aug 5, 2024

Conversation

turbocrime
Copy link
Contributor

fixes #1636

idb interface is updated to require valid input.

block processor saveGasPrices call for native and alt gas prices are both updated to correctly construct the input.

@turbocrime turbocrime requested review from Valentine1898 and a team August 3, 2024 01:07
Copy link

changeset-bot bot commented Aug 3, 2024

🦋 Changeset detected

Latest commit: bc2e1ff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 11 packages
Name Type
@penumbra-zone/storage Major
@penumbra-zone/types Minor
@penumbra-zone/getters Patch
@penumbra-zone/query Major
@penumbra-zone/services Major
minifront Patch
node-status Patch
@penumbra-zone/crypto-web Major
@repo/ui Patch
@penumbra-zone/wasm Major
@penumbra-zone/perspective Major

Not sure what this means? Click here to learn what changesets are.

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

Copy link
Contributor

@TalDerei TalDerei left a comment

Choose a reason for hiding this comment

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

replicated and confirmed this bug locally. However, this doesn't seem to resolve the problem? 60d0c2e still breaks syncing for me

Copy link
Collaborator

@grod220 grod220 left a comment

Choose a reason for hiding this comment

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

What environments are passing gas prices where the assetId is set explicitly as undefined? Is this on a local chain instance?

Comment on lines 172 to +183
if (compactBlock.gasPrices) {
await this.indexedDb.saveGasPrices(
new GasPrices({
assetId: this.stakingAssetId,
...compactBlock.gasPrices,
}),
);
await this.indexedDb.saveGasPrices({
...toPlainMessage(compactBlock.gasPrices),
assetId: toPlainMessage(this.stakingAssetId),
});
}
if (compactBlock.altGasPrices.length) {
for (const gasPrice of compactBlock.altGasPrices) {
await this.indexedDb.saveGasPrices(gasPrice);
for (const altGas of compactBlock.altGasPrices) {
await this.indexedDb.saveGasPrices({
...toPlainMessage(altGas),
assetId: getAssetIdFromGasPrices(altGas),
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

issue: can we just put the guard inside storage so callers don't have to worry about this?

  async saveGasPrices(value: PartialMessage<GasPrices>): Promise<void> {
    if (!value.assetId) {
      value.assetId = this.stakingTokenAssetId;
    }
    
    await this.u.update({
      table: 'GAS_PRICES',
      value: new GasPrices(value).toJson() as Jsonified<GasPrices>,
    });
  }

This also doesn't require toPlainMessage which isn't entirely obvious from a glance how it serializes all the fields.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, scratch that. We have to handle Um/alts differently. Something like this would work:

// used in main block processor loop
await this.saveGasPrices(compactBlock);

=====
// additional method

  private async saveGasPrices(block: CompactBlock) {
    const pricesToSave = [...block.altGasPrices];

    // Undefined field implies UM. Manually overriding as this is commonly not passed.
    if (block.gasPrices) {
      pricesToSave.push(
        new GasPrices({
          ...block.gasPrices,
          assetId: this.stakingAssetId,
        }),
      );
    }

    for (const price of pricesToSave) {
      if (!price.assetId) {
        console.warn(
          `BlockHeight: ${block.height}. No asset id for gas prices: ${JSON.stringify(price.toJson())}`,
        );
        continue;
      }
      await this.indexedDb.saveGasPrices(price);
    }
  }

Copy link
Contributor Author

@turbocrime turbocrime Aug 3, 2024

Choose a reason for hiding this comment

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

callers should be explicit in intent. a well-typed parameter means callers don't have to worry - the type system will protect them.

unanticipated fallback behavior upon absent input (not default input, but literally absent input) is a greater source of concern.

plainmessage does not serialize fields. it represents the message type as a plain object. it is the same as the class instance, sans class instance methods. any class instance or object literal meeting the message type satisfies plainmessage.

this information is available from the docstring on the type and on the function. its existence is explained and its use in specific cases, and very specifically in db interaction, are recommended in bufbuild/connectrpc documentation.

it is the simplest way to express that a parameter requires data present for every field of the message type.

other options:

  1. using the class type. led us to the present situation
  2. using Required on the class type. would require non-message input
  3. a literal redefinition of what the plainmessage type provides

import { GasPrices } from '@buf/penumbra-zone_penumbra.bufbuild_es/penumbra/core/component/fee/v1/fee_pb.js';
import { createGetter } from './utils/create-getter.js';

export const getAssetIdFromGasPrices = createGetter((gp?: GasPrices) => gp?.assetId);
Copy link
Contributor

Choose a reason for hiding this comment

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

If getAssetIdFromGasPrices is called with a GasPrices object where assetId is undefined, it will still return undefined.

Copy link
Contributor Author

@turbocrime turbocrime Aug 3, 2024

Choose a reason for hiding this comment

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

hmm. getters are supposed to assert.

this solved the case on the testnet i was using but it's true the inner could be empty.

true correctness presumably requires much idb refactor and some helper methods.

i will add something here but larger interface work is out of scope

Copy link
Contributor Author

@turbocrime turbocrime Aug 5, 2024

Choose a reason for hiding this comment

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

okay - if an AssetId class instance is present, it always contains a uint8array inner. so this

  1. guards against the condition of an absent assetid (no inner, invalid keypath)
  2. does not guard against the condition of a present assetid with default zero-length inner (bad inner, valid keypath)
  3. does not guard against the impossible condition of a present assetid with an absent inner (can't happen by getter input type)

in the case of 3, the parameter of the getter requires a message class instance, and a class instance of gas prices will never have a present assetid with an absent inner. this is because children of the class instance emitted from a constructor are always other class instances, and so assetId may be absent and thus inner is undefined (case 1), or present and thus inner is defined of some length (case 2)

client method outputs use constructor and helper methods like fromJson use constructor

so the getter performs its purpose, but does not perform length verification. thus #1639

Comment on lines +179 to +183
for (const altGas of compactBlock.altGasPrices) {
await this.indexedDb.saveGasPrices({
...toPlainMessage(altGas),
assetId: getAssetIdFromGasPrices(altGas),
});
Copy link
Contributor

@TalDerei TalDerei Aug 3, 2024

Choose a reason for hiding this comment

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

we need an additional conditional check before spreading the compactBlock.altGasPrices, something like

const altGasPrices = {
  ...toPlainMessage(altGas),
  assetId: getAssetIdFromGasPrices(altGas),
};

if (altGasPrices.assetId) {
  await this.indexedDb.saveGasPrices(altGasPrices);
} else {
  continue syncing without error
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, you're right. Updated my example above.

@TalDerei
Copy link
Contributor

TalDerei commented Aug 3, 2024

What environments are passing gas prices where the assetId is set explicitly as undefined?

can you elaborate on what you mean by "environments" here?

@grod220
Copy link
Collaborator

grod220 commented Aug 3, 2024

can you elaborate on what you mean by "environments" here?

Currently on mainnet, the assetId field is either present with data or absent entirely. There is not a block that passes an assetId set to undefined.

@TalDerei
Copy link
Contributor

TalDerei commented Aug 3, 2024

Currently on mainnet, the assetId field is either present with data or absent entirely. There is not a block that passes an assetId set to undefined.

currently no environments, which is why this bug hasn't publicly surfaced in the wild. I replicated this on a local chain instance

@turbocrime
Copy link
Contributor Author

turbocrime commented Aug 3, 2024

What environments are passing gas prices where the assetId is set explicitly as undefined? Is this on a local chain instance?

the present version of the fullnode compact block stream on um price update, apparently. using testnet infra with prices, experienced this on friday.

this should be fixed even if the condition is not present on a known production chain right now, because it halts sync entirely.

the issue seems conditional for unknown reasons, but persistent when it occurs.

@turbocrime turbocrime changed the base branch from main to turbocrime/idb-asserts August 5, 2024 03:15
@turbocrime turbocrime merged commit d421847 into turbocrime/idb-asserts Aug 5, 2024
@turbocrime turbocrime deleted the turbocrime/save-gas-prices branch August 5, 2024 06:13
turbocrime added a commit that referenced this pull request Aug 5, 2024
* assert lengths of binary input to idb
* type idb methods to require keypath inputs
* fix: correctly process compact block gas prices (#1637)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Block processor fails to set gas prices from a compact block
3 participants