sign_for RPC command improvements (RIPD-1036):

o The sign_for RPC command automatically fills in an empty
   "SigningPubKey" field if the field is missing.

 o The sign_for command returns the Signers list inside the
   tx_json.  This re-establishes symmetry with the
   submit_multisigned command.  It also means the returned
   tx_blob might be useful, since it contains the multisignature.

 o The sign_for command also now allows the inclusion of a Signers
   array field in the input tx_json.  If a Signers array is present,
   the new signature is incorporated into the passed array.  This
   supports a model where multisignatures are accumulated serially.

 o Syntax hints are improved.

src/ripple/app/main/Main.cpp

@@ -149,11 +149,11 @@ void printHelp (const po::options_description& desc)
            "     ripple_path_find <json> [<ledger>]\n"
            "     version\n"
            "     server_info\n"
-           "     sign <private_key> <json> [offline]\n"
+           "     sign <private_key> <tx_json> [offline]\n"
-           "     sign_for <signer_address> <signer_private_key> <json> [offline]\n"
+           "     sign_for <signer_address> <signer_private_key> <tx_json> [offline]\n"
            "     stop\n"
-           "     submit <tx_blob>|[<private_key> <json>]\n"
+           "     submit <tx_blob>|[<private_key> <tx_json>]\n"
-           "     submit_multisigned\n"
+           "     submit_multisigned <tx_json>\n"
            "     tx <id>\n"
            "     validation_create [<seed>|<pass_phrase>|<key>]\n"
            "     validation_seed [<seed>|<pass_phrase>|<key>]\n"

src/ripple/rpc/impl/TransactionSign.cpp

@@ -29,6 +29,7 @@
 #include <ripple/net/RPCErr.h>
 #include <ripple/protocol/Sign.h>
 #include <ripple/protocol/ErrorCodes.h>
+#include <ripple/protocol/STAccount.h>
 #include <ripple/protocol/STParsedJSON.h>
 #include <ripple/protocol/TxFlags.h>
 #include <ripple/rpc/impl/KeypairForSignature.h>
@@ -114,7 +115,7 @@ static error_code_i acctMatchesPubKey (
         return rpcBAD_SECRET;
     }
 
-    // If we *can* get to the accountRoot, check for MASTER_DISABLED
+    // If we *can* get to the accountRoot, check for MASTER_DISABLED.
     auto const& sle = *accountState;
     if (isMasterKey)
     {
@@ -300,12 +301,12 @@ checkTxJsonFields (
 struct transactionPreProcessResult
 {
     Json::Value const first;
-    std::shared_ptr<STTx const> const second;
+    std::shared_ptr<STTx> const second;
 
     transactionPreProcessResult () = delete;
     transactionPreProcessResult (transactionPreProcessResult const&) = delete;
     transactionPreProcessResult (transactionPreProcessResult&& rhs)
-    : first (std::move (rhs.first))    // VS2013 won't default this
+    : first (std::move (rhs.first))    // VS2013 won't default this.
     , second (std::move (rhs.second))
     { }
 
@@ -319,7 +320,7 @@ struct transactionPreProcessResult
     , second ()
     { }
 
-    transactionPreProcessResult (std::shared_ptr<STTx const>&& st)
+    transactionPreProcessResult (std::shared_ptr<STTx>&& st)
     : first ()
     , second (std::move (st))
     { }
@@ -509,11 +510,11 @@ transactionPreProcessImpl (
 static
 std::pair <Json::Value, Transaction::pointer>
 transactionConstructImpl (std::shared_ptr<STTx const> const& stpTrans,
-    Rules const& rules, bool validateSig, Application& app, ApplyFlags flags)
+    Rules const& rules, Application& app, ApplyFlags flags)
 {
     std::pair <Json::Value, Transaction::pointer> ret;
 
-    // Turn the passed in STTx into a Transaction
+    // Turn the passed in STTx into a Transaction.
     Transaction::pointer tpTrans;
     {
         std::string reason;
@@ -540,8 +541,7 @@ transactionConstructImpl (std::shared_ptr<STTx const> const& stpTrans,
 
             // Check the signature if that's called for.
             auto sttxNew = std::make_shared<STTx const> (sit);
-            if (validateSig &&
+            if (checkValidity(app.getHashRouter(),
-                checkValidity(app.getHashRouter(),
                 *sttxNew, rules, app.config(), flags).first != Validity::Valid)
             {
                 ret.first = RPC::make_error (rpcINTERNAL,
@@ -694,7 +694,7 @@ Json::Value transactionSign (
     // Make sure the STTx makes a legitimate Transaction.
     std::pair <Json::Value, Transaction::pointer> txn =
         transactionConstructImpl (
-            preprocResult.second, ledger->rules(), true, app, flags);
+            preprocResult.second, ledger->rules(), app, flags);
 
     if (!txn.second)
         return txn.first;
@@ -730,7 +730,7 @@ Json::Value transactionSubmit (
     // Make sure the STTx makes a legitimate Transaction.
     std::pair <Json::Value, Transaction::pointer> txn =
         transactionConstructImpl (
-            preprocResult.second, ledger->rules(), true, app, flags);
+            preprocResult.second, ledger->rules(), app, flags);
 
     if (!txn.second)
         return txn.first;
@@ -768,16 +768,64 @@ static Json::Value checkMultiSignFields (Json::Value const& jvRequest)
     if (!tx_json.isMember (jss::Sequence))
         return RPC::missing_field_error ("tx_json.Sequence");
 
-    if (!tx_json.isMember ("SigningPubKey"))
+    if (!tx_json.isMember (sfSigningPubKey.getJsonName()))
         return RPC::missing_field_error ("tx_json.SigningPubKey");
 
-    if (!tx_json["SigningPubKey"].asString().empty())
+    if (!tx_json[sfSigningPubKey.getJsonName()].asString().empty())
         return RPC::make_error (rpcINVALID_PARAMS,
             "When multi-signing 'tx_json.SigningPubKey' must be empty.");
 
     return Json::Value ();
 }
 
+// Sort and validate an stSigners array.
+//
+// Returns a null Json::Value if there are no errors.
+static Json::Value sortAndValidateSigners (
+    STArray& signers, AccountID const& signingForID)
+{
+    if (signers.empty ())
+        return RPC::make_param_error ("Signers array may not be empty.");
+
+    // Signers must be sorted by Account.
+    std::sort (signers.begin(), signers.end(),
+        [](STObject const& a, STObject const& b)
+    {
+        return (a[sfAccount] < b[sfAccount]);
+    });
+
+    // Signers may not contain any duplicates.
+    auto const dupIter = std::adjacent_find (
+        signers.begin(), signers.end(),
+        [] (STObject const& a, STObject const& b)
+        {
+            return (a[sfAccount] == b[sfAccount]);
+        });
+
+    if (dupIter != signers.end())
+    {
+        std::ostringstream err;
+        err << "Duplicate Signers:Signer:Account entries ("
+            << toBase58((*dupIter)[sfAccount])
+            << ") are not allowed.";
+        return RPC::make_param_error(err.str ());
+    }
+
+    // An account may not sign for itself.
+    if (signers.end() != std::find_if (signers.begin(), signers.end(),
+        [&signingForID](STObject const& elem)
+        {
+            return elem[sfAccount] == signingForID;
+        }))
+    {
+        std::ostringstream err;
+        err << "A Signer may not be the transaction's Account ("
+            << toBase58(signingForID) << ").";
+        return RPC::make_param_error(err.str ());
+    }
+    return {};
+}
+
 } // detail
 
 /** Returns a Json::objectValue. */
@@ -808,6 +856,17 @@ Json::Value transactionSignFor (
             RPC::invalid_field_message (accountField));
     }
 
+    // If the tx_json.SigningPubKey field is missing, insert an empty one.
+    // RIPD-1036.
+   if (! jvRequest.isMember (jss::tx_json))
+        return RPC::missing_field_error (jss::tx_json);
+
+    {
+        Json::Value& tx_json (jvRequest [jss::tx_json]);
+        if (!tx_json.isMember (sfSigningPubKey.getJsonName()))
+            tx_json[sfSigningPubKey.getJsonName()] = "";
+    }
+
     // When multi-signing, the "Sequence" and "SigningPubKey" fields must
     // be passed in by the caller.
     using namespace detail;
@@ -834,7 +893,6 @@ Json::Value transactionSignFor (
     if (!preprocResult.second)
         return preprocResult.first;
 
-    // Make sure the multiSignAddrID can legitimately multi-sign.
     {
         // Make sure the account and secret belong together.
         std::shared_ptr<SLE const> sle = cachedRead(*ledger,
@@ -847,40 +905,37 @@ Json::Value transactionSignFor (
             return rpcError (err);
     }
 
+    // Inject the newly generated signature into tx_json.Signers.
+    auto& sttx = preprocResult.second;
+    {
+        // Make the signer object that we'll inject.
+        STObject signer (sfSigner);
+        signer[sfAccount] = *signerAccountID;
+        signer.setFieldVL (sfTxnSignature, multiSignature);
+        signer.setFieldVL (sfSigningPubKey, multiSignPubKey.getAccountPublic());
+
+        // If there is not yet a Signers array, make one.
+        if (!sttx->isFieldPresent (sfSigners))
+            sttx->setFieldArray (sfSigners, {});
+
+        auto& signers = sttx->peekFieldArray (sfSigners);
+        signers.emplace_back (std::move (signer));
+
+        // The array must be sorted and validated.
+        auto err = sortAndValidateSigners (signers, (*sttx)[sfAccount]);
+        if (RPC::contains_error (err))
+            return err;
+    }
+
     // Make sure the STTx makes a legitimate Transaction.
     std::pair <Json::Value, Transaction::pointer> txn =
         transactionConstructImpl (
-            preprocResult.second, ledger->rules(), false, app, flags);
+            sttx, ledger->rules(), app, flags);
 
     if (!txn.second)
         return txn.first;
 
-    Json::Value json = transactionFormatResultImpl (txn.second);
+    return transactionFormatResultImpl (txn.second);
-    if (RPC::contains_error (json))
-        return json;
-
-    // Finally, do what we were called for: return a Signers array.  Build
-    // a Signer object to insert into the Signers array.
-    Json::Value signer (Json::objectValue);
-
-    signer[sfAccount.getJsonName ()] = toBase58 (*signerAccountID);
-
-    signer[sfSigningPubKey.getJsonName ()] =
-        strHex (multiSignPubKey.getAccountPublic ());
-
-    signer[sfTxnSignature.getJsonName ()] = strHex (multiSignature);
-
-    // Give the Signer an object name and put it in the Signers array.
-    Json::Value nameSigner (Json::objectValue);
-    nameSigner[sfSigner.getJsonName ()] = std::move (signer);
-
-    Json::Value signers (Json::arrayValue);
-    signers.append (std::move (nameSigner));
-
-    // Inject the Signers into the json.
-    json[sfSigners.getName ()] = std::move(signers);
-
-    return json;
 }
 
 /** Returns a Json::objectValue. */
@@ -952,7 +1007,7 @@ Json::Value transactionSubmitMultiSigned (
             return err;
     }
 
-    // Grind through the JSON in tx_json to produce a STTx
+    // Grind through the JSON in tx_json to produce a STTx.
     std::shared_ptr<STTx> stpTrans;
     {
         STParsedJSONObject parsedTx_json ("tx_json", tx_json);
@@ -1024,7 +1079,7 @@ Json::Value transactionSubmitMultiSigned (
     if (signers.empty ())
         return RPC::make_param_error("tx_json.Signers array may not be empty.");
 
-    // the Signers array may only contain Signer objects.
+    // The Signers array may only contain Signer objects.
     if (std::find_if_not(signers.begin(), signers.end(), [](STObject const& obj)
         {
             return (
@@ -1039,46 +1094,14 @@ Json::Value transactionSubmitMultiSigned (
             "Signers array may only contain Signer entries.");
     }
 
-    // Signers must be sorted by Account.
+    // The array must be sorted and validated.
-    std::sort (signers.begin(), signers.end(),
+    auto err = sortAndValidateSigners (signers, srcAddressID);
-        [](STObject const& a, STObject const& b)
+    if (RPC::contains_error (err))
-    {
+        return err;
-        return (a.getAccountID (sfAccount) < b.getAccountID (sfAccount));
-    });
-
-    // Signers may not contain any duplicates.
-    auto const dupIter = std::adjacent_find (
-        signers.begin(), signers.end(),
-        [] (STObject const& a, STObject const& b)
-        {
-            return (a.getAccountID (sfAccount) == b.getAccountID (sfAccount));
-        });
-
-    if (dupIter != signers.end())
-    {
-        std::ostringstream err;
-        err << "Duplicate Signers:Signer:Account entries ("
-            << toBase58(dupIter->getAccountID(sfAccount))
-            << ") are not allowed.";
-        return RPC::make_param_error(err.str ());
-    }
-
-    // An account may not sign for itself.
-    if (signers.end() != std::find_if (signers.begin(), signers.end(),
-        [&srcAddressID](STObject const& elem)
-        {
-            return elem.getAccountID (sfAccount) == srcAddressID;
-        }))
-    {
-        std::ostringstream err;
-        err << "A Signer may not be the transaction's Account ("
-            << toBase58(srcAddressID) << ").";
-        return RPC::make_param_error(err.str ());
-    }
 
     // Make sure the SerializedTransaction makes a legitimate Transaction.
     std::pair <Json::Value, Transaction::pointer> txn =
-        transactionConstructImpl (stpTrans, ledger->rules(), true, app, flags);
+        transactionConstructImpl (stpTrans, ledger->rules(), app, flags);
 
     if (!txn.second)
         return txn.first;

src/ripple/rpc/tests/JSONRPC.test.cpp

@@ -98,7 +98,7 @@ R"({
 {
 "",
 "",
-"Missing field 'tx_json.SigningPubKey'.",
+"Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "Pass in Sequence and Fee with minimal payment.",
@@ -118,7 +118,7 @@ R"({
 {
 "",
 "",
-"Missing field 'tx_json.SigningPubKey'.",
+"A Signer may not be the transaction's Account (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh).",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "Add 'fee_mult_max' field.",
@@ -138,7 +138,7 @@ R"({
 {
 "",
 "",
-"Missing field 'tx_json.SigningPubKey'.",
+"Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "fee_mult_max is ignored if 'Fee' is present.",
@@ -159,7 +159,7 @@ R"({
 {
 "",
 "",
-"Missing field 'tx_json.SigningPubKey'.",
+"A Signer may not be the transaction's Account (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh).",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "Invalid 'fee_mult_max' field.",
@@ -179,7 +179,7 @@ R"({
 {
 "Invalid field 'fee_mult_max', not a number.",
 "Invalid field 'fee_mult_max', not a number.",
-"Missing field 'tx_json.SigningPubKey'.",
+"Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "Invalid value for 'fee_mult_max' field.",
@@ -199,7 +199,7 @@ R"({
 {
 "Fee of 10 exceeds the requested tx limit of 0",
 "Fee of 10 exceeds the requested tx limit of 0",
-"Missing field 'tx_json.SigningPubKey'.",
+"Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "Missing 'Amount'.",
@@ -587,7 +587,7 @@ R"({
 {
 "Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.Fee'.",
-"Missing field 'tx_json.SigningPubKey'.",
+"Missing field 'tx_json.Fee'.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "Valid transaction if 'offline' is true.",
@@ -608,7 +608,7 @@ R"({
 {
 "",
 "",
-"Missing field 'tx_json.SigningPubKey'.",
+"A Signer may not be the transaction's Account (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh).",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "'offline' and 'build_path' are mutually exclusive.",
@@ -630,7 +630,7 @@ R"({
 {
 "Field 'build_path' not allowed in this context.",
 "Field 'build_path' not allowed in this context.",
-"Missing field 'tx_json.SigningPubKey'.",
+"Field 'build_path' not allowed in this context.",
 "Missing field 'tx_json.SigningPubKey'."}},
 
 { "A 'Flags' field may be specified.",
@@ -833,7 +833,7 @@ R"({
 "Missing field 'tx_json.Sequence'.",
 "Missing field 'tx_json.Sequence'."}},
 
-{ "Missing 'SigningPubKey' in sign_for.",
+{ "Missing 'SigningPubKey' in sign_for is automatically filled in.",
 R"({
     "command": "doesnt_matter",
     "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
@@ -850,9 +850,119 @@ R"({
 {
 "Secret does not match account.",
 "Secret does not match account.",
-"Missing field 'tx_json.SigningPubKey'.",
+"",
 "Missing field 'tx_json.SigningPubKey'."}},
 
+{ "In sign_for, an account may not sign for itself.",
+R"({
+    "command": "doesnt_matter",
+    "account": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+    "secret": "a",
+    "tx_json": {
+        "Account": "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+        "Amount": "1000000000",
+        "Destination": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+        "Fee": 50,
+        "Sequence": 0,
+        "TransactionType": "Payment"
+    }
+})",
+{
+"",
+"",
+"A Signer may not be the transaction's Account (rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA).",
+"Missing field 'tx_json.SigningPubKey'."}},
+
+{ "Cannot put duplicate accounts in Signers array",
+R"({
+    "command": "doesnt_matter",
+    "account": "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+    "secret": "masterpassphrase",
+    "tx_json": {
+        "Account" : "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+        "Amount" : "1000000000",
+        "Destination" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+        "Fee" : "50",
+        "Sequence" : 0,
+        "Signers" : [
+            {
+                "Signer" : {
+                    "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+                    "SigningPubKey" : "0330E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020",
+                    "TxnSignature" : "304502210080EB23E78A841DDC5E3A4F10DE6EAF052207D6B519BF8954467ADB221B3F349002202CA458E8D4E4DE7176D27A91628545E7B295A5DFC8ADF0B5CD3E279B6FA02998"
+                }
+            }
+        ],
+        "SigningPubKey" : "",
+        "TransactionType" : "Payment"
+    }
+})",
+{
+"Secret does not match account.",
+"Secret does not match account.",
+"Duplicate Signers:Signer:Account entries (rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh) are not allowed.",
+""}},
+
+{ "Correctly append to pre-established Signers array",
+R"({
+    "command": "doesnt_matter",
+    "account": "rPcNzota6B8YBokhYtcTNqQVCngtbnWfux",
+    "secret": "c",
+    "tx_json": {
+        "Account" : "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+        "Amount" : "1000000000",
+        "Destination" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+        "Fee" : "50",
+        "Sequence" : 0,
+        "Signers" : [
+            {
+                "Signer" : {
+                    "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+                    "SigningPubKey" : "0330E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020",
+                    "TxnSignature" : "304502210080EB23E78A841DDC5E3A4F10DE6EAF052207D6B519BF8954467ADB221B3F349002202CA458E8D4E4DE7176D27A91628545E7B295A5DFC8ADF0B5CD3E279B6FA02998"
+                }
+            }
+        ],
+        "SigningPubKey" : "",
+        "TransactionType" : "Payment"
+    }
+})",
+{
+"Secret does not match account.",
+"Secret does not match account.",
+"",
+""}},
+
+{ "Append to pre-established Signers array with bad signature",
+R"({
+    "command": "doesnt_matter",
+    "account": "rPcNzota6B8YBokhYtcTNqQVCngtbnWfux",
+    "secret": "c",
+    "tx_json": {
+        "Account" : "rnUy2SHTrB9DubsPmkJZUXTf5FcNDGrYEA",
+        "Amount" : "1000000000",
+        "Destination" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+        "Fee" : "50",
+        "Sequence" : 0,
+        "Signers" : [
+            {
+                "Signer" : {
+                    "Account" : "rHb9CJAWyB4rj91VRWn96DkukG4bwdtyTh",
+                    "SigningPubKey" : "0330E7FC9D56BB25D6893BA3F317AE5BCF33B3291BD63DB32654A313222F7FD020",
+                    "TxnSignature" : "304502210080EB23E78A841DDC5E3A4F10DE6EAF052207D6B519BF8954467ACB221B3F349002202CA458E8D4E4DE7176D27A91628545E7B295A5DFC8ADF0B5CD3E279B6FA02998"
+                }
+            }
+        ],
+        "SigningPubKey" : "",
+        "TransactionType" : "Payment"
+    }
+})",
+{
+"Secret does not match account.",
+"Secret does not match account.",
+"Invalid signature.",
+"Invalid signature."}},
+
 { "Non-empty 'SigningPubKey' in sign_for.",
 R"({
     "command": "doesnt_matter",