fix opRequestGas #673

Sladuca · 2023-12-05T16:41:11Z

Motivation

opRequestGas returning wrong amount sometime when multiple joinsplits are needed

Solution

rewrite it with a different algorithm that iteratively checks the cost and adds joinsplits if more is needed.

Proof

https://www.loom.com/share/ec05313a3fd34e50a8040ee8f4836355?sid=ff0ab067-8503-4dbf-aa0d-2bc9d46018c8

PR Checklist

added tests
updated documentation
added changeset if necessary
tested in dev/testnet
tested site with snap (we haven't automated this yet)
re-built & tested circuits if any of them changed
updated contracts storage layout (if contracts were updated)

changeset-bot · 2023-12-05T16:41:16Z

🦋 Changeset detected

Latest commit: a11ba48

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

This PR includes changesets to release 5 packages

Name	Type
@nocturne-xyz/client	Patch
@nocturne-xyz/frontend-sdk	Patch
@nocturne-xyz/op-request-plugins	Patch
@nocturne-xyz/bundler	Patch
@nocturne-xyz/test-actor	Patch

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

Sladuca · 2023-12-05T16:41:23Z

fix opRequestGas #673 👈 (View in Graphite)
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

luketchang

couple comments, I may be me misunderstanding some parts but based on what I understood had some questions

luketchang · 2023-12-07T21:07:10Z

packages/client/src/opRequestGas.ts

+      (acc, { value }) => acc + value,
+      0n
+    );
+    // console.log("initial notes for gas asset value", initialNotesForGasAssetValue)


stale comment

luketchang · 2023-12-07T21:26:44Z

packages/client/src/opRequestGas.ts

+      const additionalCompEstimate =
+        compEstimate - initialNotesForGasAssetValue;


Doesn't initialNotesForGasAssetValue include gas asset amount that is being unwrapped for the actual action (i.e. needs to be preserved for action itself)? Shouldn't we only be including gas token value that is not yet being unwrapped (i.e. would have been joinsplit output note but instead we'll unwrap for gas comp)?

yes, that's why it's being subtracted.

luketchang · 2023-12-07T21:28:09Z

packages/client/src/opRequestGas.ts

-        const extraNotes = await gatherNotes(
+      const additionalCompEstimate =
+        compEstimate - initialNotesForGasAssetValue;
+      if (currentGasNotesValue >= additionalCompEstimate) {


currentGasNotesValue is always guaranteed to be 0 first iteration (because its set to 0n on L222), this is expected?

yes. if compEstimate - initialNotesForGasAssetValue is negative, then the unwrapped notes already have enough to cover gas and we don't have to do anything. I should probably make this more clear in the comment above and/or add additional comments explaining it inline.

actually good catch, this isn't quite right, it should be the difference between the total initial note value and total initial JS request value

luketchang · 2023-12-07T21:49:30Z

packages/client/src/opRequestGas.ts

+      usedNotes.set(gasAsset, [
+        ...(initialNotesForGasAsset ?? []),
+        ...newGasNotes,
+      ]);
+      const params = {
+        executionGasLimit,
+        numJoinSplits: computeNumJoinSplits(usedNotes),
+        numUniqueAssets: computeNumUniqueAssets(usedNotes, initialRefundAssets),
+      };
+
+      currentGasEstimate = gasCompensationForParams(params);
+      if (initialNotesForGasAsset !== undefined) {
+        usedNotes.set(gasAsset, initialNotesForGasAsset);
+      } else {
+        usedNotes.delete(gasAsset);
+      }


not following the code in the below highlighted section. If there were initially used notes in the right gas asset, why do we need to re-set them in the usedNotes map on L285? Didn't we just set usedNotes gas asset to include any new gas notes on L273?

And for the else block where we delete gas asset from usedNotes, what if we needed some of them for the action itself (not for gas)? I understand that we'd delete if we're not using them at all given there's not enough for gas asset but not sure that's guaranteed

currentGasEstimate = gasCompensationForParams(params); if (initialNotesForGasAsset !== undefined) { usedNotes.set(gasAsset, initialNotesForGasAsset); } else { usedNotes.delete(gasAsset); }

We re-set them because we destructively modified usedNotes. We do this so that we can figure out exactly which notes are going to be used, compute the number of JSs and refunds required, compute a new estimate, and, during the next iteration of the loop, check if we have enough, trying again (from scratch, but with a larger comp estimate) if we don't. We have to re-set them because the next iteration could destructively reset them.

What matters for the output of this function is that the modified or additional joinsplit request amount has enough to cover the additional comp.

usedNotes is actually kind of a hack. It's there because opRequestGas takes in JS requests and outputs JS requests, but JS requests aren't necessarily 1-1 with notes. The proper way to "fix" this requires getting rid of this "stage" of the op preparation pipeline entirely and merging it into prepareOperation, but that's a more disruptive of a code change than I am comfortable making at the moment.

Ok so basically the 2nd half of the loop (the half where both possible return statements have been passed) is meant to yield new params for num notes and gas est so the upper half (with the 2 return paths) are invoked next iteration

correct. it's pretty fucked, I tried to refactor it but it kept breaking kek.

luketchang · 2023-12-07T23:05:56Z

actors/test-actor/src/actor.ts

+      await this.client.pruneOptimisticNullifiers();
+      setTimeout(pruneOptimsiticNullifiers, opIntervalSeconds);
+    };
+    void pruneOptimsiticNullifiers();


Typo: Optimistic

luketchang · 2023-12-07T23:08:49Z

actors/test-actor/src/actor.ts

+    const pruneOptimsiticNullifiers = async () => {
+      await this.client.pruneOptimisticNullifiers();
+      setTimeout(pruneOptimsiticNullifiers, opIntervalSeconds);
+    };
+    void pruneOptimsiticNullifiers();


Doesn't NocturneClient already do this?
https://github.com/nocturne-xyz/monorepo/blob/main/packages/client/src/NocturneClient.ts#L90-L94

Sladuca changed the title ~~[WIP] fix opRequestGas~~ fix opRequestGas Dec 5, 2023

Sladuca marked this pull request as ready for review December 5, 2023 21:27

Sladuca added 4 commits December 5, 2023 16:28

add test to repro, checkpoint

569bcfc

rewrite opRequestGas

755dfbd

changeset

a43a555

console logs

8c0a6e6

Sladuca force-pushed the seb/patch-note-gathering branch from a9ade41 to 8c0a6e6 Compare December 5, 2023 21:28

Sladuca added 3 commits December 6, 2023 12:39

add failure tests

ffdf64e

test actor optimistic nf

5cb837e

ensure test actor submits in fast batch

098952a

Sladuca requested a review from luketchang as a code owner December 7, 2023 20:00

luketchang reviewed Dec 7, 2023

View reviewed changes

Sladuca added 3 commits December 7, 2023 18:09

use notes - requests

66c562c

checkpoint

43a770b

fetch all notes from scratch on each iteration

23678e5

Sladuca force-pushed the seb/patch-note-gathering branch from 835e158 to 23678e5 Compare December 11, 2023 15:59

interval

a11ba48

		const additionalCompEstimate =
		compEstimate - initialNotesForGasAssetValue;

Uh oh!

fix opRequestGas #673

Are you sure you want to change the base?

fix opRequestGas #673

Uh oh!

Conversation

Sladuca commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation

Solution

Proof

PR Checklist

Uh oh!

changeset-bot bot commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

Sladuca commented Dec 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luketchang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sladuca Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sladuca Dec 7, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Sladuca commented Dec 5, 2023 •

edited

Loading

changeset-bot bot commented Dec 5, 2023 •

edited

Loading

Sladuca commented Dec 5, 2023 •

edited

Loading

Sladuca Dec 7, 2023 •

edited

Loading

Sladuca Dec 7, 2023 •

edited

Loading