Correct swap claim height check #4239

cronokirby · 2024-04-18T23:43:07Z

Describe your changes

This changes the circuit check in swap claims to check that the exact epoch and block where a swap was made match that output data for the batch swap of that epoch and block. Previously were effectively only checking that the intra-block value was the same, but not the epoch.

TODO: Write migration for batch swap output datas

Checklist before requesting a review

If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason:

A migration will be needed to augment batch swap output data

cronokirby · 2024-04-18T23:43:50Z

Not quite sure why the tests aren't passing, will pick back up tomorrow morning.

conorsch · 2024-04-19T15:28:20Z

These changes should be based on v0.71.0, so that the fix is compatible with the existing testnet. Because the changes are consensus-breaking, we should bump the minor version in order to release. To that end, I've pushed a release/v0.72.x branch, branched from the v0.71.0, that should be targeted with this PR.

proto/penumbra/penumbra/core/component/dex/v1/dex.proto

crates/core/component/dex/src/component/router/route_and_fill.rs

hdevalence · 2024-04-19T17:53:25Z

crates/core/component/dex/src/batch_swap_output_data.rs

+    pub epoch: FqVar,
+    pub block: FqVar,


What's the rationale behind making these an FqVar rather than an alternate representation?

Also, could we give them different names to clarify that they're not standalone values, but components of a Position? Like

pub position_epoch_component: FqVar, pub position_block_component: FqVar,

or some other name? I'm just worried that calling it block could suggest it's the block height, which it isn't.

Changed to store a PositionVar now

No I meant, what's the rationale for the representation? I'm not opposed to using an FqVar, I'm just wondering how we decide one or the other. Can we write up a brief rationale for why we picked one or the other?

A PositionVar has a position stored internally as both an FqVar and a series of 48 bit constraints - 16 bits each for the epoch, block, and commitment segment of the position. Since we need to compare the epoch and block from the position from the state commitment proof with the epoch and block from the BSOD, and we're already bit constraining to make two FqVars for the epoch and block in both cases, another (I think cheaper in constraints) option is to compare as bits directly. Comparing as FqVars also works though.

Hmm, in that case wouldn't making the two prefix components FqVars be cheaper? Since we already have the bit-constrained components of the actual position, we can check that they're equal to the provided prefix components without needing to bit-constrain the prefix components, right?

yep, you're right. That seems like the way to go to save the additional bit constraints

Yeah my thinking initially was that using FqVars avoids some overhead; I think using PositionVar is clearer though since it directly matches the domain type.

Not sure that comparing bits is cheaper in constraints: checking that (x0 + 2 * x1 + ...) =? (y0 + 2 * y1 + ...) can be done in a single R1CS constraint. Checking x0 =? y0, x1 =? y1 would require several

Yeah, that seems like a good reason to use FqVars as in the original design, I think it's clearer than using a Position because it means we don't add any instance variables that don't have meaning but could be (mis)used in the circuit.

redshiftzero

I know there is a migration to do, but the other changes LGTM

erwanor · 2024-04-22T17:23:59Z

crates/core/component/dex/src/component/router/route_and_fill.rs

            trading_pair,
            delta_1,
            delta_2,
            lambda_1,
            lambda_2,
            unfilled_1,
            unfilled_2,
+            sct_position_prefix: (
+                u16::try_from(epoch.index).expect("epoch index should be small enough"),
+                u16::try_from(block_height - epoch.start_height)


Can we add a comment about relative block conversion?

I'd like to re-pitch adding tct protos: #4118

so other systems like the web code doesn't have to manually re-create this serialization logic happening in the .into()'s

conorsch · 2024-04-23T23:43:08Z

Took a stab at testing this in a pairing session with @cronokirby today. A few commits were pushed as part of that exploration. We want to do some more testing before we're ready to ship this.

hdevalence · 2024-04-24T21:38:26Z

crates/bin/pd/src/migrate/testnet72.rs

+        // Translate inside compact block storage.
+        translate_compact_block_storage(ctx.clone(), &mut delta).await?;
+
+        delta.put_block_height(0u64);


Why do we reset the block height here?

This is so we can do an InitChain handshake with comet and allow it to initialize its internal view before block sync/proposals. The flow looks like this:

pd <- info query -- comet pd -- app height=0 -> comet pd <- init chain req -- comet pd -- init chain resp -> comet

unfortunately, the CometBft spec seem to have been reorganized again, so it will take me a minute to fish out the link

Interesting. So the idea is that setting it to 0 signals that the app is ready for genesis, then once pd sends the init chain message, it will get set to h+1 as set in the genesis file?

That's right, it will consume the genesis as a result of reading a zero app height, and then pull the specified initialHeight

erwanor · 2024-04-24T22:31:22Z

crates/bin/pd/src/migrate/testnet72.rs

+        let (key, compactblack_bytes): (String, Vec<u8>) = r?;
+        let block = pb::compact_block::v1::CompactBlock::decode(compactblack_bytes.as_slice())?;
+        let block = ctx.translate_compact_block(block).await?;
+        delta.put_proto(key, block);


Here the structure is correct, but we need to be reading from/writing to nonverifiable storage (the key looks correct).

conorsch · 2024-04-24T23:24:18Z

crates/bin/pd/src/migrate/testnet72.rs

+    let mut stream = delta.nonverifiable_prefix_raw("compactblock/".as_bytes());
+    while let Some(r) = stream.next().await {
+        let (key, compactblack_bytes) = r?;
+        let block = pb::compact_block::v1::CompactBlock::decode(compactblack_bytes.as_slice())?;


Nit: s/black/block/ in var names here.

conorsch · 2024-04-24T23:25:14Z

Most recent round of testing on a private fork of the network showed good results. 👍 Let's make sure to squash this patch into a single commit, as we'll need to cherry-pick it back onto main post-release.

We recently removed mention of zeroing-out `priv_validator_state.json` for CometBFT as part of the chain upgrade docs. During migration testing on a private fork today, though, several of us observed that without this change, the network would not resume. More research required to understand the root cause here. Refs #4245, #4239.

Make swap_claim check the exact epoch and block in circuit Read epoch from state when creating batch swap output This moves the epoch read directly to the route_and_fill method. Depcrate epoch_starting_height in BSOD Store epoch and block instead of position within BatchSwapOutputDataVar Write migration Add comments about relative block conversion Fix warnings Increment total halt count Change which migration runs Drop extra references to storage before releasing it Use nonverifiable storage for compact block translation This is where they actually are

conorsch · 2024-04-25T16:00:05Z

Squashed into single commit, so it's cherry-pickable. Will wait on CI, then merge into release branch and get things moving.

erwanor

ACK, thanks for your work on this @cronokirby @conorsch

Refs #4239, #4271.

This is a cherry-pick of PR #4239, by: Author: Lucas Meier <[email protected]> Date: Thu Apr 18 14:55:50 2024 -0700 This PR: 1. fix a bug in the swap claim circuit 2. includes a migration routine for testnet 71 Swap claim proofs take a `BatchSwapOutputData` as part of their public inputs. This data is used to compute a user's pro-rated share of a batch swap at a given block. For that reason, it is important the output data used in the proof be correct. A swap claim must be bound to the specific BSOD that was produced by that user's swap. The swap claim circuit's validation only checked that the supplied BSOD was produced at the same relative block height as the user's swap. It did not check that the epochs were the same. As a result, it was possible to generate swap claim proofs using a BSOD produced during a different epoch, on potentially much more advantageous terms.

## Describe your changes This is a cherry-pick of #4239, by: Author: Lucas Meier <[email protected]> Date: Thu Apr 18 14:55:50 2024 -0700 This PR: 1. fix a bug in the swap claim circuit 2. includes a migration routine for testnet 71 Swap claim proofs take a `BatchSwapOutputData` as part of their public inputs. This data is used to compute a user's pro-rated share of a batch swap at a given block. For that reason, it is important the output data used in the proof be correct. A swap claim must be bound to the specific BSOD that was produced by that user's swap. The swap claim circuit's validation only checked that the supplied BSOD was produced at the same relative block height as the user's swap. It did not check that the epochs were the same. As a result, it was possible to generate swap claim proofs using a BSOD produced during a different epoch, on potentially much more advantageous terms. ## Issue ticket number and link ## Checklist before requesting a review - [x] If this code contains consensus-breaking changes, I have added the "consensus-breaking" label. Otherwise, I declare my belief that there are not consensus-breaking changes, for the following reason: > This is consensus breaking --------- Co-authored-by: Lucas Meier <[email protected]>

cronokirby added consensus-breaking breaking change to execution of on-chain data state-breaking breaking change to on-chain data labels Apr 18, 2024

cronokirby had a problem deploying to smoke-test April 18, 2024 23:43 — with GitHub Actions Failure

erwanor self-requested a review April 19, 2024 00:04

redshiftzero self-requested a review April 19, 2024 16:22

cratelyn added this to the Sprint 4 milestone Apr 19, 2024

hdevalence reviewed Apr 19, 2024

View reviewed changes

proto/penumbra/penumbra/core/component/dex/v1/dex.proto Outdated Show resolved Hide resolved

hdevalence reviewed Apr 19, 2024

View reviewed changes

crates/core/component/dex/src/component/router/route_and_fill.rs Outdated Show resolved Hide resolved

hdevalence reviewed Apr 19, 2024

View reviewed changes

cronokirby changed the base branch from main to release/v0.72.x April 19, 2024 18:27

cronokirby force-pushed the batch-swap-output-correct-position branch from 50b1d7c to 0571414 Compare April 19, 2024 18:52

cronokirby had a problem deploying to smoke-test April 19, 2024 18:52 — with GitHub Actions Failure

cronokirby force-pushed the batch-swap-output-correct-position branch from 0571414 to 873b1c7 Compare April 19, 2024 18:56

cronokirby temporarily deployed to smoke-test April 19, 2024 18:56 — with GitHub Actions Inactive

cronokirby temporarily deployed to smoke-test April 19, 2024 19:28 — with GitHub Actions Inactive

cronokirby force-pushed the batch-swap-output-correct-position branch from d0f12bd to 5962448 Compare April 19, 2024 20:03

cronokirby temporarily deployed to smoke-test April 19, 2024 20:03 — with GitHub Actions Inactive

cronokirby temporarily deployed to smoke-test April 19, 2024 21:05 — with GitHub Actions Inactive

cronokirby changed the title ~~WIP~~ Correct swap claim height check Apr 19, 2024

cronokirby force-pushed the batch-swap-output-correct-position branch from 676864a to 65407f4 Compare April 22, 2024 16:31

cronokirby had a problem deploying to smoke-test April 22, 2024 16:31 — with GitHub Actions Error

cronokirby force-pushed the batch-swap-output-correct-position branch from 65407f4 to 3acf558 Compare April 22, 2024 16:51

cronokirby temporarily deployed to smoke-test April 22, 2024 16:51 — with GitHub Actions Inactive

redshiftzero approved these changes Apr 22, 2024

View reviewed changes

erwanor reviewed Apr 22, 2024

View reviewed changes

cronokirby temporarily deployed to smoke-test April 22, 2024 17:44 — with GitHub Actions Inactive

cratelyn modified the milestones: Sprint 4, Sprint 5 Apr 22, 2024

cronokirby had a problem deploying to smoke-test April 23, 2024 19:07 — with GitHub Actions Error

cronokirby temporarily deployed to smoke-test April 23, 2024 19:16 — with GitHub Actions Inactive

cronokirby temporarily deployed to smoke-test April 23, 2024 22:30 — with GitHub Actions Inactive

cronokirby had a problem deploying to smoke-test April 23, 2024 23:04 — with GitHub Actions Error

cronokirby temporarily deployed to smoke-test April 23, 2024 23:12 — with GitHub Actions Inactive

hdevalence reviewed Apr 24, 2024

View reviewed changes

erwanor reviewed Apr 24, 2024

View reviewed changes

cronokirby temporarily deployed to smoke-test April 24, 2024 22:36 — with GitHub Actions Inactive

conorsch reviewed Apr 24, 2024

View reviewed changes

conorsch mentioned this pull request Apr 24, 2024

docs: re-add priv val state munging for upgrades #4267

Merged

1 task

conorsch force-pushed the batch-swap-output-correct-position branch from 93a593a to 0b3f45e Compare April 25, 2024 15:59

conorsch temporarily deployed to smoke-test April 25, 2024 15:59 — with GitHub Actions Inactive

erwanor approved these changes Apr 25, 2024

View reviewed changes

conorsch merged commit 3b0fa70 into release/v0.72.x Apr 25, 2024
5 checks passed

conorsch deleted the batch-swap-output-correct-position branch April 25, 2024 17:02

conorsch mentioned this pull request Apr 25, 2024

docs: chain upgrade for testnet 72 #4271

Merged

1 task

conorsch added a commit that referenced this pull request Apr 25, 2024

docs: update archive url for joining testnet

22be12d

Refs #4239, #4271.

conorsch mentioned this pull request Apr 25, 2024

docs: update archive url for joining testnet #4272

Merged

1 task

conorsch added a commit that referenced this pull request Apr 25, 2024

docs: update archive url for joining testnet

2973fac

Refs #4239, #4271.

conorsch mentioned this pull request Apr 25, 2024

publish release artifact containing only proving keys #4130

Closed

erwanor mentioned this pull request Apr 29, 2024

dex: correct swap claim bsod check #4281

Merged

1 task

conorsch mentioned this pull request May 15, 2024

governance: emergency halt should require 1/3 vote #4389

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct swap claim height check #4239

Correct swap claim height check #4239

cronokirby commented Apr 18, 2024 •

edited

Loading

cronokirby commented Apr 18, 2024

conorsch commented Apr 19, 2024

hdevalence Apr 19, 2024

cronokirby Apr 19, 2024

hdevalence Apr 19, 2024

redshiftzero Apr 19, 2024

hdevalence Apr 19, 2024

redshiftzero Apr 19, 2024

cronokirby Apr 19, 2024

cronokirby Apr 19, 2024

hdevalence Apr 19, 2024

redshiftzero left a comment

erwanor Apr 22, 2024

cronokirby Apr 23, 2024

grod220 Apr 29, 2024

conorsch commented Apr 23, 2024

hdevalence Apr 24, 2024

erwanor Apr 24, 2024

hdevalence Apr 25, 2024

erwanor Apr 25, 2024 •

edited

Loading

erwanor Apr 24, 2024

conorsch Apr 24, 2024

conorsch commented Apr 24, 2024

conorsch commented Apr 25, 2024

erwanor left a comment

Correct swap claim height check #4239

Correct swap claim height check #4239

Conversation

cronokirby commented Apr 18, 2024 • edited Loading

Describe your changes

Checklist before requesting a review

cronokirby commented Apr 18, 2024

conorsch commented Apr 19, 2024

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

redshiftzero 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

conorsch commented Apr 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwanor Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

conorsch commented Apr 24, 2024

conorsch commented Apr 25, 2024

erwanor left a comment

Choose a reason for hiding this comment

cronokirby commented Apr 18, 2024 •

edited

Loading

erwanor Apr 25, 2024 •

edited

Loading