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

UIP-8: Transparent Addresses #4950

Merged
merged 18 commits into from
Dec 13, 2024
Merged

UIP-8: Transparent Addresses #4950

merged 18 commits into from
Dec 13, 2024

Conversation

hdevalence
Copy link
Member

Describe your changes

This commit experiments with a "truncated" address format, which aims at maximizing compatibility with other chains. This is kind of a janky hack, but may be worth doing anyways -- and could allow users to work around current and future Bech32m encoding compatibility issues (though our goal should be to ensure compatibility).

Pros

  • Uses Bech32, like other Cosmos chains
  • Fits in 32 bytes -- looks exactly like a cosmos except for a different Bech32 HRP
  • As an alternate encoding, requires no changes to other parts of Penumbra, can be parsed directly into an Address

Cons

  • A third address format adds UX complexity
  • The truncated address points at a random account number
  • Only one truncated address per IVK (reuse of the address causes linkability)
  • Not compatible with FMD (clues created with the truncated address are not detectable)

How it works

A Penumbra address has three components: the diversifier d, the transmission key pk_d, and the clue key ck_d. To truncate the address to a 32-byte encoding:

  • The diversifier is required to be [0; 16], omitted from the encoding, and filled in with the hardcoded value during decoding;
  • The transmission key is encoded as 32 bytes;
  • The clue key is stripped, and filled in on the decoding side with the identity element.

The fixed diversifier means that there's only one per IVK, and also that the truncated address points at a random sub-account: the fixed value [0u8; 16] is decrypted with the diversifier key to a random address index. Stripping the clue key and filling it in with a dummy value means that generated clues are not detectable, but should not have other effects on the protocol, and since FMD is currently unused this seems fine.

To allow the use of truncated addresses in IBC transfers, the Ics20Withdrawal action would need to be changed to have a use_truncated_address parameter.

Issue ticket number and link

No issue - starting point for forum discussion

Checklist before requesting a review

  • I have added guiding text to explain how a reviewer should test these changes. N/A - starting point for discussion

  • If this code contains consensus-breaking changes, I have added the "consensus-breaking" label.

@hdevalence hdevalence added the consensus-breaking breaking change to execution of on-chain data label Dec 3, 2024
@hdevalence
Copy link
Member Author

Truncated address: ptrunc16t45lct6x4w6an6hvv7zp9p49qx0teg78qyukcgfk76ch87leggq0wlmdc
Reconstructed address: penumbra169wv6gf7r2fp0p0z6dsdlr7ntjetp2v2s4q77l434vw2qsdg3lqzlpyy6krgueygk0z3m2c77galpszc6q053nd4tjyu03d6hawlw4992ahdasgazqy54k39g2ezu536etjzkr
Address index: AddressIndex { account: 1587124636, randomizer: "c4c0afca894fb03acc8465c9" }
Actual address for index: penumbra1yy6frkygnsmut4ad47zwcnkg35ywmld3pekg28qge0t3c4y8vfjhaavadu8czpmlksgp6jht3q42h45qjf6fvyhet20vdgvspd76gj2h8fhaqa8raewhy48zp4c36zgw0nqj37

@hdevalence
Copy link
Member Author

hdevalence commented Dec 11, 2024

Since UIP-7 changes the Ics20Withdrawal action to deprecate the old compat encoding and allow specifying a t-addr, I think we'll want to have some other changes:

  • Add an Address::encode_as_transparent_address() -> Option<String> that checks that d, ck_d are zero and returns Some(encoding) if so or None if not; refactor the existing generation/encoding logic through this method
  • Add the new flag to the ICS20 withdrawal action
    • I think we should keep the old flag in the domain types even if it's ignored (so that we don't have issues parsing old txs -- prost will drop unknown fields causing things to stop round tripping)
  • Change existing IBC logic to ignore the compat flag and use the t-addr flag instead
  • Add cli flags to pcli that use the new field

Then we can test logic by creating a new testnet with this branch, connect to noble testnet, and make testnet USDC transfers with t-addresses

@hdevalence hdevalence changed the title [WIP]: Truncated address formats [WIP]: UIP-7: Transparent Addresses Dec 11, 2024
@@ -264,10 +264,15 @@ pub enum TxCmd {
/// The selected fee tier to multiply the fee amount by.
#[clap(short, long, default_value_t)]
fee_tier: FeeTier,
/// Whether to use a Bech32(non-m) address for the withdrawal.
/// Whether to use a Bech32(non-m) address for the withdrawal (deprecated).
/// Required for some chains for a successful acknowledgement.
#[clap(long)]
use_compat_address: bool,
Copy link
Member

@redshiftzero redshiftzero Dec 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to keep the use_compat_address option for any reason or good to remove it from pcli?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could drop it from pcli

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we want to have a pcli view address --transparent-address similar to pcli view address --compat?

This will be handy for testing roundtrips.

@cronokirby
Copy link
Contributor

Just an fyi, I think we want to be directing this towards release/0.81.x instead of main, right?

@redshiftzero
Copy link
Member

oops yeah, thanks!

@conorsch
Copy link
Contributor

conorsch commented Dec 13, 2024

N.B. I just rebased release/v0.81.x on top of latest main, 707192c, to ensure we have the fixes from 0.80.10 that ensure relayer stability, and then rebased truncated-addresses on top of the refreshed release/v0.81.x, and force-pushed.

@erwanor erwanor merged commit 83e5b69 into release/v0.81.x Dec 13, 2024
12 checks passed
@erwanor erwanor deleted the truncated-addresses branch December 13, 2024 22:23
conorsch pushed a commit that referenced this pull request Dec 14, 2024
This commit experiments with a "truncated" address format, which aims at
maximizing compatibility with other chains. This is kind of a janky
hack, but may be worth doing anyways -- and could allow users to work
around current and future Bech32m encoding compatibility issues (though
our goal should be to ensure compatibility).

- Uses Bech32, like other Cosmos chains
- Fits in 32 bytes -- looks exactly like a cosmos except for a different
Bech32 HRP
- As an alternate encoding, requires no changes to other parts of
Penumbra, can be parsed directly into an `Address`

- A third address format adds UX complexity
- The truncated address points at a random account number
- Only one truncated address per IVK (reuse of the address causes
linkability)
- Not compatible with FMD (clues created with the truncated address are
not detectable)

A Penumbra address has three components: the diversifier `d`, the
transmission key `pk_d`, and the clue key `ck_d`. To truncate the
address to a 32-byte encoding:
- The diversifier is required to be `[0; 16]`, omitted from the
encoding, and filled in with the hardcoded value during decoding;
- The transmission key is encoded as 32 bytes;
- The clue key is stripped, and filled in on the decoding side with the
identity element.

The fixed diversifier means that there's only one per IVK, and also that
the truncated address points at a random sub-account: the fixed value
`[0u8; 16]` is decrypted with the diversifier key to a random address
index. Stripping the clue key and filling it in with a dummy value means
that generated clues are not detectable, but should not have other
effects on the protocol, and since FMD is currently unused this seems
fine.

To allow the use of truncated addresses in IBC transfers, the
`Ics20Withdrawal` action would need to be changed to have a
`use_truncated_address` parameter.

No issue - starting point for forum discussion

- [ ] ~~I have added guiding text to explain how a reviewer should test
these changes.~~ N/A - starting point for discussion

- [x] If this code contains consensus-breaking changes, I have added the
"consensus-breaking" label.

---------

Co-authored-by: redshiftzero <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking breaking change to execution of on-chain data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants