wallet: derive all keys in generateNonce for multisig #767

rithvikvibhu · 2022-09-12T08:36:49Z

Multisig accounts will generate different nonce given the same nameHash, p2sh address and value as only "own" public key is used. This PR derives public keys from all keys and picks the lexicographically smallest one so all parties generate the same nonce. Implements @pinheadmz's suggestion: https://t.me/hns_tech/70247

Tests

2 tests are added, one for regular pubkeyhash accounts and another for 2-of-2 multisig. Assertions compare to specific hardcoded values to ensure that this PR is not a breaking change (at least for regular wallets).

Running the test on master passes the first (pubkeyhash account), but fails on the second (multisig) because alice and bob generate different nonces.

coveralls · 2022-09-12T08:41:49Z

Coverage increased (+0.2%) to 68.199% when pulling 5f48556 on rithvikvibhu:multisig-generatenonce into 234a597 on handshake-org:master.

pinheadmz

Great solution, elegant code patch. I wonder if this should be published as (maybe very short) HIP since it's an application-level standard we'll need for interoperability with new HNS wallets. In addition, such a HIP may also want to begin to describe some kind of PSBT format that includes bid values from the tx author to the tx signers. Hopefully we have a future where big multisig corporations are bidding huge amounts on names and this kind of convinience would be helpful.

test/wallet-test.js

rithvikvibhu · 2022-09-15T17:10:50Z

I wonder if this should be published as (maybe very short) HIP since it's an application-level standard we'll need for interoperability with new HNS wallets.

👍 Will have a draft ready along with Bob's multisig PR

handshake-org/hsd#767

pinheadmz · 2022-11-11T17:29:04Z

It just occurred to me that this is also a potentially breaking change. A multisig party may have already placed a bid with their own key's nonce and instead of the group key like you've implemented. Attempting to recover that wallet using importnonce after this PR is merged would not work.

I'm not sure what the prettiest code would be but to be totally covered I think the logic for importnonce may need to derive both old and new nonces if the wallet is multisig. As long as both nonces are stored in the db, the reveal action will find the correct one and finish the TX.

rithvikvibhu · 2022-11-11T18:33:44Z

I looked for all places generateNonce (and by extension generateBlind) are used:

Get Nonce for Bid HTTP API (docs)
importnonce RPC method
makeBid in wallet

So yes, it's a breaking change when:

it is a multisig account, and
has placed bids that: (a) aren't revealed yet, and (b) can still be revealed, and
the wallet is recovered from seed / ledger after the bid was placed, and
own key is not the first key lexically (but unlikely, so we can ignore this point)

Storing both nonces in db doubles what's stored on disk for all current multisig wallets as well as those created in the future.
Instead, how about one of these (in the order I prefer them, first being the best of them):

Option 1:

new param to importnonce like multisig-legacy: bool that uses the old generateNonce (defaults to false)
same change to the HTTP api
Maybe: when this is true, we generate both old and new. If false, then only new.

Option 2:

a script/api-method/function/whatever that reads all existing BidBlinds and generates old nonces for them
useful if one wants to importnonce (many bids) without change and in the end call this to generate and store old nonces

Option 3:

no invisible upgrade path, warn users not to have active and unrevealed bids (in multisig) before update
I'm considering this as an option (even if it's last) because multisig isn't widely used yet and regular wallets aren't affected
in case a wallet really needs to be recovered, a script that does option 2 can be run separately

I like the first one. Was thinking of a wallet migration or similar, but it wouldn't work here because it affects new wallets, whereas migrations are for existing ones.

pinheadmz · 2022-11-11T18:56:16Z

I like the direction of option 1, just adding a --legacy flag or something. I think what that could do is generate and store nonces using every public key in the multisig set. That would ensure that ANY member of the party would be able to repair these bids, assuming they know the secret bid value.

We may even just want to do this without a flag (by default) since it is a bit of an edge case anyway. That would just be for rpc/httpimportnonce and not all generateNonce's.

Storing both nonces in db doubles what's stored on disk for all current multisig wallets as well as those created in the future.

I'm not too worried about storing a few extra hashes in the DB for now. Truth is, the wallet DB stores a ridiculous amount of extra data (e.g. everyone else's bids on auctions you participated in, future bids on re-opened names, etc) and pruning this garbage will be another big task that will increase in urgency as time goes on...

rithvikvibhu · 2022-11-16T10:03:32Z

Okay so now the way it works is:

Note: Doesn't affect pkh accounts, only multisig.

Normal behavior (makeBid, etc.):

Smallest public key is used to generate nonce
Only 1 blind is stored based on that nonce

Importnonce RPC/HTTP behavior (forAllKeys is true):

generateNonce returns N nonces (one for each key)
generateBlind stores N blinds, but returns the own-pub-key blind only
This way, the importnonce response doesn't change and is backward compatible

pinheadmz · 2022-11-29T15:19:38Z

lib/wallet/wallet.js

   */

-  async generateNonce(nameHash, address, value) {
+  async generateNonce(nameHash, address, value, forAllKeys) {


I don't love how this function maybe returns an array and maybe returns a single value.

What if we...

rename this function generateNonces() and it always returns an array

re-add new function generateNonce() which calls generateNonces() and only returns first element

this way, existing generateNonce() calls don't have to be modified. I think if we did the same with generateBlind() -> generateBlinds() then we'd only need to directly call the "plural" versions from the rpc, right? That would cover the edge case.

The http call is kinda funny because it doesn't add anything to db, its just like a utility if a user wants to see their nonce? Does Bob wallet use that endpoint anywhere? It almost makes more sense to change that get to a put and also call generateBlind() same as the rpc call. That would make this PR a breaking change, though.

That would make this PR a breaking change, though.

but actually thats ok because we're going to call this 5.0.0 anyway i think ?

Yeah I didn't like it either. Now it has generateNonce(s) and generateBlind(s). But generateNonce cannot call its plural method [0] because the order of the nonces/blinds is different:

hsd behavior has been own public key everywhere

now we switch to smallest public key

So generateNonce has the new style (smallest) whereas generateNonces returns first based on own key so that there's no breaking change.

The http call is kinda funny because it doesn't add anything to db ... That would make this PR a breaking change, though.

I don't think Bob uses that remote call since it always has access to the wallet directly. But since it's breaking, can be kept aside for a different PR

Also, each commit here undos a lot of code from the previous commit so might be easier to just look at overall PR changes. Will squash if the code makes sense now.

pinheadmz · 2022-11-30T20:51:38Z

lib/wallet/rpc.js

+    // Return only first blind (based on own key)
+    // to stay backward-compatible
+    return blinds[0].toString('hex');


eh, backwards-compat isnt so precious here, lets

return blinds.map(b => b.toString('hex'));

It'll be a breaking change I guess, if you only have one key youll get an array with a single item but I'm ok with that. Just needs to go in the changelog.

Now everything (internal methods, RPC, HTTP) all use smallest key and blinds and nonces are arrays. Added to CHANGELOG.

pinheadmz · 2022-11-30T20:59:26Z

test/wallet-test.js

+      // but blinds for all key are saved in db
+      assert(await bob.txdb.hasBlind(expectedBlinds.alice));
+      assert(await bob.txdb.hasBlind(expectedBlinds.bob));
+    });


I think you should also try to verify that both alice and bob can create a valid reveal even if the nonce is not based on their own key. That might even be best in wallet-rpc-test.js since thats the command thats been set up to cover the edge case.

Added a multisig auction test in wallet-rpc-test from OPEN to CLOSED, including a failed no-blind reveal and importnonce that makes it work.

nodech · 2022-12-01T12:24:10Z

lib/wallet/wallet.js

+    const publicKeys = await this._getNoncePublicKeys(address, value);
+
+    // Use smallest public key
+    publicKeys.sort(Buffer.compare);


~~API tries to be side-channel resistant where possible, so you could use const {safeCompare} = require('bcrypto/lib/safe')~~
does it matter in the pk case ? I think it is not necessary.
Also after reviewing, I think safeCompare is not what I thought it was.

nodech

I think can we can avoid duplicate blind as well as additional flags in RPC/API by utilizing almost unused flags in account.

Right now flags have 7 bits unused, we can take 2nd bit for this. We can introduce new flag in account - e.g. this.multisigBlinds = true

Existing wallets in the db, will have this flag set to 0 - meaning old behaviour. New wallets will default to new flag. This adds overhead to techdebt, but avoids all API changes. This does not even need migration. Can be released with minor.

lib/wallet/wallet.js

pinheadmz · 2022-12-01T15:28:42Z

Right now flags have 7 bits unused, we can take 2nd bit for this. We can introduce new flag in account - e.g. this.multisigBlinds = true

@nodech as discussed, this doesn't solve anything because after this is merged, we can't guarantee everyone in every multisig party is running updated software. So in a recovery scenario (with importnonce) we still need to try all nonces.

pinheadmz · 2022-12-01T15:31:16Z

lib/wallet/http.js

@@ -1029,7 +1029,8 @@ class HTTP extends Server {
      }

      const nameHash = rules.hashName(name);
-      const nonce = await req.wallet.generateNonce(nameHash, address, bid);
+      const nonces = await req.wallet.generateNonces(nameHash, address, bid);
+      const nonce = nonces[0];


I'm not sure but this endpoint might need to return an array as well. Let's take a look at bob's repair bid functionality. IIRC, we use the HTTP endpoint (which does not save anything to DB) to generate blinds until we get the right one, then call importnonce once with the correct values?

Bob always has access to the wallet and calls generateNonce (not generateBlind that saves blind). It doesn't depend on RPC/HTTP at all:

const wallet = await this.node.wdb.get(this.name); ... const nonce = await wallet.generateNonce(nameHash, from, value); const blind = Rules.blind(value, nonce).toString('hex');

https://github.com/kyokan/bob-wallet/blob/ff199207817101f51a9d6065f74f65f637f41c2d/app/background/wallet/service.js#L1093

Oh just saw that the actual repair does use the HTTP API:
https://github.com/kyokan/bob-wallet//blob/master/app/pages/Auction/RepairBid.js#L46-L50

I think we can make Bob use the wallet directly (similar to previous comment) to not depend on HTTP at all,
or will need to expose generateNonces plural (requires changes to hs-client also).

pinheadmz

New test is great! just nits left

lib/wallet/rpc.js

test/wallet-rpc-test.js

rithvikvibhu · 2022-12-20T14:50:33Z

Addressed all comments and cleaned up commits.

Breaking change for RPC importnonce and HTTP "Regenerate Nonce"

pinheadmz reviewed Sep 15, 2022

View reviewed changes

test/wallet-test.js Outdated Show resolved Hide resolved

test/wallet-test.js Outdated Show resolved Hide resolved

test/wallet-test.js Outdated Show resolved Hide resolved

rithvikvibhu force-pushed the multisig-generatenonce branch from c595a40 to 5e5ab24 Compare September 15, 2022 17:06

rithvikvibhu added a commit to rithvikvibhu/bob-wallet that referenced this pull request Oct 7, 2022

TEMP: use hsd from branch multisig-generatenonce

9018023

handshake-org/hsd#767

rithvikvibhu mentioned this pull request Oct 7, 2022

Multisig Wallets kyokan/bob-wallet#569

Merged

2 tasks

rithvikvibhu requested a review from pinheadmz October 7, 2022 19:42

rithvikvibhu force-pushed the multisig-generatenonce branch from 5e5ab24 to 1064f61 Compare October 11, 2022 06:33

pinheadmz added this to the hsd 5.0.0 milestone Nov 11, 2022

rithvikvibhu force-pushed the multisig-generatenonce branch from 4f0a4ba to efda192 Compare November 16, 2022 09:58

pinheadmz reviewed Nov 29, 2022

View reviewed changes

rithvikvibhu force-pushed the multisig-generatenonce branch from d1615bc to 18328f9 Compare November 29, 2022 19:22

pinheadmz reviewed Nov 30, 2022

View reviewed changes

nodech reviewed Dec 1, 2022

View reviewed changes

nodech requested changes Dec 1, 2022

View reviewed changes

lib/wallet/wallet.js Outdated Show resolved Hide resolved

lib/wallet/wallet.js Outdated Show resolved Hide resolved

lib/wallet/wallet.js Outdated Show resolved Hide resolved

lib/wallet/wallet.js Outdated Show resolved Hide resolved

lib/wallet/wallet.js Outdated Show resolved Hide resolved

pinheadmz reviewed Dec 1, 2022

View reviewed changes

pinheadmz reviewed Dec 19, 2022

View reviewed changes

rithvikvibhu force-pushed the multisig-generatenonce branch from bf56b6a to 637c85b Compare December 20, 2022 14:50

rithvikvibhu added 2 commits December 20, 2022 13:58

wallet: add generateNonces and generateBlinds

acc4208

wallet: make nonce APIs use generateNonces & generateBlinds

017a0d7

Breaking change for RPC importnonce and HTTP "Regenerate Nonce"

pinheadmz force-pushed the multisig-generatenonce branch 2 times, most recently from 18d77ed to 0b5fb98 Compare December 20, 2022 19:27

rithvikvibhu and others added 2 commits December 21, 2022 11:38

changelog: mention breaking change for nonces

c78a7a4

test: rebase wallet test on master (see handshake-org#781)

5f48556

pinheadmz force-pushed the multisig-generatenonce branch from 0b5fb98 to 5f48556 Compare December 21, 2022 16:38

pinheadmz merged commit b456147 into handshake-org:master Dec 21, 2022

rithvikvibhu deleted the multisig-generatenonce branch December 21, 2022 19:25

nodech added wallet part of the codebase wallet-rpc part of the codebase breaking-minor Backwards compatible - Release version breaking-major Backwards incompatible - Release version and removed breaking-minor Backwards compatible - Release version labels Jan 9, 2023

nodech mentioned this pull request Jan 17, 2023

pkg: Release v5.0.0 #798

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: derive all keys in generateNonce for multisig #767

wallet: derive all keys in generateNonce for multisig #767

rithvikvibhu commented Sep 12, 2022

coveralls commented Sep 12, 2022 •

edited

Loading

pinheadmz left a comment

rithvikvibhu commented Sep 15, 2022

pinheadmz commented Nov 11, 2022

rithvikvibhu commented Nov 11, 2022

pinheadmz commented Nov 11, 2022

rithvikvibhu commented Nov 16, 2022

pinheadmz Nov 29, 2022 •

edited

Loading

rithvikvibhu Nov 29, 2022

rithvikvibhu Nov 29, 2022

pinheadmz Nov 30, 2022

rithvikvibhu Dec 17, 2022

pinheadmz Nov 30, 2022

rithvikvibhu Dec 17, 2022

nodech Dec 1, 2022 •

edited

Loading

nodech left a comment •

edited

Loading

pinheadmz commented Dec 1, 2022

pinheadmz Dec 1, 2022

rithvikvibhu Dec 7, 2022

rithvikvibhu Dec 7, 2022

pinheadmz left a comment

rithvikvibhu commented Dec 20, 2022

wallet: derive all keys in generateNonce for multisig #767

wallet: derive all keys in generateNonce for multisig #767

Conversation

rithvikvibhu commented Sep 12, 2022

Tests

coveralls commented Sep 12, 2022 • edited Loading

pinheadmz left a comment

Choose a reason for hiding this comment

rithvikvibhu commented Sep 15, 2022

pinheadmz commented Nov 11, 2022

rithvikvibhu commented Nov 11, 2022

pinheadmz commented Nov 11, 2022

rithvikvibhu commented Nov 16, 2022

pinheadmz Nov 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nodech Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

nodech left a comment • edited Loading

Choose a reason for hiding this comment

pinheadmz commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pinheadmz left a comment

Choose a reason for hiding this comment

rithvikvibhu commented Dec 20, 2022

coveralls commented Sep 12, 2022 •

edited

Loading

pinheadmz Nov 29, 2022 •

edited

Loading

nodech Dec 1, 2022 •

edited

Loading

nodech left a comment •

edited

Loading