[wallet] Modify change identification to use hdsplit keypath #12983

instagibbs commented at 9:54 PM on April 13, 2018: member

Keypath data is a much more reliable way of detecting change outputs.

There are a number of cases where the legacy detection fails, leading to odd results, such as a wallet rescan that results in missed results in listtransactions with self-sends.

instagibbs renamed this:
~~Modify change identification to use hdsplit keypath~~
[wallet] Modify change identification to use hdsplit keypath
on Apr 13, 2018

instagibbs commented at 9:55 PM on April 13, 2018: member

I will add listtransaction test soon.

fanquake added the label Wallet on Apr 14, 2018

instagibbs force-pushed on Apr 14, 2018

instagibbs commented at 10:12 PM on April 14, 2018: member

pushed a test that covered a corner case this fixes

I realize the test itself carries a comment describing yet another corner case I've stumbled on that doesn't have any super easy fix. For the purposes of the testing what we care about I just set keypool=2 for a single node near the end of the test, but I could also just have the entire test with this setting, as it changes nothing about the overarching tests in place.

instagibbs force-pushed on Apr 15, 2018

instagibbs referenced this in commit 564826435c on Apr 16, 2018

instagibbs referenced this in commit 274104a0cb on Apr 16, 2018

instagibbs referenced this in commit 28b1948ec7 on Apr 16, 2018

instagibbs referenced this in commit 4bd035d0b5 on Apr 16, 2018

in src/wallet/wallet.cpp:1344 in 843e07cd5f outdated

1350 | +            CKeyID key_id = GetKeyForDestination(*this, dest);
1351 | +            if (!key_id.IsNull()) {
1352 | +                auto it = mapKeyMetadata.find(key_id);
1353 | +                if (it != mapKeyMetadata.end()) {
1354 | +                    // Check for path
1355 | +                    if (it->second.hdKeypath.find("m/0'/1'/") != std::string::npos) {

jonasschnelli commented at 8:51 AM on April 17, 2018:

Not for this PR, but we should probably factor out the static key path strings... maybe add a IsChangeKeypath().

instagibbs commented at 2:06 PM on April 17, 2018:

Yes, I think we should be more systematic about our keypath string handling; would make it easier for people like me with various wallet forks and for Core as we move forward with additional derivation features.

jonasschnelli commented at 8:52 AM on April 17, 2018: contributor

utACK 843e07cd5ffb4c9f8d73ef0e2aeed03303122a5b Seems to not create any performance impacts.

jonasschnelli approved

instagibbs referenced this in commit b22e3138b5 on Apr 18, 2018

instagibbs referenced this in commit 6a776a753e on Apr 18, 2018

instagibbs referenced this in commit f62872c5d4 on Apr 18, 2018

instagibbs referenced this in commit 82e1cdcd66 on Apr 18, 2018

instagibbs referenced this in commit 20dc5dc5a9 on Apr 18, 2018

instagibbs referenced this in commit 08a57cb3f2 on Apr 18, 2018

sipa commented at 11:39 PM on May 3, 2018: member

I'm afraid this is not sufficient. It's possible to import change keys/addresses using importmulti (see the "internal" field); these aren't identified by having a change-like keypath. Furthermore, this approach will become even less reliable if it's possible to upgrade from pre-hd or pre-hdsplit wallets, as the old keys should remain being treated as change.

You could use being present in a change-like key path as a shortcut still and fall back to the other mechanism otherwise, but I doubt that's very useful.

instagibbs commented at 1:39 AM on May 4, 2018: member

@sipa actually that suggestion is just fine for my needs, even if you aren't too excited about it.

instagibbs force-pushed on May 8, 2018

Modify change identification to use hdsplit keypath 89eab914c4

test hdplit change detection 10b18f48d4

instagibbs force-pushed on May 8, 2018

instagibbs commented at 6:13 PM on May 8, 2018: member

oh hmm, I see why now. I'll just carry it downstream for now.

instagibbs closed this on May 8, 2018

instagibbs referenced this in commit bfc350117f on May 15, 2018

instagibbs referenced this in commit 6ecda18587 on May 15, 2018

instagibbs referenced this in commit 513d484c57 on Jul 29, 2018

instagibbs referenced this in commit 0e97d4e25f on Jul 29, 2018

instagibbs referenced this in commit bb02c5ecf6 on Aug 13, 2018

instagibbs referenced this in commit 3e63e57e1a on Aug 13, 2018

MarcoFalke locked this on Sep 8, 2021