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.

Sign up for GitHub

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

feat(query): update block processor to include subaccounts into LPs #279

Merged
merged 4 commits into from
Jan 30, 2025
Merged

feat(query): update block processor to include subaccounts into LPs #279

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3de4a34
feat(query): update block processor to include subaccounts into LPs
VanishMax Jan 28, 2025
da480fb
refactor(query): use `isControlledAddress` as it was before
VanishMax Jan 29, 2025
b86dfff
chore: update deps
VanishMax Jan 30, 2025
65c385c
fix(query): update tests
VanishMax Jan 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 14 additions & 9 deletions packages/query/src/block-processor.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@
import { processActionDutchAuctionSchedule } from './helpers/process-action-dutch-auction-schedule';
import { processActionDutchAuctionWithdraw } from './helpers/process-action-dutch-auction-withdraw';
import { RootQuerier } from './root-querier';
import { IdentityKey } from '@penumbra-zone/protobuf/penumbra/core/keys/v1/keys_pb';
import { AddressIndex, IdentityKey } from '@penumbra-zone/protobuf/penumbra/core/keys/v1/keys_pb';
import { getDelegationTokenMetadata } from '@penumbra-zone/wasm/stake';
import { toPlainMessage } from '@bufbuild/protobuf';
import { getAssetIdFromGasPrices } from '@penumbra-zone/getters/compact-block';
Expand Down Expand Up @@ -317,7 +317,7 @@
spentNullifiers,
recordsByCommitment,
blockTx,
addr => this.viewServer.isControlledAddress(addr),
addr => this.viewServer.getIndexByAddress(addr),

Check failure on line 320 in packages/query/src/block-processor.ts

View workflow job for this annotation

GitHub Actions / Lint 

Unsafe return of a value of type error

Check failure on line 320 in packages/query/src/block-processor.ts

View workflow job for this annotation

GitHub Actions / Lint 

Unsafe call of a(n) `error` type typed value
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: should we actually remove this? It's used soley in getRelevantIbcRelay to associate IBC actions with their respective sub-accounts indices, but if the objective of this PR is only handling LP positions, it might be unnecessary. We don't have a use case for filtering IBC actions by account indexes yet. thoughts?

that said, we can still keep the getIndexByAddress modifications in the view service method already implemented in penumbra-zone/web#2006.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

put back the isControlledAddress method. Truly, no need to know AddressIndex for IBC relay actions for now

);

// this simply stores the new records with 'rehydrated' sources to idb
Expand Down Expand Up @@ -494,7 +494,7 @@

// Nullifier is published in network when a note is spent or swap is claimed.
private async resolveNullifiers(nullifiers: Nullifier[], height: bigint) {
const spentNullifiers = new Set<Nullifier>();
const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>();
Copy link

Choose a reason for hiding this comment

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

Before we merge, I want to circle back to this and a few other things with the data modeling. What's the lifetime of the spent nullifier map? How big is a SNR? An OOM seems somewhat unlikely but I don't totally understand the approach yet.

Copy link
Contributor

Choose a reason for hiding this comment

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

this is a good flag, still thinking through the proposed data structure.

the spentNullifiers map is a local variable that only lives for the duration of processing a single block in processBlock(). It isn’t a class field or stored anywhere long term. SNRs are stored in the spendable_notes indexeddb table using the saveSpendableNote() storage method, and the state payload is marked as spent by appending a "heightSpent" field in storage.

SNRs are ~700 bytes.

Copy link

Choose a reason for hiding this comment

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

OK this seems totally fine, round it to 1KB, that's 1M SNRs. sgtm

Copy link
Contributor

@TalDerei TalDerei Jan 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

after reviewing the code, Map<Nullifier, SpendableNoteRecord | SwapRecord> is actually the optimal and clever approach to minimize db fetches required to understand what subaccount a particular LP position is eventually associated with.

stepping through the code, prior to calling resolveNullifiers() , indexdb stores the successfully trial decrypted spendable notes in a table who's primary key is the commitment and secondary index is the nullifier. So in theory, you have all the information to know which SNR is associated with what commitment and nullifier directly in storage, but don't yet know which transaction that commitment is associated with, because we haven't processed and saved the transaction information at this stage of the block processor.

resolveNullifiers() will subsequently query any SNR / SwapRecord stored in indexeddb that match any nullifier in the incoming compact block, and the record <> nullifier pair are stored in the spentNullifiers map.

up until this point in the block processing, we know a few things:

  • which SNR / SwapRecord is associated with the user
  • what commitment + nullifier is associated with what SNR / SwapRecord
  • don't know which transaction is associated with the SNR / SwapRecord or commitment

then, if we found a commitment or nullifier relevant to the user in that block, we request all the transactions for that block height from the full node, and filter the specific transaction relevant to the user. This is where the original spentNullifiers map comes into play. We have a spentNullifiers map (Map<Nullifier, SpendableNoteRecord | SwapRecord>) and commitmentRecords map (Map<StateCommitment, SpendableNoteRecord | SwapRecord>), and once we know which commitment / nullifier inside the transaction's actions is associated with the user's commitment / nullifier (stored in those maps in memory), we know we transaction is the user's, and we return the transaction associated with it's relevant subacount by deriving the AddressIndex from the SNR itself.

After identifying the transaction, we have a special method called processTransactions that will identify the LP position relevant to the user and save it alongside the subaccount.

anyways, if we didn't originally have this map, once we knew which nullifier is associated with the user, we'd need to perform another db query to fetch the relevant SNR from the table, and subsequently derive the AddressIndex.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for this great explanation, @TalDerei! This is exactly what the PR does in the smallest details

const readOperations = [];
const writeOperations = [];

Expand All @@ -518,8 +518,6 @@
continue;
}

spentNullifiers.add(nullifier);

if (record instanceof SpendableNoteRecord) {
record.heightSpent = height;
const writePromise = this.indexedDb.saveSpendableNote({
Expand All @@ -535,6 +533,8 @@
});
writeOperations.push(writePromise);
}

spentNullifiers.set(nullifier, record);
}

// Await all writes in parallel
Expand All @@ -548,9 +548,12 @@
* such as metadata, liquidity positions, etc.
*/
private async processTransactions(txs: RelevantTx[]) {
for (const { data } of txs) {
for (const { data, subaccount } of txs) {
for (const { action } of data.body?.actions ?? []) {
await Promise.all([this.identifyAuctionNfts(action), this.identifyLpNftPositions(action)]);
await Promise.all([
this.identifyAuctionNfts(action),
this.identifyLpNftPositions(action, subaccount),
]);
}
}
}
Expand Down Expand Up @@ -582,7 +585,7 @@
* - generate all possible position state metadata
* - update idb
*/
private async identifyLpNftPositions(action: Action['action']) {
private async identifyLpNftPositions(action: Action['action'], subaccount?: AddressIndex) {
if (action.case === 'positionOpen' && action.value.position) {
for (const state of POSITION_STATES) {
const metadata = getLpNftMetadata(computePositionId(action.value.position), state);
Expand All @@ -597,12 +600,14 @@
await this.indexedDb.addPosition(
computePositionId(action.value.position),
action.value.position,
subaccount,
);
}
if (action.case === 'positionClose' && action.value.positionId) {
await this.indexedDb.updatePosition(
action.value.positionId,
new PositionState({ state: PositionState_PositionStateEnum.CLOSED }),
subaccount,
);
}
if (action.case === 'positionWithdraw' && action.value.positionId) {
Expand All @@ -618,7 +623,7 @@
penumbraAssetId: getAssetId(metadata),
});

await this.indexedDb.updatePosition(action.value.positionId, positionState);
await this.indexedDb.updatePosition(action.value.positionId, positionState, subaccount);
}
}

Expand Down
134 changes: 90 additions & 44 deletions packages/query/src/helpers/identify-txs.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import {
IbcRelay,
} from '@penumbra-zone/protobuf/penumbra/core/component/ibc/v1/ibc_pb';
import { addressFromBech32m } from '@penumbra-zone/bech32m/penumbra';
import { Address } from '@penumbra-zone/protobuf/penumbra/core/keys/v1/keys_pb';
import { Address, AddressIndex } from '@penumbra-zone/protobuf/penumbra/core/keys/v1/keys_pb';
import { Packet } from '@penumbra-zone/protobuf/ibc/core/channel/v1/channel_pb';
import {
MsgAcknowledgement,
Expand Down Expand Up @@ -214,25 +214,27 @@ describe('getNullifiersFromActions', () => {
});

describe('identifyTransactions', () => {
const MAIN_ACCOUNT = new AddressIndex({ account: 0 });

test('returns empty arrays when no relevant transactions are found', async () => {
const tx = new Transaction();
const blockTx = [tx];
const spentNullifiers = new Set<Nullifier>();
const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>();
const commitmentRecords = new Map<StateCommitment, SpendableNoteRecord | SwapRecord>();

const result = await identifyTransactions(
spentNullifiers,
commitmentRecords,
blockTx,
() => false,
() => MAIN_ACCOUNT,
);

expect(result.relevantTxs).toEqual([]);
expect(result.recoveredSourceRecords).toEqual([]);
});

test('identifies relevant transactions and recovers sources', async () => {
// Transaction 1: Matching nullifier
test('identifies relevant transactions by nullifiers', async () => {
// Relevant nullifier
const nullifier = new Nullifier({ inner: new Uint8Array([1, 2, 3]) });
const tx1 = new Transaction({
body: new TransactionBody({
Expand All @@ -251,19 +253,16 @@ describe('identifyTransactions', () => {
}),
});

// Transaction 2: Matching commitment
const commitment = new StateCommitment({ inner: new Uint8Array([4, 5, 6]) });
// Irrelevant nullifier
const tx2 = new Transaction({
body: new TransactionBody({
actions: [
new Action({
action: {
case: 'output',
value: new Output({
body: new OutputBody({
notePayload: {
noteCommitment: commitment,
},
case: 'spend',
value: new Spend({
body: new SpendBody({
nullifier: new Nullifier({ inner: new Uint8Array([4, 5, 6]) }),
}),
}),
},
Expand All @@ -272,8 +271,35 @@ describe('identifyTransactions', () => {
}),
});

// Transaction 3: Irrelevant commitment
const tx3 = new Transaction({
const spendableNoteRecord = new SpendableNoteRecord({
addressIndex: MAIN_ACCOUNT,
source: BLANK_TX_SOURCE,
});

const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>([
[nullifier, spendableNoteRecord],
]);
const commitmentRecords = new Map<StateCommitment, SpendableNoteRecord | SwapRecord>();

const result = await identifyTransactions(
spentNullifiers,
commitmentRecords,
[
tx1, // relevant
tx2, // irrelevant
],
() => MAIN_ACCOUNT,
);

expect(result.relevantTxs.length).toBe(1);
expect(result.relevantTxs[0]?.data.equals(tx1)).toBeTruthy();
expect(result.relevantTxs[0]?.subaccount?.equals(MAIN_ACCOUNT)).toBeTruthy();
});

test('identifies relevant transactions by commitments and recovers sources', async () => {
// Matching commitment
const commitment = new StateCommitment({ inner: new Uint8Array([4, 5, 6]) });
const tx1 = new Transaction({
body: new TransactionBody({
actions: [
new Action({
Expand All @@ -282,7 +308,7 @@ describe('identifyTransactions', () => {
value: new Output({
body: new OutputBody({
notePayload: {
noteCommitment: new StateCommitment({ inner: new Uint8Array([7, 8, 9]) }),
noteCommitment: commitment,
},
}),
}),
Expand All @@ -292,16 +318,18 @@ describe('identifyTransactions', () => {
}),
});

// Transaction 4: Irrelevant nullifier
const tx4 = new Transaction({
// Irrelevant commitment
const tx2 = new Transaction({
body: new TransactionBody({
actions: [
new Action({
action: {
case: 'spend',
value: new Spend({
body: new SpendBody({
nullifier: new Nullifier({ inner: new Uint8Array([4, 5, 6]) }),
case: 'output',
value: new Output({
body: new OutputBody({
notePayload: {
noteCommitment: new StateCommitment({ inner: new Uint8Array([7, 8, 9]) }),
},
}),
}),
},
Expand All @@ -310,16 +338,16 @@ describe('identifyTransactions', () => {
}),
});

const spentNullifiers = new Set<Nullifier>([nullifier]);

const spendableNoteRecord = new SpendableNoteRecord({
addressIndex: MAIN_ACCOUNT,
source: BLANK_TX_SOURCE,
});

const commitmentRecords = new Map<StateCommitment, SpendableNoteRecord | SwapRecord>([
[commitment, spendableNoteRecord], // Expecting match
[new StateCommitment({ inner: new Uint8Array([1, 6, 9]) }), new SpendableNoteRecord()], // not expecting match
]);
const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>();

const spentNullifiersBeforeSize = spentNullifiers.size;
const commitmentRecordsBeforeSize = commitmentRecords.size;
Expand All @@ -328,15 +356,15 @@ describe('identifyTransactions', () => {
commitmentRecords,
[
tx1, // relevant
tx2, // relevant
tx3, // not
tx4, // not
tx2, // not
],
() => false,
() => MAIN_ACCOUNT,
);

expect(result.relevantTxs.length).toBe(2);
expect(result.relevantTxs.length).toBe(1);
expect(result.recoveredSourceRecords.length).toBe(1);
expect(result.relevantTxs[0]?.data.equals(tx1)).toBeTruthy();
expect(result.relevantTxs[0]?.subaccount?.equals(MAIN_ACCOUNT)).toBeTruthy();

// Source was recovered
expect(result.recoveredSourceRecords[0]!.source?.equals(BLANK_TX_SOURCE)).toEqual(false);
Expand All @@ -351,6 +379,8 @@ describe('identifyTransactions', () => {
'penumbra1e8k5cyds484dxvapeamwveh5khqv4jsvyvaf5wwxaaccgfghm229qw03pcar3ryy8smptevstycch0qk3uu0rgkvtjpxy3cu3rjd0agawqtlz6erev28a6sg69u7cxy0t02nd4';
const unknownAddr =
'penumbracompat1147mfall0zr6am5r45qkwht7xqqrdsp50czde7empv7yq2nk3z8yyfh9k9520ddgswkmzar22vhz9dwtuem7uxw0qytfpv7lk3q9dp8ccaw2fn5c838rfackazmgf3ahhwqq0da';
const getIndexByAddress = (addr: Address) =>
addr.equals(new Address(addressFromBech32m(knownAddr))) ? MAIN_ACCOUNT : undefined;

test('identifies relevant MsgRecvPacket', async () => {
const txA = new Transaction({
Expand All @@ -364,15 +394,19 @@ describe('identifyTransactions', () => {
},
});
const blockTx = [txA, txB];
const spentNullifiers = new Set<Nullifier>();
const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>();
const commitmentRecords = new Map<StateCommitment, SpendableNoteRecord | SwapRecord>();

const result = await identifyTransactions(spentNullifiers, commitmentRecords, blockTx, addr =>
addr.equals(new Address(addressFromBech32m(knownAddr))),
const result = await identifyTransactions(
spentNullifiers,
commitmentRecords,
blockTx,
getIndexByAddress,
);

expect(result.relevantTxs.length).toBe(1);
expect(result.relevantTxs[0]?.data.equals(txA)).toBeTruthy();
expect(result.relevantTxs[0]?.subaccount?.equals(MAIN_ACCOUNT)).toBeTruthy();
expect(result.recoveredSourceRecords.length).toBe(0);
});

Expand All @@ -388,11 +422,14 @@ describe('identifyTransactions', () => {
},
});
const blockTx = [txA, txB];
const spentNullifiers = new Set<Nullifier>();
const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>();
const commitmentRecords = new Map<StateCommitment, SpendableNoteRecord | SwapRecord>();

const result = await identifyTransactions(spentNullifiers, commitmentRecords, blockTx, addr =>
addr.equals(new Address(addressFromBech32m(knownAddr))),
const result = await identifyTransactions(
spentNullifiers,
commitmentRecords,
blockTx,
getIndexByAddress,
);

expect(result.relevantTxs.length).toBe(1);
Expand All @@ -412,11 +449,14 @@ describe('identifyTransactions', () => {
},
});
const blockTx = [txA, txB];
const spentNullifiers = new Set<Nullifier>();
const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>();
const commitmentRecords = new Map<StateCommitment, SpendableNoteRecord | SwapRecord>();

const result = await identifyTransactions(spentNullifiers, commitmentRecords, blockTx, addr =>
addr.equals(new Address(addressFromBech32m(knownAddr))),
const result = await identifyTransactions(
spentNullifiers,
commitmentRecords,
blockTx,
getIndexByAddress,
);

expect(result.relevantTxs.length).toBe(1);
Expand All @@ -435,11 +475,14 @@ describe('identifyTransactions', () => {
},
});
const blockTx = [tx];
const spentNullifiers = new Set<Nullifier>();
const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>();
const commitmentRecords = new Map<StateCommitment, SpendableNoteRecord | SwapRecord>();

const result = await identifyTransactions(spentNullifiers, commitmentRecords, blockTx, addr =>
addr.equals(new Address(addressFromBech32m(knownAddr))),
const result = await identifyTransactions(
spentNullifiers,
commitmentRecords,
blockTx,
getIndexByAddress,
);

expect(result.relevantTxs.length).toBe(0);
Expand Down Expand Up @@ -473,11 +516,14 @@ describe('identifyTransactions', () => {
},
});
const blockTx = [tx];
const spentNullifiers = new Set<Nullifier>();
const spentNullifiers = new Map<Nullifier, SpendableNoteRecord | SwapRecord>();
const commitmentRecords = new Map<StateCommitment, SpendableNoteRecord | SwapRecord>();

const result = await identifyTransactions(spentNullifiers, commitmentRecords, blockTx, addr =>
addr.equals(new Address(addressFromBech32m(knownAddr))),
const result = await identifyTransactions(
spentNullifiers,
commitmentRecords,
blockTx,
getIndexByAddress,
);

expect(result.relevantTxs.length).toBe(0);
Expand Down
Loading
Loading