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.

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

Confidential transfers currently broken? #6146

Open
0x0ece opened this issue Jan 17, 2024 · 5 comments
Open

Confidential transfers currently broken? #6146

0x0ece opened this issue Jan 17, 2024 · 5 comments

Comments

@0x0ece
Copy link
Contributor

0x0ece commented Jan 17, 2024

SPL currently depends on Solana SDK 1.17.6.

I think that this commit, included as of SDK 1.17.7, changes the generators of bulletproofs and therefore makes all range proofs invalid (confidential transfer, withdraw, confidential transfer with fee).

Specifically:

While:

  • Launch solana-test-validator v1.17.7+, e.g. current 1.17.16
  • Run the token transfers demo
    => doesn't work

The fix would be to upgrade the sdk version (or revert to the old generators).

@0x0ece
Copy link
Contributor Author

0x0ece commented Jan 17, 2024

FYI, I was able to update up to 1.17.13 without compilation issues (see linked PR).

On 1.17.14 I'm getting:

error[E0004]: non-exhaustive patterns: `&UiExtension::GroupPointer(_)`, `&UiExtension::GroupMemberPointer(_)`, `&UiExtension::TokenGroup(_)` and 1 more not covered
   --> token/cli/src/output.rs:557:11
    |
557 |     match ui_extension {
    |           ^^^^^^^^^^^^ patterns `&UiExtension::GroupPointer(_)`, `&UiExtension::GroupMemberPointer(_)`, `&UiExtension::TokenGroup(_)` and 1 more not covered
    |
note: `UiExtension` defined here
   --> /home/ecesena/.cargo/registry/src/index.crates.io-6f17d22bba15001f/solana-account-decoder-1.17.14/src/parse_token_extension.rs:36:5
    |
15  | pub enum UiExtension {
    | --------------------
...
36  |     GroupPointer(UiGroupPointer),
    |     ^^^^^^^^^^^^ not covered
37  |     GroupMemberPointer(UiGroupMemberPointer),
    |     ^^^^^^^^^^^^^^^^^^ not covered
38  |     TokenGroup(UiTokenGroup),
    |     ^^^^^^^^^^ not covered
39  |     TokenGroupMember(UiTokenGroupMember),
    |     ^^^^^^^^^^^^^^^^ not covered
    = note: the matched value is of type `&UiExtension`
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern as shown, or multiple match arms
    |
916 ~         ),
917 ~         _ => todo!(),
    |

For more information about this error, try `rustc --explain E0004`.
error: could not compile `spl-token-cli` (lib) due to previous error

@joncinque
Copy link
Contributor

joncinque commented Jan 17, 2024

@samkim-crypto is this expected?

Edit: thanks for the report and the fix! It's normal that 1.17.14+ has this issue, since there are new enum variants in that UiExtension type

@0x0ece
Copy link
Contributor Author

0x0ece commented Jan 18, 2024

FYI, I created a zk-ledger using everything on 1.17.13:

  • solana-test-validator is 1.17.13
  • solana is 1.17.13
  • spl-token is from the PR, with all deps pointing to 1.17.13
    => this works.

zk-ledger contains essentially the txs that were in the confidential transfer demo (deposit, confidential tf, withdraw).

Next, I upgraded my solana git repo to 1.17.16

  • if I run solana-ledger-tool 1.17.16 verify against the zk-ledger created with 1.17.13, it's successful (it was not successful against my old zk-ledger created with 1.17.6)

However, if I run solana-test-validator on 1.17.16

  • solana is either .13 or .16 (tested both)
  • spl-token is as above, linking .13
    => unfortunately creating the zk-ledger doesn't work, which prob means there's something else happening between .13 and .16...

In summary:

  • tx created with .6 stop workin on .7+ (this issue)
  • tx created with .13 still work on .16 (good news, ie the PR fixes something)
  • upgrading spl-token to just .13 won't fix, because it doesn't seem to be working against .16 (bad news, i.e. the PR may not fix everything)

@0x0ece
Copy link
Contributor Author

0x0ece commented Jan 18, 2024

In the process of exploration I also found this breaking change in v1.17.11. This is a non-issue wrt the PR/fix because we're skipping directly to 1.17.13.

I think the only way to avoid these issues across versions would be to include some tests with a binary blob. The current unit tests generate a ZK proof and verify it, thus they hide this type of cross-version incompatibilities.

For example this is a test that works on 1.17.13 and on 1.17.16:

    #[test]
    fn test_batched_range_proof_u128_firedancer_correctness() {
        let instruction_str = "09\
            d01237233159ce1d03b60584908f89191739fae2ccf2afab53a50f969f4eda12\
            e27e15681cca6af9c573ec822c7a4ff1037ef5a929ebd86cdb060170020a1601\
            c49560d2152ef4e657dd39190be3175b9d15a29ada9aa1d168d4fec53dc1a970\
            d475b1bc2801c967a64a099c2a5cc154780940235c78ac889544635314ceb254\
            0000000000000000000000000000000000000000000000000000000000000000\
            0000000000000000000000000000000000000000000000000000000000000000\
            0000000000000000000000000000000000000000000000000000000000000000\
            0000000000000000000000000000000000000000000000000000000000000000\
            4010201000000000\
            3ea7b3695740b168afa889c293f3173c160b3bcb2d895764f603642cf317790a\
            18165de4651dcfcca59f5fb798568eec30f6363c8bc52690cc62d5d59d73ae0f\
            d6d6f1799feac39f9f81daec8c17b9c6eaf628cf3b2bc5f5494a05962b8f3c6b\
            dcc2f9113e1e6828dbd7a06bd5904f0e2e36c7efb5e3a2d3e043856476bc2423\
            e7dcbebeca3f3e58ad8284a76c687e679ce6802dc3f7bd2420f15b517fd2b300\
            8aa04ede6f30a4d092eab9bd31ca65019f1abda573e561d0a0d9e3ef00204003\
            51abb1f943eb19d9462628591d826df0558d8b422d0cd10317ace85ca43a1d02\
            ce24d3bca0d5466e039571abb102b6637dc64f276f704de3d723e851a98f6a4e\
            7a668af3d1ec86f2831b4cf8bf56cefa842239613553414a5cb58c3986337042\
            56c803c3fb92a96769638a3adeb0755851f8af6b0c052e390ceecfe40ea36d07\
            0a7a547f7191ddc2dfa7388e0394e9799a03788996bc870bd564a9fd4978c432\
            26be25ac7b56feded658ad8eded6e29b7daf88ae175eaec43a4683eb9b4ea71e\
            5e6b04897ed3b8f328fa275e5db540e99f5a1198f78d4478191db52b307b8b42\
            ba13cab64c338035fb61aa01be9d19cd5fa3c336b08750372cb9b020af67b535\
            924a8f78f244d3bf695aafac8bc22e3f19edc9d20cb88a986dd997265c65b86a\
            306b6f2e3565413ce040342a84542fdcde368cd35f1b6d22b8af05474e149207\
            1cee6159c7a9cbf72d08e28cecee6d7d8a34a96e6636bce9aca0f03d6561cf16\
            ce68efd3a16c92e8a79c0645087d1b1df14c65c0d71fb6aa4bbb666f167c6b48\
            bca8ec4e34b5f70a4522609f4add869d997d3448a54693fa3174910698570227\
            b2ae1e51f280816b59655a62c3cab69d034860771d15a9f9ab6391bfb5e03d3a\
            928d8843df67de8b3c1aa96b26dc182facfe3cc580188f393035bc0dc47c1c0c\
            7b99a799c954aa0213cf804c8ae563d50a07d95053fca38bab6ce49502b36b0a\
            b96c923297a16e0b45cfb878c168cfa6dfcf22055c360427237d287109cdb90d";
        let instruction_data = hex::decode(instruction_str).unwrap();
        let proof_data = ProofInstruction::proof_data::<BatchedRangeProofU128Data, BatchedRangeProofContext>(instruction_data.as_slice()).unwrap();
        let res = proof_data.verify_proof();
        println!("{:?}", res);
        assert!(res.is_ok());
    }

@samkim-crypto
Copy link
Contributor

Yes, thanks for pointing this out and the fix! This is the expected behavior and the two PRs that you pointed out are the only ones that should affect confidential transfer proofs. The fact that spl depending on 17.13 does not seem to be working against 17.16 is a little odd and seems to require looking into for me. There is solana-labs/solana#34765, which disables u256 range proof, though that shouldn't really affect confidential transfers without fee.

Adding the tests with binary blobs of proofs though seem like a good idea!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants