Remove unused per-controller storage delegate (#17258)

* Remove unused DoesDevicePairingExist function.

* Remove write-only mPairedDevicesUpdated field

* Remove write-only mPairedDevices from DeviceController.

The comment about initializing the commissionee device pool was out
of date; the code being removed does not touch that pool.

* Remove use of no-longer-existing storage keys from disabled chip-tool code.

* Remove unused storage delegate from ControllerDeviceInitParams.

* Remove write-only storage delegate field from ControllerInitParams.

* Remove write-only storage delegate field from Controller::SetupParams.

Author: bzbarsky-apple

examples/chip-tool/commands/common/CHIPCommand.cpp

@@ -334,7 +334,6 @@ CHIP_ERROR CHIPCommand::InitializeCommissioner(std::string key, chip::FabricId f
         commissionerParams.controllerNOC      = nocSpan;
     }
 
-    commissionerParams.storageDelegate = &mCommissionerStorage;
     // TODO: Initialize IPK epoch key in ExampleOperationalCredentials issuer rather than relying on DefaultIpkValue
     commissionerParams.operationalCredentialsDelegate = mCredIssuerCmds->GetCredentialIssuer();
     commissionerParams.controllerVendorId             = chip::VendorId::TestVendor1;

examples/chip-tool/commands/pairing/CommissionedListCommand.cpp

@@ -35,12 +35,8 @@ CHIP_ERROR CommissionedListCommand::PrintInformation()
     uint16_t pairedNodesIdsSize = sizeof(pairedNodesIds);
     memset(pairedNodesIds, 0, pairedNodesIdsSize);
 
-    PERSISTENT_KEY_OP(static_cast<uint64_t>(0), chip::kPairedDeviceListKeyPrefix, key,
-                      ReturnLogErrorOnFailure(mStorage.SyncGetKeyValue(key, pairedNodesIds, pairedNodesIdsSize)));
-
-    chip::SerializableU64Set<chip::Controller::kNumMaxPairedDevices> devices;
-    devices.Deserialize(chip::ByteSpan((uint8_t *) pairedNodesIds, pairedNodesIdsSize));
-
+    // TODO: Get the list of paired node IDs.  chip-tool needs to store that as
+    // devices get paired.
     uint16_t pairedDevicesCount = 0;
     while (pairedNodesIds[pairedDevicesCount] != 0x0 && pairedDevicesCount < chip::Controller::kNumMaxPairedDevices)
     {
@@ -69,13 +65,12 @@ CHIP_ERROR CommissionedListCommand::PrintInformation()
 
 CHIP_ERROR CommissionedListCommand::PrintDeviceInformation(chip::NodeId deviceId)
 {
+    // TODO: Controller::SerializedDevice and Controller::SerializableDevice are
+    // gone.  Need to figure out what chip-tool should actually store/retrieve
+    // here.
     chip::Controller::SerializedDevice deviceInfo;
     uint16_t size = sizeof(deviceInfo.inner);
 
-    PERSISTENT_KEY_OP(deviceId, chip::kPairedDeviceKeyPrefix, key,
-                      ReturnLogErrorOnFailure(mStorage.SyncGetKeyValue(key, deviceInfo.inner, size)));
-    VerifyOrReturnError(size <= sizeof(deviceInfo.inner), CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR);
-
     chip::Controller::SerializableDevice serializable;
     constexpr size_t maxlen = BASE64_ENCODED_LEN(sizeof(serializable));
     const size_t len        = strnlen(chip::Uint8::to_const_char(&deviceInfo.inner[0]), maxlen);

examples/platform/linux/AppMain.cpp

@@ -456,7 +456,6 @@ CHIP_ERROR InitCommissioner()
     ReturnErrorOnFailure(gGroupDataProvider.Init());
     factoryParams.groupDataProvider = &gGroupDataProvider;
 
-    params.storageDelegate                = &gServerStorage;
     params.operationalCredentialsDelegate = &gOpCredsIssuer;
 
     ReturnErrorOnFailure(gOpCredsIssuer.Initialize(gServerStorage));

src/controller/CHIPDeviceController.cpp

@@ -105,9 +105,7 @@ constexpr uint32_t kSessionEstablishmentTimeout = 40 * kMillisecondsPerSecond;
 
 DeviceController::DeviceController()
 {
-    mState                    = State::NotInitialized;
-    mStorageDelegate          = nullptr;
-    mPairedDevicesInitialized = false;
+    mState = State::NotInitialized;
 }
 
 CHIP_ERROR DeviceController::Init(ControllerInitParams params)
@@ -118,7 +116,6 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
     VerifyOrReturnError(params.systemState->SystemLayer() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
     VerifyOrReturnError(params.systemState->UDPEndPointManager() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 
-    mStorageDelegate = params.storageDelegate;
 #if CONFIG_NETWORK_LAYER_BLE
     VerifyOrReturnError(params.systemState->BleLayer() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
 #endif
@@ -231,8 +228,6 @@ CHIP_ERROR DeviceController::Shutdown()
         mSystemState->SessionMgr()->ExpireAllPairingsForFabric(mFabricInfo->GetFabricIndex());
     }
 
-    mStorageDelegate = nullptr;
-
     if (mFabricInfo != nullptr)
     {
         mFabricInfo->Reset();
@@ -246,16 +241,6 @@ CHIP_ERROR DeviceController::Shutdown()
     return CHIP_NO_ERROR;
 }
 
-bool DeviceController::DoesDevicePairingExist(const PeerId & deviceId)
-{
-    if (InitializePairedDeviceList() == CHIP_NO_ERROR)
-    {
-        return mPairedDevices.Contains(deviceId.GetNodeId());
-    }
-
-    return false;
-}
-
 void DeviceController::ReleaseOperationalDevice(NodeId remoteDeviceId)
 {
     VerifyOrReturn(mState == State::Initialized && mFabricInfo != nullptr,
@@ -329,64 +314,6 @@ void DeviceController::OnFirstMessageDeliveryFailed(const SessionHandle & sessio
     }
 }
 
-CHIP_ERROR DeviceController::InitializePairedDeviceList()
-{
-    CHIP_ERROR err   = CHIP_NO_ERROR;
-    uint8_t * buffer = nullptr;
-
-    VerifyOrExit(mStorageDelegate != nullptr, err = CHIP_ERROR_INCORRECT_STATE);
-
-    if (!mPairedDevicesInitialized)
-    {
-        constexpr uint16_t max_size = sizeof(uint64_t) * kNumMaxPairedDevices;
-        buffer                      = static_cast<uint8_t *>(chip::Platform::MemoryCalloc(max_size, 1));
-        uint16_t size               = max_size;
-
-        VerifyOrExit(buffer != nullptr, err = CHIP_ERROR_INVALID_ARGUMENT);
-
-        CHIP_ERROR lookupError = CHIP_NO_ERROR;
-        PERSISTENT_KEY_OP(static_cast<uint64_t>(0), kPairedDeviceListKeyPrefix, key,
-                          lookupError = mStorageDelegate->SyncGetKeyValue(key, buffer, size));
-
-        // It's ok to not have an entry for the Paired Device list. We treat it the same as having an empty list.
-        if (lookupError != CHIP_ERROR_KEY_NOT_FOUND)
-        {
-            VerifyOrExit(size <= max_size, err = CHIP_ERROR_INVALID_DEVICE_DESCRIPTOR);
-            err = SetPairedDeviceList(ByteSpan(buffer, size));
-            SuccessOrExit(err);
-        }
-    }
-
-exit:
-    if (buffer != nullptr)
-    {
-        chip::Platform::MemoryFree(buffer);
-    }
-    if (err != CHIP_NO_ERROR)
-    {
-        ChipLogError(Controller, "Failed to initialize the device list with error: %" CHIP_ERROR_FORMAT, err.Format());
-    }
-
-    return err;
-}
-
-CHIP_ERROR DeviceController::SetPairedDeviceList(ByteSpan serialized)
-{
-    CHIP_ERROR err = mPairedDevices.Deserialize(serialized);
-
-    if (err != CHIP_NO_ERROR)
-    {
-        ChipLogError(Controller, "Failed to recreate the device list with buffer %.*s\n", static_cast<int>(serialized.size()),
-                     serialized.data());
-    }
-    else
-    {
-        mPairedDevicesInitialized = true;
-    }
-
-    return err;
-}
-
 CHIP_ERROR DeviceController::GetPeerAddressAndPort(PeerId peerId, Inet::IPAddress & addr, uint16_t & port)
 {
     VerifyOrReturnError(mState == State::Initialized, CHIP_ERROR_INCORRECT_STATE);
@@ -412,7 +339,6 @@ ControllerDeviceInitParams DeviceController::GetControllerDeviceInitParams()
         .sessionManager     = mSystemState->SessionMgr(),
         .exchangeMgr        = mSystemState->ExchangeMgr(),
         .udpEndPointManager = mSystemState->UDPEndPointManager(),
-        .storageDelegate    = mStorageDelegate,
         .fabricsTable       = mSystemState->Fabrics(),
     };
 }
@@ -423,7 +349,6 @@ DeviceCommissioner::DeviceCommissioner() :
     mDeviceNOCChainCallback(OnDeviceNOCChainGeneration, this), mSetUpCodePairer(this)
 {
     mPairingDelegate           = nullptr;
-    mPairedDevicesUpdated      = false;
     mDeviceBeingCommissioned   = nullptr;
     mDeviceInPASEEstablishment = nullptr;
 }
@@ -632,10 +557,6 @@ CHIP_ERROR DeviceCommissioner::EstablishPASEConnection(NodeId remoteDeviceId, Re
     VerifyOrExit(mState == State::Initialized, err = CHIP_ERROR_INCORRECT_STATE);
     VerifyOrExit(mDeviceInPASEEstablishment == nullptr, err = CHIP_ERROR_INCORRECT_STATE);
 
-    // This will initialize the commissionee device pool if it has not already been initialized.
-    err = InitializePairedDeviceList();
-    SuccessOrExit(err);
-
     // TODO(#13940): We need to specify the peer address for BLE transport in bindings.
     if (params.GetPeerAddress().GetTransportType() == Transport::Type::kBle ||
         params.GetPeerAddress().GetTransportType() == Transport::Type::kUndefined)
@@ -1380,9 +1301,6 @@ CHIP_ERROR DeviceCommissioner::OnOperationalCredentialsProvisioningCompletion(De
 
     mSystemState->SystemLayer()->CancelTimer(OnSessionEstablishmentTimeoutCallback, this);
 
-    mPairedDevices.Insert(device->GetDeviceId());
-    mPairedDevicesUpdated = true;
-
     if (mPairingDelegate != nullptr)
     {
         mPairingDelegate->OnStatusUpdate(DevicePairingDelegate::SecurePairingSuccess);

src/controller/CHIPDeviceController.h

@@ -81,14 +81,12 @@ namespace Controller {
 using namespace chip::Protocols::UserDirectedCommissioning;
 
 constexpr uint16_t kNumMaxActiveDevices = CHIP_CONFIG_CONTROLLER_MAX_ACTIVE_DEVICES;
-constexpr uint16_t kNumMaxPairedDevices = 128;
 
 // Raw functions for cluster callbacks
 void OnBasicFailure(void * context, CHIP_ERROR err);
 
 struct ControllerInitParams
 {
-    PersistentStorageDelegate * storageDelegate                     = nullptr;
     DeviceControllerSystemState * systemState                       = nullptr;
     DeviceDiscoveryDelegate * deviceDiscoveryDelegate               = nullptr;
     OperationalCredentialsDelegate * operationalCredentialsDelegate = nullptr;
@@ -148,12 +146,6 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr
 
     CHIP_ERROR GetPeerAddressAndPort(PeerId peerId, Inet::IPAddress & addr, uint16_t & port);
 
-    /**
-     *   This function returns true if the device corresponding to `deviceId` has previously been commissioned
-     *   on the fabric.
-     */
-    bool DoesDevicePairingExist(const PeerId & deviceId);
-
     /**
      *   This function finds the device corresponding to deviceId, and establishes a secure connection with it.
      *   Once the connection is successfully establishes (or if it's already connected), it calls `onConnectedDevice`
@@ -247,21 +239,15 @@ class DLL_EXPORT DeviceController : public SessionRecoveryDelegate, public Abstr
 
     State mState;
 
-    SerializableU64Set<kNumMaxPairedDevices> mPairedDevices;
-    bool mPairedDevicesInitialized;
-
     PeerId mLocalId          = PeerId();
     FabricId mFabricId       = kUndefinedFabricId;
     FabricInfo * mFabricInfo = nullptr;
 
-    PersistentStorageDelegate * mStorageDelegate = nullptr;
     // TODO(cecille): Make this configuarable.
     static constexpr int kMaxCommissionableNodes = 10;
     Dnssd::DiscoveredNodeData mCommissionableNodes[kMaxCommissionableNodes];
     DeviceControllerSystemState * mSystemState = nullptr;
 
-    CHIP_ERROR InitializePairedDeviceList();
-    CHIP_ERROR SetPairedDeviceList(ByteSpan pairedDeviceSerializedSet);
     ControllerDeviceInitParams GetControllerDeviceInitParams();
 
     OperationalCredentialsDelegate * mOperationalCredentialsDelegate;
@@ -575,11 +561,6 @@ class DLL_EXPORT DeviceCommissioner : public DeviceController,
     DeviceProxy * mDeviceBeingCommissioned               = nullptr;
     CommissioneeDeviceProxy * mDeviceInPASEEstablishment = nullptr;
 
-    /* This field is true when device pairing information changes, e.g. a new device is paired, or
-       the pairing for a device is removed. The DeviceCommissioner uses this to decide when to
-       persist the device list */
-    bool mPairedDevicesUpdated;
-
     CommissioningStage mCommissioningStage = CommissioningStage::kSecurePairing;
     bool mRunCommissioningAfterConnection  = false;
 

src/controller/CHIPDeviceControllerFactory.cpp

@@ -253,7 +253,6 @@ void DeviceControllerFactory::PopulateInitParams(ControllerInitParams & controll
     controllerParams.controllerNOC                  = params.controllerNOC;
     controllerParams.controllerICAC                 = params.controllerICAC;
     controllerParams.controllerRCAC                 = params.controllerRCAC;
-    controllerParams.storageDelegate                = params.storageDelegate;
 
     controllerParams.systemState        = mSystemState;
     controllerParams.controllerVendorId = params.controllerVendorId;

src/controller/CHIPDeviceControllerFactory.h

@@ -42,8 +42,6 @@ struct SetupParams
 {
     OperationalCredentialsDelegate * operationalCredentialsDelegate = nullptr;
 
-    PersistentStorageDelegate * storageDelegate = nullptr;
-
     /* The following keypair must correspond to the public key used for generating
     controllerNOC. It's used by controller to establish CASE sessions with devices */
     Crypto::P256Keypair * operationalKeypair = nullptr;

src/controller/CommissioneeDeviceProxy.h

@@ -67,7 +67,6 @@ struct ControllerDeviceInitParams
     SessionManager * sessionManager                               = nullptr;
     Messaging::ExchangeManager * exchangeMgr                      = nullptr;
     Inet::EndPointManager<Inet::UDPEndPoint> * udpEndPointManager = nullptr;
-    PersistentStorageDelegate * storageDelegate                   = nullptr;
 #if CONFIG_NETWORK_LAYER_BLE
     Ble::BleLayer * bleLayer = nullptr;
 #endif
@@ -168,8 +167,6 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat
      *   Update data of the device.
      *
      *   This function will set new IP address, port and MRP retransmission intervals of the device.
-     *   Since the device settings might have been moved from RAM to the persistent storage, the function
-     *   will load the device settings first, before making the changes.
      *
      * @param[in] addr   Address of the device to be set.
      * @param[in] config MRP parameters
@@ -190,7 +187,7 @@ class CommissioneeDeviceProxy : public DeviceProxy, public SessionReleaseDelegat
      * @brief
      * Called to indicate this proxy has been paired successfully.
      *
-     * This causes the secure session parameters to be loaded and stores the session details in the session manager.
+     * This stores the session details in the session manager.
      */
     CHIP_ERROR SetConnected();
 

src/controller/java/AndroidDeviceControllerWrapper.cpp

@@ -125,12 +125,11 @@ AndroidDeviceControllerWrapper::AllocateNew(JavaVM * vm, jobject deviceControlle
     initParams.bleLayer = DeviceLayer::ConnectivityMgr().GetBleLayer();
 #endif
     initParams.listenPort                      = CHIP_PORT + 1;
-    setupParams.storageDelegate                = wrapper.get();
     setupParams.pairingDelegate                = wrapper.get();
     setupParams.operationalCredentialsDelegate = opCredsIssuer;
-    initParams.fabricIndependentStorage        = setupParams.storageDelegate;
+    initParams.fabricIndependentStorage        = wrapper.get();
 
-    wrapper->mGroupDataProvider.SetStorageDelegate(setupParams.storageDelegate);
+    wrapper->mGroupDataProvider.SetStorageDelegate(wrapper.get());
 
     CHIP_ERROR err = wrapper->mGroupDataProvider.Init();
     if (err != CHIP_NO_ERROR)

src/controller/python/OpCredsBinding.cpp

@@ -98,7 +98,6 @@ class OperationalCredentialsAdapter : public OperationalCredentialsDelegate
 } // namespace chip
 
 extern chip::Controller::Python::StorageAdapter * pychip_Storage_GetStorageAdapter();
-extern chip::Controller::Python::StorageAdapter * sStorageAdapter;
 extern chip::Credentials::GroupDataProviderImpl sGroupDataProvider;
 extern chip::Controller::ScriptDevicePairingDelegate sPairingDelegate;
 
@@ -362,7 +361,6 @@ ChipError::StorageType pychip_OpCreds_AllocateController(OpCredsContext * contex
     VerifyOrReturnError(err == CHIP_NO_ERROR, err.AsInteger());
 
     Controller::SetupParams initParams;
-    initParams.storageDelegate                = sStorageAdapter;
     initParams.pairingDelegate                = &sPairingDelegate;
     initParams.operationalCredentialsDelegate = context->mAdapter.get();
     initParams.operationalKeypair             = &ephemeralKey;

src/controller/python/chip/internal/CommissionerImpl.cpp

@@ -137,7 +137,6 @@ extern "C" chip::Controller::DeviceCommissioner * pychip_internal_Commissioner_N
         factoryParams.groupDataProvider = &gGroupDataProvider;
 
         commissionerParams.pairingDelegate = &gPairingDelegate;
-        commissionerParams.storageDelegate = &gServerStorage;
 
         err = ephemeralKey.Initialize();
         SuccessOrExit(err);

src/darwin/Framework/CHIP/CHIPDeviceController.mm

@@ -265,7 +265,6 @@ - (BOOL)startup:(_Nullable id<CHIPPersistentStorageDelegate>)storageDelegate
 
         params.groupDataProvider = _groupDataProvider;
         params.fabricIndependentStorage = _persistentStorageDelegateBridge;
-        commissionerParams.storageDelegate = _persistentStorageDelegateBridge;
         commissionerParams.pairingDelegate = _pairingDelegateBridge;
 
         commissionerParams.operationalCredentialsDelegate = _operationalCredentialsDelegate;

src/lib/support/PersistentStorageMacros.h

@@ -27,9 +27,6 @@
 
 namespace chip {
 
-constexpr const char kPairedDeviceListKeyPrefix[] = "ListPairedDevices";
-constexpr const char kPairedDeviceKeyPrefix[]     = "PairedDevice";
-
 // This macro generates a key for storage using a node ID and a key prefix, and performs the given action
 // on that key.
 #define PERSISTENT_KEY_OP(node, keyPrefix, key, action)                                                                            \