Retire RequireFullyCanonicalSig

include/xrpl/protocol/PublicKey.h

@@ -223,19 +223,14 @@ publicKeyType(PublicKey const& publicKey)
 verifyDigest(
     PublicKey const& publicKey,
     uint256 const& digest,
-    Slice const& sig,
-    bool mustBeFullyCanonical = true) noexcept;
+    Slice const& sig) noexcept;
 
 /** Verify a signature on a message.
     With secp256k1 signatures, the data is first hashed with
     SHA512-Half, and the resulting digest is signed.
 */
 [[nodiscard]] bool
-verify(
-    PublicKey const& publicKey,
-    Slice const& m,
-    Slice const& sig,
-    bool mustBeFullyCanonical = true) noexcept;
+verify(PublicKey const& publicKey, Slice const& m, Slice const& sig) noexcept;
 
 /** Calculate the 160-bit node ID from a node public key. */
 NodeID

include/xrpl/protocol/STTx.h

@@ -103,22 +103,15 @@ class STTx final : public STObject, public CountedObject<STTx>
         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)
-        const;
+    checkSign(Rules const& rules) const;
 
     Expected<void, std::string>
-    checkBatchSign(
-        RequireFullyCanonicalSig requireCanonicalSig,
-        Rules const& rules) const;
+    checkBatchSign(Rules const& rules) const;
 
     // SQL Functions with metadata.
     static std::string const&
@@ -140,40 +133,25 @@ class STTx final : public STObject, public CountedObject<STTx>
 
 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(
-        RequireFullyCanonicalSig requireCanonicalSig,
-        Rules const& rules,
-        STObject const& sigObject) const;
+    checkSign(Rules const& rules, STObject const& sigObject) const;
 
     Expected<void, std::string>
-    checkSingleSign(
-        RequireFullyCanonicalSig requireCanonicalSig,
-        STObject const& sigObject) const;
+    checkSingleSign(STObject const& sigObject) const;
 
     Expected<void, std::string>
-    checkMultiSign(
-        RequireFullyCanonicalSig requireCanonicalSig,
-        Rules const& rules,
-        STObject const& sigObject) const;
+    checkMultiSign(Rules const& rules, STObject const& sigObject) const;
 
     Expected<void, std::string>
-    checkBatchSingleSign(
-        STObject const& batchSigner,
-        RequireFullyCanonicalSig requireCanonicalSig) const;
+    checkBatchSingleSign(STObject const& batchSigner) const;
 
     Expected<void, std::string>
-    checkBatchMultiSign(
-        STObject const& batchSigner,
-        RequireFullyCanonicalSig requireCanonicalSig,
-        Rules const& rules) const;
+    checkBatchMultiSign(STObject const& batchSigner, 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/PublicKey.cpp

@@ -220,16 +220,14 @@ bool
 verifyDigest(
     PublicKey const& publicKey,
     uint256 const& digest,
-    Slice const& sig,
-    bool mustBeFullyCanonical) noexcept
+    Slice const& sig) noexcept
 {
     if (publicKeyType(publicKey) != KeyType::secp256k1)
         LogicError("sign: secp256k1 required for digest signing");
     auto const canonicality = ecdsaCanonicality(sig);
     if (!canonicality)
         return false;
-    if (mustBeFullyCanonical &&
-        (*canonicality != ECDSACanonicality::fullyCanonical))
+    if (*canonicality != ECDSACanonicality::fullyCanonical)
         return false;
 
     secp256k1_pubkey pubkey_imp;
@@ -267,18 +265,13 @@ verifyDigest(
 }
 
 bool
-verify(
-    PublicKey const& publicKey,
-    Slice const& m,
-    Slice const& sig,
-    bool mustBeFullyCanonical) noexcept
+verify(PublicKey const& publicKey, Slice const& m, Slice const& sig) noexcept
 {
     if (auto const type = publicKeyType(publicKey))
     {
         if (*type == KeyType::secp256k1)
         {
-            return verifyDigest(
-                publicKey, sha512Half(m), sig, mustBeFullyCanonical);
+            return verifyDigest(publicKey, sha512Half(m), sig);
         }
         else if (*type == KeyType::ed25519)
         {

src/libxrpl/protocol/STTx.cpp

@@ -237,10 +237,7 @@ STTx::sign(
 }
 
 Expected<void, std::string>
-STTx::checkSign(
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules,
-    STObject const& sigObject) const
+STTx::checkSign(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()
-            ? checkMultiSign(requireCanonicalSig, rules, sigObject)
-            : checkSingleSign(requireCanonicalSig, sigObject);
+        return signingPubKey.empty() ? checkMultiSign(rules, sigObject)
+                                     : checkSingleSign(sigObject);
     }
     catch (std::exception const&)
     {
@@ -260,18 +256,16 @@ STTx::checkSign(
 }
 
 Expected<void, std::string>
-STTx::checkSign(
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules) const
+STTx::checkSign(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(
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules) const
+STTx::checkBatchSign(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)
-                : checkBatchSingleSign(signer, requireCanonicalSig);
+                ? checkBatchMultiSign(signer, rules)
+                : checkBatchSingleSign(signer);
 
             if (!result)
                 return result;
@@ -395,10 +387,7 @@ STTx::getMetaSQL(
 }
 
 static Expected<void, std::string>
-singleSignHelper(
-    STObject const& sigObject,
-    Slice const& data,
-    bool const fullyCanonical)
+singleSignHelper(STObject const& sigObject, Slice const& data)
 {
     // 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(
-                PublicKey(makeSlice(spk)),
-                data,
-                makeSlice(signature),
-                fullyCanonical);
+            validSig =
+                verify(PublicKey(makeSlice(spk)), data, makeSlice(signature));
         }
     }
     catch (std::exception const&)
@@ -432,33 +418,24 @@ singleSignHelper(
 }
 
 Expected<void, std::string>
-STTx::checkSingleSign(
-    RequireFullyCanonicalSig requireCanonicalSig,
-    STObject const& sigObject) const
+STTx::checkSingleSign(STObject const& sigObject) const
 {
     auto const data = getSigningData(*this);
-    bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) ||
-        (requireCanonicalSig == STTx::RequireFullyCanonicalSig::yes);
-    return singleSignHelper(sigObject, makeSlice(data), fullyCanonical);
+    return singleSignHelper(sigObject, makeSlice(data));
 }
 
 Expected<void, std::string>
-STTx::checkBatchSingleSign(
-    STObject const& batchSigner,
-    RequireFullyCanonicalSig requireCanonicalSig) const
+STTx::checkBatchSingleSign(STObject const& batchSigner) const
 {
     Serializer msg;
     serializeBatch(msg, getFlags(), getBatchTransactionIDs());
-    bool const fullyCanonical = (getFlags() & tfFullyCanonicalSig) ||
-        (requireCanonicalSig == STTx::RequireFullyCanonicalSig::yes);
-    return singleSignHelper(batchSigner, msg.slice(), fullyCanonical);
+    return singleSignHelper(batchSigner, msg.slice());
 }
 
 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),
-                    fullyCanonical);
+                    makeSlice(signature));
             }
         }
         catch (std::exception const& e)
@@ -535,14 +511,8 @@ multiSignHelper(
 }
 
 Expected<void, std::string>
-STTx::checkBatchMultiSign(
-    STObject const& batchSigner,
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules) const
+STTx::checkBatchMultiSign(STObject const& batchSigner, 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(
-    RequireFullyCanonicalSig requireCanonicalSig,
-    Rules const& rules,
-    STObject const& sigObject) const
+STTx::checkMultiSign(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 @@ class Apply_test : public beast::unit_test::suite
         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 @@ class STTx_test : public beast::unit_test::suite
         BEAST_EXPECT(!defaultRules.enabled(featureAMM));
 
         unexpected(
-            !j.checkSign(STTx::RequireFullyCanonicalSig::yes, defaultRules),
-            "Transaction fails signature test");
+            !j.checkSign(defaultRules), "Transaction fails signature test");
 
         Serializer rawTxn;
         j.add(rawTxn);