[dataset] move Active Dataset replacement check in PendingDatasetManager (#10217)

This commit refactors the logic that determines whether to replace the
current Active Dataset with a Pending Dataset upon its delay timer
expiration.

Instead of performing the check within the more general-purpose
`DatasetManager::Save()` method, the logic is now moved directly
into `PendingDatasetManager::HandleDelayTimer()`. This change makes
the code more specific and prevents unintended checks in other
situations where `Save()` is used (e.g., when Active Dataset is
directly set).

The conditions remain the same: The Pending Dataset's Active
Timestamp must be newer, or the Pending Dataset must contain a
different network key.

This commit also adds `test-029-pending-dataset-key-change.py`
to validate the roll-back of Active Timestamp using Pending
Dataset.

src/core/meshcop/dataset.cpp

@@ -586,13 +586,6 @@ void Dataset::RemoveTlv(Tlv *aTlv)
 }
 
 Error Dataset::ApplyConfiguration(Instance &aInstance) const
-{
-    bool isNetworkKeyUpdated;
-
-    return ApplyConfiguration(aInstance, isNetworkKeyUpdated);
-}
-
-Error Dataset::ApplyConfiguration(Instance &aInstance, bool &aIsNetworkKeyUpdated) const
 {
     Mac::Mac   &mac        = aInstance.Get<Mac::Mac>();
     KeyManager &keyManager = aInstance.Get<KeyManager>();
@@ -600,8 +593,6 @@ Error Dataset::ApplyConfiguration(Instance &aInstance, bool &aIsNetworkKeyUpdate
 
     SuccessOrExit(error = ValidateTlvs());
 
-    aIsNetworkKeyUpdated = false;
-
     for (const Tlv *cur = GetTlvsStart(); cur < GetTlvsEnd(); cur = cur->GetNext())
     {
         switch (cur->GetType())
@@ -634,19 +625,8 @@ Error Dataset::ApplyConfiguration(Instance &aInstance, bool &aIsNetworkKeyUpdate
             break;
 
         case Tlv::kNetworkKey:
-        {
-            NetworkKey networkKey;
-
-            keyManager.GetNetworkKey(networkKey);
-
-            if (cur->ReadValueAs<NetworkKeyTlv>() != networkKey)
-            {
-                aIsNetworkKeyUpdated = true;
-            }
-
             keyManager.SetNetworkKey(cur->ReadValueAs<NetworkKeyTlv>());
             break;
-        }
 
 #if OPENTHREAD_FTD
         case Tlv::kPskc:
@@ -671,12 +651,6 @@ exit:
     return error;
 }
 
-void Dataset::ConvertToActive(void)
-{
-    RemoveTlv(Tlv::kPendingTimestamp);
-    RemoveTlv(Tlv::kDelayTimer);
-}
-
 const char *Dataset::TypeToString(Type aType) { return (aType == kActive) ? "Active" : "Pending"; }
 
 #if OPENTHREAD_CONFIG_PLATFORM_KEY_REFERENCES_ENABLE

src/core/meshcop/dataset.hpp

@@ -647,26 +647,6 @@ public:
      */
     Error ApplyConfiguration(Instance &aInstance) const;
 
-    /**
-     * Applies the Active or Pending Dataset to the Thread interface.
-     *
-     * @param[in]  aInstance              A reference to the OpenThread instance.
-     * @param[out] aIsNetworkKeyUpdated   Variable to return whether network key was updated.
-     *
-     * @retval kErrorNone   Successfully applied configuration, @p aIsNetworkKeyUpdated is changed.
-     * @retval kErrorParse  The dataset has at least one TLV with invalid format.
-     *
-     */
-    Error ApplyConfiguration(Instance &aInstance, bool &aIsNetworkKeyUpdated) const;
-
-    /**
-     * Converts a Pending Dataset to an Active Dataset.
-     *
-     * Removes the Delay Timer and Pending Timestamp TLVs.
-     *
-     */
-    void ConvertToActive(void);
-
     /**
      * Returns a pointer to the start of Dataset TLVs sequence.
      *

src/core/meshcop/dataset_manager.cpp

@@ -143,11 +143,10 @@ exit:
     return error;
 }
 
-Error DatasetManager::Save(const Dataset &aDataset)
+Error DatasetManager::Save(const Dataset &aDataset, bool aAllowOlderTimestamp)
 {
     Error error = kErrorNone;
     int   compare;
-    bool  isNetworkKeyUpdated = false;
 
     if (aDataset.ReadTimestamp(GetType(), mTimestamp) == kErrorNone)
     {
@@ -155,13 +154,13 @@ Error DatasetManager::Save(const Dataset &aDataset)
 
         if (IsActiveDataset())
         {
-            SuccessOrExit(error = aDataset.ApplyConfiguration(GetInstance(), isNetworkKeyUpdated));
+            SuccessOrExit(error = aDataset.ApplyConfiguration(GetInstance()));
         }
     }
 
     compare = Timestamp::Compare(mTimestampValid ? &mTimestamp : nullptr, mLocal.GetTimestamp());
 
-    if (isNetworkKeyUpdated || compare > 0)
+    if ((compare > 0) || aAllowOlderTimestamp)
     {
         mLocal.Save(aDataset);
 
@@ -669,15 +668,45 @@ exit:
 
 void PendingDatasetManager::HandleDelayTimer(void)
 {
-    Dataset dataset;
+    Dataset   dataset;
+    Timestamp activeTimestamp;
+    bool      shouldReplaceActive = false;
 
     IgnoreError(Read(dataset));
-    LogInfo("pending delay timer expired");
+    LogInfo("Pending delay timer expired");
 
-    dataset.ConvertToActive();
+    // Determine whether the Pending Dataset should replace the
+    // current Active Dataset. This is allowed if the Pending
+    // Dataset's Active Timestamp is newer, or the Pending Dataset
+    // contains a different key.
 
-    IgnoreError(Get<ActiveDatasetManager>().Save(dataset));
+    SuccessOrExit(dataset.Read<ActiveTimestampTlv>(activeTimestamp));
 
+    if (Timestamp::Compare(&activeTimestamp, Get<ActiveDatasetManager>().GetTimestamp()) > 0)
+    {
+        shouldReplaceActive = true;
+    }
+    else
+    {
+        NetworkKey newKey;
+        NetworkKey currentKey;
+
+        SuccessOrExit(dataset.Read<NetworkKeyTlv>(newKey));
+        Get<KeyManager>().GetNetworkKey(currentKey);
+        shouldReplaceActive = (currentKey != newKey);
+    }
+
+    VerifyOrExit(shouldReplaceActive);
+
+    // Convert Pending Dataset to Active by removing the Pending
+    // Timestamp and the Delay Timer TLVs.
+
+    dataset.RemoveTlv(Tlv::kPendingTimestamp);
+    dataset.RemoveTlv(Tlv::kDelayTimer);
+
+    IgnoreError(Get<ActiveDatasetManager>().Save(dataset, /* aAllowOlderTimestamp */ true));
+
+exit:
     Clear();
 }
 

src/core/meshcop/dataset_manager.hpp

@@ -51,8 +51,12 @@ namespace ot {
 
 namespace MeshCoP {
 
+class PendingDatasetManager;
+
 class DatasetManager : public InstanceLocator
 {
+    friend class PendingDatasetManager;
+
 public:
     /**
      * Callback function pointer, invoked when a response to a MGMT_SET request is received or times out.
@@ -149,7 +153,7 @@ public:
      * @retval kErrorParse  The dataset has at least one TLV with invalid format.
      *
      */
-    Error Save(const Dataset &aDataset);
+    Error Save(const Dataset &aDataset) { return Save(aDataset, /* aAllowOlderTimestamp */ false); }
 
     /**
      * Sets the Operational Dataset for the partition read from a given message.
@@ -347,6 +351,7 @@ private:
 
     bool  IsActiveDataset(void) const { return GetType() == Dataset::kActive; }
     bool  IsPendingDataset(void) const { return GetType() == Dataset::kPending; }
+    Error Save(const Dataset &aDataset, bool aAllowOlderTimestamp);
     void  SignalDatasetChange(void) const;
     void  SyncLocalWithLeader(const Dataset &aDataset);
     Error SendSetRequest(const Dataset &aDataset);

tests/toranj/cli/test-029-pending-dataset-key-change.py

@@ -0,0 +1,127 @@
+#!/usr/bin/env python3
+#
+#  Copyright (c) 2024, The OpenThread Authors.
+#  All rights reserved.
+#
+#  Redistribution and use in source and binary forms, with or without
+#  modification, are permitted provided that the following conditions are met:
+#  1. Redistributions of source code must retain the above copyright
+#     notice, this list of conditions and the following disclaimer.
+#  2. Redistributions in binary form must reproduce the above copyright
+#     notice, this list of conditions and the following disclaimer in the
+#     documentation and/or other materials provided with the distribution.
+#  3. Neither the name of the copyright holder nor the
+#     names of its contributors may be used to endorse or promote products
+#     derived from this software without specific prior written permission.
+#
+#  THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
+#  AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+#  IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+#  ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
+#  LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+#  CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+#  SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+#  INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+#  CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+#  ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
+#  POSSIBILITY OF SUCH DAMAGE.
+
+from cli import verify
+from cli import verify_within
+import cli
+import time
+
+# -----------------------------------------------------------------------------------------------------------------------
+# Test description:
+#
+# This test validates the rollback of the Active Timestamp when
+# applying a Pending Dataset. This rollback is permitted only when
+# the Network Key changes.
+
+test_name = __file__[:-3] if __file__.endswith('.py') else __file__
+print('-' * 120)
+print('Starting \'{}\''.format(test_name))
+
+# -----------------------------------------------------------------------------------------------------------------------
+# Creating `cli.Node` instances
+
+speedup = 20
+cli.Node.set_time_speedup_factor(speedup)
+
+leader = cli.Node()
+router = cli.Node()
+
+# -----------------------------------------------------------------------------------------------------------------------
+# Form topology
+
+leader.form('network')
+router.join(leader)
+
+verify(leader.get_state() == 'leader')
+verify(router.get_state() == 'router')
+
+# -----------------------------------------------------------------------------------------------------------------------
+# Test Implementation
+
+# Use Pending Dataset to change network name and update
+# Active Timestamp to 10.
+
+router.cli('dataset init active')
+router.cli('dataset activetimestamp 10')
+router.cli('dataset pendingtimestamp 10')
+router.cli('dataset networkname new')
+router.cli('dataset delaytimer 1000')
+router.cli('dataset commit pending')
+
+time.sleep(1.2 / speedup)
+
+# Validate that Active Dataset is updated.
+
+leader.cli('dataset init active')
+verify(leader.get_network_name() == 'new')
+verify(leader.cli('dataset activetimestamp') == ['10'])
+
+#- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+# Use Pending Dataset with an older Active Timestamp.
+# Ensure that the new dataset is not accepted.
+
+router.cli('dataset init active')
+router.cli('dataset activetimestamp 5')
+router.cli('dataset pendingtimestamp 15')
+router.cli('dataset networkname shouldfail')
+router.cli('dataset delaytimer 1000')
+router.cli('dataset commit pending')
+
+time.sleep(1.2 / speedup)
+
+leader.cli('dataset init active')
+verify(leader.get_network_name() == 'new')
+verify(leader.cli('dataset activetimestamp') == ['10'])
+
+#- - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - -
+# Use Pending Dataset with an older Active Timestamp but
+# also change the Network Key.
+
+router.cli('dataset init active')
+router.cli('dataset activetimestamp 7')
+router.cli('dataset pendingtimestamp 20')
+router.cli('dataset networkkey 00112233445566778899aabbccddeeff')
+router.cli('dataset delaytimer 1000')
+router.cli('dataset commit pending')
+
+time.sleep(1.2 / speedup)
+
+# Validate that the Active Dataset is updated and
+# Active Timestamp is rolled back.
+
+leader.cli('dataset init active')
+leader.cli('dataset')
+verify(leader.cli('dataset activetimestamp') == ['7'])
+verify(leader.get_network_key() == '00112233445566778899aabbccddeeff')
+
+# -----------------------------------------------------------------------------------------------------------------------
+# Test finished
+
+cli.Node.finalize_all_nodes()
+
+print('\'{}\' passed.'.format(test_name))

tests/toranj/openthread-core-toranj-config.h

@@ -161,6 +161,7 @@
 
 // For `toranj` test script the value is decreased so that the tests can be run faster.
 #define OPENTHREAD_CONFIG_TMF_PENDING_DATASET_MINIMUM_DELAY 1000
+#define OPENTHREAD_CONFIG_TMF_PENDING_DATASET_DEFAULT_DELAY 1000
 
 #define OPENTHREAD_CONFIG_NCP_ENABLE_MCU_POWER_STATE_CONTROL 1
 

tests/toranj/start.sh

@@ -193,6 +193,7 @@ if [ "$TORANJ_CLI" = 1 ]; then
     run cli/test-026-coaps-conn-limit.py
     run cli/test-027-slaac-address.py
     run cli/test-028-border-agent-ephemeral-key.py
+    run cli/test-029-pending-dataset-key-change.py
     run cli/test-400-srp-client-server.py
     run cli/test-401-srp-server-address-cache-snoop.py
     run cli/test-500-two-brs-two-networks.py