add mnemonicinfo, importmasterkey by scravy · Pull Request #37 · dtr-org/unit-e

scravy · 2018-08-24T09:12:17Z

This is the importmasterkey and mnemonicinfo RPCs. Tests will follow shortly, do not merge yet ;-) but you can start reviewing.

The unit-e importmasterkey is named extkeyimportmaster in particl (but I chose not to refer to it as an ext key; the extended keys are technically the the derived keys in the HD structure which also reflect the path (m' / 44 / ...).

mnemonicinfo is like a dry run and I am using it to test the mnemonic functionality for conformance with BIP32 and BIP39.

scravy · 2018-08-24T14:04:51Z

This is done and up for review now! Tests are complete.

I was facing the following issue: I had put lib uninorm (for utf8/unicode) and the mnemonic.cpp into libunite_common (src/Makefile.am). This was fine on macOS but failed to link in linux. For now I moved everything into libunite_wallet (which after all is probably even better as mnemonics strictly relate to the wallet anyhow).

Another thing which bugs me: Although we're pretty sure that node.start and node.stop in the functional tests wait for the node to actually have stopped and actually have started the test sometimes fails and to me it looks like as if the node is not yet properly handling connections :-( (talking about wallet_importmasterkey test).

scravy · 2018-08-24T14:10:45Z

I should note the following thing: I am trying to use as much bitcoin functionality as possible. Bitcoins CWallet already has methods to set the master key but I do suspect it was not their intention for them to be used /after/ a wallet has been created already. When the node comes up it automatically creates a wallet. Upon creation the wallet is preseeded with a list of reserve keys. It is thus tricky to distinguish a newly created wallet from a wallet which is already in use. Nevertheless you could import a new masterkey into an already used wallet, which is why setting a new masterkey will automatically backup your existing wallet (although in most cases this will be an empty wallet).

I'll create a ticket for this. I could imagine that we might want to detect a newly created wallet and just prohibit overwriting the masterkey for a wallet that is already in use.

edit: Issue: #40

Gnappuraz · 2018-08-27T08:34:28Z

+  int m_deepestTxnDepth = 0;  // for stake mining

-  int m_stakeLimitHeight = 0; // for regtest, don't stake above nStakeLimitHeight
+  int m_stakeLimitHeight =


maybe we can put the comment in the line above and thus prevent the new line with a 0;

Gnappuraz · 2018-08-27T08:40:59Z

@@ -0,0 +1,163 @@
+// Copyright (c) 2010 Satoshi Nakamoto


I am confused, didn't we take this from particl? cause it seems here it was taken from bitcoin.

Copy'n'paste abfuck from my side... I merely duplicated rpcwallet and got rid of it's guts. Changing!

In addition: I'd like to point out that the code in rpcmnemonic we did not take form particl at all, it sets a master key in the existing CWallet, doing a lot less voodoo then the CHDWallet form particl.

nzmdn · 2018-08-27T09:07:11Z

+  LOCK(wallet->cs_wallet);
+  const std::string walletFileName = wallet->GetName();
+  std::string mnemonic = "";
+  if (request.params.size() > 0) {


This check seems to be redundant as the size of the request.params is checked at the beginning of the function.

Gnappuraz · 2018-08-27T09:23:22Z

@@ -0,0 +1,67 @@
+#!/usr/bin/env python3
+# Copyright (c) 2016-2017 The Bitcoin Core developers


Same copy paste license?

Gnappuraz · 2018-08-27T09:23:35Z

@@ -0,0 +1,185 @@
+#!/usr/bin/env python3
+# Copyright (c) 2016-2017 The Bitcoin Core developers


These tests are our very own.

scravy · 2018-08-27T09:33:25Z

@Nizametdinov about extkeygenesisimport and extkeyimportmaster.

The distinction between these commands was not ported. All keys are derived along a chain, i.e. m / 0'/1' (this means masternode (it's always m), default account (account 0, purpose field according to BIP43), chain (1 is internal).

The bitcoin codebase we're forking form already has support for these internal/external chain splits (FEATURE_HD_SPLIT). In particl they just opted for a ridiculously high number for the internal chain (444444) and my guess would be that they created the adresses for the genesis transactions (in the genesis blocks) in that chain, thus need to keep it. Anyways, feature-wise we do have this split and we just need to have an index for our coin :-)

For the desktop application: use importmasterkey with a seed.

scravy · 2018-08-27T09:50:16Z

@Gnappuraz I checked the copyright headers one by one again. All the copyright for ShadowCoin/BlackCoin were correct. I added the headers for particl copyright as it was indeed missing from most files in esperanza/.

Our own copyright we add before we release with a script. The script already exists and does not add if the copyright notice is already there and does add if it's not there yet.

scravy force-pushed the mnemonic-rpcs branch 12 times, most recently from cce4a8f to 78b9924 Compare August 24, 2018 14:00

scravy requested review from Gnappuraz, kostyantyn, nzmdn and thothd August 24, 2018 14:05

scravy force-pushed the mnemonic-rpcs branch 4 times, most recently from a70e6b0 to f33d3d5 Compare August 24, 2018 20:48

add mnemonicinfo, importmasterkey

2e03c07

scravy force-pushed the mnemonic-rpcs branch from f33d3d5 to 2e03c07 Compare August 24, 2018 21:03

undo travis build changes

bd98003

Gnappuraz reviewed Aug 27, 2018

View reviewed changes

factor in review comments

bd730fb

nzmdn reviewed Aug 27, 2018

View reviewed changes

remove redundant check

ac8d0ba

scravy mentioned this pull request Aug 27, 2018

non-staking related particl wallet changes #12

Closed

3 tasks

Gnappuraz reviewed Aug 27, 2018

View reviewed changes

scravy mentioned this pull request Aug 27, 2018

detect empty wallet or create new wallet #40

Open

credit where credit is due

fcf4fe9

Gnappuraz approved these changes Aug 27, 2018

View reviewed changes

scravy mentioned this pull request Aug 27, 2018

WIP: apply clang-format -style=Google on *.cpp and *.h #34

Closed

remove copyright where no credit is due

0341a86

scravy force-pushed the mnemonic-rpcs branch from 8b8a992 to 0341a86 Compare August 27, 2018 10:02

nzmdn approved these changes Aug 27, 2018

View reviewed changes

scravy merged commit c395aee into master Aug 27, 2018

scravy deleted the mnemonic-rpcs branch August 27, 2018 10:42

scravy added this to the Implement BIP39 (seed wallet from mnemonics) milestone Sep 4, 2018

scravy added the wallet label Sep 4, 2018

scravy self-assigned this Sep 4, 2018

		@@ -0,0 +1,67 @@
		#!/usr/bin/env python3
		# Copyright (c) 2016-2017 The Bitcoin Core developers

		@@ -0,0 +1,185 @@
		#!/usr/bin/env python3
		# Copyright (c) 2016-2017 The Bitcoin Core developers

Conversation

scravy commented Aug 24, 2018

Uh oh!

scravy commented Aug 24, 2018

Uh oh!

scravy commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gnappuraz Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Gnappuraz Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

scravy Aug 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

scravy Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

nzmdn Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

scravy Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

Gnappuraz Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

Gnappuraz Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

scravy Aug 27, 2018

Choose a reason for hiding this comment

Uh oh!

scravy commented Aug 27, 2018

Uh oh!

scravy commented Aug 27, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

scravy commented Aug 24, 2018 •

edited

Loading

Gnappuraz Aug 27, 2018 •

edited

Loading

scravy Aug 27, 2018 •

edited

Loading