From 71497658928ef7cf3039938e6862f9be70f18dea Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Fri, 2 Oct 2015 16:47:44 -0700 Subject: [PATCH] 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 | 8 +- src/ripple/rpc/impl/TransactionSign.cpp | 181 +++++++++++++----------- src/ripple/rpc/tests/JSONRPC.test.cpp | 132 +++++++++++++++-- 3 files changed, 227 insertions(+), 94 deletions(-) diff --git a/src/ripple/app/main/Main.cpp b/src/ripple/app/main/Main.cpp index f4473e0d37..67d2930a35 100644 --- a/src/ripple/app/main/Main.cpp +++ b/src/ripple/app/main/Main.cpp @@ -149,11 +149,11 @@ void printHelp (const po::options_description& desc) " ripple_path_find []\n" " version\n" " server_info\n" - " sign [offline]\n" - " sign_for [offline]\n" + " sign [offline]\n" + " sign_for [offline]\n" " stop\n" - " submit |[ ]\n" - " submit_multisigned\n" + " submit |[ ]\n" + " submit_multisigned \n" " tx \n" " validation_create [||]\n" " validation_seed [||]\n" diff --git a/src/ripple/rpc/impl/TransactionSign.cpp b/src/ripple/rpc/impl/TransactionSign.cpp index 092708e297..6acfa273e0 100644 --- a/src/ripple/rpc/impl/TransactionSign.cpp +++ b/src/ripple/rpc/impl/TransactionSign.cpp @@ -29,6 +29,7 @@ #include #include #include +#include #include #include #include @@ -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 const second; + std::shared_ptr 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&& st) + transactionPreProcessResult (std::shared_ptr&& st) : first () , second (std::move (st)) { } @@ -509,11 +510,11 @@ transactionPreProcessImpl ( static std::pair transactionConstructImpl (std::shared_ptr const& stpTrans, - Rules const& rules, bool validateSig, Application& app, ApplyFlags flags) + Rules const& rules, Application& app, ApplyFlags flags) { std::pair 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 const& stpTrans, // Check the signature if that's called for. auto sttxNew = std::make_shared (sit); - if (validateSig && - checkValidity(app.getHashRouter(), + if (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 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 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 = 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 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); - 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; + return transactionFormatResultImpl (txn.second); } /** 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 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. - std::sort (signers.begin(), signers.end(), - [](STObject const& a, STObject const& b) - { - 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 ()); - } + // The array must be sorted and validated. + auto err = sortAndValidateSigners (signers, srcAddressID); + if (RPC::contains_error (err)) + return err; // Make sure the SerializedTransaction makes a legitimate Transaction. std::pair txn = - transactionConstructImpl (stpTrans, ledger->rules(), true, app, flags); + transactionConstructImpl (stpTrans, ledger->rules(), app, flags); if (!txn.second) return txn.first; diff --git a/src/ripple/rpc/tests/JSONRPC.test.cpp b/src/ripple/rpc/tests/JSONRPC.test.cpp index 1542945bd2..edeb58daf8 100644 --- a/src/ripple/rpc/tests/JSONRPC.test.cpp +++ b/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",