feat(core, proto)!: add bech32m addresses #1124

SuperFluffy · 2024-05-30T14:57:46Z

Summary

Adds bech32m addresses.

Background

bech32m encoded addresses are the de-facto standard in cosmos. By using a "astria" prefix (in the human readable prefix, bech32 HRP sense) we align astria with the rest of the cosmos ecosystem.

Changes

Add a field bech32m to astria.primitive.v1.Address
Deprecate astria.primitive.v1.Address.inner
Change std::fmt::Display for Address to use bech32m encoding
Serialize Address in terms of its protobuf-json mapping
Update the Address:try_from_raw constructor to allow ingesting protobufs that have their inner or bech32m or populated. inner and bech32m are mutually exclusive, only one may be set.
Protobufs with inner set are assumed to have prefix "astria".
Introduce an AddressBuilder type state builder that requires both a prefix and array/byte slice be set.
Update all services to the new way of constructing addresses, but with the prefix "astria" set everywhere
Add a SignedTransaction::address method that constructs an address from the verification key and the chain_id stored in the unsigned transaction's transaction_params::chain_id.
Provide a builder for TransactionParams enforcing that chain IDs are have hrp compatible names.

Testing

update snapshot tests for address-as-json serialization
add unit tests:
- addresses with only bytes are accepted
- addresses with only bech32m are accepted
- addresses with both bytes and bech32m are are rejected
- protobufs created from the common Address type only have bech32m populated
- various tests to assert Address invariants (mainly maximum length of prefix + fixed-size address always being encodeable to a string)
- updated all tests that somehow make use of addresses (mainly sequencer, bridge-withdrawer, composer)

Breaking Changelist

This patch is marked breaking because the sequencer snapshot tests are triggered. They all contain actions that themselves contain protobuf address. This patch is backward but not forward compatible.

Related Issues

closes #943

crates/astria-core/src/primitive/v1/mod.rs

noot

looks good!

crates/astria-core/src/primitive/v1/mod.rs

joroshiba · 2024-06-04T20:14:37Z

...uencer__app__tests_breaking_changes__app_execute_transaction_with_every_action_snapshot.snap

why are the app_hashes changing? I would expect the data hash to change but not the app_hash?

It contains a SudoAddressChangeAction, which itself contains new_address: Address. Hence the change.

proto/primitives/astria/primitive/v1/types.proto

… bytes and bech32m fields mutually exclusive

SuperFluffy · 2024-06-06T17:54:42Z

After the review comments I received from @joroshiba and @noot I went back to the drawing board and made the Address type prefix aware. This lead to some far reaching changes that I am listing here:

I have added a deprecated = true flag to the astria::primitive::v1::Address::inner field.
protobuf Addresses with an inner field set are assumed to have "astria" as their prefix.
Address::inner and Adress::bech32m are mutually exclusive: only one may be set.
When going from the common Address type to the protobuf Address message type, only bech32m is populated, inner is left empty.

All services assume a prefix of "astria", but it will be easy to make that configurable.

SuperFluffy · 2024-06-06T18:26:29Z

crates/astria-core/src/crypto.rs

@@ -147,28 +140,28 @@ impl VerificationKey {
    // Silence the clippy lint because the function body asserts that the panic
    // cannot happen.
    #[allow(clippy::missing_panics_doc)]
-    pub fn address(&self) -> &Address {
-        self.address.get_or_init(|| {


Removed the memoization for the time being. We can bring it back later.

crates/astria-core/src/primitive/v1/mod.rs

noot · 2024-06-07T17:07:27Z

crates/astria-sequencer/src/app/mod.rs

@@ -961,7 +961,7 @@ impl App {
    /// Executes a signed transaction.
    #[instrument(name = "App::execute_transaction", skip_all, fields(
        signed_transaction_hash = %telemetry::display::base64(&signed_tx.sha256_of_proto_encoding()),
-        sender = %signed_tx.verification_key().address(),
+        sender = %telemetry::display::base64(&signed_tx.verification_key().address_bytes()),


should this display as bech32?

Can't right now because all we have is the sender (and hence the verification key), but no prefix, and hence no bech32m.

I am not sure how to address this at this point.

noot · 2024-06-07T17:09:30Z

crates/astria-sequencer/src/mempool.rs

@@ -62,13 +62,16 @@ impl PartialOrd for TransactionPriority {
 pub(crate) struct EnqueuedTransaction {
    tx_hash: [u8; 32],
    signed_tx: Arc<SignedTransaction>,
+    address: Address,


this seems weird, is this for caching purposes?

So signed_tx.verification_key().address() was used somewhere in this mempool module - I would need to read the code again, but basically I have replaced all instances of .address() by this Address. Maybe that's actually unnecessary and the new address_bytes() would be sufficient.

… params

* main: fix: ignore RUSTSEC-2021-0139 (#1171) chore(sequencer-relayer)!: remove functionality to restrict relaying blocks to only those proposed by a given validator (#1168) chore(metrics): update `metric_name` macro to handle a collection of names (#1163) fix(bridge-withdrawer): skip linting generated contract code (#1172) fix(core, sequencer): prefix removal source non-refund ics20 packet (#1162) chore(docs): add sequencer-relayer doc to specs (#1126) feat(bridge-withdrawer): sync logic (#1165) chore(withdrawer): replace contracts with `astria-bridge-contracts` submodule (#1164) feat(sequencer)!: implement bridge sudo and withdrawer addresses (#1142) feat(sequencer): implement refund to rollup logic upon ics20 transfer refund (#1161) feat(bridge-withdrawer): bridge withdrawer startup (#1160) feat(core, proto)!: add bech32m addresses (#1124) feat(withdrawer): bridged ERC20 token withdrawals (#1149) feat(sequencer-relayer)!: add chain IDs for sequencer and Celestia to config env vars (#1063) test(bridge-withdrawer): add submitter tests (#1133) chore: bump penumbra deps (#1159) feat(sequencer): implement `bridge/account_last_tx_hash` abci query (#1158) fix(withdrawer): use block subscription in batcher; send to destination_chain_address (#1157) fix(withdrawer): update AstriaWithdrawer to check that withdrawal value is sufficient (#1148) chore(ci): build bridge withdrawer images (#1156)

## Summary This patch allows configuring the base address prefix in sequencer's genesis file and enforces this prefix in all actions. Similar to the native fee, the base prefix is set once during `init-chain` and is then available globally and never changed. ## Background #1124 changed astria addresses from opaque bytes to bech32m addresses, which themselves are encoded as strings on the wire. Where previously sequencer implicitly assumed that all addresses were `"astria"` prefixed bech32m addresses, this is now enforced. ## Changes - Update sequencer `GenesisState` to use the canonical serde implementation of all `astria-core` types. - Add a field `.address_prefixes.base` to the `GenesisState`. - Ensure that all addresses in `GenesisState` are consistenst with the prefix. - Inject the genesis base address prefix into sequencer's global state. - Add a check for the global address prefix to all sequencer actions that carry a `astria.primitive.v1.core.Address` (this is done via `AppHandler::check_stateless`). These are specifically: - `TransferAction` - `SudoAddressChangeAction` - `BridgeLockAction` - `BridgeSudoChangeAction` - `BridgeUnlockAction` - `InitBridgeAccountAction` - `IbcRelayerChangeAction` - Slightly change all storage keys that use addresses to be explicit about using bytes and provide snapshot tests (specifically in `accounts::state_ext`, `bridge::state_ext`, `ibc::state_ext`) - Removed: unsigned transaction parameters no longer require a bech32m chain ID name - Update composer, bridge-withdrawer to have a configurable address prefix: - `ASTRIA_BRIDGE_WITHDRAWER_SEQUENCER_ADDRESS_PREFIX` - `ASTRIA_COMPOSER_SEQUENCER_ADDRESS_PREFIX` - Update all `astria-cli` commands to take a configurable prefix. ## Testing + Update all tests and ensure they still run. + Add tests for genesis construction. + Add genesis snapshot tests. + Add snapshots tests for IBC denoms (to accompany the genesis snapshot) ## Breaking Changelist This change is breaking at the network level requiring sequencer re-genesis. Non-bech32m addresses in actions are now rejected, as well as addresses that have a prefix other then the one set at genesis. In addition, the storage key for all collections involving an address have changed. --------- Co-authored-by: Jordan Oroshiba <jordan@astria.org>

## Summary Removes the non-bech32m address bytes field. ## Background The change introduced in #1124 is a breaking change that ripples through the rest of the Astria stack. While in principle we can keep the non-bech32m address around for backward compatibility, in practice it's cleaner to regenesis. ## Changes - Remove `astria.primitive.v1.Address.inner`, reserving its number and name. ## Testing Updates and removed all unit tests. All blackbox tests that use addresses in some form still work. ## Breaking Changelist Since this is protobuf breaking it is also network breaking.

## Summary Added a lazily-initialized field `address_bytes` to `VerificationKey`. ## Background Testing showed that `address_bytes` was called multiple times on a given `VerificationKey` (up to 11 times in some cases). Each time the key's bytes were being hashed, so this change ensures that hashing only happens once for a given verification key instance. Note that this was implemented previously in #1111 and was then reverted in #1124. However, when reverted, the manual impls of `PartialEq`, `Eq`, `PartialOrd`, `Ord` and `Hash` were left as-is, as were the unit tests for these. Hence this PR doesn't need to make any changes to these trait impls or tests. ## Changes - Added `address_bytes: OnceLock<[u8; ADDRESS_LEN]>` to `VerificiationKey`. ## Testing No new tests required. ## Related Issues Closes #1351.

feat(core, proto): add bech32m addresses

13aae9c

SuperFluffy marked this pull request as ready for review May 30, 2024 14:57

SuperFluffy requested review from a team, joroshiba and noot as code owners May 30, 2024 14:57

github-actions bot added the proto pertaining to the Astria Protobuf spec label May 30, 2024

SuperFluffy temporarily deployed to BUF May 30, 2024 14:58 — with GitHub Actions Inactive

SuperFluffy added the core pertaining to the astria-core crate label May 30, 2024

ensure proto changes are not breaking (json name)

7615490

SuperFluffy temporarily deployed to BUF May 30, 2024 15:03 — with GitHub Actions Inactive

noot reviewed May 31, 2024

View reviewed changes

crates/astria-core/src/primitive/v1/mod.rs Outdated Show resolved Hide resolved

noot approved these changes Jun 3, 2024

View reviewed changes

Merge branch 'main' into superfluffy/bech32-addresses

d8346b8

SuperFluffy temporarily deployed to BUF June 4, 2024 12:40 — with GitHub Actions Inactive

accept breaking changes

b9ab7f3

github-actions bot added the sequencer pertaining to the astria-sequencer crate label Jun 4, 2024

SuperFluffy temporarily deployed to BUF June 4, 2024 13:04 — with GitHub Actions Inactive

SuperFluffy changed the title ~~feat(core, proto): add bech32m addresses~~ feat(core, proto)!: add bech32m addresses Jun 4, 2024

add tests, remove deprecation notice on address bytes

8ca0479

SuperFluffy temporarily deployed to BUF June 4, 2024 13:19 — with GitHub Actions Inactive

clippy

fb522c3

SuperFluffy temporarily deployed to BUF June 4, 2024 13:32 — with GitHub Actions Inactive

joroshiba reviewed Jun 4, 2024

View reviewed changes

make addresses prefix aware

64a3682

SuperFluffy requested a review from noot June 6, 2024 16:26

SuperFluffy added 3 commits June 6, 2024 19:25

rip out arrayvec, ensure that prefixes are valid hrp, make deprecated…

cf5de1b

… bytes and bech32m fields mutually exclusive

add comments on protobuf address message

e369919

Merge branch 'main' into superfluffy/bech32-addresses

9098d36

github-actions bot added the composer pertaining to composer label Jun 6, 2024

use astria address prefix constant everywhere

ff3f68d

SuperFluffy temporarily deployed to BUF June 6, 2024 18:18 — with GitHub Actions Inactive

clippy

5de78bc

SuperFluffy temporarily deployed to BUF June 6, 2024 18:20 — with GitHub Actions Inactive

fix doctest, clippy

6b70b86

SuperFluffy temporarily deployed to BUF June 6, 2024 18:25 — with GitHub Actions Inactive

SuperFluffy commented Jun 6, 2024

View reviewed changes

SuperFluffy requested a review from joroshiba June 7, 2024 09:26

noot reviewed Jun 7, 2024

View reviewed changes

crates/astria-core/src/primitive/v1/mod.rs Outdated Show resolved Hide resolved

noot reviewed Jun 7, 2024

View reviewed changes

SuperFluffy added 2 commits June 7, 2024 20:25

remove commented out code

7ccb839

provide address method on signed tx, stricter builder for transaction…

e81d360

… params

SuperFluffy temporarily deployed to BUF June 7, 2024 20:08 — with GitHub Actions Inactive

clippy

4eca1f5

SuperFluffy temporarily deployed to BUF June 7, 2024 20:13 — with GitHub Actions Inactive

SuperFluffy enabled auto-merge June 7, 2024 20:14

SuperFluffy added this pull request to the merge queue Jun 7, 2024

Merged via the queue into main with commit ab8705f Jun 7, 2024
41 checks passed

SuperFluffy deleted the superfluffy/bech32-addresses branch June 7, 2024 20:26

This was referenced Jun 17, 2024

chore(core, proto)!: remove non-bech32m address bytes #1186

Merged

feat(sequencer)!: allow configuring base address prefix #1201

Merged

Fraser999 mentioned this pull request Sep 4, 2024

chore: memoize address_bytes of verification key #1444

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core, proto)!: add bech32m addresses #1124

feat(core, proto)!: add bech32m addresses #1124

SuperFluffy commented May 30, 2024 •

edited

Loading

noot left a comment

joroshiba Jun 4, 2024

SuperFluffy Jun 5, 2024

SuperFluffy commented Jun 6, 2024 •

edited

Loading

SuperFluffy Jun 6, 2024

noot Jun 7, 2024

SuperFluffy Jun 7, 2024

noot Jun 7, 2024

SuperFluffy Jun 7, 2024

feat(core, proto)!: add bech32m addresses #1124

feat(core, proto)!: add bech32m addresses #1124

Conversation

SuperFluffy commented May 30, 2024 • edited Loading

Summary

Background

Changes

Testing

Breaking Changelist

Related Issues

noot left a comment

Choose a reason for hiding this comment

joroshiba Jun 4, 2024

Choose a reason for hiding this comment

SuperFluffy Jun 5, 2024

Choose a reason for hiding this comment

SuperFluffy commented Jun 6, 2024 • edited Loading

SuperFluffy Jun 6, 2024

Choose a reason for hiding this comment

noot Jun 7, 2024

Choose a reason for hiding this comment

SuperFluffy Jun 7, 2024

Choose a reason for hiding this comment

noot Jun 7, 2024

Choose a reason for hiding this comment

SuperFluffy Jun 7, 2024

Choose a reason for hiding this comment

SuperFluffy commented May 30, 2024 •

edited

Loading

SuperFluffy commented Jun 6, 2024 •

edited

Loading