fix: validate chunk order and identity when deserializing chunked transactions

[danielmarv]

Description:

Transaction.from_bytes() accepted a TransactionList for a chunked transaction (FileAppendTransaction, TopicMessageSubmitTransaction) whose chunk groups were reordered or belonged to different chunked transactions. The restored transaction then re-serialized and executed the chunks in input order: file contents were appended out of order, and topic message chunks were submitted out of sequence.

• Validate the chunk groups of chunked transaction types:
  • all chunk transaction IDs share the same payer account;
  • transactionValidStart strictly increases from one chunk group to the next;
  • consensusSubmitMessage only: chunkInfo.number is the chunk's 1-based position, chunkInfo.total equals the number of chunk groups, and chunkInfo.initialTransactionID equals the first chunk's transaction ID.
• from_bytes() only falls back to the single Transaction format when the bytes do not decode to a non-empty TransactionList. Validation errors now reach the caller as a ValueError with the real cause, instead of being replaced by an unrelated parse error.
• Split the private _process_transaction_list_bytes() into _parse_transaction_list() and _process_transaction_list(). No public API changes.

Related issue(s):

Fixes #2710

Notes for reviewer:

• Root cause: transaction_ids is collected in first-seen input order, and _validate_transaction_pairs() builds its expected layout from it, so a reordered list always matched itself. The new checks read each chunk group's own transactionID and chunkInfo instead.
• Reject, don't sort: malformed lists are rejected, not silently reordered.
• Strictly increasing, not an exact +1ns step: Java and the JS TopicMessageSubmitTransaction space chunk valid starts 1ns apart, but the JS FileAppendTransaction uses a configurable chunkInterval (default 10ns). Neither carries nanos overflow into seconds, so valid starts are compared as (seconds, nanos).
• Single-chunk topic messages: Python and Java omit chunkInfo for a single chunk, while JS sets total=1, number=1. Both are accepted. A lone chunk of a larger message (e.g. total=2) is now rejected as incomplete.
• Bodies without node account IDs: the chunk checks also run on this path, which previously skipped chunk validation entirely.
• Known limit: FileAppendTransaction carries no chunk count, so a list with its last chunk(s) removed cannot be detected. Topic messages are covered by chunkInfo.total.
• Updated existing tests: four tests built chunk sequences that are now invalid; they now use sequential chunk IDs:
  • test_validate_transaction_bodies_chunk_transaction and test_validate_transaction_bodies_chunk_transaction_mutiple_node_ids used different payers (0.0.2 and 0.0.3);
  • test_validate_transaction_bodies_for_chunk_file_append_tx and test_validate_transaction_bodies_for_chunk_message_submit_tx built chunk IDs with TransactionId.generate(), which subtracts a random 5–8s, so the second chunk was often earlier than the first. The topic test also gave both chunks number=1 and unrelated initialTransactionIDs.
• Fixed the transaction_ids annotation in _process_transaction_list() (list[Transaction] → list[TransactionId]).
• Tests & Linters:
  • New tests/unit/chunked_transaction_from_bytes_test.py (66 cases) tests through Transaction.from_bytes() for both types, with one and two nodes:
    • round-trips of 1–3 chunks, signed and unsigned, give identical bytes, and 10ns chunk spacing is accepted;
    • swapped chunk groups, different payers, non-increasing valid starts, wrong chunkInfo.number/total, mismatched initialTransactionID, missing chunkInfo and an incomplete topic message are rejected with a specific message. All 34 rejection cases fail against main.
  • New tests in transaction_freeze_and_bytes_test.py for the single Transaction fallback (previously untested) and for a TransactionList validation error reaching the caller.
  • uv run pytest tests/unit: 3404 passed.
  • All pre-commit hooks (gitleaks, end-of-file-fixer, trailing-whitespace, ruff-check, ruff-format, bandit) pass.
  • Integration tests have not been run locally.

Checklist

• Documented (Code comments, README, etc.)
• Tested (unit, integration, etc.)

[codecov]

Codecov Report

❌ Patch coverage is 96.07843% with 2 lines in your changes missing coverage. Please review.

@@            Coverage Diff             @@
##             main    #2736      +/-   ##
==========================================
+ Coverage   95.68%   95.70%   +0.01%     
==========================================
  Files         169      169              
  Lines       11080    11119      +39     
==========================================
+ Hits        10602    10641      +39     
  Misses        478      478              

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Repository: hiero-ledger/hiero-sdk-python/.coderabbit.yaml
• Review profile: ASSERTIVE
• Plan: Advanced
• Run ID: 697eca11-7a05-4922-97be-712f2dd4eca3

📥 Commits

Reviewing files that changed from the base of the PR and between 7260ea7 and 78dd43f.

📒 Files selected for processing: 6

• src/hiero_sdk_python/transaction/transaction.py
• tests/unit/chunked_transaction_from_bytes_test.py
• tests/unit/file_append_transaction_test.py
• tests/unit/topic_message_submit_transaction_test.py
• tests/unit/transaction_freeze_and_bytes_test.py
• tests/unit/transaction_test.py

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

Transaction.from_bytes now parses non-empty TransactionList data without masking its validation errors with legacy parsing. Chunk validation checks transaction ID sequence and topic-message chunk metadata, including when node IDs are absent. Unit tests cover restoration, valid sequences, and invalid chunk groups.

Changes

Transaction restoration

Layer / File(s)	Summary
TransactionList deserialization src/hiero_sdk_python/transaction/transaction.py, tests/unit/transaction_freeze_and_bytes_test.py	Transaction.from_bytes processes a decoded, non-empty TransactionList. It falls back to single-transaction parsing only when decoding fails or the list is empty. Tests cover single-transaction restoration and preservation of a mixed-type validation error.
Chunk identity and sequence validation src/hiero_sdk_python/transaction/transaction.py, tests/unit/chunked_transaction_from_bytes_test.py, tests/unit/file_append_transaction_test.py, tests/unit/topic_message_submit_transaction_test.py, tests/unit/transaction_test.py	Chunk validation requires a shared payer and strictly increasing valid-start timestamps. Topic-message chunks must have consistent numbers, totals, and initial transaction IDs. Tests cover valid and invalid sequences, including transactions without node IDs.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Severity of issue fixed: Medium

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant Transaction.from_bytes
  participant TransactionList decoder
  participant Chunk validation
  Caller->>Transaction.from_bytes: Provide serialized bytes
  Transaction.from_bytes->>TransactionList decoder: Decode TransactionList
  TransactionList decoder-->>Transaction.from_bytes: Return decoded list
  Transaction.from_bytes->>Chunk validation: Validate chunk bodies
  Chunk validation-->>Transaction.from_bytes: Return validation result
  Transaction.from_bytes-->>Caller: Return restored transaction or validation error

Loading

Merge Risk: ⚪ Minimal · up to 78dd4

Normal checks can proceed; no identified issue currently blocks merging the transaction validation changes.

Security architecture risk: 🔵 Low · up to 78dd4

The change strengthens checks on imported transaction chunks and prevents validation failures from being hidden by fallback parsing. No new execution authority or broader exposure was identified. Compatibility with every supported legacy encoding remains unverified.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

• inferred — The relevant attack surface is transaction bytes that an application chooses to import and subsequently sign or execute. Potential downstream effects concern the represented file append or topic submission under that application's configured client and signing authority. Deserialization itself does not submit a transaction.

Trust Boundaries and Controls

• observed — The new controls constrain attacker-supplied chunk ordering and payer identity before a restored list is returned. They supplement existing invariant-field and node-variant checks rather than granting signing authority. Execution still adds the configured operator's signature only when the first transaction's payer matches that operator.

Resilience and Maintainability Implications

• inferred — A validation failure leaves no returned partially restored object and does not itself perform external writes. Later chunk execution remains sequential and non-atomic: exceptions can interrupt the loop after earlier submissions, and a repeated execute_all call resets the transaction-ID index to the beginning. This execution and recovery behavior is unchanged by the PR; the new validation does not establish rollback or exactly-once submission.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 79.55% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 44 functions across 6 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Title check		The title clearly and concisely describes the main change: validating chunk order and identity during deserialization.
Description check		The description directly explains the chunk validation changes, fallback behavior, tests, known limitation, and verification results.
Linked Issues check		Issue #2710 is addressed. Transaction.from_bytes() now distinguishes a non-empty TransactionList from single-transaction bytes and preserves list validation errors. The new validation checks share…
Out of Scope Changes check		The changes stay within issue #2710. Source changes implement deserialization fallback, chunk validation, error propagation, and private helper separation. Test changes cover the requested behavior an…

Full details: Linked Issues check

Explanation

Issue #2710 is addressed. Transaction.from_bytes() now distinguishes a non-empty TransactionList from single-transaction bytes and preserves list validation errors. The new validation checks shared payer accounts, strictly increasing valid starts, and topic chunk number, total, and initial transaction ID. The tests cover both chunked transaction types, one and multiple nodes, signed and unsigned round-trips, single chunks, missing node IDs, malformed ordering, payer, timestamp, and topic metadata cases. The implementation rejects malformed input instead of sorting it and keeps the helpers private.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #2710. Source changes implement deserialization fallback, chunk validation, error propagation, and private helper separation. Test changes cover the requested behavior and update existing fixtures that used invalid chunk sequences. No unrelated production behavior or public API change is demonstrated.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

_{Comment @coderabbitai help to get the list of available commands.}

[aceppaluni]

Should we add a regression test using a real serialized single Transaction that is also successfully decoded as a non-empty TransactionList?

Since protobuf parsing doesn't necessarily distinguish these two wire formats, checking only transaction_list.transaction_list may incorrectly classify the input and skip the single-transaction fallback.

What do you think?

[danielmarv]

Thanks @aceppaluni
TransactionList only defines field 1, so a single Transaction decodes as a non-empty list only if it sets its own field 1, the deprecated body, which _process_base_transaction() can't restore anyway. The formats we do restore, signedTransactionBytes (field 5) and bodyBytes + sigMap (fields 4/3), always decode to an empty list and take the fallback. This matches Java (if (!list.getTransactionListList().isEmpty())) and JS (if (list.length === 0)).

test_from_bytes_accepts_single_transaction_bytes already covers a real serialized signedTransactionBytes transaction. I've added a regression test for the legacy bodyBytes + sigMap format, plus one showing a deprecated body transaction is rejected with a ValueError.