fix: ChunkTransaction create all transaction_bytes when freeze - #2506
fix: ChunkTransaction create all transaction_bytes when freeze#2506manishdait wants to merge 16 commits into
Conversation
manishdait
added
the
notes: extra care
|
Hi, this is WorkflowBot.
|
98da211 to
0132392
Compare
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2506 +/- ##
==========================================
- Coverage 95.47% 95.41% -0.06%
==========================================
Files 165 165
Lines 10604 10607 +3
==========================================
- Hits 10124 10121 -3
- Misses 480 486 +6 🚀 New features to boost your workflow:
|
103075c to
dc31189
Compare
| airdrop_tx.add_approved_token_transfer_with_decimals(token_id, account_id, amount, 1) | ||
|
|
||
|
|
||
| def test_add_unbalanced_transfer_amount(mock_account_ids): |
There was a problem hiding this comment.
This tests was suppose to removed during the TCK changes, but the removal wasn’t caught because the previous build_transaction_body() implementation required a transaction id/node id, causing it to raise a ValueError which make the test pass.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughTransactions now prepare serialized bodies for every chunk and node during freezing. Transaction IDs and body bytes use nested collections. Chunk execution and fee estimation advance through prepared IDs. Topic and file payloads emit chunk metadata only for active multi-chunk submissions. ChangesChunked transaction flow
Estimated code review effort: 4 (Complex) | ~60 minutes Mergeability Score: 🟠 High · up to The change moves transaction-body creation to freeze time, but the current implementation can leave transactions partially frozen after preparation fails, calculate sizes from bytes different from those signed and submitted, and lose batch keys during deserialization, potentially causing submission failures or bypassing required execution checks. The PR is not merge-ready until these issues are corrected. Sequence Diagram(s)sequenceDiagram
participant Client
participant ChunkedTransaction
participant Transaction
participant Node
Client->>ChunkedTransaction: freeze_with(client)
ChunkedTransaction->>Transaction: prepare chunk transaction IDs
Transaction->>Transaction: build body for each chunk and node
Client->>ChunkedTransaction: execute_all()
ChunkedTransaction->>Transaction: select current chunk body
Transaction->>Node: submit serialized transaction
Node-->>Transaction: return response
ChunkedTransaction->>Transaction: advance to next chunk
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/integration/file_append_transaction_e2e_test.py (1)
309-319: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the chunk count explicitly.
The test relies on the reader computing
ceil(13 / 4) == 4from the inline comment. Nothing fails if the transaction silently collapses to a single chunk, because the final contents assertion passes either way. This PR changes how chunk bodies are generated, so the test should pin the chunk count. Add an assertion afterfreeze(). The comment text is also imprecise: 13 bytes at 4 bytes per chunk is exactly 4 chunks, not approximately 4.💚 Proposed assertions
content = "Hello, Hiero!" # length 13 tx = ( FileAppendTransaction() .set_file_id(file_id) - .set_chunk_size(4) # content with (13/4) bytes ie approx 4 chunks + .set_chunk_size(4) # 13 bytes at 4 bytes per chunk == 4 chunks .set_contents(content) .set_transaction_id(TransactionId.generate(env.client.operator_account_id)) .set_node_account_ids([AccountId(0, 0, 3)]) .freeze() ) + assert tx.get_required_chunks() == 4, "Content must split into exactly four chunks" + assert len(tx._transaction_ids) == 4, "One transaction ID must exist per chunk" + tx.sign(env.client.operator_private_key)Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 43c7b17d-a7b7-44a2-8851-9bd24890449e
📒 Files selected for processing (47)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.pysrc/hiero_sdk_python/file/file_append_transaction.pysrc/hiero_sdk_python/lockable_list.pysrc/hiero_sdk_python/query/fee_estimate_query.pysrc/hiero_sdk_python/transaction/chunked_transaction.pysrc/hiero_sdk_python/transaction/transaction.pytests/integration/file_append_transaction_e2e_test.pytests/integration/transaction_freeze_e2e_test.pytests/unit/account_create_transaction_test.pytests/unit/account_delete_transaction_test.pytests/unit/account_update_transaction_test.pytests/unit/batch_transaction_test.pytests/unit/chunked_transaction_test.pytests/unit/contract_delete_transaction_test.pytests/unit/contract_execute_transaction_test.pytests/unit/ethereum_transaction_test.pytests/unit/executable_test.pytests/unit/fee_estimate_query_test.pytests/unit/file_append_transaction_test.pytests/unit/file_delete_transaction_test.pytests/unit/file_update_transaction_test.pytests/unit/freeze_transaction_test.pytests/unit/lockable_list_test.pytests/unit/node_create_transaction_test.pytests/unit/node_delete_transaction_test.pytests/unit/node_update_transaction_test.pytests/unit/prng_transaction_test.pytests/unit/schedule_create_transaction_test.pytests/unit/schedule_delete_transaction_test.pytests/unit/schedule_sign_transaction_test.pytests/unit/token_airdrop_transaction_cancel_test.pytests/unit/token_airdrop_transaction_test.pytests/unit/token_associate_transaction_test.pytests/unit/token_create_transaction_test.pytests/unit/token_delete_transaction_test.pytests/unit/token_dissociate_transaction_test.pytests/unit/token_freeze_transaction_test.pytests/unit/token_mint_transaction_test.pytests/unit/token_pause_transaction_test.pytests/unit/token_unfreeze_transaction_test.pytests/unit/token_unpause_transaction_test.pytests/unit/topic_create_transaction_test.pytests/unit/topic_delete_transaction_test.pytests/unit/topic_message_submit_transaction_test.pytests/unit/topic_update_transaction_test.pytests/unit/transaction_freeze_and_bytes_test.pytests/unit/transaction_test.py
| # Multi-chunk transaction - execute all chunks | ||
| responses = [] | ||
| self._transaction_ids.set_index(0) | ||
| for _ in range(len(self._transaction_ids)): | ||
| response = super().execute(client, timeout, wait_for_receipt, validate_status) | ||
| responses.append(response) | ||
| self._transaction_ids.advance() | ||
|
|
||
| return responses |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reset the transaction ID index after the chunk loop.
The loop advances self._transaction_ids for every chunk and never resets the index. After execute_all returns, the collection points past the first chunk, or wherever advance() left it. Any later call that reads self._transaction_ids.current — transaction_id, _to_proto, body_size, or a second execute_all — then selects the wrong chunk. An exception from one chunk leaves the index in the same inconsistent state. freeze_with and body_size_all_chunks in this file, and FeeEstimateQuery._execute_chunked, all restore the index; this path must do the same.
🐛 Proposed fix
# Multi-chunk transaction - execute all chunks
responses = []
self._transaction_ids.set_index(0)
- for _ in range(len(self._transaction_ids)):
- response = super().execute(client, timeout, wait_for_receipt, validate_status)
- responses.append(response)
- self._transaction_ids.advance()
+ try:
+ for _ in range(len(self._transaction_ids)):
+ response = super().execute(client, timeout, wait_for_receipt, validate_status)
+ responses.append(response)
+ self._transaction_ids.advance()
+ finally:
+ self._transaction_ids.set_index(0)
return responses📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Multi-chunk transaction - execute all chunks | |
| responses = [] | |
| self._transaction_ids.set_index(0) | |
| for _ in range(len(self._transaction_ids)): | |
| response = super().execute(client, timeout, wait_for_receipt, validate_status) | |
| responses.append(response) | |
| self._transaction_ids.advance() | |
| return responses | |
| # Multi-chunk transaction - execute all chunks | |
| responses = [] | |
| self._transaction_ids.set_index(0) | |
| try: | |
| for _ in range(len(self._transaction_ids)): | |
| response = super().execute(client, timeout, wait_for_receipt, validate_status) | |
| responses.append(response) | |
| self._transaction_ids.advance() | |
| finally: | |
| self._transaction_ids.set_index(0) | |
| return responses |
| # TODO: Check using the signature map | ||
| # def test_sign_tracks_signing_keys_once(mock_client, private_key): | ||
| # tx = DummyChunkedTransaction(required_chunks=1) | ||
| # tx.freeze_with(mock_client) | ||
|
|
||
| tx.sign(private_key) | ||
| tx.sign(private_key) | ||
| # tx.sign(private_key) | ||
| # tx.sign(private_key) | ||
|
|
||
| assert tx._signing_keys == [private_key] | ||
| assert tx.is_signed_by(private_key.public_key()) is True | ||
| # assert tx._signing_keys == [private_key] | ||
| # assert tx.is_signed_by(private_key.public_key()) is True |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Replace the commented-out signing test.
This PR changes signing for all prebuilt chunk bodies. Do not remove coverage for repeated signing and signature deduplication. Add assertions that every body in _transaction_body_bytes has one signature after signing the same key twice.
As per path instructions, “No unjustified TODOs or skipped tests without tracking issues.”
Source: Path instructions
| assert tx._transaction_ids._locked is True | ||
| assert tx._transaction_ids.index == 0 | ||
|
|
||
| assert tx._node_account_ids._locked is True | ||
| assert tx._node_account_ids.index == 0 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Assert the lock state through a public accessor.
Both tests read tx._transaction_ids._locked, a private attribute of LockableList, while reading index as a public property. If LockableList exposes a public lock accessor, use it. If it does not, add one, because the lock state is part of the freeze contract that these tests verify. Add failure messages to these assertions as well; the unit-test guidance requires that a failing assertion states what broke.
Also applies to: 465-469
Source: Path instructions
| tx = TopicCreateTransaction(memo="Signing test") | ||
| tx.operator_account_id = AccountId(0, 0, 2) | ||
| tx.set_node_account_ids([node_account_id]) | ||
| tx.set_transaction_id(transaction_id) | ||
|
|
||
| body_bytes = tx.build_transaction_body().SerializeToString() | ||
| tx._transaction_body_bytes.setdefault(node_account_id, body_bytes) | ||
| tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Freeze the transaction before testing signing.
Both tests bypass freeze-time body construction. dict(node_account_id=body_bytes) creates the string key "node_account_id", not the AccountId key used by _to_proto(). sign() iterates only values, so these tests pass without validating the transaction-ID and node-ID lookup contract.
tests/unit/topic_create_transaction_test.py#L231-L237: replace the manual map setup withtx.freeze()and readtx._transaction_body_bytes[transaction_id][node_account_id].tests/unit/topic_delete_transaction_test.py#L80-L86: replace the manual map setup withtx.freeze()and readtx._transaction_body_bytes[transaction_id][node_account_id].
Proposed fix
- body_bytes = tx.build_transaction_body().SerializeToString()
- tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes))
+ tx.freeze()
+ body_bytes = tx._transaction_body_bytes[transaction_id][node_account_id]As per path instructions, unit tests must protect against breaking changes and test real functionality.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tx = TopicCreateTransaction(memo="Signing test") | |
| tx.operator_account_id = AccountId(0, 0, 2) | |
| tx.set_node_account_ids([node_account_id]) | |
| tx.set_transaction_id(transaction_id) | |
| body_bytes = tx.build_transaction_body().SerializeToString() | |
| tx._transaction_body_bytes.setdefault(node_account_id, body_bytes) | |
| tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes)) | |
| tx = TopicCreateTransaction(memo="Signing test") | |
| tx.operator_account_id = AccountId(0, 0, 2) | |
| tx.set_node_account_ids([node_account_id]) | |
| tx.set_transaction_id(transaction_id) | |
| tx.freeze() | |
| body_bytes = tx._transaction_body_bytes[transaction_id][node_account_id] |
| tx = TopicCreateTransaction(memo="Signing test") | |
| tx.operator_account_id = AccountId(0, 0, 2) | |
| tx.set_node_account_ids([node_account_id]) | |
| tx.set_transaction_id(transaction_id) | |
| body_bytes = tx.build_transaction_body().SerializeToString() | |
| tx._transaction_body_bytes.setdefault(node_account_id, body_bytes) | |
| tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes)) | |
| tx = TopicDeleteTransaction(topic_id=topic_id) | |
| tx.operator_account_id = AccountId(0, 0, 2) | |
| tx.set_node_account_ids([node_account_id]) | |
| tx.set_transaction_id(transaction_id) | |
| tx.freeze() | |
| body_bytes = tx._transaction_body_bytes[transaction_id][node_account_id] |
📍 Affects 2 files
tests/unit/topic_create_transaction_test.py#L231-L237(this comment)tests/unit/topic_delete_transaction_test.py#L80-L86
Source: Path instructions
| operator_id, _, node_account_id, _, _ = mock_account_ids | ||
| transaction_id = TransactionId.generate(operator_id) | ||
|
|
||
| tx = TopicUpdateTransaction(topic_id=topic_id, memo="Signature test") | ||
| tx.operator_account_id = AccountId(0, 0, 2) | ||
| tx.set_node_account_ids([node_account_id]) | ||
|
|
||
| body_bytes = tx.build_transaction_body().SerializeToString() | ||
| tx._transaction_body_bytes.setdefault(node_account_id, body_bytes) | ||
| tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use the actual node account ID as the nested key.
dict(node_account_id=body_bytes) creates the literal string key "node_account_id". The cache must use the AccountId value from node_account_id. Otherwise, this test does not validate the node-specific body lookup.
As per path instructions, unit tests must protect transaction behavior and use the actual SDK data contract.
Proposed fix
- tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes))
+ tx._transaction_body_bytes.setdefault(transaction_id, {node_account_id: body_bytes})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| operator_id, _, node_account_id, _, _ = mock_account_ids | |
| transaction_id = TransactionId.generate(operator_id) | |
| tx = TopicUpdateTransaction(topic_id=topic_id, memo="Signature test") | |
| tx.operator_account_id = AccountId(0, 0, 2) | |
| tx.set_node_account_ids([node_account_id]) | |
| body_bytes = tx.build_transaction_body().SerializeToString() | |
| tx._transaction_body_bytes.setdefault(node_account_id, body_bytes) | |
| tx._transaction_body_bytes.setdefault(transaction_id, dict(node_account_id=body_bytes)) | |
| operator_id, _, node_account_id, _, _ = mock_account_ids | |
| transaction_id = TransactionId.generate(operator_id) | |
| tx = TopicUpdateTransaction(topic_id=topic_id, memo="Signature test") | |
| tx.operator_account_id = AccountId(0, 0, 2) | |
| tx.set_node_account_ids([node_account_id]) | |
| body_bytes = tx.build_transaction_body().SerializeToString() | |
| tx._transaction_body_bytes.setdefault(transaction_id, {node_account_id: body_bytes}) |
Source: Path instructions
github-actions
Bot
added
open to community review
danielmarv
left a comment
danielmarv
left a comment
There was a problem hiding this comment.
Thanks @manishdait for the implementation but i have some blocking issues that we need to address:
query_payment.pywasn't migrated for either half of this change (body-bytes shape, andbuild_transaction_body()no longer stamping IDs), which breaks every paid query at runtime. Comments ontransaction.py:58andtransaction.py:486.- Single-chunk topic messages now emit
chunkInfo. Comment ontopic_message_submit_transaction.py:163.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.py (1)
163-168: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winBLOCKER: Add an explicit
_initial_transaction_idguard.Line 165 calls
_to_proto()without checking_initial_transaction_id. The_current_chunk_indexcheck does not establish that invariant. Raise a descriptiveValueErrorbefore constructingchunk_info.Proposed fix
if self._total_chunks > 1 and self._current_chunk_index is not None: + if self._initial_transaction_id is None: + raise ValueError( + "Multi-chunk message requires an initial transaction ID." + ) chunk_info = consensus_submit_message_pb2.ConsensusMessageChunkInfo(As per path instructions, multi-chunk serialization must guard a missing
_initial_transaction_idbefore calling_to_proto().Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 999ac881-12aa-429a-a4a4-8b35f433021b
📒 Files selected for processing (2)
src/hiero_sdk_python/consensus/topic_message_submit_transaction.pytests/unit/topic_message_submit_transaction_test.py
aceppaluni
left a comment
aceppaluni
left a comment
There was a problem hiding this comment.
Follow up to the earlier conversation regarding query.py:
Would it be useful to grep the repository for all direct callers of build_transaction_body() outside the normal freeze path and make sure none relied on IDs being injected there?
aceppaluni
added
the
status: update branch
70e91af to
8359352
Compare
35d0642 to
c916936
Compare
github-actions
Bot
added
open to community review
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @manishdait i think the main thing that is diffficult about this PR in my eyes is some of the methods are not that clear, due to existing causes, but makes new changes hard to reivew, maybe creating more sub functions would help, but then again perhaps that changes too much
| ) | ||
| return consensus_submit_message_pb2.ConsensusSubmitMessageTransactionBody( | ||
| topicID=self.topic_id._to_proto() if self.topic_id else None, | ||
| message=chunk_content, |
There was a problem hiding this comment.
maybe we can make a _current_chunk_slice in chunked_transaction.py and use it here
|
|
||
| def _execute_single(self, url: str, mode: FeeEstimateMode) -> FeeEstimateResponse: | ||
| data = self._post(url, self._transaction.to_bytes()) | ||
| data = self._post(url, self._transaction._to_proto().SerializeToString()) |
There was a problem hiding this comment.
maybe can be a to_bytes()
There was a problem hiding this comment.
we can use that but it will change when, the to_bytes method get change for the serialization pr
exploreriii
removed
queue:junior-committer
e2b29d7 to
15ccb44
Compare
15ccb44 to
d1319dc
Compare
392840c to
83cb915
Compare
…action is freeze Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
Signed-off-by: Manish Dait <daitmanish88@gmail.com>
83cb915 to
7056064
Compare
5 participants
Description:
This PR fix the ChunkTransaction to make the transaction_bytes for every chunk once freeze instead of during execution
Changes Made
_transaction_idsto Transaction classTransaction._transaction_body_bytestosict[TransactionId, dict[AccountId, bytes]]freeze()method for the ChunkTransactionRelated issue(s):
Fixes #2480
Notes for reviewer:
Transaction._transaction_body_bytestosict[TransactionId, dict[AccountId, bytes]], should not affect backward compatibility it is internal use onlybuild_base_transactionfrom_bytes/to_bytescause it need to refactor in follow up PR.Checklist