M/N multisig support #4036

naughtyfox · 2018-06-21T12:10:04Z

Extended multisignatures to schemes M/N.

I saved compatibility with previous API flow for multisig wallets creation. 2/4 wallets creation process looks as follows:

[wallet 5BUgPa]: prepare_multisig
Wallet password: 
MultisigV1XQAM98AQLYTAR83FDuGbn3Crvtgm9jRyKKEryVwcHC5j8sCaryT339hNBrbWXQUaEe4NUB7vv9BdPHTuZuUFvSoVbC273qbGrJGcAcDHbP91oXXd46xUVSzknRDQQ8L9ddjmZX789VveWRYUeyrP6Q5UVpEzmDn1NpfRA4NA21UxihEx
Send this multisig info to all other participants, then use make_multisig <threshold> <info1> [<info2>...] with others' multisig info
This includes the PRIVATE view key, so needs to be disclosed only to that multisig wallet's participants 

[wallet 5BUgPa]: make_multisig 2 MultisigV1A9TsWdMbPHtgvM6FDuYR7nPovfNdTPteSPFZwFgcNeR6PNsrmGjPvHbhiq7Ra1sMs9VrW5ev5MBX5aP1zTXmFLPcRcnj8i4zvoJ7qZS7YqyGdSgXfvwcoqS3bbtmASNmHivZNCyJkno5mdB1JNxD5wqUtNVUiQwRRfifTK6WFaNZjE2H MultisigV1hzHY9B7TuKaMF6cjjA53j4418G164HFzuVjtKGiam5YaVJm2rgTnXWZSHsQgPwW4abXpTS74SB8KbNs69DfzrsWB69ghrcRz3xuDUi4nnwEvxmAk8ECQQicbXiVfuWPyijdGUuiRHvnGBw6G3nXiWTYoKE5oLLgo7iGryFi68Vrp9MT2 MultisigV1SKvVDZ1ReJXMDbugtaJBiEEQ9hCVGD51we3b7w4pV5bPTCoigR7G2qoMQcaAgp87pXZCY9wDsHbRUHGwJYaGQ1NofLJJVP7they2rM348cpjEgAPVUoLxa52wjMP7pZQsfMeBCpZWKtiPL91J7R7uWes2DYH1gBRgpsHhFVjEeGrEDzx
Wallet password: 
Another step is needed
MultisigxV1jag57V1msEsWDDtYtTmaBCAR97maah4H2hxLeXfrXRWUEtNigxghaLA1ZjmjYiTyjYSXTucNMEeNvLBKtqrUME5ZNvtX3iiLLmJCzoXAMn2YhYDTUEpVi1bMN8fv3oxE9tQWNbghwVYazvjgBhXcRoNuh989GT84GEuDpjPcfSYDrosPeGj8KKH1AmsbQPCNoA7ppi6pSwjrzxJJW436KbKqHBwkAkhQVevWjj8Gmsbgg5ZQZcFRfFpMbJp8DXEtX32rKzEP
Send this multisig info to all other participants, then use exchage_multisig_keys <info1> [<info2>...] with others' multisig info

[wallet 51sLpF (out of sync)]: exchange_multisig_keys MultisigxV16oHHLiL2WsyEsmRj1fc37YJTmQs4bgmERBGRTKSoFxtR253qC31hnnTE7wy8xLFumwWmQzyAdjJjzR2cwfatJkpU6HX6WFSqE6DDsn5xyrPNiXifA6QCSfZxycnGA1roQ76p79BWRqr7yFzWUKu2uZYMDDYre85YEyZp5emiVz2JXeakQwARLCMk9neSSUsZFbPmPcMNmby9gQvScPgTXmKeBUKAjBK7njF6M1hGEkcKdPT2caiSYaX8V1mrZGpSxaESWDV9 MultisigxV1P5bu44mS6SqLWfh5f8AuNzDotbMT6iEf7LVaFfixJnyuM2VBNiwcJSbBbFifbgj7VidFZjzV1kpi64gRQk1aigGS6HX6WFSqE6DDsn5xyrPNiXifA6QCSfZxycnGA1roQ76pGSNmJhNYMSUNTdB2HviM6hFL2E9Uypy5eMjuQ9B4Wx4oBpXWmfjGbTk1qBUqPhyWwwU58jMqa9oQLSbv9TX8T82CYdFUEEgVVAvM1hoHjzpofLLxzPKFjyCofZ2UK3HsoLeq MultisigxV1BjP86fURDjY2L31oxTC64GDfDdPfqogahNdYLw2evHbo8PYmCtWxM8u7wSb6LY2VVrR4zW7vpq7QkGGkV2mYh19M79BWRqr7yFzWUKu2uZYMDDYre85YEyZp5emiVz2JXeakGSNmJhNYMSUNTdB2HviM6hFL2E9Uypy5eMjuQ9B4Wx4oE3351oRkZD86TH4z5SjThn2do9u6UXgDRExMNatSRcKVUCKFnmLLdCwdLYJATYMc2wFgq7rVG2LMdMX8jffrgVpC
Wallet password: 
Another step is needed
MultisigxV1AExg753mNvX17iyTbFX6Tq3sjpJ2YcPsgWi9jQkM2rMKctP8R5sBzZeCRDPXmCvHF54Gs7BKCj7W6AgPiwaLEJVF1r8MKR6yUmkeJMfexLhwun6VGqErFWFMV2SY1wXK5jNvFqLpiK3Ng6QjiWnAiqDccLajgFZvT5uLrXFgJm8JgrF5domBJYwSgZ89sfeC2RP45uQq6ksxGL3pYBThLwoWNieJS1A7P4MRqiiCYc2NqstSfzRVBeEZ3ezGhGNF89E2zL2YJos52A14AdFdF6PNUdK7UhMuGntMeBH8bgKAAuz1bfa57JDe4dwoC1rRTuhvL2Ewhq49PaQeBtntqHJuDnHwYme6cGkJGrEJoMqaX9tSajc1FkBgBop7hvkC6cq8BtMCjmZT
Send this multisig info to all other participants, then use exchange_multisig_keys <info1> [<info2>...] with others' multisig info

[wallet 51sLpF]: exchange_multisig_keys MultisigxV1PUq5CDZ9oWf3jUcWdx8aC32pWeCySeE7wJJH1aFaPVyAetYD2Bm9yh4PUSNuLtScvV2NKZzbQWQCgJq5saoLvCmVh5h5WtcM25Pc23k5ovZFjY8LnRjfT4DbkTCHv7kh2oPPRsW3Ax9XKFFEjRPh5DECjzCQMWwKXa9KneZmiQWB6kohV9nAHzG3ohYcHQq8E9NZR8UxsMDjRpmTxj6FrDPFCj2DXavpoe9reN94owQvP1dCc1XacHsVg7RKSKQMgUj8DNnS MultisigxV1dLf4xkCR7tD7C2cRhR1YATW5yJVFm2XiANVuMQjwsiM2etYD2Bm9yh4PUSNuLtScvV2NKZzbQWQCgJq5saoLvCmVd6syGgSUGA5hh8tkn8A6vpNhbwP5PjfBhfREypkSWuXjRsW3Ax9XKFFEjRPh5DECjzCQMWwKXa9KneZmiQWB6kohRuXSZGmcDNxYQoS5WSvC3P2qKQLTvLxULCDXUqbz6rJzA1st1tg4iRVdjdiWbyVPKRSBSU9oCx3wKKUkqhjqCGJR MultisigxV16FJqCGcFCnBTNTQmbY3CN7C5HqW62EMhXAh6Nax6rZ6UetYD2Bm9yh4PUSNuLtScvV2NKZzbQWQCgJq5saoLvCmVh5h5WtcM25Pc23k5ovZFjY8LnRjfT4DbkTCHv7kh2oPPd6syGgSUGA5hh8tkn8A6vpNhbwP5PjfBhfREypkSWuXj6tQM6LDnKbnJ8QkB9CePUK7q7wYM7x8JbJmj4D26Mko2DYAB5vbtmYjZxeapFTHuWYNgcgB4KAqoLFTb4M5Z9eGo
Wallet password: 
Multisig wallet has been successfully created. Current wallet type: 2/4
[wallet 526g4j]: address
0  526g4jUk2MxYjKFGrq655vAC9CBYLM66sWLn1VzXeAukE52fUbUkjsecpLLAzgBt7C9mTkutf29fnUEpPt39KUYZJ5votVx  Primary address

For classic N-1/N wallets instead of exchange_multisig_keys you may also use finalize_multisig command as you did before. Signing and submitting multisignature transactions procedure as well as partial key images exchange remain the same. In other words, only exchange_multisig_keys was added from API perspective.

Support for arbitrary scheme has been built also into:

wallet api
waller rpc
monero-gen-trusted-multisig utility

Two members m_multisig_rounds_passed and m_multisig_derivations have been added in wallet2 class to be able to continue creation procedure if the wallet has been closed between rounds.

On of the first transactions sent from 2/4 wallet on stagenet is 244697a55e460e2188bb43b7a95ecf51298de1b6bc6a81f88d6d170e943ad0ef, from 2/5 wallet - fd3bb0a12e44cdc4aa7834032c9ac15c7b0528c0bf9644d851e4d99d47606368. I may provide these wallets if needed :)

Tested with wallet api, wallet cli and wallet rpc on Ubuntu 17.10.
Core tests have also been added.

naughtyfox · 2018-07-12T12:20:16Z

@moneromooo-monero @stoffu and @luigi1111 please review my changes

moneromooo-monero

A few minor comments. No idea whether the scheme does what it's said to do though, that'll require a luigi.

moneromooo-monero · 2018-07-13T11:55:24Z

src/gen_multisig/gen_multisig.cpp

-          tools::fail_msg_writer() << genms::tr("Error finalizing multisig");
-          return false;
-        }
+          extra_info[n] = wallets[n]->exchange_multisig_keys(pwd_container->password(), pkeys, signers);


Why remove the error handling ?

because it throws in case of error

moneromooo-monero · 2018-07-13T11:57:40Z

src/multisig/multisig.cpp

@@ -81,6 +81,43 @@ namespace cryptonote
    }
  }
  //-----------------------------------------------------------------
+  std::vector<crypto::public_key> generate_multisig_derivations(const account_keys &keys, const std::vector<crypto::public_key> &derivations)


Badly named ? It uses derivations to create public keys.

because sk * Pk = Pk. I can rename it if you want

I do not understand the point.

It uses derivations from previous round to generate derivations for new one.

It does not generate derivations though, but public keys. Maybe you call these public keys derivations in your scheme ? If so, it's just confusing :)

I always thought about key derivations as functions spawning keys from another keys. In our case spawned key is public. Maybe I was wrong :) Do you want me to rename it?

moneromooo-monero · 2018-07-13T11:59:29Z

src/multisig/multisig.cpp

@@ -138,4 +175,8 @@ namespace cryptonote
    return true;
  }
  //-----------------------------------------------------------------
+  uint32_t multisig_rounds_required(uint32_t participants, uint32_t threshold)
+  {


Could do with an assert that the numbers are valid (op doesn't get <= 0).

this case is checked before this function called. but i can just in case

moneromooo-monero · 2018-07-13T11:59:53Z

src/simplewallet/simplewallet.cpp

@@ -887,7 +887,7 @@ bool simple_wallet::make_multisig(const std::vector<std::string> &args)
    {
      success_msg_writer() << tr("Another step is needed");
      success_msg_writer() << multisig_extra_info;
-      success_msg_writer() << tr("Send this multisig info to all other participants, then use finalize_multisig <info1> [<info2>...] with others' multisig info");
+      success_msg_writer() << tr("Send this multisig info to all other participants, then use exchage_multisig_keys <info1> [<info2>...] with others' multisig info");


fixed, but github doesn't show it for some reason

moneromooo-monero · 2018-07-13T12:13:41Z

src/simplewallet/simplewallet.cpp

+        return true;
+      } else {
+        uint32_t threshold, total;
+        m_wallet->multisig(NULL, &threshold, &total);


This should probably check the return value for true (and ready being true too)

I did the check at the first then decided it's redundant because we already have check the wallet is multisig above. Do you think we need to check it here again?

moneromooo-monero · 2018-07-13T13:35:33Z

src/wallet/wallet2.cpp

  for (size_t i = 0; i < info.size(); ++i)
  {
    THROW_WALLET_EXCEPTION_IF(!verify_multisig_info(info[i], secret_keys[i], public_keys[i]),
-        error::wallet_internal_error, "Bad multisig info: " + info[i]);
+      error::wallet_internal_error, "Bad multisig info: " + info[i]);


moneromooo-monero · 2018-07-13T13:36:29Z

src/wallet/wallet2.cpp

@@ -3604,74 +3794,54 @@ std::string wallet2::make_multisig(const epee::wipeable_string &password,
    else
    {
      THROW_WALLET_EXCEPTION_IF(public_keys[i] == local_pkey, error::wallet_internal_error,
-          "Found local spend public key, but not local view secret key - something very weird");
+        "Found local spend public key, but not local view secret key - something very weird");


Is your editor munging these automatically ? Must be a setting to stop it.

Yes, it's editor's fault. The project uses different identations and my IDE goes crazy about it

moneromooo-monero · 2018-07-13T13:41:53Z

src/wallet/wallet_rpc_server_commands_defs.h

@@ -1916,6 +1916,31 @@ namespace wallet_rpc
    };
  };

+  struct COMMAND_RPC_EXCHANGE_MULTISIG_KEYS


Bump RPC version at the top

moneromooo-monero · 2018-07-13T13:42:57Z

tests/core_tests/chaingen.h

@@ -549,7 +549,7 @@ inline bool do_replay_file(const std::string& filename)
        std::vector<crypto::public_key> spend_public_keys; \
        for (const auto &k: all_multisig_keys) \
          spend_public_keys.push_back(k); \
-        crypto::public_key spend_pkey = cryptonote::generate_multisig_N1_N_spend_public_key(spend_public_keys); \
+        crypto::public_key spend_pkey = cryptonote::generate_multisig_M_N_spend_public_key(spend_public_keys); \


Can you add some tests for the new threshold cases ?

I can... But the problem is core tests don't use wallet or it's parts and I need to copy and paste some code into appropriate macro...

moneromooo-monero · 2018-07-13T13:48:18Z

src/wallet/wallet2.cpp

+  CHECK_AND_ASSERT_THROW_MES(!derivations.empty(), "empty pkeys");
+  bool ready = false;
+  CHECK_AND_ASSERT_THROW_MES(multisig(&ready), "The wallet is not multisig");
+  CHECK_AND_ASSERT_THROW_MES(!ready, "Multisig wallet creation process has already been finished");


Probably should be a sanity check for signers size here.

naughtyfox · 2018-07-13T15:44:58Z

I fixed part of comments, part of comments need to be discussed. I will add some core tests soon.

naughtyfox · 2018-07-17T11:13:54Z

@moneromooo-monero I added core tests for my functionality as promised. Please check it out. I also fixed some of comments, others need to be discussed further :)

ph4r05 · 2018-07-24T12:16:15Z

@naughtyfox I was trying to find a (white)paper describing the M/N multisig scheme you implemented but I haven't found it. Can you pls link the paper with the scheme?

Thanks!

naughtyfox · 2018-07-24T13:56:08Z

@ph4r05 unfortunately I couldn't find public multisig paper either. We discussed details about extending it to arbitrary M/N scheme in #monero-research-lab IRC channel. The only part of my work here is to extend key exchange rounds to support M/N. This is enough to be integrated in partial key images exchange and MLSTAG signatures. I can do a write up to describe better what happens under the hood with wallet's keys. Would it be ok for you?

ph4r05 · 2018-07-24T14:16:24Z

@naughtyfox Aaah I see.

I think the write-up describing how exactly the multisig variants work (also with the M/N) would be very beneficial to the Monero community. Without following the IRC channels or "reverse-engineering" it from the source code it may be quite complicated to get an idea how multisig in Monero works.

IMO, Having published documentation would help to increase the transparency and security of the Monero, so other security researches could assess the multisig scheme security (and to check whether implementation matches the desired scheme).

Unfortunately, there is no MRL-xx document yet on the multisig topic, as far as I know.
There is a description in the @moneromooo-monero commit message introducing the Multisigs moneromooo-monero@b99bd7eecf

There is also a paper on eprint describing basic Monero transactions, but again without multisigs.
https://eprint.iacr.org/2018/535

naughtyfox · 2018-07-24T14:36:17Z

This is true.

Recently we tried to fill the gap and issued post on hackernoon about basics in monero multisignatures - https://hackernoon.com/monero-multisignatures-explained-46b247b098a7. And of course, it's about N/N and N-1/N schemes only.

I'll write some doc about multisignatures and will try to cover keys exchange and partial key images topics.

PS I almost forgot! @UkoeHB wrote the book about monero internals and there is unpublished chapter about multisignatures - https://github.com/UkoeHB/Monero-RCT-report/blob/master/multisig_chapter-1-0.pdf

moneromooo-monero · 2018-07-24T14:44:14Z

Surae's almost done with the multisig paper.
You can ask him for a link to the current draft.

b-g-goodell · 2018-07-26T14:42:04Z

View access for current draft: https://v2.overleaf.com/read/bfjfkdgnhgvh we'll be submitting it for publication soon so I'm not tremendously willing to include the more general case for this document.. but I can be persuaded, especially if someone has a clearish write-up to include! I can't look at it very soon but I agree that at least the more general M/N description should at least end up in the Zero to Monero document

naughtyfox · 2018-07-30T15:53:03Z

I made some kind of writing to explain in more human readable manner what's been done in this PR. Unfortunately, I don't have math-writing knack so I did it in google docs (again) in more or less free manner. You may check this out here - https://docs.google.com/document/d/1lo2oLyXP8O8g98FkNK9feUvZx98H6h9fgvBZn5-Qiwg/edit?usp=sharing

moneromooo-monero · 2018-08-24T14:14:50Z

Please rebase.

b-g-goodell · 2018-08-27T16:15:01Z

Multisig paper submitted to IACR. Link incoming in a few days.

naughtyfox · 2018-08-28T10:03:59Z

rebased

naughtyfox · 2018-09-26T16:36:13Z

Me and @b-g-goodell had a conversation about hashing multisig in the middle of key exchange rounds and why rounds differs as pointed in this discussion - #4036 (comment). I wrote the doc to clarify why it's needed - https://docs.google.com/document/d/1CGzdx8AeaDgWI3HZ1HpYQDtsPY1iirqGfOjvrMhlpss/edit?usp=sharing

naughtyfox · 2018-09-27T15:10:18Z

rebased

* support in wallet2 * support in monero-wallet-cli * support in monero-wallet-rpc * support in wallet api * support in monero-gen-trusted-multisig * unit tests for multisig wallets creation

naughtyfox · 2018-10-01T16:32:40Z

src/wallet/wallet2.cpp

-  MINFO("Creating spend key...");
-  std::vector<crypto::secret_key> multisig_keys;
-  rct::key spend_pkey, spend_skey;
+  // In common multisig scheme there are 4 types of key exchange rounds:


Tried to explain how key exchange process goes

naughtyfox · 2018-10-01T16:38:28Z

src/wallet/api/wallet.cpp

+        clearStatus();
+        checkMultisigWalletNotReady(m_wallet);
+
+        return m_wallet->exchange_multisig_keys(epee::wipeable_string(m_password), info);


Do I need to SCOPED_WALLET_UNLOCK(); here as simplewallet does?

naughtyfox · 2018-10-01T16:46:04Z

@moneromooo-monero @b-g-goodell I added some descriptive comments. Also added multisig unit tests for 2/4 and 2/5 schemes which is passed. Rewrote multisig wallets creation function in unit tests also to make it common and more simple (i'm ready to discuss it).

I also have some questions about when I should use keys unlockers like SCOPED_WALLET_UNLOCK() in wallet api calls? It's kinda not transparent for me.

moneromooo-monero · 2018-10-02T12:18:41Z

You use SCOPED_WALLET_UNLOCK when you want the wallet keys to be unlocked till the end of the scope. You use LOCK_IDLE_SCOPE when you want to be the only one accessing wallet2 till the end of the scope. The former implies the latter.

fluffypony

Reviewed

9acf42d Multisig M/N functionality core tests added (naughtyfox) 9f3963e Arbitrary M/N multisig schemes: * support in wallet2 * support in monero-wallet-cli * support in monero-wallet-rpc * support in wallet api * support in monero-gen-trusted-multisig * unit tests for multisig wallets creation (naughtyfox)

naughtyfox force-pushed the multisig-mn branch from e7f7b44 to 4e57501 Compare July 12, 2018 10:12

naughtyfox changed the title ~~[WIP] M/N multisig support~~ M/N multisig support Jul 12, 2018

naughtyfox force-pushed the multisig-mn branch from 4e57501 to 0ac2e20 Compare July 12, 2018 12:18

moneromooo-monero reviewed Jul 13, 2018

View reviewed changes

naughtyfox force-pushed the multisig-mn branch 2 times, most recently from e7f7b44 to a704527 Compare July 13, 2018 15:43

naughtyfox force-pushed the multisig-mn branch from a704527 to 9e5883e Compare July 17, 2018 11:09

naughtyfox force-pushed the multisig-mn branch 3 times, most recently from 211e89d to 7c419f5 Compare July 17, 2018 13:05

naughtyfox force-pushed the multisig-mn branch 2 times, most recently from e7f7b44 to 9990099 Compare August 28, 2018 10:03

naughtyfox force-pushed the multisig-mn branch from 9990099 to 12c7e82 Compare September 27, 2018 15:09

naughtyfox force-pushed the multisig-mn branch from 12c7e82 to 869b7c1 Compare October 1, 2018 13:43

Arbitrary M/N multisig schemes:

9f3963e

* support in wallet2 * support in monero-wallet-cli * support in monero-wallet-rpc * support in wallet api * support in monero-gen-trusted-multisig * unit tests for multisig wallets creation

Multisig M/N functionality core tests added

9acf42d

naughtyfox force-pushed the multisig-mn branch from 869b7c1 to 9acf42d Compare October 1, 2018 16:17

naughtyfox commented Oct 1, 2018

View reviewed changes

fluffypony approved these changes Oct 7, 2018

View reviewed changes

fluffypony merged commit 9acf42d into monero-project:master Oct 7, 2018

rbrunner7 mentioned this pull request Nov 4, 2018

Generalized M/N multisig not yet working reliably #4793

Closed

stoffu mentioned this pull request Sep 15, 2020

Aeon upstream 11 aeonix/aeon#193

Merged

M/N multisig support #4036

M/N multisig support #4036

Conversation

naughtyfox commented Jun 21, 2018 • edited Loading

naughtyfox commented Jul 12, 2018

moneromooo-monero left a 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

naughtyfox Jul 13, 2018 • 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

naughtyfox Jul 17, 2018 • 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

naughtyfox Jul 17, 2018 • 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

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

naughtyfox commented Jul 13, 2018 • edited Loading

naughtyfox commented Jul 17, 2018

ph4r05 commented Jul 24, 2018

naughtyfox commented Jul 24, 2018

ph4r05 commented Jul 24, 2018

naughtyfox commented Jul 24, 2018 • edited Loading

moneromooo-monero commented Jul 24, 2018

b-g-goodell commented Jul 26, 2018

naughtyfox commented Jul 30, 2018

moneromooo-monero commented Aug 24, 2018

b-g-goodell commented Aug 27, 2018

naughtyfox commented Aug 28, 2018

naughtyfox commented Sep 26, 2018

naughtyfox commented Sep 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naughtyfox commented Oct 1, 2018

moneromooo-monero commented Oct 2, 2018

fluffypony left a comment

Choose a reason for hiding this comment

naughtyfox commented Jun 21, 2018 •

edited

Loading

naughtyfox Jul 13, 2018 •

edited

Loading

naughtyfox Jul 17, 2018 •

edited

Loading

naughtyfox Jul 17, 2018 •

edited

Loading

naughtyfox commented Jul 13, 2018 •

edited

Loading

naughtyfox commented Jul 24, 2018 •

edited

Loading