New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multisig #2134
Multisig #2134
Conversation
This needs review from a qualified cryptographer and/or the cryptography community (i.e. the latter by writing it up in a paper and seeking review) before being deployed live but enabling it for testnet would be fine. (Comment on cryptography not code) |
src/ringct/rctSigs.cpp
Outdated
{ | ||
LOG_PRINT_L1("Error in verRct: " << e.what()); | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep both clauses so nothing's uncaught?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should not be anything throwing ints, strings, etc. But I'll add a ... anyway.
{ | ||
LOG_PRINT_L1("Error in verRct: " << e.what()); | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here, should we keep both clauses so nothing's uncaught?
src/simplewallet/simplewallet.cpp
Outdated
{ | ||
uint32_t threshold; | ||
m_wallet->multisig(&threshold); | ||
uint32_t signers_needed = threshold - signers - 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
anonimal moneromooo: a potential underflow in simplewallet.cpp:794? I can't find a case where we ensure exported txs signer count is *not* 0 when signing multisig tx.
* anonimal assuming threshold has the potential to be 0 or < signers count
anonimal threshold appears defaulted to NULL, so I'm assuming the potential remains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checks are those:
THROW_WALLET_EXCEPTION_IF(exported_txs.m_signers.size() > m_multisig_threshold,
error::wallet_internal_error, "Transaction was signed by too many signers");
THROW_WALLET_EXCEPTION_IF(exported_txs.m_signers.size() == m_multisig_threshold,
error::wallet_internal_error, "Transaction is already fully signed");
The end of the signing function ends with the local key being added to the signers set, so it cannot be 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return sign_multisig_tx(exported_txs, filename, txids);
exported_txs.m_signers.insert(hash);
Thank you for clarifying 😄! I hadn't looked closer beyond those checks (like I said on IRC, I had only gave a once-over review).
27f705e
to
8a57bba
Compare
Is this an implementation of the design posted (and then later disappeared) by shen noether? |
|
8a57bba
to
816a676
Compare
816a676
to
deae798
Compare
This now uses random k values |
43365a9
to
f9a8429
Compare
Since people are testing and reporting bugs, I've now pushed a first fix to https://github.com/moneromooo-monero/bitmonero/tree/multisig2 and will continue there until fixes are ready to be merged to this PR. Note that the multisig2 branch contains multisig RPC too, not well tested yet, but feel free to test it too :) |
src/simplewallet/simplewallet.cpp
Outdated
catch (const std::exception &e) | ||
{ | ||
LOG_ERROR("Error exporting multisig info: " << e.what()); | ||
fail_msg_writer() << "Error exporting multisig info: " << e.what(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tr(...)
is forgotten in here and many other places.
src/simplewallet/simplewallet.cpp
Outdated
for (auto &ptx: txs.m_ptx) | ||
{ | ||
m_wallet->commit_tx(ptx); | ||
success_msg_writer(true) << tr("Money successfully sent, transaction: ") << get_transaction_hash(ptx.tx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps make the message more informative like "Transaction successfully submitted", as per #2191?
initialize_transfer_details(a, x, ver); | ||
return; | ||
} | ||
a & x.m_multisig_info; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the following should be inserted here:
a & x.m_key_image_partial;
src/wallet/wallet2.cpp
Outdated
crypto::public_key tx_key = get_tx_pub_key_from_received_outs(td); | ||
cryptonote::keypair in_ephemeral; | ||
crypto::key_image ki; | ||
td.m_multisig_k = rct::skGen(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this rarely needed, because td.m_multisig_k
is already set inside process_new_transaction()
? I think the only situation to have to set this field again would be when a complete multisig tx was sent to the daemon (which means td.m_multisig_k
was set to zero in commit_tx()
) but was never mined and dropped from the mempool for some reason. To take that into account, I'd do as follows:
if (td.m_multisig_k == rct::zero())
td.m_multisig_k = rct::skGen();
f9a8429
to
83a5f13
Compare
Safer that way about multisig_k. All the rest comments done except for == for private keys, for which there is no such operator and I've not added one. |
src/wallet/wallet2.h
Outdated
@@ -701,15 +767,17 @@ namespace tools | |||
}; | |||
} | |||
BOOST_CLASS_VERSION(tools::wallet2, 18) | |||
BOOST_CLASS_VERSION(tools::wallet2::transfer_details, 7) | |||
BOOST_CLASS_VERSION(tools::wallet2::transfer_details, 9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the increase by 2?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I squashed two patches into one, both increasing the version. So nobody but me will ever see v8 :) Guess I can set it to 8 before merging.
src/wallet/wallet2.cpp
Outdated
for (const auto &source: sd.sources) | ||
indices.push_back(source.real_output); | ||
THROW_WALLET_EXCEPTION_IF(!rct::signMultisig(ptx.tx.rct_signatures, indices, k, ptx.msout, rct::sk2rct(get_account().get_keys().m_spend_secret_key)), | ||
error::wallet_internal_error, "Failed signing, transaction likely malformed"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think multisig_k
should be cleared here in order to prevent an attacker from stealing the spend secret key in the following way:
Consider a scenario with a 3/3 multisig wallet, where Alice, Bob and Charlie are the participants. Alice and Charlie are colluding together to steal the spend secret key from Bob. The shared view secret key is a
while the spend secret keys of Alice, Bob and Charlie are b_Alice
, b_Bob
and b_Charlie
, respectively.
1. Alice creates a transaction and passes it to Bob.
At this point, the partial signature scalar amounts to:
s_step1 = k_Alice - c * (Hs(a*R) + b_Alice)
2. Bob signs it with his spend secret key and passes it to Charlie.
At this point, the partial signature scalar amounts to:
s_step2 = k_Alice + k_Bob - c * (Hs(a*R) + b_Alice + b_Bob)
3. Charlie shares the partially signed transaction with Alice.
At this point, Alice and Charlie can know Bob's contribution to the signature scalar by taking the difference:
s_diff = s_step2 - s_step1
= k_Bob - c * b_Bob
4. Alice creates another transaction and passes it to Bob, saying "Sorry, there was a mistake in the transaction I made previously, so please sign again this new transaction."
5. Bob signs the new transaction and passes it to Charlie.
6. Charlie shares the partially signed new transaction with Alice.
At this point, Alice and Charlie do exactly the same computation as above, and obtain another instance of Bob's contribution to the signature scalar using his same k_Bob
and a different c_new
:
s_diff_new = k_Bob - c_new * b_Bob
7. Alice and Charlie can extract Bob's spend secret key as
b_Bob = (s_diff_new - s_diff) / (c_new - c)
To prevent this attack, the multisig_k
should never be reused, and it should be cleared every time a participant signs a transaction (either via transfer
or sign_multisig
). If a partially signed transaction is to be cancelled, the export/import of multisig info should be performed again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I was overlooking the section a few lines below.
But I don't see the same code inserted at the end of wallet2::transfer_selected_rct
. Isn't it necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to save_multisig_tx. It can't be in the transfer_selected_rct etc, as tx creation might fail, and/or the user might cancel a tx on prompt. This would mean the user can't redo another tx (using the same inputs). Having the zero in save_multisig_tx means the data is zeroed when the tx actually gets out. It's also in commit_tx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in save_multisig_tx
:
for (auto &ptx: txs.m_ptx)
{
for (auto &e: ptx.construction_data.sources)
e.multisig_kLRki.k = rct::zero();
}
is good for not revealing the secret to other participants, but this doesn't clear m_transfer[idx].m_multisig_k
used as input; i.e. the reuse of multisig_k is still possible.
It can't be in the transfer_selected_rct etc, as tx creation might fail, and/or the user might cancel a tx on prompt. This would mean the user can't redo another tx (using the same inputs).
Then the multisig_k can be cleared in the end of create_transactions_2
, maybe with some on-accept function callback mechanism to allow cancellation. I think there should be a way around.
It's also in commit_tx.
Yes, I see it:
// tx generated, get rid of used k values
for (size_t idx: ptx.selected_transfers)
m_transfers[idx].m_multisig_k = rct::zero();
But this is called only by the last participant who does submit_multisig
, and doesn't prevent the reuse of multisig_k for other participants.
Sorry I was wrong. I was surprised that I still observe occasional failure of signature verification for fully signed multisig tx and am trying to figure out why. |
I've seen that too. I thought it was due to my N-1/N changes, good (in a way) to know it's also in this branch. I guess I broke it recently because I did not see that happen before. Occasional breakage maybe hints at a missing sc_reduce32 somewhere... |
src/wallet/wallet2.cpp
Outdated
try | ||
{ | ||
MFATAL("import_multisig: refreshing"); | ||
refresh(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now I identified a possible scenario where the fully signed multisig tx can be rejected by the daemon:
Consider a 2/2 multisig wallet where the participants are Alice and Bob.
-
Fund arrives at the multisig address.
-
Alice does
export_multisig msinfo_alice
-
Bob does
export_multisig msinfo_bob
-
Bob does
import_multisig msinfo_alice
-
Alice does
import_multisig msinfo_bob
-
Alice does
transfer
orsweep_all
to createmultisig_monero_tx
-
Bob does
sign_multisig multisig_monero_tx
to make it fully signed -
Bob does
submit_multisig multisig_monero_tx
which gets rejected by the daemon
The problem is that when Bob does import_multisig
, his multisig_k
gets renewed due to refresh()
being called within wallet2::import_multisig
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's impossible to avoid doing refresh()
here, as a workaround perhaps we could show a warning message like:
Warning:
import_multisig
resets some internal data, so unless you initiate a multisig transaction, you need to share your multisig info again with others byexport_multisig
{ | ||
LOG_ERROR("Multisig error: " << e.to_string()); | ||
fail_msg_writer() << tr("Multisig error: ") << e.what(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be also added to sweep_main
and sweep_unmixable
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
src/wallet/wallet2.h
Outdated
BOOST_CLASS_VERSION(tools::wallet2::transfer_details, 7) | ||
BOOST_CLASS_VERSION(tools::wallet2::transfer_details, 9) | ||
BOOST_CLASS_VERSION(tools::wallet2::multisig_info, 1) | ||
BOOST_CLASS_VERSION(tools::wallet2::multisig_tx_set, 1) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps use version 0 since these are new?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, but it's arbitrary anyway.
The random failure is caused by the recent change to shuffle outputs randomly, which cause the subsequent signers to sometimes end up with a different order, and the tx prefixes don't match. I'll either pass the order around, or sort them in a deterministic way based on public information. |
I was missing that problem which never occurred to me before (but I could reproduce it easily). But that seems to occur at wallet2.cpp L3732 with |
Roughly what is the ratio of failing txes to all txes ? Mine are getting accepted by the daemon, but I might need to try more. |
4014bd5
to
029be1b
Compare
Rebased on top of bulletproofs ( |
029be1b
to
20bb1e5
Compare
created new 2/3 multisig wallet on testnet, sent 5 tx to it, synced data console: |
The same seems to happen without multisig, so I'll ignore that for the purposes of this patch. Please open a new bug so it doesn't fall through the cracks. |
Scheme by luigi1111
Scheme by luigi1111: Multisig for RingCT on Monero 2 of 2 User A (coordinator): Spendkey b,B Viewkey a,A (shared) User B: Spendkey c,C Viewkey a,A (shared) Public Address: C+B, A Both have their own watch only wallet via C+B, a A will coordinate spending process (though B could easily as well, coordinator is more needed for more participants) A and B watch for incoming outputs B creates "half" key images for discovered output D: I2_D = (Hs(aR)+c) * Hp(D) B also creates 1.5 random keypairs (one scalar and 2 pubkeys; one on base G and one on base Hp(D)) for each output, storing the scalar(k) (linked to D), and sending the pubkeys with I2_D. A also creates "half" key images: I1_D = (Hs(aR)+b) * Hp(D) Then I_D = I1_D + I2_D Having I_D allows A to check spent status of course, but more importantly allows A to actually build a transaction prefix (and thus transaction). A builds the transaction until most of the way through MLSAG_Gen, adding the 2 pubkeys (per input) provided with I2_D to his own generated ones where they are needed (secret row L, R). At this point, A has a mostly completed transaction (but with an invalid/incomplete signature). A sends over the tx and includes r, which allows B (with the recipient's address) to verify the destination and amount (by reconstructing the stealth address and decoding ecdhInfo). B then finishes the signature by computing ss[secret_index][0] = ss[secret_index][0] + k - cc[secret_index]*c (secret indices need to be passed as well). B can then broadcast the tx, or send it back to A for broadcasting. Once B has completed the signing (and verified the tx to be valid), he can add the full I_D to his cache, allowing him to verify spent status as well. NOTE: A and B *must* present key A and B to each other with a valid signature proving they know a and b respectively. Otherwise, trickery like the following becomes possible: A creates viewkey a,A, spendkey b,B, and sends a,A,B to B. B creates a fake key C = zG - B. B sends C back to A. The combined spendkey C+B then equals zG, allowing B to spend funds at any time! The signature fixes this, because B does not know a c corresponding to C (and thus can't produce a signature). 2 of 3 User A (coordinator) Shared viewkey a,A "spendkey" j,J User B "spendkey" k,K User C "spendkey" m,M A collects K and M from B and C B collects J and M from A and C C collects J and K from A and B A computes N = nG, n = Hs(jK) A computes O = oG, o = Hs(jM) B anc C compute P = pG, p = Hs(kM) || Hs(mK) B and C can also compute N and O respectively if they wish to be able to coordinate Address: N+O+P, A The rest follows as above. The coordinator possesses 2 of 3 needed keys; he can get the other needed part of the signature/key images from either of the other two. Alternatively, if secure communication exists between parties: A gives j to B B gives k to C C gives m to A Address: J+K+M, A 3 of 3 Identical to 2 of 2, except the coordinator must collect the key images from both of the others. The transaction must also be passed an additional hop: A -> B -> C (or A -> C -> B), who can then broadcast it or send it back to A. N-1 of N Generally the same as 2 of 3, except participants need to be arranged in a ring to pass their keys around (using either the secure or insecure method). For example (ignoring viewkey so letters line up): [4 of 5] User: spendkey A: a B: b C: c D: d E: e a -> B, b -> C, c -> D, d -> E, e -> A Order of signing does not matter, it just must reach n-1 users. A "remaining keys" list must be passed around with the transaction so the signers know if they should use 1 or both keys. Collecting key image parts becomes a little messy, but basically every wallet sends over both of their parts with a tag for each. Thia way the coordinating wallet can keep track of which images have been added and which wallet they come from. Reasoning: 1. The key images must be added only once (coordinator will get key images for key a from both A and B, he must add only one to get the proper key actual key image) 2. The coordinator must keep track of which helper pubkeys came from which wallet (discussed in 2 of 2 section). The coordinator must choose only one set to use, then include his choice in the "remaining keys" list so the other wallets know which of their keys to use. You can generalize it further to N-2 of N or even M of N, but I'm not sure there's legitimate demand to justify the complexity. It might also be straightforward enough to support with minimal changes from N-1 format. You basically just give each user additional keys for each additional "-1" you desire. N-2 would be 3 keys per user, N-3 4 keys, etc. The process is somewhat cumbersome: To create a N/N multisig wallet: - each participant creates a normal wallet - each participant runs "prepare_multisig", and sends the resulting string to every other participant - each participant runs "make_multisig N A B C D...", with N being the threshold and A B C D... being the strings received from other participants (the threshold must currently equal N) As txes are received, participants' wallets will need to synchronize so that those new outputs may be spent: - each participant runs "export_multisig FILENAME", and sends the FILENAME file to every other participant - each participant runs "import_multisig A B C D...", with A B C D... being the filenames received from other participants Then, a transaction may be initiated: - one of the participants runs "transfer ADDRESS AMOUNT" - this partly signed transaction will be written to the "multisig_monero_tx" file - the initiator sends this file to another participant - that other participant runs "sign_multisig multisig_monero_tx" - the resulting transaction is written to the "multisig_monero_tx" file again - if the threshold was not reached, the file must be sent to another participant, until enough have signed - the last participant to sign runs "submit_multisig multisig_monero_tx" to relay the transaction to the Monero network
Useful to speed tests up and avoid unnecessary leftover files
It exports raw transactions, so they may be used by other tools, for instance to be relayed to the network externally.
Thanks to kenshi84 for help getting this work
While there, move the wallet2 ctor to the cpp file as it's a huge amount of init list now, and remove an unused one.
20bb1e5
to
ceabc4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed
ceabc4f change the N-1/N multisig second message signer for auth (moneromooo-monero) 55c2845 core_tests: multisig test now tests multiple inputs (moneromooo-monero) 98db7ee wallet: factor multisig info parsing (moneromooo-monero) 31a97e7 wallet: use raw encrypted data in multisig import/export RPC (moneromooo-monero) 2fa707d wallet: add multisig sign/submit RPC (moneromooo-monero) e36f5b6 Match surae's recommendation to derive multisig keys (moneromooo-monero) a36c261 wallet2: fix slow multisig unit tests with subaddress patch (moneromooo-monero) fa56971 make multisig work with subaddresses (moneromooo-monero) dffa0dc simplewallet: add export_raw_multisig command (moneromooo-monero) 7f4c220 simplewallet: add multisig to wallet type in wallet_info output (moneromooo-monero) 2652903 wallet: guard against partly initialized multisig wallet (moneromooo-monero) 66e34e8 add multisig core test and factor multisig building blocks (moneromooo-monero) f4eda44 N-1/N multisig (moneromooo-monero) cd64c79 multisig address generation RPC (moneromooo-monero) fff871a gen_multisig: generates multisig wallets if participants trust each other (moneromooo-monero) 95a21a7 wallet2: allow empty wallet filename to avoid saving data (moneromooo-monero) b84b356 tests: add multisig unit tests (moneromooo-monero) 4c31332 Add N/N multisig tx generation and signing (moneromooo-monero) 6d219a9 wallet: add multisig key generation (moneromooo-monero)
No description provided.