From f1c29ae20b7322bfdb586165169bfbbeb4ebc3d5 Mon Sep 17 00:00:00 2001 From: Scott Schurr Date: Thu, 6 Aug 2015 14:52:27 -0700 Subject: [PATCH] Allow an account to be exclusively multisigned (RIPD-182): An account can be made signable with only its regular key by disabling the master key. Now an account can also be made exclusively multisigned by both disabling the master key and having no regular key. In order to prevent an account from becoming unsignable the network uses these rules: o An account can always add or replace a regular key or a SignerList as long as the fee and reserve can be met by the account. o The master key on an account can be disabled if either a regular key or a SignerList (or both) is present on the account. Either the regular key or the SignerList can be used to re-enable the master key later if that is desired. o The regular key on an account may only be removed if either the master key is enabled or the account has a SignerList (or both). o The SignerList on an account may only be removed if either the master key is enabled or a regular key is present (or both). As a consequence of this change, the tecMASTER_DISABLED error code is renamed to tecNO_ALTERNATIVE_KEY. The error code number (130 decimal) is unchanged. --- src/ripple/app/tests/MultiSign.test.cpp | 76 ++++++++++++++ src/ripple/app/tx/impl/SetAccount.cpp | 12 ++- src/ripple/app/tx/impl/SetRegularKey.cpp | 7 +- src/ripple/app/tx/impl/SetSignerList.cpp | 121 ++++++++++++----------- src/ripple/app/tx/impl/SetSignerList.h | 10 +- src/ripple/protocol/TER.h | 2 +- src/ripple/protocol/impl/TER.cpp | 2 +- src/ripple/test/jtx/impl/Env_test.cpp | 2 +- 8 files changed, 162 insertions(+), 70 deletions(-) diff --git a/src/ripple/app/tests/MultiSign.test.cpp b/src/ripple/app/tests/MultiSign.test.cpp index 758239690a..993ba1c1bc 100644 --- a/src/ripple/app/tests/MultiSign.test.cpp +++ b/src/ripple/app/tests/MultiSign.test.cpp @@ -527,6 +527,81 @@ public: env.require (owners (alice, 0)); } + // We want to always leave an account signable. Make sure the that we + // disallow removing the last way a transaction may be signed. + void test_keyDisable() + { + using namespace jtx; + Env env(*this); + Account const alice {"alice", KeyType::ed25519}; + env.fund(XRP(1000), alice); + + // There are three negative tests we need to make: + // M0. A lone master key cannot be disabled. + // R0. A lone regular key cannot be removed. + // L0. A lone signer list cannot be removed. + // + // Additionally, there are 6 positive tests we need to make: + // M1. The master key can be disabled if there's a regular key. + // M2. The master key can be disabled if there's a signer list. + // + // R1. The regular key can be removed if there's a signer list. + // R2. The regular key can be removed if the master key is enabled. + // + // L1. The signer list can be removed if the master key is enabled. + // L2. The signer list can be removed if there's a regular key. + + // Master key tests. + // M0: A lone master key cannot be disabled. + env(fset (alice, asfDisableMaster), + sig(alice), ter(tecNO_ALTERNATIVE_KEY)); + + // Add a regular key. + Account const alie {"alie", KeyType::ed25519}; + env(regkey (alice, alie)); + + // M1: The master key can be disabled if there's a regular key. + env(fset (alice, asfDisableMaster), sig(alice)); + + // R0: A lone regular key cannot be removed. + env(regkey (alice, disabled), sig(alie), ter(tecNO_ALTERNATIVE_KEY)); + + // Add a signer list. + env(signers(alice, 1, {{bogie, 1}}), sig (alie)); + + // R1: The regular key can be removed if there's a signer list. + env(regkey (alice, disabled), sig(alie)); + + // L0; A lone signer list cannot be removed. + auto const baseFee = env.config.FEE_DEFAULT; + env(signers(alice, jtx::none), msig(bogie), + fee(2 * baseFee), ter(tecNO_ALTERNATIVE_KEY)); + + // Enable the master key. + env(fclear (alice, asfDisableMaster), msig(bogie), fee(2 * baseFee)); + + // L1: The signer list can be removed if the master key is enabled. + env(signers(alice, jtx::none), msig(bogie), fee(2 * baseFee)); + + // Add a signer list. + env(signers(alice, 1, {{bogie, 1}}), sig (alice)); + + // M2: The master key can be disabled if there's a signer list. + env(fset (alice, asfDisableMaster), sig(alice)); + + // Add a regular key. + env(regkey (alice, alie), msig(bogie), fee(2 * baseFee)); + + // L2: The signer list can be removed if there's a regular key. + env(signers(alice, jtx::none), sig(alie)); + + // Enable the master key. + env(fclear (alice, asfDisableMaster), sig(alie)); + + // R2: The regular key can be removed if the master key is enabled. + env(regkey (alice, disabled), sig(alie)); + } + // See if every kind of transaction can be successfully multi-signed. void test_txTypes() { @@ -621,6 +696,7 @@ public: test_masterSigners(); test_regularSigners(); test_heterogeneousSigners(); + test_keyDisable(); test_txTypes(); } }; diff --git a/src/ripple/app/tx/impl/SetAccount.cpp b/src/ripple/app/tx/impl/SetAccount.cpp index a998265cb4..628bdd98f8 100644 --- a/src/ripple/app/tx/impl/SetAccount.cpp +++ b/src/ripple/app/tx/impl/SetAccount.cpp @@ -191,8 +191,18 @@ SetAccount::doApply () return tecNEED_MASTER_KEY; } - if (!sle->isFieldPresent (sfRegularKey)) + if ((!sle->isFieldPresent (sfRegularKey)) && + (!view().peek (keylet::signers (account_)))) + { + // Account has no regular key or multi-signer signer list. + + // Prevent transaction changes until we're ready. + if ((RIPPLE_ENABLE_MULTI_SIGN) || + view().flags() & tapENABLE_TESTING) + return tecNO_ALTERNATIVE_KEY; + return tecNO_REGULAR_KEY; + } j_.trace << "Set lsfDisableMaster."; uFlagsOut |= lsfDisableMaster; diff --git a/src/ripple/app/tx/impl/SetRegularKey.cpp b/src/ripple/app/tx/impl/SetRegularKey.cpp index a9ba213373..8cd67cd001 100644 --- a/src/ripple/app/tx/impl/SetRegularKey.cpp +++ b/src/ripple/app/tx/impl/SetRegularKey.cpp @@ -77,8 +77,11 @@ SetRegularKey::doApply () } else { - if (sle->isFlag (lsfDisableMaster)) - return tecMASTER_DISABLED; + if (sle->isFlag (lsfDisableMaster) && + !view().peek (keylet::signers (account_))) + // Account has disabled master key and no multi-signer signer list. + return tecNO_ALTERNATIVE_KEY; + sle->makeFieldAbsent (sfRegularKey); } diff --git a/src/ripple/app/tx/impl/SetSignerList.cpp b/src/ripple/app/tx/impl/SetSignerList.cpp index 6f838f656e..e5257f8d6b 100644 --- a/src/ripple/app/tx/impl/SetSignerList.cpp +++ b/src/ripple/app/tx/impl/SetSignerList.cpp @@ -45,7 +45,7 @@ SetSignerList::determineOperation(STTx const& tx, { // Check the quorum. A non-zero quorum means we're creating or replacing // the list. A zero quorum means we're destroying the list. - auto const quorum = tx.getFieldU32(sfSignerQuorum); + auto const quorum = tx[sfSignerQuorum]; std::vector sign; Operation op = unknown; @@ -68,8 +68,7 @@ SetSignerList::determineOperation(STTx const& tx, op = destroy; } - return std::make_tuple(tesSUCCESS, - quorum, sign, op); + return std::make_tuple(tesSUCCESS, quorum, sign, op); } TER @@ -115,21 +114,16 @@ SetSignerList::preflight (PreflightContext const& ctx) TER SetSignerList::doApply () { - // All operations require our ledger index. Compute that once and pass it - // to our handlers. - uint256 const index = getSignerListIndex (account_); - // Perform the operation preCompute() decided on. switch (do_) { case set: - return replaceSignerList (index); + return replaceSignerList (); case destroy: - return destroySignerList (index); + return destroySignerList (); default: - // Fall through intentionally break; } assert (false); // Should not be possible to get here. @@ -210,24 +204,27 @@ SetSignerList::validateQuorumAndSignerEntries ( } TER -SetSignerList::replaceSignerList (uint256 const& index) +SetSignerList::replaceSignerList () { - // This may be either a create or a replace. Preemptively destroy any + auto const accountKeylet = keylet::account (account_); + auto const ownerDirKeylet = keylet::ownerDir (account_); + auto const signerListKeylet = keylet::signers (account_); + + // This may be either a create or a replace. Preemptively remove any // old signer list. May reduce the reserve, so this is done before // checking the reserve. - if (TER const ter = destroySignerList (index)) - return ter; + if (TER const ter = removeSignersFromLedger ( + accountKeylet, ownerDirKeylet, signerListKeylet)) + return ter; - auto const sle = view().peek( - keylet::account(account_)); + auto const sle = view().peek(accountKeylet); // Compute new reserve. Verify the account has funds to meet the reserve. - std::uint32_t const oldOwnerCount = sle->getFieldU32 (sfOwnerCount); + auto const oldOwnerCount = (*sle)[sfOwnerCount]; std::uint32_t const addedOwnerCount = ownerCountDelta (signers_.size ()); auto const newReserve = - view().fees().accountReserve( - oldOwnerCount + addedOwnerCount); + view().fees().accountReserve(oldOwnerCount + addedOwnerCount); // We check the reserve against the starting balance because we want to // allow dipping into the reserve to pay fees. This behavior is consistent @@ -236,20 +233,14 @@ SetSignerList::replaceSignerList (uint256 const& index) return tecINSUFFICIENT_RESERVE; // Everything's ducky. Add the ltSIGNER_LIST to the ledger. - auto signerList = std::make_shared(ltSIGNER_LIST, index); + auto signerList = std::make_shared(signerListKeylet); view().insert (signerList); - writeSignersToLedger (signerList); - - // Lambda for call to dirAdd. - auto describer = [&] (SLE::ref sle, bool dummy) - { - ownerDirDescriber (sle, dummy, account_); - }; + writeSignersToSLE (signerList); // Add the signer list to the account's directory. std::uint64_t hint; - TER result = dirAdd(ctx_.view (), - hint, getOwnerDirIndex (account_), index, describer); + TER result = dirAdd(ctx_.view (), hint, ownerDirKeylet.key, + signerListKeylet.key, describeOwnerDir (account_)); JLOG(j_.trace) << "Create signer list for account " << toBase58(account_) << ": " << transHuman (result); @@ -260,57 +251,60 @@ SetSignerList::replaceSignerList (uint256 const& index) signerList->setFieldU64 (sfOwnerNode, hint); // If we succeeded, the new entry counts against the creator's reserve. - adjustOwnerCount(view(), - sle, addedOwnerCount); + adjustOwnerCount(view(), sle, addedOwnerCount); return result; } TER -SetSignerList::destroySignerList (uint256 const& index) +SetSignerList::destroySignerList () { - // See if there's an ltSIGNER_LIST for this account. - SLE::pointer signerList = - view().peek (keylet::signers(index)); + auto const accountKeylet = keylet::account (account_); + // Destroying the signer list is only allowed if either the master key + // is enabled or there is a regular key. + SLE::pointer ledgerEntry = view().peek (accountKeylet); + if ((ledgerEntry->isFlag (lsfDisableMaster)) && + (!ledgerEntry->isFieldPresent (sfRegularKey))) + return tecNO_ALTERNATIVE_KEY; - // If the signer list doesn't exist we've already succeeded in deleting it. - if (!signerList) - return tesSUCCESS; + auto const ownerDirKeylet = keylet::ownerDir (account_); + auto const signerListKeylet = keylet::signers (account_); + return removeSignersFromLedger( + accountKeylet, ownerDirKeylet, signerListKeylet); +} +TER +SetSignerList::removeSignersFromLedger (Keylet const& accountKeylet, + Keylet const& ownerDirKeylet, Keylet const& signerListKeylet) +{ // We have to examine the current SignerList so we know how much to // reduce the OwnerCount. - std::int32_t removeFromOwnerCount = 0; - auto const k = keylet::signers(account_); - SLE::pointer accountSignersList = - view().peek (k); - if (accountSignersList) - { - STArray const& actualList = - accountSignersList->getFieldArray (sfSignerEntries); - removeFromOwnerCount = ownerCountDelta (actualList.size ()) * -1; - } + SLE::pointer signers = view().peek (signerListKeylet); + + // If the signer list doesn't exist we've already succeeded in deleting it. + if (!signers) + return tesSUCCESS; + + STArray const& actualList = signers->getFieldArray (sfSignerEntries); + int const removeFromOwnerCount = ownerCountDelta (actualList.size()) * -1; // Remove the node from the account directory. - std::uint64_t const hint (signerList->getFieldU64 (sfOwnerNode)); + auto const hint = (*signers)[sfOwnerNode]; - TER const result = dirDelete(ctx_.view (), false, hint, - getOwnerDirIndex (account_), index, false, (hint == 0)); + TER const result = dirDelete(ctx_.view(), false, hint, + ownerDirKeylet.key, signerListKeylet.key, false, (hint == 0)); if (result == tesSUCCESS) adjustOwnerCount(view(), - view().peek(keylet::account(account_)), - removeFromOwnerCount); + view().peek(accountKeylet), removeFromOwnerCount); - ctx_.view ().erase (signerList); + ctx_.view().erase (signers); return result; } -// VFALCO NOTE This name is misleading, the signers -// are not written to the ledger they are -// added to the SLE. void -SetSignerList::writeSignersToLedger (SLE::pointer ledgerEntry) +SetSignerList::writeSignersToSLE (SLE::pointer const& ledgerEntry) const { // Assign the quorum. ledgerEntry->setFieldU32 (sfSignerQuorum, quorum_); @@ -333,7 +327,9 @@ SetSignerList::writeSignersToLedger (SLE::pointer ledgerEntry) ledgerEntry->setFieldArray (sfSignerEntries, toLedger); } -std::size_t +// The return type is signed so it is compatible with the 3rd argument +// of adjustOwnerCount() (which must be signed). +int SetSignerList::ownerCountDelta (std::size_t entryCount) { // We always compute the full change in OwnerCount, taking into account: @@ -347,7 +343,12 @@ SetSignerList::ownerCountDelta (std::size_t entryCount) // o And each signer in the list costs 1 more OwnerCount unit. // So, at a minimum, adding a SignerList with 1 entry costs 3 OwnerCount // units. A SignerList with 8 entries would cost 10 OwnerCount units. - return 2 + entryCount; + // + // The static_cast should always be safe since entryCount should always + // be in the range from 1 to 8. We've got a lot of room to grow. + assert (entryCount >= STTx::minMultiSigners); + assert (entryCount <= STTx::maxMultiSigners); + return 2 + static_cast(entryCount); } } // namespace ripple diff --git a/src/ripple/app/tx/impl/SetSignerList.h b/src/ripple/app/tx/impl/SetSignerList.h index 7b458a713f..e360bd64f0 100644 --- a/src/ripple/app/tx/impl/SetSignerList.h +++ b/src/ripple/app/tx/impl/SetSignerList.h @@ -76,12 +76,14 @@ private: AccountID const& account, beast::Journal j); - TER replaceSignerList (uint256 const& index); - TER destroySignerList (uint256 const& index); + TER replaceSignerList (); + TER destroySignerList (); - void writeSignersToLedger (SLE::pointer ledgerEntry); + TER removeSignersFromLedger (Keylet const& accountKeylet, + Keylet const& ownerDirKeylet, Keylet const& signerListKeylet); + void writeSignersToSLE (SLE::pointer const& ledgerEntry) const; - static std::size_t ownerCountDelta (std::size_t entryCount); + static int ownerCountDelta (std::size_t entryCount); }; } // ripple diff --git a/src/ripple/protocol/TER.h b/src/ripple/protocol/TER.h index d025a8ae2f..3a0da87fae 100644 --- a/src/ripple/protocol/TER.h +++ b/src/ripple/protocol/TER.h @@ -185,7 +185,7 @@ enum TER tecNO_LINE_REDUNDANT = 127, tecPATH_DRY = 128, tecUNFUNDED = 129, // Deprecated, old ambiguous unfunded. - tecMASTER_DISABLED = 130, + tecNO_ALTERNATIVE_KEY = 130, tecNO_REGULAR_KEY = 131, tecOWNERS = 132, tecNO_ISSUER = 133, diff --git a/src/ripple/protocol/impl/TER.cpp b/src/ripple/protocol/impl/TER.cpp index 661d8164ec..54df2f5b58 100644 --- a/src/ripple/protocol/impl/TER.cpp +++ b/src/ripple/protocol/impl/TER.cpp @@ -48,7 +48,7 @@ bool transResultInfo (TER code, std::string& token, std::string& text) { tecNO_LINE_REDUNDANT, "tecNO_LINE_REDUNDANT", "Can't set non-existent line to default." }, { tecPATH_DRY, "tecPATH_DRY", "Path could not send partial amount." }, { tecPATH_PARTIAL, "tecPATH_PARTIAL", "Path could not send full amount." }, - { tecMASTER_DISABLED, "tecMASTER_DISABLED", "Master key is disabled." }, + { tecNO_ALTERNATIVE_KEY, "tecNO_ALTERNATIVE_KEY", "The operation would remove the last way to sign a transaction."}, { tecNO_REGULAR_KEY, "tecNO_REGULAR_KEY", "Regular key is not set." }, { tecUNFUNDED, "tecUNFUNDED", "One of _ADD, _OFFER, or _SEND. Deprecated." }, diff --git a/src/ripple/test/jtx/impl/Env_test.cpp b/src/ripple/test/jtx/impl/Env_test.cpp index 10c7a7db58..a40e904b60 100644 --- a/src/ripple/test/jtx/impl/Env_test.cpp +++ b/src/ripple/test/jtx/impl/Env_test.cpp @@ -332,7 +332,7 @@ public: env.require(nflags("alice", asfDisableMaster)); env(fset("alice", asfDisableMaster), sig("alice")); env.require(flags("alice", asfDisableMaster)); - env(regkey("alice", disabled), ter(tecMASTER_DISABLED)); + env(regkey("alice", disabled), ter(tecNO_ALTERNATIVE_KEY)); env(noop("alice")); env(noop("alice"), sig("alice"), ter(tefMASTER_DISABLED)); env(noop("alice"), sig("eric"));