From 49d1890769c23339f5dd13a393dbc07eb9db66d6 Mon Sep 17 00:00:00 2001 From: Mark Travis Date: Mon, 7 Oct 2024 17:04:29 -0700 Subject: [PATCH] Silently remove duplicate credentials instead of failing the transaction. Conforms to Credentials spec. --- src/test/app/PermissionedDomains_test.cpp | 45 ++++++++---- src/test/jtx/PermissionedDomains.h | 3 +- src/test/jtx/impl/PermissionedDomains.cpp | 73 +++++++------------ .../app/tx/detail/PermissionedDomainSet.cpp | 40 ++++------ 4 files changed, 73 insertions(+), 88 deletions(-) diff --git a/src/test/app/PermissionedDomains_test.cpp b/src/test/app/PermissionedDomains_test.cpp index f1016f3558b..ca9b42e12a6 100644 --- a/src/test/app/PermissionedDomains_test.cpp +++ b/src/test/app/PermissionedDomains_test.cpp @@ -165,16 +165,33 @@ class PermissionedDomains_test : public beast::unit_test::suite .removeMember("Issuer"); env(txJsonMutable, fee(setFee), ter(temMALFORMED)); - // Make 2 identical credentials. - pd::Credentials const credentialsDup{ - {alice2, pd::toBlob("credential1")}, - {alice3, pd::toBlob("credential2")}, - {alice2, pd::toBlob("credential1")}, - {alice5, pd::toBlob("credential4")}, - }; - env(pd::setTx(account, credentialsDup, domain), - fee(setFee), - ter(temMALFORMED)); + // Make 2 identical credentials. The duplicate should be silently + // removed. + { + pd::Credentials const credentialsDup{ + {alice7, pd::toBlob("credential6")}, + {alice2, pd::toBlob("credential1")}, + {alice3, pd::toBlob("credential2")}, + {alice2, pd::toBlob("credential1")}, + {alice5, pd::toBlob("credential4")}, + }; + BEAST_EXPECT(pd::sortCredentials(credentialsDup).size() == 4); + env(pd::setTx(account, credentialsDup, domain), + fee(setFee)); + + uint256 d; + if (domain) + d = *domain; + else + d = pd::getNewDomain(env.meta()); + env.close(); + auto objects = pd::getObjects(account, env); + auto const fromObject = pd::credentialsFromJson(objects[d]); + auto const sortedCreds = pd::sortCredentials(credentialsDup); + BEAST_EXPECT( + pd::credentialsFromJson(objects[d]) == + pd::sortCredentials(credentialsDup)); + } // Have equal issuers but different credentials and make sure they // sort correctly. @@ -187,7 +204,7 @@ class PermissionedDomains_test : public beast::unit_test::suite {alice2, pd::toBlob("credential6")}, }; BEAST_EXPECT( - credentialsSame != *pd::sortCredentials(credentialsSame)); + credentialsSame != pd::sortCredentials(credentialsSame)); env(pd::setTx(account, credentialsSame, domain), fee(setFee)); uint256 d; @@ -198,10 +215,10 @@ class PermissionedDomains_test : public beast::unit_test::suite env.close(); auto objects = pd::getObjects(account, env); auto const fromObject = pd::credentialsFromJson(objects[d]); - auto const sortedCreds = *pd::sortCredentials(credentialsSame); + auto const sortedCreds = pd::sortCredentials(credentialsSame); BEAST_EXPECT( pd::credentialsFromJson(objects[d]) == - *pd::sortCredentials(credentialsSame)); + pd::sortCredentials(credentialsSame)); } } @@ -266,7 +283,7 @@ class PermissionedDomains_test : public beast::unit_test::suite { BEAST_EXPECT( credentials10.size() == PermissionedDomainSet::PD_ARRAY_MAX); - BEAST_EXPECT(credentials10 != *pd::sortCredentials(credentials10)); + BEAST_EXPECT(credentials10 != pd::sortCredentials(credentials10)); env(pd::setTx(alice, credentials10), fee(setFee)); auto tx = env.tx()->getJson(JsonOptions::none); domain2 = pd::getNewDomain(env.meta()); diff --git a/src/test/jtx/PermissionedDomains.h b/src/test/jtx/PermissionedDomains.h index 95e4541ee66..20c5467b128 100644 --- a/src/test/jtx/PermissionedDomains.h +++ b/src/test/jtx/PermissionedDomains.h @@ -21,7 +21,6 @@ #define RIPPLE_TEST_JTX_PERMISSIONEDDOMAINS_H_INCLUDED #include -#include namespace ripple { namespace test { @@ -64,7 +63,7 @@ Credentials credentialsFromJson(Json::Value const& object); // Sort credentials the same way as PermissionedDomainSet -std::optional +Credentials sortCredentials(Credentials const& input); // Get account_info diff --git a/src/test/jtx/impl/PermissionedDomains.cpp b/src/test/jtx/impl/PermissionedDomains.cpp index f773140b266..5405ada76b1 100644 --- a/src/test/jtx/impl/PermissionedDomains.cpp +++ b/src/test/jtx/impl/PermissionedDomains.cpp @@ -121,56 +121,37 @@ credentialsFromJson(Json::Value const& object) return ret; } -// Sort credentials the same way as PermissionedDomainSet. None can -// be identical. -std::optional +// Sort credentials the same way as PermissionedDomainSet. Silently +// remove duplicates. +Credentials sortCredentials(Credentials const& input) { - try - { - Credentials ret = input; - std::sort( - ret.begin(), - ret.end(), - [](Credential const& left, Credential const& right) -> bool { - if (left.first < right.first) - return true; - if (left.first == right.first) - { - if (left.second < right.second) - return true; - if (left.second == right.second) - throw std::runtime_error("duplicate"); - } - return false; - /* - if (left.getAccountID(sfIssuer) < right.getAccountID(sfIssuer)) - return true; - if (left.getAccountID(sfIssuer) == right.getAccountID(sfIssuer)) - { - if (left.getFieldVL(sfCredentialType) < - right.getFieldVL(sfCredentialType)) - { - return true; - } - if (left.getFieldVL(sfCredentialType) == - right.getFieldVL(sfCredentialType)) - { - throw std::runtime_error("duplicate credentials"); - } - return false; - } - return false; - return left.first < right.first; - */ - }); - return ret; - } - catch (...) + Credentials ret = input; + + std::set cSet; + for (auto const& c : ret) + cSet.insert(c); + if (ret.size() > cSet.size()) { - std::cerr << "wtf\n"; - return std::nullopt; + ret = Credentials(); + for (auto const& c : cSet) + ret.push_back(c); } + + std::sort( + ret.begin(), + ret.end(), + [](Credential const& left, Credential const& right) -> bool { + if (left.first < right.first) + return true; + if (left.first == right.first) + { + if (left.second < right.second) + return true; + } + return false; + }); + return ret; } // Get account_info diff --git a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp index f3a5c2aaac7..b3c4fa2f684 100644 --- a/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp +++ b/src/xrpld/app/tx/detail/PermissionedDomainSet.cpp @@ -21,7 +21,7 @@ #include #include #include - +#include #include namespace ripple { @@ -95,12 +95,19 @@ PermissionedDomainSet::doApply() // All checks have already been done. Just update credentials. Same logic // for either new domain or updating existing. + // Silently remove duplicates. auto updateSle = [this](std::shared_ptr const& sle) { auto credentials = ctx_.tx.getFieldArray(sfAcceptedCredentials); - // TODO when credentials are merged, it is possible that this will - // also need to sort on the CredentialType field in case there - // are multiple issuers in each set of credentials. The spec - // needs to be ironed out. + std::map hashed; + for (auto const& c : credentials) + hashed.insert({c.getHash(HashPrefix::transactionID), c}); + if (credentials.size() > hashed.size()) + { + credentials = STArray(); + for (auto const& e : hashed) + credentials.push_back(e.second); + } + credentials.sort( [](STObject const& left, STObject const& right) -> bool { if (left.getAccountID(sfIssuer) < right.getAccountID(sfIssuer)) @@ -112,11 +119,6 @@ PermissionedDomainSet::doApply() { return true; } - if (left.getFieldVL(sfCredentialType) == - right.getFieldVL(sfCredentialType)) - { - throw std::runtime_error("duplicate credentials"); - } } return false; }); @@ -128,14 +130,7 @@ PermissionedDomainSet::doApply() // Modify existing permissioned domain. auto sleUpdate = view().peek( {ltPERMISSIONED_DOMAIN, ctx_.tx.getFieldH256(sfDomainID)}); - try - { - updateSle(sleUpdate); - } - catch (...) - { - return temMALFORMED; - } + updateSle(sleUpdate); view().update(sleUpdate); } else @@ -146,14 +141,7 @@ PermissionedDomainSet::doApply() auto slePd = std::make_shared(pdKeylet); slePd->setAccountID(sfOwner, account_); slePd->setFieldU32(sfSequence, ctx_.tx.getFieldU32(sfSequence)); - try - { - updateSle(slePd); - } - catch (...) - { - return temMALFORMED; - } + updateSle(slePd); view().insert(slePd); auto const page = view().dirInsert( keylet::ownerDir(account_), pdKeylet, describeOwnerDir(account_));