-
Notifications
You must be signed in to change notification settings - Fork 56
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
Account ID network identifier and checksum #1024
Comments
I think we should probably go either directly with
I like this, but we could probably just move the whole last byte of the prefix to the front (and maybe pad with two zeros) - this way, for a given version/type/storage mode, the first two characters will remain stable.
It would be great for the human-readable part to include two things:
Regarding the second point: right now we deal only with account IDs, but there could be other types of "addresses" in the future that we may want to encode. For example, we could have a pay-to-Falcon-public-key ( At the same time, we probably don't want to make the HRP too long. One option is to make it very compact. For example, use
|
That's a good point. It would be great to make it extensible so we can add more address types in the future. One of the issues with using a very compact hrp is that it may easily conflict with one of the registered HRPs (https://github.com/satoshilabs/slips/blob/master/slip-0173.md) and I think we should try to avoid that. To be fair, it looks like none of your suggestions currently collide, but with this compact scheme it seems likely this could happen as more variants are added. Also, if we want to register ours in that registry (which we might want to do), then we'd have a lot of them to register. Since the address is one of the most end-user-facing things there is, I think simplicity and readability are the most important thing. Ideally it should be very easy for a user to determine whether an address is for Miden (mainnet) and not any other blockchain or Miden testnet/devnet. For that reason, I think we should avoid encoding things into the hrp as it won't have any meaning for users and because it doesn't seem to be intended for that use case. Instead we can encode this in the data part of bech32. So I would still prefer Address Type in Data Part For the address type we could prepend it to the data part, again making it a multiple of #[repr(u8)]
pub enum AddressType {
AccountId = 0,
FalconKey = 8,
} and then an account ID will have a
That way, those who know (devs) can still distinguish the address types but for end-users it'll be just part of the address. Is there a reason why end-users would have to know what type of an address it is and therefore why it perhaps should be part of the HRP? Address Type in HRP Something I would find less appealing but could also be fine with is if we append a single character after the network identifier to encode address types. The hrp is part of the checksum, so we don't loose any safety there. This could look like this:
But this would be pretty much the same as encoding it in the data part, and it looks less clean to me because it reads "midena" rather than "miden". |
Ah - I didn't know about this registry, but it seems like it still may be OK to use a compact identifier. For example, Bitcoin uses So, I'm thinking for HRP we could use:
As far as I can tell, none of these are currently used.
Yes, good point - let's move the "address type" into the data portion. I also like the idea of using a single letter to define address type - i.g.,
I would maybe still move the byte upfront (to go right after the address type character). So, putting this all together, we could have:
One other thing that came to mind - is there actually high enough value in distinguishing between mainnet/testnet/devent address? It seems to me that distinguishing between them may come with significant code complexity. Maybe just using |
I think there is user value. People do often cross-contaminate addresses so removing a common mistake could be worth it. We should be able to find a nice abstraction for this on the node side I think. Is there actually a way to identify the node's network? Right now the node doesn't know what network it is. Since this is protocol level this means the protocol needs to be somewhat network aware then? @PhilippGackstatter would this scheme allow for custom networks? As in, how do I run a local network, or even a public unofficial one? Would it have to masquerade as a main/test/devnet? Or is there some mapping function given some name/network string? |
Agreed there is value. I was thinking more of whether it outweighs the complexities on the code side. For example, i'm imagining that we'd add something like
|
Agreed as well. There are cases where people sent funds to testnet addresses and lost them.
I assumed we would eventually need a
While we run in a centralized setting, 1. is probably not essential, since we have full control so using constants is fine, as we can just recompile. But eventually you'd want to be able to specify two protocol versions at the same time and define a block heigh at which you switch to the new one. For a given network, say mainnet or a "localhost network" we would initialize the node with a protocol config. The client would need to fetch this from the node and use it in various places, one of them being when encoding/decoding an
I think this wouldn't work for the webClient and any other bindings afaict, as typically, users can't change the feature flags. Plus that would imply they need a rust toolchain. So unfortunately, I think we need to make this a runtime configuration. |
Starknet does this slightly differently by adding the network ID into tx hashes. To facilitate this, the RPC also has a I can't really evaluate the trade-off.
I would definitely prefer option (1) 😅 Controlling execution via feature flags is almost never the right thing imo. The I imagine |
As a follow-up to the AccountID changes in #140, we can consider adding a user-facing encoding for account IDs. The purpose of this encoding should be
The reason for not including these in the account ID directly is to keep the ID as small as it can be, since it will be used within the VM. Moreover, these two properties can be ensured at some point higher in the stack (e.g. in RPC APIs). Here the encoded ID can be verified to be for the right network, that its checksum is correct and then unwrapped into the actual account ID. From that point on, only the ID is passed around. This layered approach seems better and sufficient to keep the lower layers network agnostic.
Bech32
One option is to use bech32:
Bitcoin previously used base58 encoding and then moved to bech32, though I think both are still in use.
The motivation for using bech32 over base58 is described in the motivation section of the BIP: https://github.com/bitcoin/bips/blob/master/bip-0173.mediawiki#motivation
The following examples use the
bech32
crate with the latestBech32m
checksum algorithm from https://github.com/bitcoin/bips/blob/master/bip-0350.mediawiki. So bech32 has the Checksum property built in.Its human-readable part gives us the Network Identifier. For example, using
"miden"
as the HRP for mainnet, an account ID would look like so:This is the printed account ID encoded in bech32.
One optional thing we could do is prepend an additional byte at the beginning of the byte encoded account ID. This first byte could be defined as
account_id_version * 8
. Choosing a multiple of 8 means the first character after the1
separator is different. This means one can tell apart different account ID versions by looking at this first character.Each version would then start with a unique character:
This might be nice to have, but again, optional.
Overall bech32 gives us the checksum, a network identifier of our choice and an implementation via the
bech32
crate which seems well-maintained and with significant usage (i.e. little risk of loosing maintenance). Bech32 appears well thought through and matches our use case very closely (ID/address encoding).Base 58 Check
There is a base58 check implementation which appears maintained: https://docs.rs/base58ck/0.2.0/base58ck/
Although with the arguments given in the above motivation for bech32 I'm not sure it's worth using. We'd add a dependence on SHA-256 with this. On the plus side the encoded address is shorter.
Hex Encoding with checksum
We can also make our own encoding in hex and add a byte for the network identifier and a custom checksum (e.g. using the
crc
crate), but we'd have to do more significant engineering work to ensure its correctness and error detection capability over a more battle-tested solution.Other Options
Are there other good options I've missed? If you have any thoughts or opinions on this, feel free to comment.
The text was updated successfully, but these errors were encountered: