Skip to content
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

wallet-rpc: restore from multisig seed (and fix generation) #8914

Merged

Conversation

jeffro256
Copy link
Contributor

@jeffro256 jeffro256 commented Jun 22, 2023

If the field enable_multisig_experimental in the command restore_deterministic_wallet is set to true, monero-wallet-rpc will attempt to generate the wallet from a multisig seed instead of a normal seed, allowing the multisig wallet user to not have to resync initialization data. This multisig seed is the same data provided from query_key_info.

Edit: Also restoring from multisig seed now works when M <= N-2 and with any number of private multisig keys.

Copy link
Contributor

@vtnerd vtnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This multisig seed - how is it presented to the user (you can tell I've never used it before)? Possibly some confusion with the standard seed.

src/wallet/wallet_rpc_server.cpp Outdated Show resolved Hide resolved
@jeffro256
Copy link
Contributor Author

The multisig seed can be extracted from the “seed” command in the CLI or the “query_key_info” RPC command.

@jeffro256
Copy link
Contributor Author

Yeah ”seed” could potentially be confusing since it’s not just a seed but also threshold information and signer participant configuration data.

@jeffro256 jeffro256 changed the title wallet-rpc: restore from multisig keys feature wallet-rpc: restore from multisig seed feature Jun 24, 2023
@woodser
Copy link
Contributor

woodser commented Jul 10, 2023

@jeffro256 would you be able to PR this against the release branch too, so I can incorporate this change into my next release?

@jeffro256
Copy link
Contributor Author

@woodser It's open now

@woodser
Copy link
Contributor

woodser commented Jul 19, 2023

I'm getting errors testing restoring multisig wallets from seed.

First, successfully restored 2/2 or 2/3 multisig wallets fail on transfer or transfer_split with error "No transaction created", whereas the transaction succeeds if the wallet is not first restored from seed.

Second, 2/4 multisig wallets fail to restore at all with error: "invalid multisig seed".

@woodser
Copy link
Contributor

woodser commented Jul 19, 2023

Should the restoration be based on

m_wallet->generate(m_wallet_file, std::move(rc.second).password(), multisig_keys, create_address_file);
, including restoring with a seed passphrase or offset?

@jeffro256
Copy link
Contributor Author

I'm getting errors testing restoring multisig wallets from seed.

First, successfully restored 2/2 or 2/3 multisig wallets fail on transfer or transfer_split with error "No transaction created", whereas the transaction succeeds if the wallet is not first restored from seed.

Second, 2/4 multisig wallets fail to restore at all with error: "invalid multisig seed".

The multisig seed includes configuration data so you can't change the number of participants and get a valid multisig wallet. You will need to generate a new multisig wallet for each M/N combination.

@woodser
Copy link
Contributor

woodser commented Jul 19, 2023

@jeffro256 Yes, these are tests for separate wallets. The M/N is never changed per wallet.

@jeffro256
Copy link
Contributor Author

Can I see the logs for the "invalid multisig seed" error? The line numbers for that error might help determine what is going on

@woodser
Copy link
Contributor

woodser commented Jul 19, 2023

It's thrown from here when the wallet is generated: https://github.com/monero-project/monero/blob/65035231aac56ece05302d039a718a03e58f1060/src/wallet/wallet2.cpp#L5082

Let me know if you want more log context from the key image exchange.

@woodser
Copy link
Contributor

woodser commented Jul 19, 2023

I'm exchanging keys N - M + 1 times here in my test, assuming that is right?

@jeffro256
Copy link
Contributor Author

It's thrown from here when the wallet is generated:

https://github.com/monero-project/monero/blob/65035231aac56ece05302d039a718a03e58f1060/src/wallet/wallet2.cpp#L5082

Let me know if you want more log context from the key image exchange.

That specific assertion is only necessary for old multisig when thresholds were limited to N/N or (N-1)/N

@jeffro256
Copy link
Contributor Author

I don't know why it's still there

@jeffro256
Copy link
Contributor Author

Okay I pushed changes to do three things: 1) In wallet2::generate() (the multisig overload), I removed the restriction for only N/N and N-1/N multisig, 2) In that same method, I changed the calculation for n_multisig_keys to factor in the size of the multisig seed data since the size of the private multisig key vector can vary based on the rounds of finalization, and 3) I removed the option to turn the multisig seed into Electrum words since the smallest possible seed (2/2) needs at least 172 words to encode it (without a checksum) and that feature was unused in the codebase.

@jeffro256
Copy link
Contributor Author

I also updated the functional tests to actually try a transfer of funds after restoring from the multisig seed

@jeffro256 jeffro256 changed the title wallet-rpc: restore from multisig seed feature wallet-rpc: restore from multisig seed (and fix generation) Jul 20, 2023
@woodser
Copy link
Contributor

woodser commented Jul 20, 2023

Can you also push the changes to #8942 so I can test it? Thanks.

src/wallet/wallet2.cpp Outdated Show resolved Hide resolved
src/wallet/wallet2.cpp Outdated Show resolved Hide resolved
src/wallet/wallet_rpc_server.cpp Show resolved Hide resolved
Comment on lines +989 to +1002
// Given M (threshold) and N (total), calculate the number of private multisig keys each
// signer should have. This value is equal to (N - 1) choose (N - M)
// Prereq: M >= 1 && N >= M && N <= 16
uint64_t num_priv_multisig_keys_post_setup(uint64_t threshold, uint64_t total)
{
THROW_WALLET_EXCEPTION_IF(threshold < 1 || total < threshold || threshold > 16,
tools::error::wallet_internal_error, "Invalid arguments to num_priv_multisig_keys_post_setup");

uint64_t n_multisig_keys = 1;
for (uint64_t i = 2; i <= total - 1; ++i) n_multisig_keys *= i; // multiply by (N - 1)!
for (uint64_t i = 2; i <= total - threshold; ++i) n_multisig_keys /= i; // divide by (N - M)!
for (uint64_t i = 2; i <= threshold - 1; ++i) n_multisig_keys /= i; // divide by ((N - 1) - (N - M))!
return n_multisig_keys;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@UkoeHB This formula appears to work correctly to calculate the number of private multisig keys per signer, thanks!

Comment on lines +203 to +265
def remake_some_multisig_wallets_by_multsig_seed(self, threshold):
N = len(self.wallet)
signers_to_remake = set()
num_signers_to_remake = random.randint(1, N) # Do at least one
while len(signers_to_remake) < num_signers_to_remake:
signers_to_remake.add(random.randint(0, N - 1))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed the function to restore some not all multisig wallets in case bugs would arise when a seed-restored interacted w/ a normal wallet.

Comment on lines +113 to +124
self.create_multisig_wallets(1, 2, '4A8RnBQixry4VXkqeWhmg8L7vWJVDJj4FN9PV4E7Mgad5ZZ6LKQdn8dYJP2RePX6HMaSkbvTbrWUFhDNcNcHgtNmQ4S8RSB')
self.import_multisig_info([0, 1], 5)
txid = self.transfer([0])
self.import_multisig_info([0, 1], 6)
self.check_transaction(txid)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added a test case for a 1/2 multisig wallet since none of the cases yet have 1 as the threshold

@woodser
Copy link
Contributor

woodser commented Jul 20, 2023

With the latest push to the release branch, 2/4 multisig wallets are restoring successfully.

However, calling transfer or transfer_split after restoring any multisig wallet still gives the error "No transaction created". Hopefully this is reproducible in the unit tests.

It works as expected without restoring the multisig wallet from seed.

@selsta
Copy link
Collaborator

selsta commented Aug 1, 2023

@woodser are all your issues resolved?

@woodser
Copy link
Contributor

woodser commented Aug 2, 2023

@selsta No this isn't resolved yet; still waiting for a fix to the "No transactions created" error after restoring from seed.

@jeffro256
Copy link
Contributor Author

So I took the time to setup the monero-java JUnit tests using the PR branch that @woodser was using using a patched (with this PR) wallet rpc server. What I found was that the "no transactions created" error for me was 1) temporary and 2) affected both restoring with multisig seed and not restoring. I think that there might be a race condition in tests somewhere that is exacerbated by restoring by seed since the cache is flushed, but I have not been able to reliably reproduce this error; the test cases usually passes for me when restoring from multisig seed.

@woodser
Copy link
Contributor

woodser commented Aug 10, 2023

After some digging based on @jeffro256 's comment, I found a bug in the Java tests when restoring from seed, where the keys were not being exchanged correctly (fixed in woodser/monero-java@13004cc).

It's working now with that fix. :)

@luigi1111 luigi1111 merged commit 3b67d5f into monero-project:master Aug 17, 2023
jeffro256 added a commit to jeffro256/monero that referenced this pull request Aug 20, 2023
The changes to the multisig tests in monero-project#8914 and monero-project#8904 affected each other, this PR cleans up the code and fixes that issue.
@jeffro256 jeffro256 deleted the restore_multisig_seed_rpc branch August 20, 2023 03:20
jeffro256 added a commit to jeffro256/monero that referenced this pull request Aug 20, 2023
The changes to the multisig tests in monero-project#8914 and monero-project#8904 affected each other, this PR cleans up the code and fixes that issue.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants