Unify calculation of wallet id for multisig wallets

[paweljakubas]

• I have unified how wallet id is calculated in multisig wallets. Basically I wanted to achieve the following:

1. for a given wallet created from mnemonic and account pubic key (and having the same payment/staking script) we end up with the same wallet id. This is not the case for shelley wallets where wallet id from mnemonic and account public key are different as the first is calculated on pub key of rootK, and the second is calculated based on account public key. I think this is wrong, and for normal wallet account public key should be the source of wallet id calculation! As multisig are not operational yet, I propose to use the unified calculation of wallet id
2. As we can have situation that for a given account public key we can have many different multisig wallet, I propose to add script(s) imprint to accXPub and after that hash it. thanks to that we could delete a given shared wallet (using wallet id), rather than many shared wallets sharing accXPub, we have distinct differentiation of them.
3. Why to take imprint from Script Cosigner rather than Script KeyHash? Because we can have pending shared wallet and not completed Script KeyHash value, but we need wallet id even in pending stage as DB is affected and the shared walet already started lifecycle

Comments

Issue Number

adp-1621

[piotr-iohk]

This test became quite lengthy and not easy to follow... If I'm reading it correctly the ids are not the same because of different templates (active_from:120) vs. (active_from:100)?

Maybe it would make sense to separate this test into two separate ones? One verifying the case with the same ids and the other verifying the case with different ids...

[paweljakubas]

indeed, the same accXPub but difference in any script should give rise to different wallet id. I wanted to add those expectations under already present test to avoid consuming faucet. But we can separate it 👍

[piotr-iohk]

Those tests should not consume faucet as no coins are involved.

[paweljakubas]

ok, Piotr, constructed dedicated test just for you ;-)

[piotr-iohk]

for a given wallet created from mnemonic and account pubic key (and having the same payment/staking script) we end up with the same wallet id. This is not the case for shelley wallets where wallet id from mnemonic and account public key are different as the first is calculated on pub key of rootK, and the second is calculated based on account public key. I think this is wrong, and for normal wallet account public key should be the source of wallet id calculation! As multisig are not operational yet, I propose to use the unified calculation of wallet id

I wonder what might have been motivation for different wallet ids in those circumstances in Shelley wallets though... Wallets from accPubKey are mainly for use with HW device and I'm wondering is there any circumstance that makes it actually desirable to be like that. I'm wondering because there is a specific integration test that verifies this in case of Shelley wallets: https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core-integration/src/Test/Integration/Scenario/API/Shelley/HWWallets.hs#L322-L347

Also question what should be the correct way of calculating wallet id... should account public key be source of it? What if we want a multi-account wallet 🤔

[paweljakubas]

@piotr-iohk in shelley wallets we are, at this moment, assuming account ix=0, and in case of the same wallet from mnemonic (here account ix=0), and wallet from account public key that has the same accXPub as the wallet from mnemonic they have different wallet ids. I think this is wrong. For shelley wallets the difference should stem from different account public keys. This will be crucial if we want to support multi-account support. Why? because for any account and the same mnemonic we will have the same wallet id. Which will have consequences, as wallet id is used to pinpoint the resource, so when you delete wallet with a given wallet id you will erase ALL accounts for a given mnemonic. And I am pretty sure we don't want this. Etc Etc

[paweljakubas]

BUT, this PR tackles multisig wallets only. And I intend to do it right for multisig wallets as we don't have to deal with backward compatibility. Also for multisig wallets accXPub is not enough to define wallet id. Scripts should be taken into consideration as we have 1-to-many relation in (accXPub - scripr possibilities)

[paweljakubas]

maybe we need to put here not mempty but cosigner#0 as we expect in that case multisig wallet to just delegate using accXPubIx/2/0 where accXPubIx stems from ix of multisig wallet...

[paweljakubas]

yes, it needs to be changed. otherwise the following two wallets will give different wallet id, although they are the same:

1. accXPub, spending: something
2. accXPub, spending: something, staking: cosigner#0

[paweljakubas]

and I adopted already this approach in IntersectMBO/cardano-addresses#185 which should be consistent with what cardano-wallet gives

[Anviking]

yes, it needs to be changed. otherwise the following two wallets will give different wallet id, although they are the same:

accXPub, spending: something
accXPub, spending: something, staking: cosigner#0

Asking as a someone pretty unfamiliar with multi-sig: does this mean you could create "Multi-sig wallet 1" using account 0 and anyOf [self, Alice] as payment script, and then "Multi-sig wallet 2" using account 0 and anyOf [self, Alice, Bob] and have those be completely separate wallets despite using the same account for payment keys?

[Anviking]

Also, is there a place where I can read about the intended user workflow for multi-sig? I saw you write "spawn multisig wallet with ix=2" elsewhere, so take it's intended that you're adding accounts to the same mnemonic / root key, but maybe there's a place to read about it to get the full picture

[HeinrichApfelmus]

yes, it needs to be changed. otherwise the following two wallets will give different wallet id, although they are the same:

accXPub, spending: something
accXPub, spending: something, staking: cosigner#0

Are you sure, Pawel? 🤔 I suppose that this depends on what exactly the meaning of Nothing for dTemplateM is. It's perfectly conceivable that these cases are actually:

1. accXPub, spending: something, staking: none (i.e. enterprise address)
2. accXPub, spending: something, staking: cosigner#0

In other words, if there is no script template for delegation, then the wallet only contains enterprise addresses / addresses that do not have a staking part.

[paweljakubas]

ok, you are right @HeinrichApfelmus . If we want enterprise addresses to be supported in multisig then we need to allow empty delegation !!!
I will revert commit.
@Anviking Indeed, same accXPub and different script is different entity, having separate address space, and should be identified separately IMHO

[HeinrichApfelmus]

Yup. Just for reference: The multi-signature SharedKey does indeed create enterprise addresses when the staking template is empty:

Address $ case dTemplate of
    Just dTemplate' ->
        createBaseAddress pScript (dScript dTemplate')
    Nothing ->
        createEnterpriseAddress pScript

[HeinrichApfelmus]

Preliminary comments:

1. I agree, both on avoiding a dependence on the private key, and on keeping backwards compatibility.
2. and 3. The walletId has both internal use (database) and external use (software that uses the REST API to manage wallets). Which uses do we primarily want to satisfy here? (I agree with 2. and 3. for internal use.)

Observation: For Shelley wallets, the design is very nice in that wallet can always be restored from a single mnemonic. This cannot be the case for multi-signature wallets anymore, as the signatures by other parties are not part of the wallet.

[paweljakubas]

@HeinrichApfelmus one needs (a) mnemonic, (b) account ix and (c) scripts to be able to reconstruct multisig wallet. Usually (if it contains other cosigner in any script) it will need accXPub of all cosigners engaged to be in ACTIVE state. Only then it can discover its resources, eg. addresses.

Hence, for multi-party multisig wallet coordination servers will play its role, also for the task of restoration.

But that is the nature of multi-signature wallets. It collects extended public keys from parties (via PATCHing), which are used for further derivation of keys (hence extended is required), then parties deliver witnesses to tx via PATCHing to get from partially-signed tx to fully-signed tx.

[HeinrichApfelmus]

Commented on cardano-addresses: The use of show makes it more difficult for non-Haskell code to construct wallet IDs. I propose to export

serializeScriptTemplate :: Script Cosigner -> ByteString

from cardano-addresses and then use it here.

… Later, after version bumps.

[paweljakubas]

reused bits from cardano-addresses

[paweljakubas]

bors r+

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

[paweljakubas]

bors r+

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

[paweljakubas]

bors r+

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

[paweljakubas]

bors retry

[iohk-bors]

Build failed:

• ci/hydra-build:required

[paweljakubas]

bors retry

[iohk-bors]

Build failed:

• buildkite/cardano-wallet

[paweljakubas]

bors retry

[iohk-bors]

Build succeeded:

• buildkite/cardano-wallet
• ci/hydra-build:required