BIP139: Wallet Metadata Backup Format#2130
Conversation
aa3b808 to
45b4a5b
Compare
|
Could you perhaps expound the relationship to #1951? |
seedhammer
left a comment
There was a problem hiding this comment.
This BIP seems to overlap #2099. It would be ideal to have a single way of encoding wallet metadata.
| - `timestamp`: Optional integer Unix timestamp representing account creation time in | ||
| seconds. | ||
| - `iso_8601_datetime`: optional string representing account creation time in ISO 8601 | ||
| format. | ||
| - `block_height`: Optional integer representing account creation time in bitcoin block | ||
| height unit. |
There was a problem hiding this comment.
3 different ways of representing the birth date doesn't seem ideal. Also "iso_8601_datetime" specifies the data format, not the purpose of the field. I'd prefer short, purposeful names (e.g. "birthblock" or "birthdate") and leave the type and format out of the name.
There was a problem hiding this comment.
I'm open to represent in a different way, but few points why it's end up like this:
- we use a backup format based on this in Liana with the
timestampfield - we had a discussion w/ @ethicnology here where he seems prefering iso 8601, I've used a simplified approach in this draft compared to what discussed, but I'm eager to get feedback.
- it comes to my mind later that
block_heightrather than unix timestamp could make more sense
There was a problem hiding this comment.
I was about to comment on there being three different birth time fields as well. Seconding @seedhammer’s concern here.
| - `version`: Optional integer version of the backup format. | ||
| - `bip`: Optional integer value representing the number of this BIP. | ||
| - `name`: Optional string wallet name. | ||
| NOTE: `alias` is an alias of `name`. |
There was a problem hiding this comment.
Why mention alias in a new BIP?
There was a problem hiding this comment.
because we already use name in Liana backup format, but in the end I think alias is more clear
| - `name`: Optional string account name. | ||
| NOTE: `alias` is an alias of `name`. | ||
| - `description`: Optional string account description. | ||
| - `active`: Optional boolean field indicating if the account is active. |
There was a problem hiding this comment.
active doesn't seem very useful for a backup. In fact, some of these fields feel like the union of all possible metadata fields useful to any wallet software. If so, this leads to the question: what is the standard for adding new fields to this standard?
I think it's better to specify a small(er) set of widely useful fields.
There was a problem hiding this comment.
iirc it was requested by @ethicnology to represent an usage they already have in Bull Bitcoin wallet
There was a problem hiding this comment.
and I just notice it is also something tracked in bitcoin core:
https://github.com/bitcoin/bitcoin/blob/2b541eeb363cd43fc225c7c7f9691c925b0e4f9d/src/wallet/rpc/backup.cpp#L515
| - `bip39_mnemonic`: Optional string containing mnemonic words following BIP39. | ||
| Since backups may be stored online, storing mainnet mnemonics is strongly discouraged. |
There was a problem hiding this comment.
It feels wrong to specify a field which is immediately "strongly discouraged".
There was a problem hiding this comment.
Need a rewording, I think mainnet mnemonic should be strongly discouraged, but it could be useful for testnet, like in a bug report, join a testnet/signet/regtest backup of a wllet can make it easy to import the wallet state in order to reproduce the bug.
| - `network`: Optional string network identifier. | ||
| Valid values are `bitcoin` (mainnet), `testnet3`, `testnet4`, `signet`, and `regtest`. |
There was a problem hiding this comment.
Non-main networks are not useful for backups. Can the "network" field be left out entirely?
There was a problem hiding this comment.
cf previous message for rationale
|
Thanks for reviewing related work, @seedhammer! |
|
Thanks for review @seedhammer, I'll reply inline |
I think they have different use case:
|
|
Something that I should add in the bip abstract is, that I see this bip as a "central repository" where a wallet implementer can "register" a field usage & share it's usage of the format in a test vector. The intend is not to force anyone to be interoperable in both way with every other implem, but rather to make it easy if one want to be interoperable in at least one way. |
45b4a5b to
4b692ac
Compare
4b692ac to
761dd17
Compare
761dd17 to
aeee423
Compare
aeee423 to
32a6893
Compare
murchandamus
left a comment
There was a problem hiding this comment.
Just a quick first pass. I think I’d be interested in seeing a few more wallet developers take a look at this and provide their thoughts on how this would match their needs.
3d74fd1 to
1fc70c4
Compare
|
@murchandamus thanks for your review, comments addressed. |
1fc70c4 to
087708d
Compare
|
Assigned BIP139 |
b0b0667 to
b17c8a5
Compare
|
It took me a moment to figure out what the issue was: the preamble needed to be indented. |
|
|
||
| ## Encryption | ||
|
|
||
| This format can be encrypted following [BIP-XXXX](https://github.com/bitcoin/bips/pull/1951). |
There was a problem hiding this comment.
| This format can be encrypted following [BIP-XXXX](https://github.com/bitcoin/bips/pull/1951). | |
| This format can be encrypted following [BIP-0138](https://github.com/bitcoin/bips/pull/1951). |
|
I'm not sure if it's a good idea to create one kitchen-sink JSON blob for everything. BIP138 can easily handle multiple JSON blobs each specified in their own BIP. |
|
Hey, any update on this? Is there something I could help with? I was waiting for the open review to be addressed to take another look, but I wanted to check that we are not crossing our wires in regard to where it’s currently held up. |
b17c8a5 to
94fdbc5
Compare
I think the OP was misleading, look like I've copy/pasted from BIP-138 PR w/o edit. This spec is only related to BIP-139 in the way that it could be encryptd with it but have an independent purpose. |
Thanks for the BIP # @murchandamus! Sorry for late reply, I was a bit underwater on past weeks, I think all comments are adressed now. |
murchandamus
left a comment
There was a problem hiding this comment.
Thanks for the update. I gave this another read and have a few more comments. Where do you see this document regarding your planned work. It seems like it is getting close to publication, if that fits your plans.
If you have some specific people in mind that should perhaps review this document, I could try to help get their attention.
| This BIP also acts as a central repository where a wallet implementer can register its | ||
| usage of a field and share that usage through a test vector. |
There was a problem hiding this comment.
I would advise against collecting fields in the BIP directly. We have had several BIPs in the past that were extensible in this form and in each case the authors eventually got tired of reviewing amendments to their BIP. I would suggest that you instead pursue a model like BIP174 (PSBT), where only the registering of the new fields in collected centrally in an auxiliary file for this express purpose, but fields have to be described in their own BIPs. — Maybe separate BIPs are a bit too high of a hurdle in this specific case and there could instead be a description of the fields in the registry directly, but I would heavily recommend not putting the onus of frequently needing to review BIP amendments on your future self.
| ### version | ||
|
|
||
| This BIP defines version 1 of this specification. |
There was a problem hiding this comment.
Nit: BIPs themselves have now an optional version header, so this may be confusing in the future if the document gets updated.
I assume you are talking about "version 1 of the backup format", i.e. the value stored with the version key in the backup, maybe this could be stated a bit more explicitly, though.
| `bip_392` (silent payments), or any arbitrary string representing metadata needed to | ||
| find and spend coins for an account. | ||
| - `output_type`: Optional string describing the output category of the account. | ||
| Values used by bitcoin core are `legacy`, `p2sh-segwit`, `bech32`, and `bech32m`. |
There was a problem hiding this comment.
Spelled with capital letters just below (and should be spelled capitalized).
| Values used by bitcoin core are `legacy`, `p2sh-segwit`, `bech32`, and `bech32m`. | |
| Values used by Bitcoin Core are `legacy`, `p2sh-segwit`, `bech32`, and `bech32m`. |
| - `change_descriptor`: Optional string or object representing an explicit change-side | ||
| descriptor, paired with `descriptor`. Intended for wallets that do not use BIP-389 | ||
| multipath descriptors (e.g. Bitcoin Core). |
There was a problem hiding this comment.
Ambiguous whether Bitcoin Core is an example or counter-example:
| - `change_descriptor`: Optional string or object representing an explicit change-side | |
| descriptor, paired with `descriptor`. Intended for wallets that do not use BIP-389 | |
| multipath descriptors (e.g. Bitcoin Core). | |
| - `change_descriptor`: Optional string or object representing an explicit change-side | |
| descriptor, paired with `descriptor`. Intended for wallets that do not use BIP-389 | |
| multipath descriptors (as e.g. Bitcoin Core does). |
| - `descriptor_id`: Optional string containing a stable hexadecimal identifier for the | ||
| receive `descriptor`. Its construction is implementation-defined, but it MUST be stable | ||
| across exports of the same descriptor. |
There was a problem hiding this comment.
If you have an idea how to standardize descriptor identifiers, I think that might be a good BIP in itself to write.
cc: @achow101
| - `timestamp`: Optional integer Unix timestamp representing account creation time in | ||
| seconds. | ||
| - `iso_8601_datetime`: optional string representing account creation time in ISO 8601 | ||
| format. | ||
| - `block_height`: Optional integer representing account creation time in bitcoin block | ||
| height unit. |
There was a problem hiding this comment.
I was about to comment on there being three different birth time fields as well. Seconding @seedhammer’s concern here.
| - `key`: Optional string representing the public key fingerprint in hexadecimal form. | ||
| - `alias`: Optional string user-defined alias for the key. | ||
| - `role`: Optional string role of the key in wallet operations. | ||
| See [Key Roles](#key-roles). | ||
| - `key_type`: Optional string describing ownership of the key. | ||
| See [Key Types](#key-types). | ||
| - `key_status`: Optional string describing the status of the key. | ||
| See [Key Status](#key-status). | ||
| - `bip85_derivation_path`: Optional string describing the | ||
| [BIP-0085](https://github.com/bitcoin/bips/blob/master/bip-0085.mediawiki) derivation | ||
| path used to derive this key from the master key. |
There was a problem hiding this comment.
It strikes me as odd that all fields here are optional. Is that intended? Perhaps there should be at least an "A or B is required"?
This is a bip for wallet metadata backup format.
Mailing list post: https://groups.google.com/g/bitcoindev/c/ylPeOnEIhO8