Skip to content

Instantly share code, notes, and snippets.

Embed
What would you like to do?

I'll first give some thoughts on the design of the wallet in general, before explaining current and future SegWit wallet support.

Current design

Conceptually, a wallet currently contains mapKeys, mapScripts, and setWatchOnly, which together determine which outputs are considered "ours" (IsMine), and how to solve and sign for them. Then the wallet further contains key metadata, key chains, and key pool at the wallet level that feed into this.

We determine whether outputs are ours using ad-hoc logic that mostly answers the question "Could we sign this?", but with some notable exceptions:

  • In the case of multisig outputs, all private keys must be available (rather than just the multisig's threshold).
  • For witness outputs, we require the P2SH-wrapped version of the witness program to be in mapScripts, even for non-P2SH witness outputs.
  • Outputs that are in setWatchOnly are ours regardless.

This logic has many problems:

  • It is overly complicated, doing too many things at once (it also answers whether something is solvable/signable)
  • Despite doing pretty much the same as signing, no code is shared between the two.
  • It is inefficient due to pattern matching all scripts and trying to recurse into them.
  • It is not selective: it can't be made to match just P2PKH or just P2PK or just P2WPKH, except by marking one of them as watch-only (which then removes the ability to sign).
  • It is hard to support extra key chains (for example publically derivable HD chains for hardware wallets).

Future design

In a future design I would like to move away from loose pieces of information that define what we consider ours when the puzzle pieces fit, and instead structure everything in records. Each record would correspond to an exact scriptPubKey (or set of scriptPubKeys), and contain several pieces of information that are currently spread out:

  • Output description:
    • Arbitrary scriptPubKey
      • redeemscript/witnessscript if needed
    • HD chain
      • scriptPubKey type(s) (P2PK, P2PKH, P2SH-P2WPKH, P2WPKH, ...); multiple can be present simultaneously
      • Minimal/maximal hdchain position (to accomodate converting pre-split-hd-and-then-upgraded wallets without changing master key)
    • Possibly more complicated records in the future (e.g. BIP124 like things)
  • Change or payment (let's not continue using the lack of a label to indicate change)
  • Optional private key information
  • Metadata which should be per address/chain rather than per key (birth date, purpose)

One record could be designated as default new payment address source, and one as default new change source. As public keys can't be derived without access to private keys in case of hardened derivation, a 'cache' with precomputed public keys is needed, which replaces keypools. Furthermore, there would need to be on-the-fly computed maps of public keys and scriptPubKeys (and inner scripts) based on the data in the record. Restricting the authorative information to just this list of records means wallet files can be small, and dumps can be very simple: just an encoding of all the records, one by one. For typical records a combined public/private encoding can be devised (like Electrum's yprv/zprv) even. A typical wallet dump would just be two extended private keys (one for change, one for not), with flags about what type scriptPubKeys to use.

One thing that does not fit in this model is labels, which will need to remain stored separately. Inherently, labels are not immutable data (they're added during normal wallet operation), but they're also not necessary for protecting balance or funds.

If a concept of 'watch only' remains, I think it should be independent from having the private key inside the wallet file. Just because your actual private key is on an airgapped system or on a hardware wallet does not mean the coins are any less yours. Watch-only could be a boolean inside records exclude it from normal view - something you perhaps want for keys/scripts that are involved in a multisig with other people, but don't actually consider fully yours.

Conversion of old wallet to new ones will probably be the trickiest part. It will involve a one-time operation at startup that tries to enumerate all possible scriptPubKeys we know about, and feed them through the old IsMine logic, converting the matching ones into ours map records and private keys.

In this new model the IsMine logic can go away:

  • Whether an output is ours will just be a simple check whether it occurs in the precomputed set of all scriptPubKeys defined by the records - entirely outside the scope of CKeyStore. This is efficient and easy to reason about.
  • For signing, CKeyStore can become a pure interface that exposes a function to get a script given its hash, and a function that exposes a key given its pubkey - nothing else. This interface gets implemented by the wallet as lookups into its records. As a result, we can still sign anything we have enough information for.
  • To determine solvability we just try to sign using a dummy signer.

To summarize, I think it's important to start thinking about wallets in terms of concise and immutable description records of what outputs are ours and how to solve and sign for them - even if that isn't how things are implemented immediately. This is in contrast to today where a wallet is an unstructured collection of keys, scripts, watch scripts, keypools, HD chains, and metadata, which all interact.

SegWit wallet support

When supporting SegWit addresses by default, we're effectively extending the existing implicit chain records to also cover P2WPKH and P2SH-P2WPKH forms. Different types of compatibility have different requirements:

  1. Problem
    • Scenario:
      • The user makes a backup
      • Upgrades to default-SegWit software
      • Creates a SegWit address
      • Restores the backup
    • Solutions:
      • 1a. Treat all keys (pre-existing or not) as implicitly SegWit-compatible, regardless of whether they have any form of marker.
      • 1b. First require an explicit wallet upgrade (that needs a backup) before SegWit addresses can be created.
    • My preference is solution (1a): it is easy to implement, has no impact on file size, and the alternative (1b) is not a very good user experience.
  2. Problem
    • Scenario:
      • The user upgrades to default-SegWit software
      • Makes a backup
      • Downgrades the software
      • Restores the backup
    • Solutions:
      • 2a. Automatically store the P2SH-P2WPKH script in the wallet for all keypool keys at first startup. This only works as long as we upgrade again before the original keypool runs out.
      • 2b. Make the wallet file incompatible with old versions at first startup (but don't require a new backup).
      • 2c. Ignore: assume downgrading while restoring a backup is not supported
    • My preference is (2b) if we want to deal with the versioning mess now, and (2c) otherwise. (2a) requires a significant blowup of the wallet file for now, without strong guarantees.
  3. Problem
    • Scenario:
      • The user upgrades to default-SegWit software
      • Creates a new address
      • Downgrades the software
    • Solutions:
      • 3a. Store the P2SH-P2WPKH script in the wallet for every newly created address.
      • 3b. Make the wallet file incompatible with old versions at first SegWit address creation (but don't require a new backup).
    • I'm fine with either solution. If (2b) is used, (3b) is implied.
  4. Problem
    • Scenario:
      • The user makes a backup
      • Upgrades to default-SegWit software
      • Creates a new address
      • Downgrades the software
      • Restores the backup
    • No solutions exist, as this situation is undetectable.
  5. Problem (only if 1a)
    • Scenario:
      • The user upgrades to default-SegWit software
      • Makes a backup
      • Creates a new address
      • Gets paid on new address
      • Restores backup
      • Downgrades the software
    • Solutions:
      • 5a. Automatically store P2SH-P2WPKH scripts to the wallet whenever a key is seen in a transaction

The SegWit wallet support PR (https://github.com/bitcoin/bitcoin/pull/11403)[#11403] currently implements (1a) and (3a). An alternative (which requires dealing with versioning issues) is (1a), (2b), and (3b).

Longer term, after migrating to the new design explained in the previous sections, I don't think our choices here matter much. (1a) means we'll have to keep watching for scriptPubKeys for SegWit versions of all old keys, but that's just an in-memory map that's slightly larger, with no effect on file size and presumably performance.

@Sjors

This comment has been minimized.

Copy link

commented Nov 17, 2017

Some links in case someone finds them useful:

  • mapKeys: is a mapping from public key to private key
  • mapScripts: maps script hash to the script itself (???)
  • setWatchOnly: as the name suggests, a set containing all watch-only (contains only scripts??)
  • IsMine

I'm confused about scenario 4. Are you assuming a non-hd wallet?

In your newly proposed scheme, would P2WPKH and P2SH-P2WPKH be the same record? If not, then can the same label point to two addresses (that happen to share a key)? The use case I have in mind here is where the wallet displays both a bech32/P2WPKH and P2SH-P2WPKH address, which the user communicates to someone, who will then pick either one depending on what their wallet supports. Or are these totally interchangeable?

@TheBlueMatt

This comment has been minimized.

Copy link

commented Nov 17, 2017

wrt 2. ii. How do you propose doing this? A -walletupgrade being the obvious solution, but requires we ship HD upgrade as well (I dont think this is that bad, but of course there's the wallets-needs-to-be-unlocked-to-do-this issue, which complicates things significantly, one option would be to add a walletupgrade RPC command to do it at runtime, since its a relatively simple thing when we're talking about HD). The other option would be to just bump it to 0xffffffff and have a second version field (or some other incompatibility, I believe the 0xffffffff bump was suggested by @jonasschnelli) but then we've accidentally introduced feature flags in a really roundabout (and kinda shitty) way. Personally I prefer using walletupgrade for HD (the UX isnt so bad - add an RPC command and when you go to request an address and you want it to be SegWit you'll get an error telling you to go do a walletupgrade RPC call, but its much more work).

Also wrt 2ii and 3ii - I think we pretty clearly need to make this optional. Even if we dont officially support anything else, I think scendario 3 always needs to be supported from one version to the next. Someone upgrading and hitting some corner case that they were relying on and being forced to downgrade is a very realistic possibility and we should avoid breaking it, where possible.

@sipa

This comment has been minimized.

Copy link
Owner Author

commented Nov 17, 2017

I'm confused about scenario 4. Are you assuming a non-hd wallet?

No, any wallet. Old software will require the presence of the P2SH-P2WPKH script in mapScripts before treating payments to P2SH-P2WPKH or P2WPKH outputs as ours. Obviously an old backup file cannot have these for addresses that were handed out later.

In your newly proposed scheme, would P2WPKH and P2SH-P2WPKH be the same record?

Yes, I think they should be. For newly created addresses in the new scheme, only the exactly requested scripts would be marked (e.g. just P2WPKH or just P2PKH), but for pre-existing ones, all of them would be set.

The use case I have in mind here is where the wallet displays both a bech32/P2WPKH and P2SH-P2WPKH address, which the user communicates to someone, who will then pick either one depending on what their wallet supports. Or are these totally interchangeable?

I'm unsure about that. If it's sufficiently cheap to create and watch for addresses, the easier choice is certainly to create two separate addresses (one bech32, one P2SH-P2WPKH) with separate keys. But perhaps an option for creating a "joint" address that contains both would be useful. In the longer term, I don't think this would be desirable though. Spending P2SH-P2WPKH is more expensive for you, so you have a financial incentive to perhaps only give out and accept money on bech32 addresses. This is part of the reason why we shouldn't continue the "accept whatever we can sign for" approach - it confuses the notion that the receiver should be in charge of what addresses he accepts coins to.

wrt 2. ii. How do you propose doing this? A -walletupgrade being the obvious solution, but requires we ship HD upgrade as well (I dont think this is that bad, but of course there's the wallets-needs-to-be-unlocked-to-do-this issue, which complicates things significantly, one option would be to add a walletupgrade RPC command to do it at runtime, since its a relatively simple thing when we're talking about HD). The other option would be to just bump it to 0xffffffff and have a second version field (or some other incompatibility, I believe the 0xffffffff bump was suggested by @jonasschnelli) but then we've accidentally introduced feature flags in a really roundabout (and kinda shitty) way. Personally I prefer using walletupgrade for HD (the UX isnt so bad - add an RPC command and when you go to request an address and you want it to be SegWit you'll get an error telling you to go do a walletupgrade RPC call, but its much more work).

I think the correct way is indeed to set the minversion field to INT_MAX, and then introduce two separate version fields (one with an int/bits of features the client software needs to support, and one with wallet features that are enabled). The first one wouldn't need a new backup to change; the second would.

The upgrade to default SegWit shouldn't force people to enable HD, I think, as that requires a new backup.

Also wrt 2ii and 3ii - I think we pretty clearly need to make this optional.

That seems reasonable, but how do you suggest we do that? Have a command line switch that permits upgrading to a new client version (see the first new version field above), and is default on (or default off)?

@TheBlueMatt

This comment has been minimized.

Copy link

commented Nov 17, 2017

I think the correct way is indeed to set the minversion field to INT_MAX, and then introduce two separate version fields (one with an int/bits of features the client software needs to support, and one with wallet features that are enabled). The first one wouldn't need a new backup to change; the second would.

Heh, you know my feelings on this one. I'm super not a fan of the complexity blowup of that design - we currently don't have any wallet features that (realistically) we believe anyone should chose not to use, and without such a feature to motivate this change, I remain unconvinced its the right direction.

The upgrade to default SegWit shouldn't force people to enable HD, I think, as that requires a new backup.

No, I believe the HD-upgrade change (and the current PR for it) should provide an optional don't-flush-keypool option. This is sufficient to not require a new backup any more than generating a new address would.

That seems reasonable, but how do you suggest we do that? Have a command line switch that permits upgrading to a new client version (see the first new version field above), and is default on (or default off)?

Indeed, I'd think it would need to look like -walletupgrade. Default-off would obviously be my preference, but it would be very nice to be able to do it at runtime as well (via RPC, ala my previous discussion of HD-upgrade stuff).

@sipa

This comment has been minimized.

Copy link
Owner Author

commented Nov 17, 2017

Heh, you know my feelings on this one. I'm super not a fan of the complexity blowup of that design - we currently don't have any wallet features that (realistically) we believe anyone should chose not to use, and without such a feature to motivate this change, I remain unconvinced its the right direction.

We currently really only have one optional feature (HD wallets). Either we support it, or we force people to upgrade and make a new backup.

No, I believe the HD-upgrade change (and the current PR for it) should provide an optional don't-flush-keypool option. This is sufficient to not require a new backup any more than generating a new address would.

Interesting point. The lack of HD means you needed backups beforehand anyway, and switching to HD is a strict improvement. But people aren't required to backup after every operation, so a restore-from-backup+upgrade-to-new-software wouldn't necessary choose the exact same key as master key. So I don't think it's correct to say no new backup is needed.

Indeed, I'd think it would need to look like -walletupgrade. Default-off would obviously be my preference, but it would be very nice to be able to do it at runtime as well (via RPC, ala my previous discussion of HD-upgrade stuff).

Bleh. It seems much easier to just automatically upgrade, and provide a recovery tool in case you need to force your wallet to work with an old version.

@TheBlueMatt

This comment has been minimized.

Copy link

commented Nov 17, 2017

We currently really only have one optional feature (HD wallets). Either we support it, or we force people to upgrade and make a new backup.
Interesting point. The lack of HD means you needed backups beforehand anyway, and switching to HD is a strict improvement. But people aren't required to backup after every operation, so a restore-from-backup+upgrade-to-new-software wouldn't necessary choose the exact same key as master key. So I don't think it's correct to say no new backup is needed.

My point here was that upgrading to HD is the equivalent of adding one new key to your keypool. I do not envision that being a burden requiring backup for most folks, especially if we do not immediately fill their keypool using the new HD key (ie API-wise, there should be absolutely no difference in terms of backups-needed here - anything visible to the user has not changed, only whether the backup they made right after this operation is still valid after keypool drains or not, which is only an improvement in backup-needed-ness).

I think its much cleaner to only ever have a version-based feature set instead of maintaining a comatibility-matrix, and am very heasitent to start down the matrix road, though doing so temporarily for one feature and then requiring upgrade through HD and SegWit wallet for whatever comes next is doable as a dirty hack to get SegWit wallet finished, if we really need to.

Bleh. It seems much easier to just automatically upgrade, and provide a recovery tool in case you need to force your wallet to work with an old version.

Hmm, a recovery tool is another option, I suppose. I guess I would be perfectly fine with that option instead as well, but certainly one is required IMO.

@sipa

This comment has been minimized.

Copy link
Owner Author

commented Nov 17, 2017

My point here was that upgrading to HD is the equivalent of adding one new key to your keypool

It's not equivalent to just topping up your keypool, as you only need a new backup when the oldest keypool entry is about to expire. An upgrade to HD requires an immediate new backup that instant.

@TheBlueMatt What do you think about (1a 2c 3a)? That avoids the versioning issue for now.

@Sjors

This comment has been minimized.

Copy link

commented Nov 18, 2017

A downgrade tool might be useful in general. I created an issue for it: bitcoin/bitcoin#11716

@TheBlueMatt

This comment has been minimized.

Copy link

commented Nov 19, 2017

It's not equivalent to just topping up your keypool, as you only need a new backup when the oldest keypool entry is about to expire. An upgrade to HD requires an immediate new backup that instant.

This is only true if you flush the keypool. If you do not flush the keypool when you upgrade to HD the only keys different between a backup and the new wallet are the ones generated after the current keypool, just as if you hadn't added an HD master key.

As for your proposal, I think we probably need to clarify a bit in the doc when rescan happens. ie in what conditions do you need a rescan. One thing that we may consider that may help some of the issues here is add a new "wallet caught up to block X with SegWit knowledge" field in addition to the existing "wallet caught up to block X" field. This would allow you to know you may need a second partial-rescan after downgrading/upgrading a wallet again, specifically helping a lot in case 2 if you then restart with the new version (eg in case a bug is reported, but then folks figure out a workaround).

I'm confused, you sais 1a2c3a, but 3a implies 2a? Anyway, I believe that was the original proposal from SF, no? I am still generally (weakly) in favor of that over other options, but obviously there is no perfect solution here.

@sipa

This comment has been minimized.

Copy link
Owner Author

commented Nov 20, 2017

This is only true if you flush the keypool. If you do not flush the keypool when you upgrade to HD the only keys different between a backup and the new wallet are the ones generated after the current keypool, just as if you hadn't added an HD master key.

I'm not convinced here, but let's discuss that elsewhere.

3A is the original proposal for SF. 2a implies 3a, not the other way around.

@TheBlueMatt

This comment has been minimized.

Copy link

commented Nov 27, 2017

There was a long IRC discussion today on this topic. Conclusion from my end was largely that 1a 3a is pretty broadly agreed upon, and there is no real "solution" to 2, its bad no matter what we do. In that context 2c may be fine, but we definitely need to ensure we document well the act of doing a partial rescan to find missing segwit coins, and probably include bitcoin/bitcoin#11281 to make it slightly less painful in Qt.

@udiWertheimer

This comment has been minimized.

Copy link

commented Feb 16, 2018

From the Future Design section:

Each record would correspond to an exact scriptPubKey (or set of scriptPubKeys)

What did you mean by set of scriptPubKeys in that context? In which cases would a record represent more than a single scriptPubKey?

@sipa

This comment has been minimized.

Copy link
Owner Author

commented Feb 19, 2018

In which cases would a record represent more than a single scriptPubKey?

When it's an HD chain node (with a range of more than one index).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.