WIP [RPC] [Wallet] walletdowngrade command (which can remove HD) #11730

Sjors commented at 6:05 PM on November 19, 2017: member

This adds a walletdowngrade [version] RPC method.

It currently only supports downgrading to v0.12, which removes the HD seed (without deleting previously generated keys). Once downgraded, there's no way back, since there's currently no mechanism to upgrade non-HD wallets.

I don't know if v0.12 downgrade support is at all useful. If not, the code can be reused for adding downgrade support for the upcoming (perhaps more risky) SegWit related changes.

Todo:

actually test with v0.12 wallet
add functional test
add release note (?)

[wallet] method to delete hdChain 027dbea66b

[rpc] downgradewallet: allow downgrade to v0.12

Add RPC method to downgrade a wallet for compatibilty with earlier versions.

Downgrade to v0.12: removes the hdseed while keeping addresses generated so far.

500cbb77bc

fanquake added the label Wallet on Nov 19, 2017

in src/wallet/rpcwallet.cpp:3352 in 500cbb77bc

3347 | +    
3348 | +    bool supported_version = std::any_of(supported_versions.begin(), supported_versions.end(), [&](uint32_t i) 
3349 | +    {
3350 | +        return i == version;
3351 | +    });
3352 | +

jonasschnelli commented at 10:59 PM on November 19, 2017:

trailing whitespace (reported by travis)

Sjors commented at 12:51 PM on November 20, 2017:

I haven't figured out yet how to run a linter. linter-gcc for Atom doesn't work out of the box.

in src/wallet/rpcwallet.cpp:3336 in 500cbb77bc

3331 | +    // Check version argument
3332 | +    
3333 | +    uint32_t version;
3334 | +    std::string version_s = request.params[0].get_str(); 
3335 | +    std::smatch match;
3336 | +    const std::regex regex("^(\\d+)\\.(\\d+)$");

jonasschnelli commented at 11:01 PM on November 19, 2017:

Not sure if you really require regex for this... although, not sure if there are downsides using regex.

in src/wallet/rpcwallet.cpp:3369 in 500cbb77bc

3364 | +        }
3365 | +    }
3366 | +    
3367 | +    // Future code to downgrade to versions below 0.12 goes here.
3368 | +
3369 | +    return NullUniValue;

jonasschnelli commented at 11:03 PM on November 19, 2017:

I guess this should give an error... (although this code parts is currently not reachable).

Sjors commented at 12:53 PM on November 20, 2017:

Actually this is reachable, if the downgrade is successful. Some other RPC methods also return this, but I probably want it to say something a bit more useful.

TheBlueMatt commented at 11:05 PM on November 19, 2017: member

Strong NACK. The use-cases for removing HD seed (mostly desire to have backups not permanently compromise funds) are all but fully addressed by #11085. As for removing SegWit, I'm not fully convinced it's the right direction, but if we do I think making it an external tool is the way to go - supporting downgrading wallets adds a ton of complexity that I don't think is realistically maintainable.

On November 19, 2017 10:05:10 AM PST, Sjors Provoost notifications@github.com wrote:

Fixes #11716.

This adds a walletdowngrade [version] RPC method.

It currently only supports downgrading to v0.12, which removes the HD seed (without deleting previously generated keys). Once downgraded, there's no way back, since there's currently no mechanism to upgrade non-HD wallets.

I don't know if v0.12 downgrade support is at all useful. If not, the code can be reused for adding downgrade support for the upcoming (perhaps more risky) SegWit related changes.

Todo:

actually test with v0.12 wallet

add functional test

add release note (?) You can view, comment on, or merge this pull request online at:

#11730

-- Commit Summary --

[wallet] method to delete hdChain

[rpc] downgradewallet: allow downgrade to v0.12

-- File Changes --

M src/wallet/rpcwallet.cpp (64) M src/wallet/wallet.cpp (12) M src/wallet/wallet.h (3)

-- Patch Links --

https://github.com/bitcoin/bitcoin/pull/11730.patch https://github.com/bitcoin/bitcoin/pull/11730.diff

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11730

luke-jr commented at 11:06 PM on November 19, 2017: member

I don't think it really makes sense to try to support downgrades. In most cases, it would simply be impossible. Better to just restore -usehd=0 (#11582).

As for Segwit changes, my understanding is that support for non-Segwit wallets will be retained.

jonasschnelli commented at 11:06 PM on November 19, 2017: contributor

I'm unsure if downgrading should be offered... there are benefits using HD (backup situations). Also, purely deleting the HD is probably not something we should do. If we going to use this, it should downgrade in a non-destructable way.

laanwj commented at 11:28 AM on November 20, 2017: member

Also, purely deleting the HD is probably not something we should do. If we going to use this, it should downgrade in a non-destructable way.

Oh man no please don't do this. If you really want to allow users to switch to non-HD key generation for some reason, just change the key generation. Start using the legacy key generation for new keys. Don't touch older keys.

All in all I'm kind of doubtful on the wallet downgrading thing. Seems a lot of extra functionality to support for little value, which will inherently be under-tested and code-rot as such. I'd prefer an external script for to handle the few cases where this is useful.

Sjors commented at 1:00 PM on November 20, 2017: member

Alright, Github will keep this code around if there's ever a situation in the future where do want wallet downgrade functionality :-)

Sjors closed this on Nov 20, 2017

DrahtBot locked this on Sep 8, 2021