Retire the feature

Signed-off-by: JCW <a1q123456@users.noreply.github.com>

include/xrpl/protocol/STTx.h

@@ -103,22 +103,15 @@ public:
         std::optional<std::reference_wrapper<SField const>> signatureTarget =
             {});
 
-    enum class RequireFullyCanonicalSig : bool { no, yes };
-
     /** Check the signature.
-        @param requireCanonicalSig If `true`, check that the signature is fully
-            canonical. If `false`, only check that the signature is valid.
         @param rules The current ledger rules.
         @return `true` if valid signature. If invalid, the error message string.
     */
     Expected<void, std::string>
-    checkSign(RequireFullyCanonicalSig requireCanonicalSig, Rules const& rules)
+    checkSign(Rules const& rules) const;
-        const;
 
     Expected<void, std::string>
-    checkBatchSign(
+    checkBatchSign(Rules const& rules) const;
-        RequireFullyCanonicalSig requireCanonicalSig,
-        Rules const& rules) const;
 
     // SQL Functions with metadata.
     static std::string const&
@@ -140,40 +133,25 @@ public:
 
 private:
     /** Check the signature.
-        @param requireCanonicalSig If `true`, check that the signature is fully
-            canonical. If `false`, only check that the signature is valid.
         @param rules The current ledger rules.
         @param sigObject Reference to object that contains the signature fields.
             Will be *this more often than not.
         @return `true` if valid signature. If invalid, the error message string.
     */
     Expected<void, std::string>
-    checkSign(
+    checkSign(Rules const& rules, STObject const& sigObject) const;
-        RequireFullyCanonicalSig requireCanonicalSig,
-        Rules const& rules,
-        STObject const& sigObject) const;
 
     Expected<void, std::string>
-    checkSingleSign(
+    checkSingleSign(STObject const& sigObject) const;
-        RequireFullyCanonicalSig requireCanonicalSig,
-        STObject const& sigObject) const;
 
     Expected<void, std::string>
-    checkMultiSign(
+    checkMultiSign(Rules const& rules, STObject const& sigObject) const;
-        RequireFullyCanonicalSig requireCanonicalSig,
-        Rules const& rules,
-        STObject const& sigObject) const;
 
     Expected<void, std::string>
-    checkBatchSingleSign(
+    checkBatchSingleSign(STObject const& batchSigner) const;
-        STObject const& batchSigner,
-        RequireFullyCanonicalSig requireCanonicalSig) const;
 
     Expected<void, std::string>
-    checkBatchMultiSign(
+    checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const;
-        STObject const& batchSigner,
-        RequireFullyCanonicalSig requireCanonicalSig,
-        Rules const& rules) const;
 
     STBase*
     copy(std::size_t n, void* buf) const override;

include/xrpl/protocol/detail/features.macro

@@ -68,7 +68,6 @@ XRPL_FIX    (RemoveNFTokenAutoTrustLine, Supported::yes, VoteBehavior::DefaultYe
 XRPL_FEATURE(FlowSortStrands,            Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FEATURE(TicketBatch,                Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FEATURE(NegativeUNL,                Supported::yes, VoteBehavior::DefaultYes)
-XRPL_FEATURE(RequireFullyCanonicalSig,   Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FEATURE(DeletableAccounts,          Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FEATURE(Checks,                     Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FEATURE(Flow,                       Supported::yes, VoteBehavior::DefaultYes)
@@ -133,6 +132,7 @@ XRPL_RETIRE_FEATURE(MultiSign)
 XRPL_RETIRE_FEATURE(MultiSignReserve)
 XRPL_RETIRE_FEATURE(NonFungibleTokensV1_1)
 XRPL_RETIRE_FEATURE(PayChan)
+XRPL_RETIRE_FEATURE(RequireFullyCanonicalSig)
 XRPL_RETIRE_FEATURE(SortedDirectories)
 XRPL_RETIRE_FEATURE(TickSize)
 XRPL_RETIRE_FEATURE(TrustSetAuth)

src/libxrpl/protocol/STTx.cpp

@@ -237,10 +237,7 @@ STTx::sign(
 }
 
 Expected<void, std::string>
-STTx::checkSign(
+STTx::checkSign(Rules const& rules, STObject const& sigObject) const
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules,
-    STObject const& sigObject) const
 {
     try
     {
@@ -249,9 +246,8 @@ STTx::checkSign(
         // multi-signing.  Otherwise we're single-signing.
 
         Blob const& signingPubKey = sigObject.getFieldVL(sfSigningPubKey);
-        return signingPubKey.empty()
+        return signingPubKey.empty() ? checkMultiSign(rules, sigObject)
-            ? checkMultiSign(requireCanonicalSig, rules, sigObject)
+                                     : checkSingleSign(sigObject);
-            : checkSingleSign(requireCanonicalSig, sigObject);
     }
     catch (std::exception const&)
     {
@@ -260,18 +256,16 @@ STTx::checkSign(
 }
 
 Expected<void, std::string>
-STTx::checkSign(
+STTx::checkSign(Rules const& rules) const
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules) const
 {
-    if (auto const ret = checkSign(requireCanonicalSig, rules, *this); !ret)
+    if (auto const ret = checkSign(rules, *this); !ret)
         return ret;
 
     /* Placeholder for field that will be added by Lending Protocol
     if (isFieldPresent(sfCounterpartySignature))
     {
         auto const counterSig = getFieldObject(sfCounterpartySignature);
-        if (auto const ret = checkSign(requireCanonicalSig, rules, counterSig);
+        if (auto const ret = checkSign(rules, counterSig);
             !ret)
             return Unexpected("Counterparty: " + ret.error());
     }
@@ -280,9 +274,7 @@ STTx::checkSign(
 }
 
 Expected<void, std::string>
-STTx::checkBatchSign(
+STTx::checkBatchSign(Rules const& rules) const
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules) const
 {
     try
     {
@@ -299,8 +291,8 @@ STTx::checkBatchSign(
         {
             Blob const& signingPubKey = signer.getFieldVL(sfSigningPubKey);
             auto const result = signingPubKey.empty()
-                ? checkBatchMultiSign(signer, requireCanonicalSig, rules)
+                ? checkBatchMultiSign(signer, rules)
-                : checkBatchSingleSign(signer, requireCanonicalSig);
+                : checkBatchSingleSign(signer);
 
             if (!result)
                 return result;
@@ -395,10 +387,7 @@ STTx::getMetaSQL(
 }
 
 static Expected<void, std::string>
-singleSignHelper(
+singleSignHelper(STObject const& sigObject, Slice const& data)
-    STObject const& sigObject,
-    Slice const& data,
-    bool const fullyCanonical)
 {
     // We don't allow both a non-empty sfSigningPubKey and an sfSigners.
     // That would allow the transaction to be signed two ways.  So if both
@@ -413,11 +402,8 @@ singleSignHelper(
         if (publicKeyType(makeSlice(spk)))
         {
             Blob const signature = sigObject.getFieldVL(sfTxnSignature);
-            validSig = verify(
+            validSig =
-                PublicKey(makeSlice(spk)),
+                verify(PublicKey(makeSlice(spk)), data, makeSlice(signature));
-                data,
-                makeSlice(signature),
-                fullyCanonical);
         }
     }
     catch (std::exception const&)
@@ -432,33 +418,24 @@ singleSignHelper(
 }
 
 Expected<void, std::string>
-STTx::checkSingleSign(
+STTx::checkSingleSign(STObject const& sigObject) const
-    RequireFullyCanonicalSig requireCanonicalSig,
-    STObject const& sigObject) const
 {
     auto const data = getSigningData(*this);
-    bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) ||
+    return singleSignHelper(sigObject, makeSlice(data));
-        (requireCanonicalSig == STTx::RequireFullyCanonicalSig::yes);
-    return singleSignHelper(sigObject, makeSlice(data), fullyCanonical);
 }
 
 Expected<void, std::string>
-STTx::checkBatchSingleSign(
+STTx::checkBatchSingleSign(STObject const& batchSigner) const
-    STObject const& batchSigner,
-    RequireFullyCanonicalSig requireCanonicalSig) const
 {
     Serializer msg;
     serializeBatch(msg, getFlags(), getBatchTransactionIDs());
-    bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) ||
+    return singleSignHelper(batchSigner, msg.slice());
-        (requireCanonicalSig == STTx::RequireFullyCanonicalSig::yes);
-    return singleSignHelper(batchSigner, msg.slice(), fullyCanonical);
 }
 
 Expected<void, std::string>
 multiSignHelper(
     STObject const& sigObject,
     std::optional<AccountID> txnAccountID,
-    bool const fullyCanonical,
     std::function<Serializer(AccountID const&)> makeMsg,
     Rules const& rules)
 {
@@ -515,8 +492,7 @@ multiSignHelper(
                 validSig = verify(
                     PublicKey(makeSlice(spk)),
                     makeMsg(accountID).slice(),
-                    makeSlice(signature),
+                    makeSlice(signature));
-                    fullyCanonical);
             }
         }
         catch (std::exception const& e)
@@ -535,14 +511,8 @@ multiSignHelper(
 }
 
 Expected<void, std::string>
-STTx::checkBatchMultiSign(
+STTx::checkBatchMultiSign(STObject const& batchSigner, Rules const& rules) const
-    STObject const& batchSigner,
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules) const
 {
-    bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) ||
-        (requireCanonicalSig == RequireFullyCanonicalSig::yes);
-
     // We can ease the computational load inside the loop a bit by
     // pre-constructing part of the data that we hash.  Fill a Serializer
     // with the stuff that stays constant from signature to signature.
@@ -551,7 +521,6 @@ STTx::checkBatchMultiSign(
     return multiSignHelper(
         batchSigner,
         std::nullopt,
-        fullyCanonical,
         [&dataStart](AccountID const& accountID) -> Serializer {
             Serializer s = dataStart;
             finishMultiSigningData(accountID, s);
@@ -561,14 +530,8 @@ STTx::checkBatchMultiSign(
 }
 
 Expected<void, std::string>
-STTx::checkMultiSign(
+STTx::checkMultiSign(Rules const& rules, STObject const& sigObject) const
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules,
-    STObject const& sigObject) const
 {
-    bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) ||
-        (requireCanonicalSig == RequireFullyCanonicalSig::yes);
-
     // Used inside the loop in multiSignHelper to enforce that
     // the account owner may not multisign for themselves.
     auto const txnAccountID = &sigObject != this
@@ -582,7 +545,6 @@ STTx::checkMultiSign(
     return multiSignHelper(
         sigObject,
         txnAccountID,
-        fullyCanonical,
         [&dataStart](AccountID const& accountID) -> Serializer {
             Serializer s = dataStart;
             finishMultiSigningData(accountID, s);

src/test/app/tx/apply_test.cpp

@@ -35,23 +35,6 @@ public:
         SerialIter sitTrans(makeSlice(*ret));
         STTx const tx = *std::make_shared<STTx const>(std::ref(sitTrans));
 
-        {
-            test::jtx::Env no_fully_canonical(
-                *this,
-                test::jtx::testable_amendments() -
-                    featureRequireFullyCanonicalSig);
-
-            Validity valid = checkValidity(
-                                 no_fully_canonical.app().getHashRouter(),
-                                 tx,
-                                 no_fully_canonical.current()->rules(),
-                                 no_fully_canonical.app().config())
-                                 .first;
-
-            if (valid != Validity::Valid)
-                fail("Non-Fully canoncial signature was not permitted");
-        }
-
         {
             test::jtx::Env fully_canonical(
                 *this, test::jtx::testable_amendments());

src/test/protocol/STTx_test.cpp

@@ -1615,8 +1615,7 @@ public:
         BEAST_EXPECT(!defaultRules.enabled(featureAMM));
 
         unexpected(
-            !j.checkSign(STTx::RequireFullyCanonicalSig::yes, defaultRules),
+            !j.checkSign(defaultRules), "Transaction fails signature test");
-            "Transaction fails signature test");
 
         Serializer rawTxn;
         j.add(rawTxn);

src/test/rpc/Feature_test.cpp

@@ -183,16 +183,16 @@ class Feature_test : public beast::unit_test::suite
         using namespace test::jtx;
         Env env{*this};
 
-        auto jrr = env.rpc("feature", "RequireFullyCanonicalSig")[jss::result];
+        auto jrr = env.rpc("feature", "Flow")[jss::result];
         BEAST_EXPECTS(jrr[jss::status] == jss::success, "status");
         jrr.removeMember(jss::status);
         BEAST_EXPECT(jrr.size() == 1);
         BEAST_EXPECT(
-            jrr.isMember("00C1FC4A53E60AB02C864641002B3172F38677E29C26C54066851"
+            jrr.isMember("740352F2412A9909880C23A559FCECEDA3BE2126FED62FC7660D6"
-                         "79B37E1EDAC"));
+                         "28A06927F11"));
         auto feature = *(jrr.begin());
 
-        BEAST_EXPECTS(feature[jss::name] == "RequireFullyCanonicalSig", "name");
+        BEAST_EXPECTS(feature[jss::name] == "Flow", "name");
         BEAST_EXPECTS(!feature[jss::enabled].asBool(), "enabled");
         BEAST_EXPECTS(
             feature[jss::vetoed].isBool() && !feature[jss::vetoed].asBool(),
@@ -200,7 +200,7 @@ class Feature_test : public beast::unit_test::suite
         BEAST_EXPECTS(feature[jss::supported].asBool(), "supported");
 
         // feature names are case-sensitive - expect error here
-        jrr = env.rpc("feature", "requireFullyCanonicalSig")[jss::result];
+        jrr = env.rpc("feature", "flow")[jss::result];
         BEAST_EXPECT(jrr[jss::error] == "badFeature");
         BEAST_EXPECT(jrr[jss::error_message] == "Feature unknown or invalid.");
     }
@@ -476,8 +476,8 @@ class Feature_test : public beast::unit_test::suite
         testcase("Veto");
 
         using namespace test::jtx;
-        Env env{*this, FeatureBitset{featureRequireFullyCanonicalSig}};
+        Env env{*this, FeatureBitset{featureFlow}};
-        constexpr char const* featureName = "RequireFullyCanonicalSig";
+        constexpr char const* featureName = "Flow";
 
         auto jrr = env.rpc("feature", featureName)[jss::result];
         if (!BEAST_EXPECTS(jrr[jss::status] == jss::success, "status"))

src/xrpld/app/tx/detail/Batch.cpp

@@ -451,8 +451,7 @@ Batch::preflightSigValidated(PreflightContext const& ctx)
         }
 
         // Check the batch signers signatures.
-        auto const sigResult = ctx.tx.checkBatchSign(
+        auto const sigResult = ctx.tx.checkBatchSign(ctx.rules);
-            STTx::RequireFullyCanonicalSig::yes, ctx.rules);
 
         if (!sigResult)
         {

src/xrpld/app/tx/detail/apply.cpp

@@ -58,13 +58,7 @@ checkValidity(
 
     if (!any(flags & SF_SIGGOOD))
     {
-        // Don't know signature state. Check it.
+        auto const sigVerify = tx.checkSign(rules);
-        auto const requireCanonicalSig =
-            rules.enabled(featureRequireFullyCanonicalSig)
-            ? STTx::RequireFullyCanonicalSig::yes
-            : STTx::RequireFullyCanonicalSig::no;
-
-        auto const sigVerify = tx.checkSign(requireCanonicalSig, rules);
         if (!sigVerify)
         {
             router.setFlags(id, SF_SIGBAD);