Persist and reload message counters on the controller

src/controller/CHIPDevice.cpp

@@ -46,9 +46,12 @@
 #include <support/CHIPMem.h>
 #include <support/CodeUtils.h>
 #include <support/ErrorStr.h>
+#include <support/PersistentStorageMacros.h>
 #include <support/SafeInt.h>
 #include <support/logging/CHIPLogging.h>
 #include <system/TLVPacketBufferBackingStore.h>
+#include <transport/MessageCounter.h>
+#include <transport/PeerMessageCounter.h>
 
 using namespace chip::Inet;
 using namespace chip::System;
@@ -150,8 +153,10 @@ CHIP_ERROR Device::SendCommands(app::CommandSender * commandObj)
 
 CHIP_ERROR Device::Serialize(SerializedDevice & output)
 {
-    CHIP_ERROR error       = CHIP_NO_ERROR;
-    uint16_t serializedLen = 0;
+    CHIP_ERROR error             = CHIP_NO_ERROR;
+    uint16_t serializedLen       = 0;
+    uint32_t localMessageCounter = 0;
+    uint32_t peerMessageCounter  = 0;
     SerializableDevice serializable;
 
     static_assert(BASE64_ENCODED_LEN(sizeof(serializable)) <= sizeof(output.inner),
@@ -164,6 +169,14 @@ CHIP_ERROR Device::Serialize(SerializedDevice & output)
     serializable.mDevicePort = Encoding::LittleEndian::HostSwap16(mDeviceAddress.GetPort());
     serializable.mAdminId    = Encoding::LittleEndian::HostSwap16(mAdminId);
 
+    Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession);
+    VerifyOrExit(connectionState != nullptr, error = CHIP_ERROR_INCORRECT_STATE);
+    localMessageCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter().Value();
+    peerMessageCounter  = connectionState->GetSessionMessageCounter().GetPeerMessageCounter().GetCounter();
+
+    serializable.mLocalMessageCounter = Encoding::LittleEndian::HostSwap32(localMessageCounter);
+    serializable.mPeerMessageCounter  = Encoding::LittleEndian::HostSwap32(peerMessageCounter);
+
     serializable.mCASESessionKeyId           = Encoding::LittleEndian::HostSwap16(mCASESessionKeyId);
     serializable.mDeviceProvisioningComplete = (mDeviceProvisioningComplete) ? 1 : 0;
 
@@ -215,10 +228,12 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input)
         IPAddress::FromString(Uint8::to_const_char(serializable.mDeviceAddr), sizeof(serializable.mDeviceAddr) - 1, ipAddress),
         error = CHIP_ERROR_INVALID_ADDRESS);
 
-    mPairing  = serializable.mOpsCreds;
-    mDeviceId = Encoding::LittleEndian::HostSwap64(serializable.mDeviceId);
-    port      = Encoding::LittleEndian::HostSwap16(serializable.mDevicePort);
-    mAdminId  = Encoding::LittleEndian::HostSwap16(serializable.mAdminId);
+    mPairing             = serializable.mOpsCreds;
+    mDeviceId            = Encoding::LittleEndian::HostSwap64(serializable.mDeviceId);
+    port                 = Encoding::LittleEndian::HostSwap16(serializable.mDevicePort);
+    mAdminId             = Encoding::LittleEndian::HostSwap16(serializable.mAdminId);
+    mLocalMessageCounter = Encoding::LittleEndian::HostSwap32(serializable.mLocalMessageCounter);
+    mPeerMessageCounter  = Encoding::LittleEndian::HostSwap32(serializable.mPeerMessageCounter);
 
     mCASESessionKeyId           = Encoding::LittleEndian::HostSwap16(serializable.mCASESessionKeyId);
     mDeviceProvisioningComplete = (serializable.mDeviceProvisioningComplete != 0);
@@ -258,10 +273,38 @@ CHIP_ERROR Device::Deserialize(const SerializedDevice & input)
     return error;
 }
 
+CHIP_ERROR Device::Persist()
+{
+    CHIP_ERROR error = CHIP_NO_ERROR;
+    if (mStorageDelegate != nullptr)
+    {
+        SerializedDevice serialized;
+        Serialize(serialized);
+
+        // TODO: no need to base-64 the serialized values AGAIN
+        PERSISTENT_KEY_OP(GetDeviceId(), kPairedDeviceKeyPrefix, key,
+                          error = mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner)));
+    }
+    return error;
+}
+
 void Device::OnNewConnection(SecureSessionHandle session)
 {
     mState         = ConnectionState::SecureConnected;
     mSecureSession = session;
+
+    // Reset the message counters here because this is the first time we get a handle to the secure session.
+    // Since CHIPDevices can be serialized/deserialized in the middle of what is conceptually a single PASE session
+    // we need to restore the session counters along with the the session information.
+    Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession);
+    VerifyOrReturn(connectionState != nullptr);
+    MessageCounter & localCounter = connectionState->GetSessionMessageCounter().GetLocalMessageCounter();
+    if (localCounter.SetCounter(mLocalMessageCounter))
+    {
+        ChipLogError(Controller, "Unable to restore local counter to %d", mLocalMessageCounter);
+    };
+    Transport::PeerMessageCounter & peerCounter = connectionState->GetSessionMessageCounter().GetPeerMessageCounter();
+    peerCounter.SetCounter(mPeerMessageCounter);
 }
 
 void Device::OnConnectionExpired(SecureSessionHandle session)
@@ -470,5 +513,24 @@ void Device::AddReportHandler(EndpointId endpoint, ClusterId cluster, AttributeI
     mCallbacksMgr.AddReportCallback(mDeviceId, endpoint, cluster, attribute, onReportCallback);
 }
 
+Device::~Device()
+{
+    if (mExchangeMgr)
+    {
+        // Ensure that any exchange contexts we have open get closed now,
+        // because we don't want them to call back in to us after this
+        // point.
+        mExchangeMgr->CloseAllContextsForDelegate(this);
+    }
+
+    if (mStorageDelegate != nullptr && mSessionManager != nullptr)
+    {
+        // If a session can be found, persist the device so that we track the newest message counter values
+        Transport::PeerConnectionState * connectionState = mSessionManager->GetPeerConnectionState(mSecureSession);
+        VerifyOrReturn(connectionState != nullptr);
+        Persist();
+    }
+}
+
 } // namespace Controller
 } // namespace chip

src/controller/CHIPDevice.h

@@ -73,11 +73,11 @@ using DeviceTransportMgr = TransportMgr<Transport::UDP /* IPv6 */
 
 struct ControllerDeviceInitParams
 {
-    DeviceTransportMgr * transportMgr        = nullptr;
-    SecureSessionMgr * sessionMgr            = nullptr;
-    Messaging::ExchangeManager * exchangeMgr = nullptr;
-    Inet::InetLayer * inetLayer              = nullptr;
-
+    DeviceTransportMgr * transportMgr                   = nullptr;
+    SecureSessionMgr * sessionMgr                       = nullptr;
+    Messaging::ExchangeManager * exchangeMgr            = nullptr;
+    Inet::InetLayer * inetLayer                         = nullptr;
+    PersistentStorageDelegate * storageDelegate         = nullptr;
     Credentials::OperationalCredentialSet * credentials = nullptr;
 #if CONFIG_NETWORK_LAYER_BLE
     Ble::BleLayer * bleLayer = nullptr;
@@ -87,16 +87,7 @@ struct ControllerDeviceInitParams
 class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEstablishmentDelegate
 {
 public:
-    ~Device()
-    {
-        if (mExchangeMgr)
-        {
-            // Ensure that any exchange contexts we have open get closed now,
-            // because we don't want them to call back in to us after this
-            // point.
-            mExchangeMgr->CloseAllContextsForDelegate(this);
-        }
-    }
+    ~Device();
 
     enum class PairingWindowOption
     {
@@ -173,13 +164,14 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
      */
     void Init(ControllerDeviceInitParams params, uint16_t listenPort, Transport::AdminId admin)
     {
-        mTransportMgr   = params.transportMgr;
-        mSessionManager = params.sessionMgr;
-        mExchangeMgr    = params.exchangeMgr;
-        mInetLayer      = params.inetLayer;
-        mListenPort     = listenPort;
-        mAdminId        = admin;
-        mCredentials    = params.credentials;
+        mTransportMgr    = params.transportMgr;
+        mSessionManager  = params.sessionMgr;
+        mExchangeMgr     = params.exchangeMgr;
+        mInetLayer       = params.inetLayer;
+        mListenPort      = listenPort;
+        mAdminId         = admin;
+        mStorageDelegate = params.storageDelegate;
+        mCredentials     = params.credentials;
 #if CONFIG_NETWORK_LAYER_BLE
         mBleLayer = params.bleLayer;
 #endif
@@ -226,6 +218,13 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
      **/
     CHIP_ERROR Deserialize(const SerializedDevice & input);
 
+    /**
+     * @brief Serialize and store the Device in persistent storage
+     *
+     * @return Returns a CHIP_ERROR on error, CHIP_NO_ERROR otherwise
+     */
+    CHIP_ERROR Persist();
+
     /**
      * @brief
      *   Called when a new pairing is being established
@@ -406,6 +405,10 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
 
     uint8_t mSequenceNumber = 0;
 
+    // Message counts start at 1
+    uint32_t mLocalMessageCounter = 1;
+    uint32_t mPeerMessageCounter  = 1;
+
     app::CHIPDeviceCallbacksMgr & mCallbacksMgr = app::CHIPDeviceCallbacksMgr::GetInstance();
 
     /**
@@ -440,6 +443,8 @@ class DLL_EXPORT Device : public Messaging::ExchangeDelegate, public SessionEsta
     uint16_t mCASESessionKeyId = 0;
 
     Credentials::OperationalCredentialSet * mCredentials = nullptr;
+
+    PersistentStorageDelegate * mStorageDelegate = nullptr;
 };
 
 /**
@@ -494,6 +499,8 @@ typedef struct SerializableDevice
     uint8_t mDeviceTransport;
     uint8_t mDeviceProvisioningComplete;
     uint8_t mInterfaceName[kMaxInterfaceName];
+    uint32_t mLocalMessageCounter; /* This field is serialized in LittleEndian byte order */
+    uint32_t mPeerMessageCounter;  /* This field is serialized in LittleEndian byte order */
 } SerializableDevice;
 
 typedef struct SerializedDevice

src/controller/CHIPDeviceController.cpp

@@ -53,6 +53,7 @@
 #include <support/CHIPMem.h>
 #include <support/CodeUtils.h>
 #include <support/ErrorStr.h>
+#include <support/PersistentStorageMacros.h>
 #include <support/SafeInt.h>
 #include <support/ScopedBuffer.h>
 #include <support/TimeUtils.h>
@@ -89,10 +90,6 @@ namespace Controller {
 
 using namespace chip::Encoding;
 
-constexpr const char kPairedDeviceListKeyPrefix[] = "ListPairedDevices";
-constexpr const char kPairedDeviceKeyPrefix[]     = "PairedDevice";
-constexpr const char kNextAvailableKeyID[]        = "StartKeyID";
-
 #if CHIP_DEVICE_CONFIG_ENABLE_MDNS
 constexpr uint16_t kMdnsPort = 5353;
 #endif
@@ -103,20 +100,6 @@ constexpr uint32_t kMaxCHIPOpCertLength = 1024;
 constexpr uint32_t kMaxCHIPCSRLength    = 1024;
 constexpr uint32_t kOpCSRNonceLength    = 32;
 
-// This macro generates a key using node ID an key prefix, and performs the given action
-// on that key.
-#define PERSISTENT_KEY_OP(node, keyPrefix, key, action)                                                                            \
-    do                                                                                                                             \
-    {                                                                                                                              \
-        constexpr size_t len = std::extent<decltype(keyPrefix)>::value;                                                            \
-        nlSTATIC_ASSERT_PRINT(len > 0, "keyPrefix length must be known at compile time");                                          \
-        /* 2 * sizeof(NodeId) to accomodate 2 character for each byte in Node Id */                                                \
-        char key[len + 2 * sizeof(NodeId) + 1];                                                                                    \
-        nlSTATIC_ASSERT_PRINT(sizeof(node) <= sizeof(uint64_t), "Node ID size is greater than expected");                          \
-        snprintf(key, sizeof(key), "%s%" PRIx64, keyPrefix, node);                                                                 \
-        action;                                                                                                                    \
-    } while (0)
-
 DeviceController::DeviceController()
 {
     mState                    = State::NotInitialized;
@@ -485,19 +468,9 @@ CHIP_ERROR DeviceController::UpdateDevice(Device * device, uint64_t fabricId)
 
 void DeviceController::PersistDevice(Device * device)
 {
-    // mStorageDelegate would not be null for a typical pairing scenario, as Pair()
-    // requires a valid storage delegate. However, test pairing usecase, that's used
-    // mainly by test applications, do not require a storage delegate. This is to
-    // reduce overheads on these tests.
-    // Let's make sure the delegate object is available before calling into it.
-    if (mStorageDelegate != nullptr && mState == State::Initialized)
+    if (mState == State::Initialized)
     {
-        SerializedDevice serialized;
-        device->Serialize(serialized);
-
-        // TODO: no need to base-64 the serialized values AGAIN
-        PERSISTENT_KEY_OP(device->GetDeviceId(), kPairedDeviceKeyPrefix, key,
-                          mStorageDelegate->SyncSetKeyValue(key, serialized.inner, sizeof(serialized.inner)));
+        device->Persist();
     }
 }
 
@@ -776,11 +749,14 @@ void DeviceController::OnCommissionableNodeFound(const chip::Mdns::Commissionabl
 
 ControllerDeviceInitParams DeviceController::GetControllerDeviceInitParams()
 {
-    return ControllerDeviceInitParams{ .transportMgr = mTransportMgr,
-                                       .sessionMgr   = mSessionMgr,
-                                       .exchangeMgr  = mExchangeMgr,
-                                       .inetLayer    = mInetLayer,
-                                       .credentials  = &mCredentials };
+    return ControllerDeviceInitParams{
+        .transportMgr    = mTransportMgr,
+        .sessionMgr      = mSessionMgr,
+        .exchangeMgr     = mExchangeMgr,
+        .inetLayer       = mInetLayer,
+        .storageDelegate = mStorageDelegate,
+        .credentials     = &mCredentials,
+    };
 }
 
 DeviceCommissioner::DeviceCommissioner() :

src/darwin/CHIPTool/CHIPTool/View Controllers/QRCode/QRCodeViewController.m

@@ -392,10 +392,9 @@ - (void)onPairingComplete:(NSError *)error
     if (error.code != CHIPSuccess) {
         NSLog(@"Got pairing error back %@", error);
     } else {
-        dispatch_after(dispatch_time(DISPATCH_TIME_NOW, DISPATCH_TIME_NOW), dispatch_get_main_queue(), ^{
+        dispatch_async(dispatch_get_main_queue(), ^{
             [self->_deviceList refreshDeviceList];
             [self retrieveAndSendWifiCredentials];
-            [self setVendorIDOnAccessory];
         });
     }
 }
@@ -623,6 +622,7 @@ - (void)onAddressUpdated:(NSError *)error
         NSLog(@"Error retrieving device informations over Mdns: %@", error);
         return;
     }
+    [self setVendorIDOnAccessory];
 }
 
 - (void)updateUIFields:(CHIPSetupPayload *)payload decimalString:(nullable NSString *)decimalString

src/lib/support/BUILD.gn

@@ -77,6 +77,7 @@ static_library("support") {
     "LifetimePersistedCounter.h",
     "PersistedCounter.cpp",
     "PersistedCounter.h",
+    "PersistentStorageMacros.h",
     "Pool.cpp",
     "Pool.h",
     "PrivateHeap.cpp",

src/lib/support/PersistentStorageMacros.h

@@ -0,0 +1,48 @@
+/*
+ *
+ *    Copyright (c) 2021 Project CHIP Authors
+ *
+ *    Licensed under the Apache License, Version 2.0 (the "License");
+ *    you may not use this file except in compliance with the License.
+ *    You may obtain a copy of the License at
+ *
+ *        http://www.apache.org/licenses/LICENSE-2.0
+ *
+ *    Unless required by applicable law or agreed to in writing, software
+ *    distributed under the License is distributed on an "AS IS" BASIS,
+ *    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ *    See the License for the specific language governing permissions and
+ *    limitations under the License.
+ */
+
+/**
+ *    @file
+ *      This file defines and implements some utlities for generating persistent storage keys
+ *
+ */
+
+#pragma once
+
+#include <support/CodeUtils.h>
+
+namespace chip {
+
+constexpr const char kPairedDeviceListKeyPrefix[] = "ListPairedDevices";
+constexpr const char kPairedDeviceKeyPrefix[]     = "PairedDevice";
+constexpr const char kNextAvailableKeyID[]        = "StartKeyID";
+
+// 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)                                                                            \
+    do                                                                                                                             \
+    {                                                                                                                              \
+        constexpr size_t len = std::extent<decltype(keyPrefix)>::value;                                                            \
+        nlSTATIC_ASSERT_PRINT(len > 0, "keyPrefix length must be known at compile time");                                          \
+        /* 2 * sizeof(NodeId) to accomodate 2 character for each byte in Node Id */                                                \
+        char key[len + 2 * sizeof(NodeId) + 1];                                                                                    \
+        nlSTATIC_ASSERT_PRINT(sizeof(node) <= sizeof(uint64_t), "Node ID size is greater than expected");                          \
+        snprintf(key, sizeof(key), "%s%" PRIx64, keyPrefix, node);                                                                 \
+        action;                                                                                                                    \
+    } while (0)
+
+} // namespace chip

src/messaging/ExchangeMgr.cpp

@@ -198,8 +198,8 @@ void ExchangeManager::OnMessageReceived(const PacketHeader & packetHeader, const
     UnsolicitedMessageHandler * matchingUMH = nullptr;
     bool sendAckAndCloseExchange            = false;
 
-    ChipLogProgress(ExchangeManager, "Received message of type %d and protocolId %d", payloadHeader.GetMessageType(),
-                    payloadHeader.GetProtocolID());
+    ChipLogProgress(ExchangeManager, "Received message of type %d and protocolId %d on exchange %d", payloadHeader.GetMessageType(),
+                    payloadHeader.GetProtocolID(), payloadHeader.GetExchangeID());
 
     // Search for an existing exchange that the message applies to. If a match is found...
     bool found = false;