-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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: add key exchange round booster #8203
multisig: add key exchange round booster #8203
Conversation
5f7a13e
to
958359d
Compare
This PR should not be merged until after #8220. |
I'm not a programmer, I don't quite understand the logic of this solution, can you explain its benefits in detail? thanks。。。 |
@gs522084 the PR description is written for non-programmers. Not sure what else I can add to it. |
This PR requires PR #8329. |
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 don't see one of the functions, so I'm wondering whether there is a missing file or something.
src/wallet/wallet2.cpp
Outdated
epee::misc_utils::auto_scope_leave_caller keys_reencryptor; | ||
if (m_ask_password == AskPasswordToDecrypt && !m_unattended && !m_watch_only) | ||
{ | ||
crypto::chacha_key chacha_key; | ||
crypto::generate_chacha_key(password.data(), password.size(), chacha_key, m_kdf_rounds); | ||
m_account.encrypt_viewkey(chacha_key); | ||
m_account.decrypt_keys(chacha_key); | ||
decrypt_keys(password); |
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.
This line didn't have to change, and arguably shouldn't change.
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.
Just trying to remove some code duplication while I'm here
else | ||
{ | ||
// make_multisig() has not been called | ||
// DANGER: If 'num_signers - threshold > 1', but this wallet's future multisig settings |
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 a hard error (exception)? Why allow it?
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.
In this case, the user passed in num_signers
and threshold
manually for a multisig account that doesn't exist yet (e.g. because you are missing one participant's initial kex message - this will happen when boosting round 2 of a 2-of-3 setup where you get the round 1 initial kex message of one participant and then boost round 2 for the other who isn't available). If the manually defined num_signers
and threshold
are different from the num_signers
and threshold
that the actual future multisig account gets, then there can be a problem (as described in the code comment).
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.
Yikes. Should the wallet track the values given in the booster request to ensure that they don't change in the future? Downstream projects can enforce this policy, but it seems doing it here if possible is best.
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 wallet should not couple multisig and normal wallet code, so that the first step of making a multisig wallet is defining M and N and those values are baked into the multisig core account file (even before initializing key exchange by defining the set of signers). But here we are...
In general, this boosting mechanism is very easy to use incorrectly since it only makes sense to use in combination with force updating the post-kex verification round (which is unsafe if used without nailing down the trust matrix/MPC theory of your use-case), so it should only be used by people who know what they are doing. Maybe we need some kind of gate for this 'enterprise-level' feature...
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.
Updating wallet2 to have safer multisig-wallet-creation semantics is out of scope for this PR. I will mark it as a 'todo potential future PR'.
958359d
to
6cb5fdd
Compare
Rebased, should be good to go now |
6cb5fdd
to
1a446e8
Compare
Force-pushed again to update the unit test for boosting a multisig wallet so that the boosted wallet uses force-update to finish the post-kex round. |
// expect that self is in the input list (this guarantees that the input list size always equals the number of intended | ||
// signers for the account [when combined with duplicate checking]) | ||
CHECK_AND_ASSERT_THROW_MES(std::find(signers.begin(), signers.end(), multisig_account.get_base_pubkey()) != signers.end(), | ||
"The local account's signer key was not found in initial multisig kex messages when converting a wallet to 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.
This is a breaking change. In existing code, adding your own initial message to the make_multisig()
command is optional.
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 only thing holding me from approving this is whether the error tracking on the uninitialized account can be improved (without too much hassle).
else | ||
{ | ||
// make_multisig() has not been called | ||
// DANGER: If 'num_signers - threshold > 1', but this wallet's future multisig settings |
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.
Yikes. Should the wallet track the values given in the booster request to ensure that they don't change in the future? Downstream projects can enforce this policy, but it seems doing it here if possible is best.
1a446e8
to
e99b422
Compare
Rebased so I can rebase the seraphis library onto this branch. |
@UkoeHB needs a rebase |
e99b422
to
ddf3af1
Compare
Rebased |
This PR adds a small optimization to multisig account creation that is especially useful for 2-of-3 groups (disclaimer: there is a bounty for it here).
Multisig account creation is a series of rounds. Each round consists of sending messages to all the other group members. A member cannot complete a round until they have received a message for that round from each other member.
A standard 2-of-3 setup ceremony looks like this:
Round 1: Members A, B, C send initial messages to all other members.
Round 2: Members A, B, C use the previous round's messages to make their personal shares of the final key. They send a second message to all other members.
Finalization: Once a member has a full set of round 2 messages, they can finish the account.
That setup ceremony requires 2 communication rounds. For 2-of-3 escrowed markets, the buyer party should instead be able to complete the ceremony in one step so they can fund the multisig wallet right away.
With boosting, we can instead have this setup ceremony:
So, by adding rounds to members A and B, member C can complete a 2-of-3 multisig account in one step.
DISCLAIMER: Kex boosting is expected to be used in conjunction with account force-updating. Extreme care must be taken when designing a system that uses these features to avoid attack vectors that lead to funds getting stuck or possibly stolen (I'd hesitate to use it at all until I or someone else does a write-up on how to do it safely).
TODO
simplewallet
(and themms
presumably @rbrunner7 ).tests/functional_tests/multisig.py
). It would be cool if someone who likes Python could help me with this.Future Work
wallet2
so creating a multisig wallet doesn't involve converting an existing normal wallet's keys. Once a multisig wallet has been created, there should be no way to 'recreate' it using a different M/N configuration. Without that refactor, a user could boost a multisig account using one M/N configuration with a normal account (before converting their normal account to multisig), then later on when they actually convert to a multisig account use a different M/N configuration. If either of those M/N configs was an (N-1)-of-N, then the user will leak their multisig private key shares for that setup when setting up the other configuration (via the key exchange ceremony).