Feature/wallet backup auto #10

marlengit · 2016-10-10T12:48:21Z

This is the auto wallet backup feature. There are 2 command line parameters to enable the feature:
-autobackupwalletpath=/mybackupfolder
-autobackupblock=1234

The autobackupwalletpath may be a directory or file name. A hash character (#) in the string will be replaced with the block number specified. If the path is a directory then the default filename will be used which is the existing wallet file name with suffix ".auto.#.bak".

The autobackupblock number is optional and will default to the MVHF fork block. At the moment this is hard coded and so this section will need to be updated when the hard fork is better defined. Beyond the hard fork users could use this parameter to perform an auto backup at any future block number.

If used with -disablewallet the feature is non-functional and outputs a conflict message to debug.log.

There is a regtest that can be run for this feature : qa/pull-tester/rpc-tests.py walletbackupauto

ftrader

compiled and tested ok on my machine, just a few rough edges w.r.t. to current req+design. Nothing major though.

I'll summarise:

the backup block parameter (autobackupblock) is currently not required, so we need to at least extend the SW-REQs. Please add a new SW-REQ-10-6 (similar to SW-REQ-10-1 but for the backup block) and add a related description in design section 5.5.1 (Configuration for wallet backup). We don't need a separate design section. The alternative would be to drop the functionality, but since it's very useful for testing, I suggest we keep it.
the handling of autobackupwalletpath is not yet completely covering the design. We can simplify the design (modify design.md and test-design docs), or implement and test it.
extracting the 114 block and testing just prior to hitting it should really be done, especially because the semantics of the parameter are quite important to verify.
missing conflict msg at startup (where other parameter conflicts are usually identified, to give the user a heads up to fix his parameter problems)
missing test case for safe shutdown if backup file cannot be written (suggest to have a node configured to write its backup to a temp directory which has been made read-only, then check that it shut down after passing the backup block height. Would need to restart it in some disconnected form to check that its wallet is pre-fork)

ftrader · 2016-10-10T17:54:42Z

qa/pull-tester/rpc-tests.py

@@ -97,6 +98,7 @@
    'fundrawtransaction.py',
    'signrawtransactions.py',
    'walletbackup.py',
+    'walletbackupauto.py', #MVHF-BU


prefer a space between comment symbol and comment, for better readability

added to next commit

ftrader · 2016-10-10T18:01:51Z

qa/rpc-tests/test_framework/util.py

@@ -230,6 +231,10 @@ def start_node(i, dirname, extra_args=None, rpchost=None, timewait=None, binary=
    # RPC tests still depend on free transactions
    args = [ binary, "-datadir="+datadir, "-server", "-keypool=1", "-discover=0", "-rest", "-blockprioritysize=50000" ]
    if extra_args is not None: args.extend(extra_args)
+    if os.getenv("PYTHON_DEBUG", ""):


debug traces left over, unmarked - i suppose they do not need to be retained.

These are useful to retain. The PYTHON_DEBUG environment variable has already been established in the test framework. Debug outputs only appear when the variable is set. In this case I have extended it to output the command line parameters of nodes. I think others will also find this useful.

ftrader · 2016-10-10T18:03:19Z

qa/rpc-tests/test_framework/util.py

@@ -260,6 +265,16 @@ def start_nodes(num_nodes, dirname, extra_args=None, rpchost=None, binary=None):
 def log_filename(dirname, n_node, logname):
    return os.path.join(dirname, "node"+str(n_node), "regtest", logname)

+#MVHF-BU


please use multi-line marker format
see https://github.com/BTCfork/hardfork_prototype_1_mvf-bu/blob/master/doc/mvf-bu-coding-guideline.md#changing-legacy-code-sections

added to next commit

ftrader · 2016-10-10T18:05:44Z

qa/rpc-tests/test_framework/util.py

@@ -260,6 +265,16 @@ def start_nodes(num_nodes, dirname, extra_args=None, rpchost=None, binary=None):
 def log_filename(dirname, n_node, logname):
    return os.path.join(dirname, "node"+str(n_node), "regtest", logname)

+#MVHF-BU
+def search_file(searchfilepath, searchtext):


would be nice to have docstrings for new functions added in code, to explain briefly what they do & return
https://www.python.org/dev/peps/pep-0257/

added to next commit

ftrader · 2016-10-10T18:37:29Z

qa/rpc-tests/walletbackupauto.py

+Exercise the auto backup wallet code.  Ported from walletbackup.sh.
+
+Test case is:
+4 nodes. 1 2 and 3 send transactions between each other, fourth node is a miner


Correct first sentence : there are 5 nodes in this test...

added to next commit

ftrader · 2016-10-10T19:28:54Z

src/validationinterface.cpp

    g_signals.SyncTransaction.disconnect_all_slots();
    g_signals.UpdatedBlockTip.disconnect_all_slots();
 }

+


extra blank line - pls remove

added to next commit

ftrader · 2016-10-10T19:33:15Z

src/wallet/wallet.cpp

+    boost::filesystem::path pathBackupWallet = strDest;
+
+    // Test for directory only, if so add default filename
+	if (boost::filesystem::is_directory(strDest))


check with MVHF-BU-DES-WABU-2 and https://github.com/BTCfork/hardfork_prototype_1_mvf-bu/blob/master/doc/mvf-bu-test-design.md#411-sw-req-10-1

some more handling is needed here e.g. if it's not a path, but only a filename

also, please check indentation in this newly added method, it looks a little inconsistent

Updated BackupWalletAuto function to handle the 4 variants listed in the test design steps.
The testing was updated to test the 4 variants.

I also found out that a hash # character will end the line when using GetArg so now the special block number replace character is @ instead.

ftrader · 2016-10-10T19:36:21Z

src/wallet/wallet.h

@@ -28,6 +29,7 @@

 #include <boost/shared_ptr.hpp>

+


extra blank line introduced

ftrader · 2016-10-10T19:36:31Z

src/wallet/wallet.h

@@ -405,6 +407,8 @@ class CWalletTx : public CMerkleTx
    bool RelayWalletTransaction();

    std::set<uint256> GetConflicts() const;
+


extra blank lines introduced

added to next commit

ftrader · 2016-10-10T19:38:46Z

src/wallet/wallet.h

@@ -571,6 +575,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface

    const CWalletTx* GetWalletTx(const uint256& hash) const;

+    // MVHF-BU


this new method should trace to what is now section 5.5.3 (Procedure to create the wallet file backup) in the design.md.

We need to add design elements to 5.5.2 and 5.5.3 , and then trace them here.

Don't worry about this now, I'm just noting it for later.

Made updates to 5.5.2 and created a pull request in the doc repo

marlengit added 5 commits October 10, 2016 11:03

Add walletbackupauto.py

0b24c2d

Added BackupWalletAuto

43e7780

Add -disablewallet tests.

39db360

Add conflict msg test

9b45727

Add MVHF-BU and Copyright comments

86ca20d

ftrader reviewed Oct 10, 2016

View reviewed changes

marlengit added 3 commits October 12, 2016 12:35

Update formatting and comments

79fb174

Update autobackupwalletpath variants

4dc4147

Update help and -disablewallet conflict messages

5031c57

ftrader merged commit ea17019 into BTCfork:master Oct 15, 2016

marlengit deleted the feature/wallet_backup_auto branch October 27, 2016 18:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/wallet backup auto #10

Feature/wallet backup auto #10

marlengit commented Oct 10, 2016 •

edited

ftrader left a comment

ftrader Oct 10, 2016

marlengit Oct 12, 2016

ftrader Oct 10, 2016

marlengit Oct 12, 2016

ftrader Oct 10, 2016

marlengit Oct 12, 2016

ftrader Oct 10, 2016 •

edited

marlengit Oct 12, 2016

ftrader Oct 10, 2016

marlengit Oct 12, 2016

ftrader Oct 10, 2016

marlengit Oct 12, 2016

ftrader Oct 10, 2016 •

edited

marlengit Oct 13, 2016

marlengit Oct 13, 2016

ftrader Oct 10, 2016

marlengit Oct 13, 2016

ftrader Oct 10, 2016

marlengit Oct 12, 2016

ftrader Oct 10, 2016

marlengit Oct 13, 2016

ftrader Oct 15, 2016

		@@ -405,6 +407,8 @@ class CWalletTx : public CMerkleTx
		bool RelayWalletTransaction();

		std::set<uint256> GetConflicts() const;

		@@ -571,6 +575,9 @@ class CWallet : public CCryptoKeyStore, public CValidationInterface

		const CWalletTx* GetWalletTx(const uint256& hash) const;

		// MVHF-BU

Feature/wallet backup auto #10

Feature/wallet backup auto #10

Conversation

marlengit commented Oct 10, 2016 • edited

ftrader 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ftrader Oct 10, 2016 • edited

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

ftrader Oct 10, 2016 • edited

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

marlengit commented Oct 10, 2016 •

edited

ftrader Oct 10, 2016 •

edited

ftrader Oct 10, 2016 •

edited