Deprecate banned accounts functionality now that CAP-0077 is active (#5426)

Resolves stellar/stellar-core-internal#702

Author: marta-lokhova

docs/software/commands.md

@@ -291,27 +291,16 @@ Most commands return their results in JSON format.
 * **bans**
   List current active bans
 
-* **banaccounts**
-  Manages the persistent list of banned accounts. Banned accounts are stored in
-  the database and survive restarts. Any transaction whose source account,
-  operation source account, fee-bump fee source, or (for Soroban transactions)
-  write footprint account entry matches a banned address will be rejected from
-  the transaction queue.
-  * `banaccounts`<br>
-    Lists the currently banned account addresses as a JSON array.<br>
-  * `banaccounts?accountids=G_ADDRESS1,G_ADDRESS2,...`<br>
-    Adds the specified addresses to the persistent ban list. Existing bans are
-    preserved (additive).<br>
-
-  Note: The `FILTERED_G_ADDRESSES` configuration option is deprecated. Any
-  addresses configured there will be automatically migrated to the persistent
-  ban list on startup.
-
-* **unbanaccounts**
-  * `unbanaccounts`<br>
-    Clears all banned accounts.<br>
-  * `unbanaccounts?accountids=G_ADDRESS1,G_ADDRESS2,...`<br>
-    Removes the specified addresses from the persistent ban list.<br>
+* **banaccounts** (deprecated)
+  Account banning has been removed; this endpoint is kept for backwards
+  compatibility and only returns a deprecation warning. See
+  [CAP-0077](https://github.com/stellar/stellar-protocol/blob/master/core/cap-0077.md)
+  for a more robust alternative. The `FILTERED_G_ADDRESSES` configuration
+  option is likewise deprecated and has no effect.
+
+* **unbanaccounts** (deprecated)
+  Account banning has been removed; this endpoint is kept for backwards
+  compatibility and only returns a deprecation warning.
 
 * **checkdb**
   Triggers the instance to perform a background check of the database's state.
@@ -399,10 +388,9 @@ Most commands return their results in JSON format.
     * "FILTERED" - transaction rejected because it contains an operation type that Stellar Core filters out. See Stellar Core configuration `EXCLUDE_TRANSACTIONS_CONTAINING_OPERATION_TYPE` for more details.
 
   Optional parameters:
-    * `force=true` - bypasses banned account filtering (see `banaccounts`),
-      allowing the transaction into the mempool even if its source account or
-      fee source is on the ban list. Other filtering (operation type, Soroban
-      key filtering) still applies. Example: `tx?blob=Base64&force=true`
+    * `force=true` - deprecated and has no effect (it used to bypass
+      banned account filtering, which has been removed). Accepted for
+      backwards compatibility; the response includes a deprecation warning.
 
 * **upgrades**
   * `upgrades?mode=get`<br>

src/database/Database.cpp

@@ -7,7 +7,6 @@
 #include "database/DatabaseConnectionString.h"
 #include "database/DatabaseTypeSpecificOperation.h"
 #include "main/Application.h"
-#include "main/BannedAccountsPersistor.h"
 #include "main/Config.h"
 #include "overlay/StellarXDR.h"
 #include "util/Decoder.h"
@@ -309,8 +308,13 @@ Database::applyMiscSchemaUpgrade(unsigned long vers)
         getRawMiscSession() << "DETACH DATABASE source_db";
         return;
     case 2:
-        // Add banned accounts table for persistent account filtering.
-        BannedAccountsPersistor::maybeDropAndCreateNew(mMiscSession.session());
+        getRawMiscSession() << "DROP TABLE IF EXISTS bannedaccounts;";
+        getRawMiscSession() << "CREATE TABLE bannedaccounts (accountid "
+                               "VARCHAR(56) PRIMARY KEY);";
+        break;
+    case 3:
+        // The account banning feature was removed; drop its table.
+        getRawMiscSession() << "DROP TABLE IF EXISTS bannedaccounts;";
         break;
     default:
         throw std::runtime_error("Unknown DB schema version");
@@ -407,12 +411,17 @@ Database::applySchemaUpgrade(unsigned long vers)
         // for Postgres and in-memory SQLite, create in the main DB.
         if (!canUseMiscDB())
         {
-            BannedAccountsPersistor::maybeDropAndCreateNew(getRawSession());
+            getRawSession() << "DROP TABLE IF EXISTS bannedaccounts;";
+            getRawSession() << "CREATE TABLE bannedaccounts (accountid "
+                               "VARCHAR(56) PRIMARY KEY);";
         }
         break;
     case 28:
         migrateLedgerHeadersToStoreState(*this);
         break;
+    case 29:
+        getRawSession() << "DROP TABLE IF EXISTS bannedaccounts;";
+        break;
 
     default:
         throw std::runtime_error("Unknown DB schema version");

src/database/Database.h

@@ -29,11 +29,11 @@ class Application;
 
 // smallest schema version supported
 static constexpr unsigned long MIN_SCHEMA_VERSION = 25;
-static constexpr unsigned long SCHEMA_VERSION = 28;
+static constexpr unsigned long SCHEMA_VERSION = 29;
 static constexpr unsigned long FIRST_MAIN_VERSION_WITH_MISC = 26;
 // Misc schema version 0 means no misc table exists yet
 static constexpr unsigned long MIN_MISC_SCHEMA_VERSION = 0;
-static constexpr unsigned long MISC_SCHEMA_VERSION = 2;
+static constexpr unsigned long MISC_SCHEMA_VERSION = 3;
 
 /**
  * Helper class for borrowing a SOCI prepared statement handle into a local

src/database/test/DatabaseTests.cpp

@@ -12,7 +12,6 @@
 #include "ledger/test/LedgerTestUtils.h"
 #include "lib/util/stdrandom.h"
 #include "main/Application.h"
-#include "main/BannedAccountsPersistor.h"
 #include "main/Config.h"
 #include "main/PersistentState.h"
 #include "overlay/BanManager.h"
@@ -715,10 +714,6 @@ TEST_CASE("schema parity across DB backends", "[db][schematest]")
     TmpDir tmpDir("schema-parity-test");
     Config cfg1 = getTestConfig(0, Config::TESTDB_BUCKET_DB_PERSISTENT);
     cfg1.DATABASE = SecretValue{"sqlite3://" + tmpDir.getName() + "/test.db"};
-    // Use non-empty FILTERED_G_ADDRESSES to test migration as well
-    cfg1.FILTERED_G_ADDRESSES = {
-        "GBO7VUL2TOKPWFAWKATIW7K3QYA7WQ63VDY5CAE6AFUUX6BHZBOC2WXC",
-        "GATDQL767ZM2JQTBEG4BQ5WKOQNGAGWZDUN4GYT2UINPEU3RT2UAMVZH"};
 
     VirtualClock clock1;
     Application::pointer app1 = createTestApplication(clock1, cfg1);
@@ -744,9 +739,6 @@ TEST_CASE("schema parity across DB backends", "[db][schematest]")
 
     // ---- PostgreSQL: compare tables and row counts ----
     Config cfg2 = getTestConfig(1, Config::TESTDB_POSTGRESQL);
-    cfg2.FILTERED_G_ADDRESSES = {
-        "GBO7VUL2TOKPWFAWKATIW7K3QYA7WQ63VDY5CAE6AFUUX6BHZBOC2WXC",
-        "GATDQL767ZM2JQTBEG4BQ5WKOQNGAGWZDUN4GYT2UINPEU3RT2UAMVZH"};
 
     VirtualClock clock2;
     Application::pointer app2 = createTestApplication(clock2, cfg2);

src/herder/Herder.h

@@ -150,11 +150,10 @@ class Herder
     // generator, and therefore can skip certain expensive validity checks
     virtual TransactionQueue::AddResult
     recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
-                    bool force = false, bool isLoadgenTx = false) = 0;
+                    bool isLoadgenTx = false) = 0;
 #else
     virtual TransactionQueue::AddResult
-    recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
-                    bool force = false) = 0;
+    recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf) = 0;
 #endif
     virtual void peerDoesntHave(stellar::MessageType type,
                                 uint256 const& itemID, Peer::pointer peer) = 0;
@@ -225,9 +224,6 @@ class Herder
     // gets the upgrades that are scheduled by this node
     virtual std::string getUpgradesJson() = 0;
 
-    // Override the filtered accounts at runtime using pre-parsed AccountIDs.
-    virtual void setFilteredAccounts(std::set<AccountID> const& accounts) = 0;
-
     virtual void forceSCPStateIntoSyncWithLastClosedLedger() = 0;
 
     // helper function to craft an SCPValue

src/herder/HerderImpl.cpp

@@ -629,8 +629,7 @@ HerderImpl::emitEnvelope(SCPEnvelope const& envelope)
 }
 
 TransactionQueue::AddResult
-HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
-                            bool force
+HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf
 #ifdef BUILD_TESTS
                             ,
                             bool isLoadgenTx
@@ -672,7 +671,7 @@ HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
     }
     else if (!tx->isSoroban())
     {
-        result = mTransactionQueue.tryAdd(tx, submittedFromSelf, force
+        result = mTransactionQueue.tryAdd(tx, submittedFromSelf
 #ifdef BUILD_TESTS
                                           ,
                                           isLoadgenTx
@@ -681,7 +680,7 @@ HerderImpl::recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
     }
     else if (mSorobanTransactionQueue)
     {
-        result = mSorobanTransactionQueue->tryAdd(tx, submittedFromSelf, force
+        result = mSorobanTransactionQueue->tryAdd(tx, submittedFromSelf
 #ifdef BUILD_TESTS
                                                   ,
                                                   isLoadgenTx
@@ -1864,16 +1863,6 @@ HerderImpl::getUpgradesJson()
     return mUpgrades.getParameters().toDebugJson(ledgerView);
 }
 
-void
-HerderImpl::setFilteredAccounts(std::set<AccountID> const& accounts)
-{
-    mTransactionQueue.setFilteredAccounts(accounts);
-    if (mSorobanTransactionQueue)
-    {
-        mSorobanTransactionQueue->setFilteredAccounts(accounts);
-    }
-}
-
 void
 HerderImpl::forceSCPStateIntoSyncWithLastClosedLedger()
 {
@@ -2552,8 +2541,6 @@ HerderImpl::maybeSetupSorobanQueue(uint32_t protocolVersion)
                     TRANSACTION_QUEUE_BAN_LEDGERS,
                     mApp.getConfig().SOROBAN_TRANSACTION_QUEUE_SIZE_MULTIPLIER,
                     recomputeKeysToFilter(protocolVersion));
-            setFilteredAccounts(
-                mApp.getBannedAccountsPersistor().getBannedAccounts());
         }
     }
     else if (mSorobanTransactionQueue)
@@ -2627,22 +2614,6 @@ HerderImpl::start()
     restoreUpgrades();
     startTxSetGCTimer();
     startCheckForDeadNodesInterval();
-
-    auto& bap = mApp.getBannedAccountsPersistor();
-    if (!mApp.getConfig().FILTERED_G_ADDRESSES.empty())
-    {
-        CLOG_WARNING(
-            Herder,
-            "FILTERED_G_ADDRESSES is deprecated and will be removed in a "
-            "future release. The current {} address(es) will be stored in the "
-            "database. You can safely remove FILTERED_G_ADDRESSES from the "
-            "config. Use 'banaccounts'/'unbanaccounts' HTTP commands to manage "
-            "banned accounts going forward.",
-            mApp.getConfig().FILTERED_G_ADDRESSES.size());
-        bap.addBannedAccounts(mApp.getConfig().FILTERED_G_ADDRESSES);
-    }
-
-    setFilteredAccounts(bap.getBannedAccounts());
 }
 
 void

src/herder/HerderImpl.h

@@ -101,11 +101,11 @@ class HerderImpl : public Herder
 #ifdef BUILD_TESTS
     TransactionQueue::AddResult
     recvTransaction(TransactionFrameBasePtr tx, bool submittedFromSelf,
-                    bool force = false, bool isLoadgenTx = false) override;
+                    bool isLoadgenTx = false) override;
 #else
-    TransactionQueue::AddResult recvTransaction(TransactionFrameBasePtr tx,
-                                                bool submittedFromSelf,
-                                                bool force = false) override;
+    TransactionQueue::AddResult
+    recvTransaction(TransactionFrameBasePtr tx,
+                    bool submittedFromSelf) override;
 #endif
 
     EnvelopeStatus recvSCPEnvelope(SCPEnvelope const& envelope) override;
@@ -195,8 +195,6 @@ class HerderImpl : public Herder
     void setUpgrades(Upgrades::UpgradeParameters const& upgrades) override;
     std::string getUpgradesJson() override;
 
-    void setFilteredAccounts(std::set<AccountID> const& accounts) override;
-
     void forceSCPStateIntoSyncWithLastClosedLedger() override;
 
     bool resolveNodeID(std::string const& s, PublicKey& retKey) override;

src/herder/TransactionQueue.cpp

@@ -109,21 +109,10 @@ TransactionQueue::TransactionQueue(Application& app, uint32 pendingDepth,
         app.getConfig().EXCLUDE_TRANSACTIONS_CONTAINING_OPERATION_TYPE;
     mFilteredTypes.insert(filteredTypes.begin(), filteredTypes.end());
 
-    for (auto const& addr : app.getConfig().FILTERED_G_ADDRESSES)
-    {
-        mFilteredAccounts.emplace(KeyUtils::fromStrKey<PublicKey>(addr));
-    }
-
     mBroadcastSeed =
         rand_uniform<uint64>(0, std::numeric_limits<uint64>::max());
 }
 
-void
-TransactionQueue::setFilteredAccounts(std::set<AccountID> const& accounts)
-{
-    mFilteredAccounts = accounts;
-}
-
 ClassicTransactionQueue::ClassicTransactionQueue(Application& app,
                                                  uint32 pendingDepth,
                                                  uint32 banDepth,
@@ -153,9 +142,7 @@ ClassicTransactionQueue::ClassicTransactionQueue(Application& app,
         app.getMetrics().NewCounter(
             {"herder", "pending-txs", "not-included-due-to-low-fee-count"}),
         app.getMetrics().NewCounter(
-            {"herder", "pending-txs", "filtered-due-to-fp-keys"}),
-        app.getMetrics().NewCounter(
-            {"herder", "pending-txs", "filtered-due-to-account-keys"}));
+            {"herder", "pending-txs", "filtered-due-to-fp-keys"}));
     mBroadcastOpCarryover.resize(1,
                                  Resource::makeEmpty(NUM_CLASSIC_TX_RESOURCES));
 }
@@ -315,8 +302,7 @@ TransactionQueue::sourceAccountPending(AccountID const& accountID) const
 TransactionQueue::AddResult
 TransactionQueue::canAdd(
     TransactionFrameBasePtr tx, AccountStates::iterator& stateIter,
-    std::vector<std::pair<TransactionFrameBasePtr, bool>>& txsToEvict,
-    bool force
+    std::vector<std::pair<TransactionFrameBasePtr, bool>>& txsToEvict
 #ifdef BUILD_TESTS
     ,
     bool isLoadgenTx
@@ -338,12 +324,6 @@ TransactionQueue::canAdd(
         mQueueMetrics->mTxsFilteredDueToFootprintKeys.inc();
         return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_FILTERED);
     }
-    if (!force && !tx->validateAccountFilterForFlooding(mFilteredAccounts))
-    {
-        mQueueMetrics->mTxsFilteredDueToAccountKeys.inc();
-        return AddResult(TransactionQueue::AddResultCode::ADD_STATUS_FILTERED);
-    }
-
     int64_t newFullFee = tx->getFullFee();
     if (newFullFee < 0 || tx->getInclusionFee() < 0)
     {
@@ -673,8 +653,7 @@ TransactionQueue::findAllAssetPairsInvolvedInPaymentLoops(
 }
 
 TransactionQueue::AddResult
-TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf,
-                         bool force
+TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf
 #ifdef BUILD_TESTS
                          ,
                          bool isLoadgenTx
@@ -692,7 +671,7 @@ TransactionQueue::tryAdd(TransactionFrameBasePtr tx, bool submittedFromSelf,
     AccountStates::iterator stateIter;
 
     std::vector<std::pair<TransactionFrameBasePtr, bool>> txsToEvict;
-    auto res = canAdd(tx, stateIter, txsToEvict, force
+    auto res = canAdd(tx, stateIter, txsToEvict
 #ifdef BUILD_TESTS
                       ,
                       isLoadgenTx
@@ -1122,9 +1101,7 @@ SorobanTransactionQueue::SorobanTransactionQueue(
         app.getMetrics().NewCounter({"herder", "pending-soroban-txs",
                                      "not-included-due-to-low-fee-count"}),
         app.getMetrics().NewCounter(
-            {"herder", "pending-soroban-txs", "filtered-due-to-fp-keys"}),
-        app.getMetrics().NewCounter(
-            {"herder", "pending-soroban-txs", "filtered-due-to-account-keys"}));
+            {"herder", "pending-soroban-txs", "filtered-due-to-fp-keys"}));
     mBroadcastOpCarryover.resize(1, Resource::makeEmptySoroban());
     mKeysToFilter = keysToFilter;
 }