fix: use Option<CryptoHash> for Transaction block_hash#135
fix: use Option<CryptoHash> for Transaction block_hash#135
Conversation
The RPC `SignedTransactionView` response contains the transaction hash but not the block hash that was used when signing. Previously, the `TryFrom<SignedTransactionView>` conversion incorrectly placed the transaction hash into the `block_hash` field, causing `get_hash()` to return wrong values. This changes `block_hash` to `Option<CryptoHash>` on `TransactionV0` and `TransactionV1`, so consumers are forced to handle the missing case. Custom borsh serialize/deserialize helpers preserve the on-chain wire format. The `SignedTransaction` hash is now pre-populated from the RPC response so `get_hash()` returns the correct value. Closes #134
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
=======================================
Coverage 50.90% 50.90%
=======================================
Files 40 40
Lines 5265 5298 +33
=======================================
+ Hits 2680 2697 +17
- Misses 2585 2601 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let signed = Self::new(Signature::from_str(&signature)?, transaction); | ||
| // Pre-populate with the correct hash from the RPC response, | ||
| // since we cannot recompute it without the block hash. | ||
| let _ = signed.hash.set(tx_hash); |
There was a problem hiding this comment.
signed.hash.set(tx_hash) returns a Result indicating whether the OnceLock was initialized. Ignoring it can mask unexpected state (e.g., if SignedTransaction::new ever starts pre-populating the cache) and could reintroduce incorrect hashes silently. Consider handling the Result explicitly (e.g., expect with a message or propagate/return an error) so failures are visible.
| let _ = signed.hash.set(tx_hash); | |
| #[allow(clippy::expect_used)] | |
| signed | |
| .hash | |
| .set(tx_hash) | |
| .expect("SignedTransaction hash OnceLock unexpectedly initialized"); |
| fn borsh_ser_optional_hash<W: std::io::Write>( | ||
| val: &Option<CryptoHash>, | ||
| writer: &mut W, | ||
| ) -> Result<(), std::io::Error> { | ||
| let hash = val.ok_or_else(|| { | ||
| std::io::Error::new( | ||
| std::io::ErrorKind::InvalidData, | ||
| "cannot borsh-serialize a Transaction whose block_hash is None \ | ||
| (this transaction was deserialized from an RPC response that \ | ||
| lacks block hash information)", | ||
| ) | ||
| })?; | ||
| BorshSerialize::serialize(&hash, writer) |
There was a problem hiding this comment.
borsh_ser_optional_hash currently calls ok_or_else directly on &Option<CryptoHash>, which works because Option<CryptoHash> is Copy today, but is a bit non-obvious and introduces an unnecessary copy. Using as_ref()/as_deref() (or a match) to get a borrowed hash before serializing would make the intent clearer and avoid relying on Copy semantics.
Summary
Fixes #134 — the
TryFrom<SignedTransactionView>conversion was placing the transaction hash into theblock_hashfield, since the RPC response doesn't include the block hash used at signing time. This causedSignedTransaction::get_hash()to return incorrect values.block_hashfromCryptoHashtoOption<CryptoHash>onTransactionV0andTransactionV1, forcing consumers to handle the missing caseserialize_with/deserialize_withto preserve the on-chain wire format (panics with a clear message if you try to serialize aNoneblock hash)SignedTransactionhash cache with the correct tx hash from the RPC responseTransaction::block_hash()accessor returningOption<CryptoHash>Breaking change:
block_hashfield type changed fromCryptoHashtoOption<CryptoHash>onTransactionV0/TransactionV1.Test plan
cargo checkpassescargo test --libpasses (all 40 tests)Optiontype