Merge branch 'develop' into ximinez/fix-batchinnersigs

Author: ximinez

include/xrpl/protocol/PublicKey.h

@@ -231,11 +231,7 @@ verifyDigest(
     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

@@ -66,8 +66,6 @@ XRPL_FIX    (UniversalNumber,            Supported::yes, VoteBehavior::DefaultNo
 XRPL_FEATURE(XRPFees,                    Supported::yes, VoteBehavior::DefaultNo)
 XRPL_FIX    (RemoveNFTokenAutoTrustLine, Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FEATURE(FlowSortStrands,            Supported::yes, VoteBehavior::DefaultYes)
-XRPL_FEATURE(RequireFullyCanonicalSig,   Supported::yes, VoteBehavior::DefaultYes)
-XRPL_FEATURE(DeletableAccounts,          Supported::yes, VoteBehavior::DefaultYes)
 XRPL_FEATURE(Flow,                       Supported::yes, VoteBehavior::DefaultYes)
 
 // The following amendments are obsolete, but must remain supported
@@ -80,9 +78,9 @@ XRPL_FEATURE(Flow,                       Supported::yes, VoteBehavior::DefaultYe
 // enabled (added to the ledger).
 //
 // If a feature remains obsolete for long enough that no clients are able
-// to vote for it, the feature can be removed (entirely?) from the code.
-XRPL_FEATURE(CryptoConditionsSuite, Supported::yes, VoteBehavior::Obsolete)
-
+// to vote for it, the feature can be removed entirely from the code. Until
+// then the feature needs to be marked explicitly as obsolete, e.g.
+// XRPL_FEATURE(Example, Supported::yes, VoteBehavior::Obsolete)
 // The following amendments have been active for at least two years. Their
 // pre-amendment code has been removed and the identifiers are deprecated.
 // All known amendments and amendments that may appear in a validated ledger
@@ -118,6 +116,8 @@ XRPL_RETIRE_FIX(TrustLinesToSelf)
 XRPL_RETIRE_FEATURE(Checks)
 XRPL_RETIRE_FEATURE(CheckCashMakesTrustLine)
 XRPL_RETIRE_FEATURE(CryptoConditions)
+XRPL_RETIRE_FEATURE(CryptoConditionsSuite)
+XRPL_RETIRE_FEATURE(DeletableAccounts)
 XRPL_RETIRE_FEATURE(DepositAuth)
 XRPL_RETIRE_FEATURE(DepositPreauth)
 XRPL_RETIRE_FEATURE(DisallowIncoming)
@@ -133,6 +133,7 @@ XRPL_RETIRE_FEATURE(MultiSignReserve)
 XRPL_RETIRE_FEATURE(NegativeUNL)
 XRPL_RETIRE_FEATURE(NonFungibleTokensV1_1)
 XRPL_RETIRE_FEATURE(PayChan)
+XRPL_RETIRE_FEATURE(RequireFullyCanonicalSig)
 XRPL_RETIRE_FEATURE(SortedDirectories)
 XRPL_RETIRE_FEATURE(TicketBatch)
 XRPL_RETIRE_FEATURE(TickSize)

include/xrpl/protocol/detail/transactions.macro

@@ -297,7 +297,7 @@ TRANSACTION(ttTRUST_SET, 20, TrustSet,
 #endif
 TRANSACTION(ttACCOUNT_DELETE, 21, AccountDelete,
     Delegation::notDelegatable,
-    featureDeletableAccounts,
+    uint256{},
     mustDeleteAcct,
     ({
     {sfDestination, soeREQUIRED},

src/libxrpl/ledger/View.cpp

@@ -1863,7 +1863,7 @@ rippleSendIOU(
         // Direct send: redeeming IOUs and/or sending own IOUs.
         auto const ter =
             rippleCreditIOU(view, uSenderID, uReceiverID, saAmount, false, j);
-        if (view.rules().enabled(featureDeletableAccounts) && ter != tesSUCCESS)
+        if (ter != tesSUCCESS)
             return ter;
         saActual = saAmount;
         return tesSUCCESS;

src/libxrpl/protocol/PublicKey.cpp

@@ -267,18 +267,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);