transaction vstorage #10633

turadg · 2024-12-06T01:20:11Z

Description

The dashboard will requires data for each transaction. In the course of designing that we explored how to design the storage node paths without creating too much cost in IAVL. (The data in HEAD carry a burden of magnitude O(k+v) for all network nodes (including non-validators) into perpetuity.)

Examining the trade-offs we arrived at:

store one node per transaction
- push all changes through it
- rely on indexer to demux
delete on demand
- when we have an API for block-safe deletion it can delete in the operation
- until then keep track of what to delete
- a core-eval can be run to cull data whenever necessary

Security Considerations

none

Scaling Considerations

One storage path per transaction, which persists until the deletion job is called.

One entry in a set to keep track of transactions to delete.

Documentation Considerations

Not developer facing

Testing Considerations

CI

Upgrade Considerations

not yet deployed

cloudflare-workers-and-pages · 2024-12-06T01:23:56Z

Deploying agoric-sdk with Cloudflare Pages

Latest commit:	`ccb9e28`
Status:	✅ Deploy successful!
Preview URL:	https://3d3a51d6.agoric-sdk.pages.dev
Branch Preview URL:	https://10578-transaction-vstorage.agoric-sdk.pages.dev

View logs

refs: #10633 ## Description Miscellaneous small fixes, docs and refactorings. These are made in the course of #10633 but that has other changes that need more work. Best reviewed by commit. ### Security Considerations none ### Scaling Considerations none ### Documentation Considerations none ### Testing Considerations CI ### Upgrade Considerations not yet deployed

dckc · 2024-12-13T15:11:40Z

packages/fast-usdc/src/types.ts

@@ -11,6 +11,7 @@ import type { PendingTxStatus } from './constants.js';
 import type { FastUsdcTerms } from './fast-usdc.contract.js';

 export type EvmHash = `0x${string}`;
+export type EvmAddress = `0x${string & { length: 40 }}`;


neat: string & { length: 40 }

packages/fast-usdc/src/cli/operator-commands.js

gibson042 · 2024-12-14T20:04:19Z

packages/internal/src/js-utils.js

+ * @param {string} str
+ * @returns unknown
+ */
+export const parseWithBigint = str =>


I don't like that this approach is vulnerable to collisions and "upgrades" any all-decimal-digits string to a bigint, even if it was user input that should remain a string. What's the reason for not using smallcaps?

const rejectOCap = cap => Fail`${cap} is not pure data`; const { toCapData, fromCapData } = makeMarshal(rejectOCap, rejectOCap, { serializeBodyFormat: "smallcaps" }); // Constraining to pure data guarantees empty slots and allows for direct use of // `body`. const serialize = value => { const { body, slots } = toCapData(value); slots.length === 0 || Fail`slots ${slots} unexpected for pure data`; return body; } const unserialize = text => fromCapData({ body: text, slots: [] }); const value = harden({ number: 42, bigint: 42n, strings: ["42", "+42"] }); const serialized = serialize(value); const restored = unserialize(serialized); console.log({ value, serialized, restored }); /* => { value: { number: 42, bigint: 42n, strings: [ '42', '+42' ] }, serialized: '#{"bigint":"+42","number":42,"strings":["42","!+42"]}', restored: { bigint: 42n, number: 42, strings: [ '42', '+42' ] } } */

I think it's a good idea worth exploring more. There are lots of trade-offs to consider so I'm opting to take it out of the critical path for FU by using the more verbose but well trodden "JSON-stringified CapData".

For ref, I track this possible optimization in/around..

wallet action, inter protocol data is wrapped in 2 JSON.stringify layers #7999

packages/boot/test/fast-usdc/fast-usdc.test.ts

samsiegart · 2024-12-16T21:09:02Z

packages/fast-usdc/test/supports.ts

-    sequence: false,
-  });
+  const storage = makeFakeStorageKit(
+    'fun', // Fast USDC Node


Optional: Could we just name this fastUsdc? Is it intentionally different than the real node? Seeing "fun" in the test file was confusing at first.

Is it intentionally different than the real node

It is intentional, to show that the contract root is configured by the core-eval and not the contract itself.

I do think that's worth keeping. I chose a three letter one to keep the paths short and wrap lines less.

samsiegart · 2024-12-16T21:13:01Z

packages/fast-usdc/test/supports.ts

+  const storage = makeFakeStorageKit(
+    'fun', // Fast USDC Node
+  );
+  storage.getPureData = path =>


Optional: It might help to document this a bit. I gather it can't have any remotables or bigints.

good point, it should have a jsdoc.

remotables aren't pure data but bigints are. (bigint is the main motivation; otherwise JSON is a good encoding of pure data)

samsiegart · 2024-12-16T21:15:13Z

packages/boot/test/fast-usdc/snapshots/fast-usdc.test.ts.md

 > The example below illustrates the schema of the data published there.
 > 
 > See also board marshalling conventions (_to appear_).

    [
      [
-        'published.fastUsdc.status.0xc81bc6105b60a234c7c50ac17816ebcd5561d366df8bf3be59ff387552761702',
-        'ADVANCING',


Question: I'm a bit confused. Below we have:

t.deepEqual( storage.getPureData(`fun.txns.${mockEvidence.txHash}`), [mockEvidence, { status: PendingTxStatus.Advancing }], 'tx is Advancing', );

I see the { status: PendingTxStatus.Advancing } part here, but not the mockEvidence. Wouldn't that be expected here?

I think this one is truncating the array to show the HEAD value. Do you want it to have the whole sequence? That's only per block and in practice the state changes would happen in different blocks but the fake vstorage treats everything as block height zero

It appears from looking at status-manager.js that the evidence is written in the same block as the status

agoric-sdk/packages/fast-usdc/src/exos/status-manager.js

Lines 161 to 162 in 33173f9

publishEvidence(txHash, evidence);

publishStatus(txHash, status);

. I think the sequence: true when creating the node ensures they're both included.

I guess this test is just looking at the last cell, but if it's important behavior that the evidence is included in the first cell, I think we should capture that in the test somehow. Why not include it in all cells like { evidence: {}, status: "" }? Are we assuming the indexer will just pick up the evidence in the first cell and save it?

Why not include it in all cells like { evidence: {}, status: "" }?

Great idea. I'll do that.

Are we assuming the indexer will just pick up the evidence in the first cell and save it?

The indexer will accumulate events to update the db row, yeah.

I was thinking of including it in all cells so we wouldn't have to track anything but the latest, it's kind of moot if we only include the evidence in the first cell, even though it's one less cell to track it's still more than one. However, it looks like documentStorageSchema doesn't support multiple cells. That probably doesn't have to be in scope, but a regular assert for the original cell with the evidence would suffice. I think it's important to verify that the evidence is published since the indexer relies on it.

I was thinking of including it in all cells so we wouldn't have to track anything but the latest,

Oh, the evidence isn't stored in a way that it can be looked up for each update.

I think it's important to verify that the evidence is published since the indexer relies on it.

I think that's covered by the unit tests but I've added the assertion to the boot test too. Do you think the A3P tests also need it?

Oh, the evidence isn't stored in a way that it can be looked up for each update.

Ah I see, yea not worth changing that part.

I think that's covered by the unit tests

I guess that makes sense. Unit tests for exhaustiveness, and a boot test just to make sure the vstorage write works for any path. It was confusing seeing the different results between the two though so thanks for updating.

Do you think the A3P tests also need it?

I'm not sure the scope of the a3p tests, seems like they're not super important for fast-usdc because they're on a single chain, but the multichain-testing tests might want to have test cases for the status updates. There weren't any such existing test cases there before this PR though so I don't think it's in scope.

dckc

I'm struggling to get a high level view.

I'd like to see a typed pattern for what a client might see at one of these vstorage keys.

dckc · 2024-12-16T21:10:55Z

packages/boot/test/fast-usdc/snapshots/fast-usdc.test.ts.md

@@ -161,14 +161,14 @@ Generated by [AVA](https://avajs.dev).

 ## makes usdc advance

-> Under "published", the "fastUsdc.status" node is delegated to the statuses of fast USDC transfers identified by their tx hashes.
+> Under "published", the "fastUsdc.txns" node is delegated to the Ethereum transactions upon which Fast USDC is acting.


Suggested change

> Under "published", the "fastUsdc.txns" node is delegated to the Ethereum transactions upon which Fast USDC is acting.

> Under "published", the "fastUsdc.txns" node is delegated to the Ethereum transactions upon which Fast USDC is acting.

I suppose txns is an abbreviation for transactions. Why not spell it out?

concision

the evidence uses txn

to make clear this is an Ethereum-identified transaction

ok, so txns isn't an abbreviation of transactions; it's a plural of the evidence property name.

Re txn distinguing between ethereum and others such as cosmos-sdk... I seem to have a gap in my education. If you have a moment to point me at what I missed, I'd appreciate it.

Re txn distinguing between ethereum and others such as cosmos-sdk

I retract #3. I didn't think it through carefully.

txs is the common Ethereum abbreviation for "transactions" and it's also used in Cosmos.

So I don't have a good reason to use txns. I do prefer txs to transactions but could go either way if you have a principle for the latter.

using conventional dictionary words for identifiers that have scopes larger than a few lines pays off, IME.

"txn" is a sufficiently established term-of-art that I find it acceptable.

dckc · 2024-12-16T21:14:20Z

packages/fast-usdc/src/exos/status-manager.js

+      sequence: true, // avoid overwriting other output in the block
+    });
+    void E(txNode).setValue(
+      JSON.stringify(pureDataMarshaller.toCapData(harden(record))),


harden(...) doesn't hurt (much) here, but if it's already a CopyRecord, it's already hardened.

harden(...) doesn't hurt (much) here

The marshaller threw without it.

if it's already a CopyRecord, it's already hardened

Not according to the structural type of CopyRecord,

export type CopyRecord<T extends Passable = any> = Record<string, T>;

The marshaller threw without it.

That's a a bug in the caller that claimed something was a CopyRecord without harden-ing it.

This is actually the cost of adding the harden(...) here: it masks such bugs.

packages/fast-usdc/src/exos/status-manager.js

dckc · 2024-12-16T21:24:48Z

packages/fast-usdc/src/exos/status-manager.js

+  /**
+   * Transactions seen *ever* by the contract.
+   *
+   * Note that these are stored in IAVL and grow monotonically. At some point in the future we may want to prune them.


I wonder what "monotonic" is intended to communicate. Monotonic is a relationship between 2 variables. Is the dependent variable here time? A constant function is non-decreasing and hence monotonic.

Suggested change

* Note that these are stored in IAVL and grow monotonically. At some point in the future we may want to prune them.

* Note that like all durable stores, this SetStore is stored in IAVL. It grows without bound (though the amount of growth per incoming message to the contract is bounded). At some point in the future we may want to prune it.

the dependent variable here time?

Yes, or blockheight, what have you.

A constant function is non-decreasing and hence monotonic.

Sure, it could stay constant if it's never used.

The suggestion is more clear, I'll take it.

dckc · 2024-12-16T21:52:19Z

packages/boot/test/fast-usdc/fast-usdc.test.ts

-    node: `fastUsdc.status`,
-    owner: `the statuses of fast USDC transfers identified by their tx hashes`,
+    node: `fastUsdc.txns`,
+    owner: `the Ethereum transactions upon which Fast USDC is acting`,


these vstorage snapshots are nice for looking at an example or two, but I wonder about how to make it easier to figure out all possible values.

How about including the name of a TypedPattern and the Type in the doc?

Maybe documentStorageSchema should support the pattern?

maybe sometime but out of scope rn, imo

dckc · 2024-12-16T21:57:56Z

packages/fast-usdc/src/exos/settler.js

         *   fullValue: NatValue;
         * }} SettlerTransferCtx
         */
        onFulfilled(_result, ctx) {
-          const { txHash, sender, fullValue } = ctx;
-          statusManager.forwarded(txHash, sender, fullValue);
+          const { txHash, nfa, fullValue } = ctx;


ctx is part of the durable storage of the relevant vow, right? So this property name change would be a breaking change if this code were deployed, right?

yes if it were deployed

When we get to upgrade testing, I hope we manage to test upgrading with pending vows.

dckc · 2024-12-16T22:15:31Z

packages/fast-usdc/test/exos/status-manager.test.ts

@@ -10,7 +10,7 @@ import type { CctpTxEvidence } from '../../src/types.js';

 type Common = Awaited<ReturnType<typeof commonSetup>>;
 type TestContext = {
-  statusNode: ERef<StorageNode>;
+  transactionsNode: ERef<StorageNode>;


odd that we spell it out internally but not externally.

txs is embedded in a string that's already very long and distinct so the motivation was there.

I agree it's probably better to make them match. I'd rather make this txsNode than have published.fastUsdc.transactions.0x19330d10D9Cc8751218eaf51E8885D058642E08A

dckc · 2024-12-16T22:30:53Z

multichain-testing/package.json

@@ -42,7 +40,54 @@
    "typescript": "~5.7.2"
  },
  "resolutions": {
-    "axios": "1.6.7"
+    "axios": "1.6.7",
+    "@agoric/cosmos": "portal:../golang/cosmos",


Did you use tooling to add all these resolutions?

agoric-sdk/multichain-testing/README.md

Line 24 in 1af5f1d

(Note that the '@agoric/*' deps will come from the parent directory due to `yarn link --relative ../../.. --all`)

samsiegart

LGTM hoping CI passes

samsiegart · 2024-12-17T03:39:01Z

packages/boot/test/fast-usdc/snapshots/fast-usdc.test.ts.md

 > The example below illustrates the schema of the data published there.
 > 
 > See also board marshalling conventions (_to appear_).

    [
      [
-        'published.fastUsdc.status.0xc81bc6105b60a234c7c50ac17816ebcd5561d366df8bf3be59ff387552761702',
-        'ADVANCING',


Oh, the evidence isn't stored in a way that it can be looked up for each update.

Ah I see, yea not worth changing that part.

I think that's covered by the unit tests

I guess that makes sense. Unit tests for exhaustiveness, and a boot test just to make sure the vstorage write works for any path. It was confusing seeing the different results between the two though so thanks for updating.

Do you think the A3P tests also need it?

I'm not sure the scope of the a3p tests, seems like they're not super important for fast-usdc because they're on a single chain, but the multichain-testing tests might want to have test cases for the status updates. There weren't any such existing test cases there before this PR though so I don't think it's in scope.

`sender` and `address` are ambiguous in FUC

- no need to support legacy Ethereum txs that do not include chain id in the tx envelope

turadg force-pushed the 10578-transaction-vstorage branch from 01ab575 to c6133c3 Compare December 7, 2024 01:39

turadg changed the title ~~ansaction vstorage~~ transaction vstorage Dec 7, 2024

turadg mentioned this pull request Dec 10, 2024

misc fastUsdc vstorage cleanup #10661

Merged

turadg force-pushed the 10578-transaction-vstorage branch 3 times, most recently from c7552eb to 5731f20 Compare December 13, 2024 02:20

dckc reviewed Dec 13, 2024

View reviewed changes

turadg commented Dec 13, 2024

View reviewed changes

packages/fast-usdc/src/cli/operator-commands.js Show resolved Hide resolved

turadg force-pushed the 10578-transaction-vstorage branch from 3607cf6 to e81fdcd Compare December 14, 2024 01:19

turadg requested review from samsiegart and 0xpatrickdev December 14, 2024 01:45

turadg force-pushed the 10578-transaction-vstorage branch from e81fdcd to 5351b38 Compare December 14, 2024 01:46

turadg marked this pull request as ready for review December 14, 2024 01:47

turadg requested a review from a team as a code owner December 14, 2024 01:47

turadg force-pushed the 10578-transaction-vstorage branch from 5351b38 to 4223710 Compare December 14, 2024 18:29

gibson042 reviewed Dec 14, 2024

View reviewed changes

turadg mentioned this pull request Dec 16, 2024

SPIKE: Pure-Data Object Notation #10701

Closed

turadg force-pushed the 10578-transaction-vstorage branch from 07e5c5a to 33173f9 Compare December 16, 2024 20:44

turadg requested review from gibson042 and dckc December 16, 2024 20:45

turadg added the force:integration Force integration tests to run on PR label Dec 16, 2024

samsiegart requested changes Dec 16, 2024

View reviewed changes

dckc reviewed Dec 16, 2024

View reviewed changes

turadg force-pushed the 10578-transaction-vstorage branch from 85c1b26 to 1af5f1d Compare December 16, 2024 23:02

turadg force-pushed the 10578-transaction-vstorage branch from bce780b to 72cc751 Compare December 17, 2024 02:04

turadg requested a review from samsiegart December 17, 2024 02:27

samsiegart approved these changes Dec 17, 2024

View reviewed changes

turadg force-pushed the 10578-transaction-vstorage branch from 627a9ea to 998db91 Compare December 17, 2024 03:55

turadg added the automerge:rebase Automatically rebase updates, then merge label Dec 17, 2024

turadg added 22 commits December 17, 2024 05:50

lint: tsc checkJs

293ac50

chore(deps): link agoric-sdk

3c63de2

feat: getValues for sequence nodes

b5698ce

refactor: distinguish NFA address

fd09c05

`sender` and `address` are ambiguous in FUC

test: trace logger

71b1859

feat: simplify seenTxs key

fd05a7e

- no need to support legacy Ethereum txs that do not include chain id in the tx envelope

feat: pureDataMarshaller

6df7f1f

test: getPureData helper

01315f1

test: provide smartWalletKit

ebdb5e9

feat: include 'sender' in CctpTxEvidence

f99e7b8

feat: vstorage status --> txns

aebb4d7

refactor: txnsNode

fc8bb1e

test: shorten node path

1637c1d

feat: publish CctpTxEvidence

2916c8f

test: observe node sequence

d82f7e7

feat: deleteCompletedTxs

f0078ee

feat: consistent publishTxnRecord (record)

dbf3934

chore(types): import CctpTxEvidence

00e7a8d

feat: defaultSerializer util

19d5e03

test: use defaultSerializer

c20866d

feat: publish OBSERVED with first evidence

7e62d8f

feat(types): TransactionRecord

ccb9e28

turadg force-pushed the 10578-transaction-vstorage branch from 998db91 to ccb9e28 Compare December 17, 2024 13:55

mergify bot merged commit a2813f6 into master Dec 17, 2024
81 checks passed

mergify bot deleted the 10578-transaction-vstorage branch December 17, 2024 14:47

	publishEvidence(txHash, evidence);
	publishStatus(txHash, status);

	> Under "published", the "fastUsdc.txns" node is delegated to the Ethereum transactions upon which Fast USDC is acting.
	> Under "published", the "fastUsdc.txns" node is delegated to the Ethereum transactions upon which Fast USDC is acting.

	* Note that these are stored in IAVL and grow monotonically. At some point in the future we may want to prune them.
	* Note that like all durable stores, this SetStore is stored in IAVL. It grows without bound (though the amount of growth per incoming message to the contract is bounded). At some point in the future we may want to prune it.

transaction vstorage #10633

transaction vstorage #10633

Conversation

turadg commented Dec 6, 2024 • edited Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

Upgrade Considerations

cloudflare-workers-and-pages bot commented Dec 6, 2024 • edited Loading

Deploying agoric-sdk with Cloudflare Pages

Choose a reason for hiding this comment

gibson042 Dec 14, 2024 • edited Loading

Choose a reason for hiding this comment

turadg Dec 16, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dckc 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg Dec 16, 2024 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samsiegart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

turadg commented Dec 6, 2024 •

edited

Loading

cloudflare-workers-and-pages bot commented Dec 6, 2024 •

edited

Loading

gibson042 Dec 14, 2024 •

edited

Loading

turadg Dec 16, 2024 •

edited

Loading

turadg Dec 16, 2024 •

edited

Loading