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

VanishMax · 2025-01-28T09:15:18Z

Relates to penumbra-zone/web#2006 (Web) and penumbra-zone/dex-explorer#329 (DEX)

To run this change:

Run pnpm dev:pack PR in feat: #1989: subaccount filter in ownedPositionIds penumbra-zone/web#2006
In Prax, run pnpm add:tgz $PATH_TO_WEB_REPO/packages/*/*.tgz
Build the extension and clear the cache, so it re-starts the sync

To test:

Wait for the sync to complete
Open up the DEX PR with updated protobuf package and run it locally
Check if the position filter works

This PR must be merged after the packages from Web are published and applied here.

VanishMax · 2025-01-28T09:30:48Z

packages/query/src/helpers/identify-txs.ts

 ): Promise<
  { relevantTx: RelevantTx; recoveredSourceRecords: RecoveredSourceRecords } | undefined
 > => {
  let txId: TransactionId | undefined; // If set, that means this tx is relevant and should be returned to the caller
+  let subaccount: AddressIndex | undefined;


this change collects subaccounts from SpendableNoteRecords but there's no way to get AddressIndex for all possible actions, that's why it might be undefined. However, it is enough to get them for the purpose of the tasks – to identify LPs per subaccount

comment: undefined AddressIndex in this context can also mean that the SNR is associated with the main account. Maybe we should eventually just store the AddressIndex for the main account as zero.

vacekj

lgtm.

erwanor

I think this is a great start, and I want to test it.

erwanor · 2025-01-28T16:06:20Z

packages/query/src/block-processor.ts

@@ -494,7 +494,7 @@ export class BlockProcessor implements BlockProcessorInterface {

  // 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>();


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.

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.

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

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.

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

TalDerei

this is really great work! left some feedback and good to merge once we bump the deps and green CI + @erwanor unblocks

TalDerei · 2025-01-29T03:46:58Z

packages/query/src/block-processor.ts

@@ -317,7 +317,7 @@ export class BlockProcessor implements BlockProcessorInterface {
        spentNullifiers,
        recordsByCommitment,
        blockTx,
-        addr => this.viewServer.isControlledAddress(addr),
+        addr => this.viewServer.getIndexByAddress(addr),


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.

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

TalDerei · 2025-01-29T03:51:52Z

packages/query/src/helpers/identify-txs.ts

 ): Promise<
  { relevantTx: RelevantTx; recoveredSourceRecords: RecoveredSourceRecords } | undefined
 > => {
  let txId: TransactionId | undefined; // If set, that means this tx is relevant and should be returned to the caller
+  let subaccount: AddressIndex | undefined;


comment: undefined AddressIndex in this context can also mean that the SNR is associated with the main account. Maybe we should eventually just store the AddressIndex for the main account as zero.

TalDerei · 2025-01-29T04:00:31Z

packages/query/src/helpers/identify-txs.ts

 * In terms of minting notes in the shielded pool, three IBC actions are relevant:
 * - MsgRecvPacket (containing an ICS20 inbound transfer)
 * - MsgAcknowledgement (containing an error acknowledgement, thus triggering a refund on our end)
 * - MsgTimeout
 */
-const hasRelevantIbcRelay = (
+const getRelevantIbcRelay = (


comment: see comments above about possibly not modifying this since we don't have a use case for it yet.

TalDerei · 2025-01-29T04:02:19Z

packages/query/src/helpers/identify-txs.ts

  const recoveredSourceRecords: RecoveredSourceRecords = [];

+  // for spend/swapClaim transaction actions


suggestion: can we please add better code comments here for extracting nullifiers + commitments from actions?

TalDerei · 2025-01-29T04:14:02Z

packages/query/src/helpers/identify-txs.ts

+  for (const [spentNullifier, spendableNoteRecord] of spentNullifiers) {
+    const nullifier = txNullifiers.find(txNullifier => spentNullifier.equals(txNullifier));
+    if (nullifier) {
      txId ??= await generateTxId(tx);
+      subaccount = getAddressIndexFromNote(spendableNoteRecord);
    }
  }

+  // For output/swap/swapClaim transaction actions
  const txCommitments = getCommitmentsFromActions(tx);
  for (const [stateCommitment, spendableNoteRecord] of commitmentRecords) {
    if (txCommitments.some(txCommitment => stateCommitment.equals(txCommitment))) {
      txId ??= await generateTxId(tx);
+      subaccount = getAddressIndexFromNote(spendableNoteRecord);


comment: minor optimization for the future; If getCommitmentsFromActions is successful, we can skip getNullifiersFromActions and relevantIbcRelay because they calculate the same txId and subaccount fields.

we can address that separately when we conduct more in-depth block processor performance optimizations. I’ll open a separate issue for it.

erwanor

I can test later, I think this has received enough eyes to not block on it

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

3de4a34

VanishMax requested a review from a team January 28, 2025 09:15

VanishMax self-assigned this Jan 28, 2025

This was referenced Jan 28, 2025

feat: #1989: subaccount filter in ownedPositionIds penumbra-zone/web#2006

Merged

feat: filter positions by subaccount penumbra-zone/dex-explorer#329

Merged

VanishMax commented Jan 28, 2025

View reviewed changes

vacekj approved these changes Jan 28, 2025

View reviewed changes

erwanor requested changes Jan 28, 2025

View reviewed changes

TalDerei self-requested a review January 29, 2025 03:41

TalDerei approved these changes Jan 29, 2025

View reviewed changes

TalDerei mentioned this pull request Jan 29, 2025

Optimize block processor: minimize computations in transaction search #280

Open

refactor(query): use isControlledAddress as it was before

da480fb

erwanor requested review from erwanor and removed request for erwanor January 29, 2025 12:57

erwanor reviewed Jan 29, 2025

View reviewed changes

erwanor approved these changes Jan 29, 2025

View reviewed changes

VanishMax added 2 commits January 30, 2025 09:02

chore: update deps

b86dfff

fix(query): update tests

65c385c

VanishMax merged commit 7bcf54d into main Jan 30, 2025
3 checks passed

VanishMax deleted the feat/position-by-subaccount branch January 30, 2025 05:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

VanishMax commented Jan 28, 2025 •

edited

Loading

VanishMax Jan 28, 2025

TalDerei Jan 29, 2025

vacekj left a comment

erwanor left a comment

erwanor Jan 28, 2025

TalDerei Jan 28, 2025

erwanor Jan 28, 2025

TalDerei Jan 29, 2025 •

edited

Loading

VanishMax Jan 29, 2025

TalDerei left a comment •

edited

Loading

TalDerei Jan 29, 2025

VanishMax Jan 29, 2025

TalDerei Jan 29, 2025

TalDerei Jan 29, 2025

TalDerei Jan 29, 2025 •

edited

Loading

TalDerei Jan 29, 2025

TalDerei Jan 29, 2025

erwanor left a comment

		const recoveredSourceRecords: RecoveredSourceRecords = [];

		// for spend/swapClaim transaction actions

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

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

Conversation

VanishMax commented Jan 28, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vacekj left a comment

Choose a reason for hiding this comment

erwanor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TalDerei Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwanor left a comment

Choose a reason for hiding this comment

VanishMax commented Jan 28, 2025 •

edited

Loading

TalDerei Jan 29, 2025 •

edited

Loading

TalDerei left a comment •

edited

Loading

TalDerei Jan 29, 2025 •

edited

Loading