Improve size calculation for our packets. (#28563)

* Improve size calculation for our packets.

We were hardcoding kMaxAppMessageLen and kMaxSecureSduLengthBytes, but those
defines were not even consistent with each other, and both were overly
conservative.  Specific changes:

1. Fix computation of CHIP_SYSTEM_HEADER_RESERVE_SIZE: we don't have any "crypto
   headers".
2. Fix computation of kMaxAppMessageLen to use our actual packet buffer sizes
   and defined header sizes.
3. Fix kMaxSecureSduLengthBytes to be consistent with kMaxAppMessageLen.

* Simplify the size computation logic.

src/app/StatusResponse.h

@@ -22,10 +22,11 @@
 #include <messaging/ExchangeContext.h>
 #include <protocols/interaction_model/Constants.h>
 #include <system/TLVPacketBufferBackingStore.h>
+#include <transport/raw/MessageHeader.h>
 
 namespace chip {
 namespace app {
-static constexpr size_t kMaxSecureSduLengthBytes = 1024;
+static constexpr size_t kMaxSecureSduLengthBytes = kMaxAppMessageLen + kMaxTagLen;
 
 class StatusResponse
 {

src/app/tests/TestReadInteraction.cpp

@@ -67,6 +67,9 @@ chip::EndpointId kInvalidTestEndpointId = 3;
 chip::DataVersion kTestDataVersion1     = 3;
 chip::DataVersion kTestDataVersion2     = 5;
 
+// Number of items in the list for MockAttributeId(4).
+constexpr int kMockAttribute4ListLength = 6;
+
 static chip::System::Clock::Internal::MockClock gMockClock;
 static chip::System::Clock::ClockBase * gRealClock;
 
@@ -1214,7 +1217,8 @@ void TestReadInteraction::TestReadChunking(nlTestSuite * apSuite, void * apConte
     NL_TEST_ASSERT(apSuite, !delegate.mGotEventResponse);
 
     chip::app::AttributePathParams attributePathParams[1];
-    // Mock Attribute 4 is a big attribute, with 6 large OCTET_STRING
+    // Mock Attribute 4 is a big attribute, with kMockAttribute4ListLength large
+    // OCTET_STRING elements.
     attributePathParams[0].mEndpointId  = Test::kMockEndpoint3;
     attributePathParams[0].mClusterId   = Test::MockClusterId(2);
     attributePathParams[0].mAttributeId = Test::MockAttributeId(4);
@@ -1234,8 +1238,10 @@ void TestReadInteraction::TestReadChunking(nlTestSuite * apSuite, void * apConte
 
         ctx.DrainAndServiceIO();
 
-        // We get one chunk with 3 array elements, and then one chunk per element.
-        NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 4);
+        // We get one chunk with 4 array elements, and then one chunk per
+        // element, and the total size of the array is
+        // kMockAttribute4ListLength.
+        NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 1 + (kMockAttribute4ListLength - 4));
         NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 6);
         NL_TEST_ASSERT(apSuite, delegate.mGotReport);
         NL_TEST_ASSERT(apSuite, !delegate.mReadError);
@@ -1380,13 +1386,16 @@ void TestReadInteraction::TestSetDirtyBetweenChunks(nlTestSuite * apSuite, void
 
         ctx.DrainAndServiceIO();
 
-        // We should receive another (3 + 1) = 4 attribute reports represeting 6
-        // array items, since the underlying path iterator should be reset to
-        // the beginning of the cluster it is currently iterating.
+        // Our list has length kMockAttribute4ListLength.  Since the underlying
+        // path iterator should be reset to the beginning of the cluster it is
+        // currently iterating, we expect to get another value for our
+        // attribute.  The way the packet boundaries happen to fall, that value
+        // will encode 4 items in the first IB and then one IB per item.
+        const int expectedIBs = 1 + (kMockAttribute4ListLength - 4);
         ChipLogError(DataManagement, "OLD: %d\n", currentAttributeResponsesWhenSetDirty);
         ChipLogError(DataManagement, "NEW: %d\n", delegate.mNumAttributeResponse);
-        NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == currentAttributeResponsesWhenSetDirty + 4);
-        NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == currentArrayItemsWhenSetDirty + 6);
+        NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == currentAttributeResponsesWhenSetDirty + expectedIBs);
+        NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == currentArrayItemsWhenSetDirty + kMockAttribute4ListLength);
         NL_TEST_ASSERT(apSuite, delegate.mGotReport);
         NL_TEST_ASSERT(apSuite, !delegate.mReadError);
         // By now we should have closed all exchanges and sent all pending acks, so
@@ -2341,7 +2350,7 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
         // paths, for a total of 58 attributes.
         //
         // Attribute 0xFFFC::0xFFF1'FC02::0xFFF1'0004 (kMockEndpoint3::MockClusterId(2)::MockAttributeId(4))
-        // is a list of 6 elements of size 256 bytes each, which cannot fit in a single
+        // is a list of kMockAttribute4ListLength elements of size 256 bytes each, which cannot fit in a single
         // packet, so gets list chunking applied to it.
         //
         // Because delegate.mNumAttributeResponse counts AttributeDataIB instances, not attributes,
@@ -2350,21 +2359,16 @@ void TestReadInteraction::TestSubscribeWildcard(nlTestSuite * apSuite, void * ap
         // in the response, there will be one AttributeDataIB for the start of the list (which will include
         // some number of 256-byte elements), then one AttributeDataIB for each of the remaining elements.
         //
-        // When EventList is enabled, for the first report for the list attribute we receive two
-        // of its items in the initial list, then 4 additional items.  For the second report we
-        // receive 3 items in the initial list followed by 3 additional items.
-        //
-        // Thus we should receive 29*2 + 4 + 3 = 65 attribute data in total.
-        constexpr size_t kExpectedAttributeResponse = 65;
+        // When EventList is enabled, for the first report for the list attribute we receive three
+        // of its items in the initial list, then the remaining items.  For the second report we
+        // receive 2 items in the initial list followed by the remaining items.
+        constexpr size_t kExpectedAttributeResponse = 29 * 2 + (kMockAttribute4ListLength - 3) + (kMockAttribute4ListLength - 2);
 #else
         // When EventList is not enabled, the packet boundaries shift and for the first
-        // report for the list attribute we receive two of its items in the initial list,
-        // then 4 additional items.  For the second report we receive 3 items in
-        // the initial list followed by 3 additional items.
-        //
-        // Thus we should receive 29*2 + 4 + 3 = 65 attribute data when the eventlist
-        // attribute is not available.
-        constexpr size_t kExpectedAttributeResponse = 65;
+        // report for the list attribute we receive four of its items in the initial list,
+        // then additional items.  For the second report we receive 4 items in
+        // the initial list followed by additional items.
+        constexpr size_t kExpectedAttributeResponse = 29 * 2 + (kMockAttribute4ListLength - 4) + (kMockAttribute4ListLength - 4);
 #endif
         NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == kExpectedAttributeResponse);
         NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 12);
@@ -3460,9 +3464,10 @@ void TestReadInteraction::TestPostSubscribeRoundtripChunkReport(nlTestSuite * ap
             gMockClock.AdvanceMonotonic(System::Clock::Milliseconds32(10));
         }
     }
-    // Two chunked reports carry 4 attributeDataIB: 1 with a list of 3 items,
-    // and then one per remaining item.
-    NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 4);
+    // We get one chunk with 4 array elements, and then one chunk per
+    // element, and the total size of the array is
+    // kMockAttribute4ListLength.
+    NL_TEST_ASSERT(apSuite, delegate.mNumAttributeResponse == 1 + (kMockAttribute4ListLength - 4));
     NL_TEST_ASSERT(apSuite, delegate.mNumArrayItems == 6);
 
     NL_TEST_ASSERT(apSuite, engine->GetNumActiveReadClients() == 0);

src/app/tests/TestWriteInteraction.cpp

@@ -579,8 +579,9 @@ void TestWriteInteraction::TestWriteHandlerReceiveInvalidMessage(nlTestSuite * a
     err           = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), app::reporting::GetDefaultReportScheduler());
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
+    // Reserve all except the last 128 bytes, so that we make sure to chunk.
     app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
-                                 static_cast<uint16_t>(900) /* reserved buffer size */);
+                                 static_cast<uint16_t>(kMaxSecureSduLengthBytes - 128) /* reserved buffer size */);
 
     ByteSpan list[5];
 
@@ -647,8 +648,9 @@ void TestWriteInteraction::TestWriteHandlerInvalidateFabric(nlTestSuite * apSuit
     err           = engine->Init(&ctx.GetExchangeManager(), &ctx.GetFabricTable(), app::reporting::GetDefaultReportScheduler());
     NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
+    // Reserve all except the last 128 bytes, so that we make sure to chunk.
     app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
-                                 static_cast<uint16_t>(900) /* reserved buffer size */);
+                                 static_cast<uint16_t>(kMaxSecureSduLengthBytes - 128) /* reserved buffer size */);
 
     ByteSpan list[5];
 

src/controller/tests/TestWriteChunking.cpp

@@ -203,21 +203,22 @@ void TestWriteChunking::TestListChunking(nlTestSuite * apSuite, void * apContext
 
     app::AttributePathParams attributePath(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);
     //
-    // We've empirically determined that by reserving 950 bytes in the packet buffer, we can fit 2
+    // We've empirically determined that by reserving all but 75 bytes in the packet buffer, we can fit 2
     // AttributeDataIBs into the packet. ~30-40 bytes covers a single write chunk, but let's 2-3x that
     // to ensure we'll sweep from fitting 2 chunks to 3-4 chunks.
     //
-    for (int i = 100; i > 0; i--)
+    constexpr size_t minReservationSize = kMaxSecureSduLengthBytes - 75 - 100;
+    for (uint32_t i = 100; i > 0; i--)
     {
         CHIP_ERROR err = CHIP_NO_ERROR;
         TestWriteCallback writeCallback;
 
         ChipLogDetail(DataManagement, "Running iteration %d\n", i);
 
-        gIterationCount = (uint32_t) i;
+        gIterationCount = i;
 
         app::WriteClient writeClient(&ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
-                                     static_cast<uint16_t>(850 + i) /* reserved buffer size */);
+                                     static_cast<uint16_t>(minReservationSize + i) /* reserved buffer size */);
 
         ByteSpan list[kTestListLength];
 
@@ -358,15 +359,16 @@ void TestWriteChunking::TestConflictWrite(nlTestSuite * apSuite, void * apContex
 
     app::AttributePathParams attributePath(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);
 
+    /* use a smaller chunk (128 bytes) so we only need a few attributes in the write request. */
+    constexpr size_t kReserveSize = kMaxSecureSduLengthBytes - 128;
+
     TestWriteCallback writeCallback1;
-    app::WriteClient writeClient1(
-        &ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
-        static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
+    app::WriteClient writeClient1(&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
+                                  static_cast<uint16_t>(kReserveSize));
 
     TestWriteCallback writeCallback2;
-    app::WriteClient writeClient2(
-        &ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
-        static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
+    app::WriteClient writeClient2(&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
+                                  static_cast<uint16_t>(kReserveSize));
 
     ByteSpan list[kTestListLength];
 
@@ -435,15 +437,16 @@ void TestWriteChunking::TestNonConflictWrite(nlTestSuite * apSuite, void * apCon
     app::AttributePathParams attributePath1(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute);
     app::AttributePathParams attributePath2(kTestEndpointId, app::Clusters::UnitTesting::Id, kTestListAttribute2);
 
+    /* use a smaller chunk (128 bytes) so we only need a few attributes in the write request. */
+    constexpr size_t kReserveSize = kMaxSecureSduLengthBytes - 128;
+
     TestWriteCallback writeCallback1;
-    app::WriteClient writeClient1(
-        &ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
-        static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
+    app::WriteClient writeClient1(&ctx.GetExchangeManager(), &writeCallback1, Optional<uint16_t>::Missing(),
+                                  static_cast<uint16_t>(kReserveSize));
 
     TestWriteCallback writeCallback2;
-    app::WriteClient writeClient2(
-        &ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
-        static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
+    app::WriteClient writeClient2(&ctx.GetExchangeManager(), &writeCallback2, Optional<uint16_t>::Missing(),
+                                  static_cast<uint16_t>(kReserveSize));
 
     ByteSpan list[kTestListLength];
 
@@ -514,7 +517,8 @@ void RunTest(nlTestSuite * apSuite, TestContext & ctx, Instructions instructions
     TestWriteCallback writeCallback;
     std::unique_ptr<WriteClient> writeClient = std::make_unique<WriteClient>(
         &ctx.GetExchangeManager(), &writeCallback, Optional<uint16_t>::Missing(),
-        static_cast<uint16_t>(900) /* use a smaller chunk so we only need a few attributes in the write request. */);
+        static_cast<uint16_t>(kMaxSecureSduLengthBytes -
+                              128) /* use a smaller chunk so we only need a few attributes in the write request. */);
 
     ConcreteAttributePath onGoingPath = ConcreteAttributePath();
     std::vector<PathStatus> status;

src/controller/tests/data_model/TestRead.cpp

@@ -1063,7 +1063,7 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
     }
 
     // Read of E2C3A* and E3C2A2, and inject a large amount of event path list, then it would try to apply previous cache
-    // latest data version and construct data version list but no enough memory, finally fully rollback data version filter. Expect
+    // latest data version and construct data version list but run out of memory, finally fully rollback data version filter. Expect
     // E2C3A* attributes in report, and E3C2A2 attribute in report
     {
         testId++;
@@ -1081,8 +1081,12 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
         readPrepareParams.mpAttributePathParamsList    = attributePathParams2;
         readPrepareParams.mAttributePathParamsListSize = 2;
 
-        readPrepareParams.mpEventPathParamsList    = eventPathParams;
-        readPrepareParams.mEventPathParamsListSize = 64;
+        readPrepareParams.mpEventPathParamsList = eventPathParams;
+        // This size needs to be big enough that we can't fit our
+        // DataVersionFilterIBs in the same packet.  Max size is
+        // ArraySize(eventPathParams);
+        static_assert(75 <= ArraySize(eventPathParams));
+        readPrepareParams.mEventPathParamsListSize = 75;
 
         err = readClient.SendRequest(readPrepareParams);
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
@@ -1252,8 +1256,8 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
 
     // Read of E1C2A*(3 attributes) and E2C3A*(5 attributes) and E2C2A*(4 attributes), and inject a large amount of event path
     // list, then it would try to apply previous cache latest data version and construct data version list with the ordering from
-    // largest cluster size to smallest cluster size(C2, C3, C1) but no enough memory, finally partially rollback data version
-    // filter with only C2. Expect E1C2A*, E2C2A* attributes(7 attributes) in report,
+    // largest cluster size to smallest cluster size(C3, C2, C1) but run out of memory, finally partially rollback data version
+    // filter with only C3. Expect E1C2A*, E2C2A* attributes(7 attributes) in report,
     {
         testId++;
         ChipLogProgress(DataManagement, "\t -- Running Read with ClusterStateCache Test ID %d", testId);
@@ -1275,8 +1279,12 @@ void TestReadInteraction::TestReadSubscribeAttributeResponseWithCache(nlTestSuit
         readPrepareParams.mpAttributePathParamsList    = attributePathParams3;
         readPrepareParams.mAttributePathParamsListSize = 3;
         readPrepareParams.mpEventPathParamsList        = eventPathParams;
-        readPrepareParams.mEventPathParamsListSize     = 62;
-        err                                            = readClient.SendRequest(readPrepareParams);
+
+        // This size needs to be big enough that we can only fit our first
+        // DataVersionFilterIB. Max size is ArraySize(eventPathParams);
+        static_assert(73 <= ArraySize(eventPathParams));
+        readPrepareParams.mEventPathParamsListSize = 73;
+        err                                        = readClient.SendRequest(readPrepareParams);
         NL_TEST_ASSERT(apSuite, err == CHIP_NO_ERROR);
 
         ctx.DrainAndServiceIO();

src/system/SystemConfig.h

@@ -293,55 +293,57 @@
 #endif // CHIP_SYSTEM_CONFIG_ZEPHYR_LOCKING && CHIP_SYSTEM_CONFIG_NO_LOCKING
 
 /**
- *  @def CHIP_SYSTEM_HEADER_RESERVE_SIZE
+ *  @def CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE
  *
  *  @brief
  *      The number of bytes to reserve in a network packet buffer to contain
- *      the CHIP message and exchange headers.
- *
- *      This number was calculated as follows:
+ *      the Matter crypto headers.
  *
- *      CHIP Crypto Header:
- *
- *          4 -- Length of encrypted block
- *          4 -- Reserve
- *          8 -- Initialization Vector
- *          8 -- Encryption Tag
+ *      This is 0, because Matter does not have any crypto headers.  This define
+ *      is still here only for backwards compatibility.
  */
 #ifndef CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE
-#define CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE 24
+#define CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE 0
 #endif
 
 /**
  *  @def CHIP_SYSTEM_HEADER_RESERVE_SIZE
  *
  *  @brief
  *      The number of bytes to reserve in a network packet buffer to contain
- *      the CHIP message and exchange headers.
+ *      the CHIP message and payload headers.
  *
  *      This number was calculated as follows:
  *
- *      CHIP Message Header:
+ *      Matter Message Header:
  *
  *          2 -- Frame Length
- *          2 -- Message Header
- *          4 -- Message Id
- *          8 -- Source Node Id
- *          8 -- Destination Node Id
- *          2 -- Key Id
+ *          1 -- Message Flags
+ *          2 -- Session ID
+ *          1 -- Security Flags
+ *          4 -- Message Counter
+ *          8 -- Source Node ID
+ *          8 -- Destination Node ID
+ *
+ *      Total: 26 bytes.
+ *
+ *      Matter Payload Header:
  *
- *      CHIP Exchange Header:
+ *          1 -- Exhange Flags
+ *          1 -- Protocol Opcode
+ *          2 -- Exchange ID
+ *          2 -- Protocol Vendor ID
+ *          2 -- Protocol ID
+ *          4 -- Acknowledged MEssage Counter
  *
- *          1 -- Application Version
- *          1 -- Message Type
- *          2 -- Exchange Id
- *          4 -- Profile Id
- *          4 -- Acknowledged Message Id
+ *      Total: 12 bytes.
  *
- *    @note A number of these fields are optional or not presently used. So most headers will be considerably smaller than this.
+ *    @note A number of these fields are optional or not presently used. So most
+ *          headers will be considerably smaller than this.
+ *    @note This calculation assumes ther are no Message Extensions or Secured Extensions.
  */
 #ifndef CHIP_SYSTEM_HEADER_RESERVE_SIZE
-#define CHIP_SYSTEM_HEADER_RESERVE_SIZE (38 + CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE)
+#define CHIP_SYSTEM_HEADER_RESERVE_SIZE (26 + 12 + CHIP_SYSTEM_CRYPTO_HEADER_RESERVE_SIZE)
 #endif /* CHIP_SYSTEM_HEADER_RESERVE_SIZE */
 
 /**