Derive the Block2 block count from the payload cap, once

docs/api.md

@@ -459,7 +459,7 @@ CoAP wire encoding/decoding (RFC 7252 + 7641 + 7959).
 #### `Block2Accumulator`
 
 ```python
-Block2Accumulator(token, *, max_blocks=32, max_payload_bytes=65536, accepted_content_formats=None)
+Block2Accumulator(token, *, max_blocks=64, max_payload_bytes=65536, accepted_content_formats=None)
 ```
 
 *class*: Bounded, token-stable Block2 representation accumulator.

smartthings_local/protocol/coap.py

@@ -44,11 +44,20 @@
 # will honour and the only one the probes have validated end-to-end.
 BLOCK_SZX = 6
 
-# Shared bounds for callers that assemble untrusted Block2 responses.  Thirty-
-# two means exactly blocks 0..31; a response that advertises another block from
-# block 31 is rejected.  The payload cap applies to the fully assembled body.
-MAX_BLOCK2_BLOCKS = 32
+# Shared bounds for callers that assemble untrusted Block2 responses.  The
+# payload cap applies to the fully assembled body; the block count is that same
+# cap expressed in blocks of the size we ask for, so the two agree by
+# construction and neither can be reached before the other.  They disagreed
+# until #122, where a 34-block /device/0 batch was refused by a count of 32
+# while sitting well inside 64 KiB.  A count of N means exactly blocks 0..N-1;
+# a response that advertises another block from block N-1 is rejected.
+#
+# Deriving the count ties it to BLOCK_SZX: asking for smaller blocks would
+# raise it, since the bound being expressed is the assembled size rather than a
+# number of round trips.  A transfer that downshifts below BLOCK_SZX therefore
+# stops on the payload cap only if it is large, and on this count otherwise.
 MAX_BLOCK2_PAYLOAD_BYTES = 64 * 1024
+MAX_BLOCK2_BLOCKS = MAX_BLOCK2_PAYLOAD_BYTES // (1 << (BLOCK_SZX + 4))
 
 # ``classify_coap_response`` outcomes.  Strings keep the helper lightweight for
 # transports that already use their own event/state machinery.
@@ -361,7 +370,8 @@ def classify_coap_response(datagram, *, token=None, request_mid=None):
 class Block2Accumulator:
     """Bounded, token-stable Block2 representation accumulator.
 
-    At most ``max_blocks`` response blocks (32 by default) and
+    At most ``max_blocks`` response blocks (64 by default, which is the
+    default ``max_payload_bytes`` in blocks of the requested size) and
     ``max_payload_bytes`` assembled bytes (64 KiB by default) are accepted.
     Block offsets must remain contiguous.  A bounded SZX downshift is accepted
     because the appliance firmware may return the requested payload size while

smartthings_local/protocol/dtls_session.py

@@ -68,7 +68,7 @@
     CF_CBOR,
     CONTENT_FORMAT,
     ETAG,
-    MAX_BLOCK2_PAYLOAD_BYTES,
+    MAX_BLOCK2_BLOCKS,
     METHOD_DELETE,
     METHOD_GET,
     METHOD_POST,
@@ -521,11 +521,10 @@ class DtlsCoapSession:
 
     HANDSHAKE_TIMEOUT_S = 12.0
     READER_RECV_TIMEOUT_S = 1.0  # short so stop_event propagates quickly
-    # Safety bound for Block2 fetches: as many SZX-6 blocks as the shared
-    # MAX_BLOCK2_PAYLOAD_BYTES cap holds (64). At 32, a 33,884-byte
-    # /device/0?if=oic.if.b batch from an LCD_A311D_OV_QMD_EU_22K oven
-    # (34 blocks) failed with BlockwiseError although it was inside the cap.
-    MAX_BLOCKS = MAX_BLOCK2_PAYLOAD_BYTES >> (BLOCK_SZX + 4)
+    # Safety bound for Block2 fetches. The shared constant is the payload cap
+    # in blocks of the size we ask for, so this agrees with the byte cap the
+    # accumulator enforces beside it; #122 arrived because they did not.
+    MAX_BLOCKS = MAX_BLOCK2_BLOCKS
 
     def __init__(self, host, port, cert_path=None, key_path=None, *,
                  cert_pem=None, key_pem=None,

smartthings_local/protocol/ocf_discovery.py

@@ -63,6 +63,8 @@
 from .coap import (
     BLOCK2_DUPLICATE,
     CF_CBOR,
+    MAX_BLOCK2_BLOCKS,
+    MAX_BLOCK2_PAYLOAD_BYTES,
     RESPONSE_MESSAGE,
     TYPE_CON,
     TYPE_NON,
@@ -82,9 +84,12 @@
 _DISCOVERY_PORT = 5683
 _MAX_ENDPOINTS = 8
 _MAX_PORTS = 8
-_MAX_BLOCKS = 32
+# Both from coap, so the plaintext directory read is bounded exactly as a
+# session GET is. They were a local 32 and a local 65536 until #122 showed
+# the two disagreeing on the DTLS side.
+_MAX_BLOCKS = MAX_BLOCK2_BLOCKS
 _MAX_DATAGRAM_BYTES = 8192
-_MAX_PAYLOAD_BYTES = 65536
+_MAX_PAYLOAD_BYTES = MAX_BLOCK2_PAYLOAD_BYTES
 _MAX_LINKS = 256
 _MAX_ENDPOINT_URIS_PER_LINK = 32
 _MAX_REQUEST_OPTION_BYTES = 1024

tests/test_block_cap_agreement.py

@@ -0,0 +1,110 @@
+"""One derivation for the Block2 block count, used at every bound.
+
+#122 raised the session's count after a 34-block `/device/0?if=oic.if.b`
+batch was refused by a count of 32 that sat well inside the 64 KiB payload
+cap enforced beside it. Two more copies of that disagreement were left: the
+module default every external caller gets, and the plaintext discovery read.
+These pin that all three now come from one place.
+"""
+
+from __future__ import annotations
+
+import secrets
+
+import pytest
+
+from smartthings_local.errors import BlockwiseError
+from smartthings_local.protocol import ocf_discovery
+from smartthings_local.protocol.coap import (
+    BLOCK2,
+    BLOCK2_COMPLETE,
+    BLOCK_SZX,
+    CF_CBOR,
+    CONTENT_FORMAT,
+    MAX_BLOCK2_BLOCKS,
+    MAX_BLOCK2_PAYLOAD_BYTES,
+    TYPE_ACK,
+    Block2Accumulator,
+    block_value,
+    build_coap,
+    parse_coap_message,
+)
+from smartthings_local.protocol.dtls_session import DtlsCoapSession
+
+_BLOCK = 1 << (BLOCK_SZX + 4)
+
+
+def test_the_block_count_is_the_payload_cap_in_blocks():
+    assert MAX_BLOCK2_BLOCKS * _BLOCK == MAX_BLOCK2_PAYLOAD_BYTES
+
+
+@pytest.mark.parametrize(
+    'blocks, payload_bytes',
+    [
+        (DtlsCoapSession.MAX_BLOCKS, MAX_BLOCK2_PAYLOAD_BYTES),
+        (ocf_discovery._MAX_BLOCKS, ocf_discovery._MAX_PAYLOAD_BYTES),
+        (MAX_BLOCK2_BLOCKS, MAX_BLOCK2_PAYLOAD_BYTES),
+    ],
+    ids=['dtls-session', 'plaintext-discovery', 'module-default'],
+)
+def test_every_bound_in_the_tree_agrees(blocks, payload_bytes):
+    # A bound that disagrees with its neighbour is the #122 defect, wherever
+    # it sits: the smaller one refuses transfers the larger one permits, and
+    # the error names the wrong cause.
+    assert blocks == MAX_BLOCK2_BLOCKS
+    assert payload_bytes == MAX_BLOCK2_PAYLOAD_BYTES
+
+
+def _feed(accumulator, body, *, token):
+    """Drive one whole Block2 transfer into `accumulator`, SZX 6 throughout."""
+    status = None
+    for number in range(-(-len(body) // _BLOCK)):
+        chunk = body[number * _BLOCK:(number + 1) * _BLOCK]
+        more = (number + 1) * _BLOCK < len(body)
+        status = accumulator.add_response(parse_coap_message(
+            build_coap(
+                TYPE_ACK, 0x45, 0x1234 + number, token,
+                ((CONTENT_FORMAT, CF_CBOR),
+                 (BLOCK2, block_value(number, int(more), BLOCK_SZX))),
+                chunk,
+            )
+        ))
+    return status
+
+
+def test_the_discovery_bounds_accept_a_transfer_past_the_old_count():
+    # 33 blocks is one past the count both of these carried before. The
+    # plaintext directory has not been measured this large on any appliance
+    # here, so this is the bound being asserted, not an observation.
+    token = secrets.token_bytes(8)
+    accumulator = Block2Accumulator(
+        token,
+        max_blocks=ocf_discovery._MAX_BLOCKS,
+        max_payload_bytes=ocf_discovery._MAX_PAYLOAD_BYTES,
+        accepted_content_formats={int.from_bytes(CF_CBOR, 'big')},
+    )
+
+    body = bytes(range(256)) * (33 * _BLOCK // 256)
+    assert _feed(accumulator, body, token=token) == BLOCK2_COMPLETE
+    assert accumulator.payload == body
+    assert accumulator.blocks_received == 33
+
+
+def test_the_shared_bounds_still_refuse_the_block_after_the_last():
+    # Both bounds land on the same block, so the one that fires first is
+    # unspecified and either error is the same answer: the body is too big.
+    token = secrets.token_bytes(8)
+    accumulator = Block2Accumulator(token)
+
+    with pytest.raises(BlockwiseError):
+        _feed(accumulator, b'y' * ((MAX_BLOCK2_BLOCKS + 1) * _BLOCK),
+              token=token)
+
+
+def test_a_transfer_filling_the_bounds_exactly_is_accepted():
+    token = secrets.token_bytes(8)
+    accumulator = Block2Accumulator(token)
+
+    body = b'z' * MAX_BLOCK2_PAYLOAD_BYTES
+    assert _feed(accumulator, body, token=token) == BLOCK2_COMPLETE
+    assert accumulator.blocks_received == MAX_BLOCK2_BLOCKS