fix: preserve raw nTxType bytes on pre-DIP-0002 transactions#726
Conversation
PR #675 made version=0 transactions decode as Classic, but it dropped the original on-wire `nTxType` u16. After decode, `consensus_encode`/ `txid` re-emitted `tx_type()` as 0, breaking byte-exact round-trip and changing the txid for the malformed mainnet transaction. Add a `TransactionType::ClassicalWithNonStandardVersionTypeBytes(u16)` variant (and a matching pseudo-payload `TransactionPayload:: ClassicalWithNonStandardVersionTypeBytesPayloadType(u16)`) that carry the raw u16 read from the wire. Encoding, txid, and sighash now emit those bytes verbatim, and the encoder skips the payload section since pre-DIP-0002 transactions have no payload section on the wire (not even a length prefix). `#[repr(u16)]` is dropped from `TransactionType` since the enum is no longer field-less; replaced `(... as u16)` casts with a new `to_u16()` method. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ 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. Review rate limit: 0/1 reviews remaining, refill in 33 minutes and 6 seconds.Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #726 +/- ##
=============================================
+ Coverage 70.84% 70.86% +0.02%
=============================================
Files 319 319
Lines 68184 68243 +59
=============================================
+ Hits 48305 48361 +56
- Misses 19879 19882 +3
|
…ck tx builders Same root cause as the prior commit: rust-dashcore's tightened decoder (PR dashpay/rust-dashcore#726, in the v0.42-dev bump) refuses to consume a special-transaction payload when the outer Transaction's version is < 3 with a non-zero nTxType, so any AssetLock transaction built with version 0 round-trips with trailing payload bytes and fails decode. The earlier commit only fixed the dpp test fixture. Three other places build AssetLock transactions the same broken way and surface in CI as "data not consumed entirely" failures across drive-abci strategy tests (address_funding_from_asset_lock + address_credit_withdrawal flows): - packages/strategy-tests/src/transitions.rs (two builders used by the drive-abci strategy_tests harness for fixed and dynamic-amount asset locks) - packages/rs-dpp/.../validate_asset_lock_transaction_structure/v0/mod.rs (test-only make_asset_lock_transaction helper) Switch each outer Transaction.version from 0 to 3. The inner AssetLockPayload.version (a separate u8 field) is left alone. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Follow-up to #675. That PR made
version == 0transactions decode as Classic so they wouldn't be rejected onUnknownSpecialTransactionType, but it dropped the original on-wirenTxTypeu16. After decode,consensus_encode()andtxid()re-emittedself.tx_type()as0, so the malformed mainnet transaction no longer round-tripped and its txid changed after decode.This PR introduces a new variant pair that preserves the raw u16:
TransactionType::ClassicalWithNonStandardVersionTypeBytes(u16)TransactionPayload::ClassicalWithNonStandardVersionTypeBytesPayloadType(u16)The pseudo-payload variant carries the raw bytes through the existing
Option<TransactionPayload>slot onTransaction, so no new struct field is added.tx_type()derives theTransactionTypevariant from it via the existingfrom_optional_payloadpath.Encoder / txid / sighash
(self.tx_type() as u16)is replaced withself.tx_type().to_u16()(had to drop#[repr(u16)]since the enum now has a tuple variant).txid(), and the sighash path skip the payload-section block when the payload is the new variant — pre-DIP-0002 transactions have no payload section on the wire (not even a length prefix), so emitting one would change the bytes.Decoder
For
version == 0:== 0→Classic(existing behavior preserved)!= 0→ClassicalWithNonStandardVersionTypeBytes(raw)(new path)Wallet routing
TransactionRouter::classify_transactiontreats the new variant as logically Classic — falls through to the standard / coinbase / coinjoin classification.Test plan
test_pre_dip2_classical_with_non_standard_version_type_bytes_roundtrip:version=0, nTxType=0x002Aand asserts the variant is preservedserializeround-triptxid() == Txid::hash(tx_bytes)test_transaction_versionstill passes (version=0, type=0still maps to plain Classic)cargo test --workspace --lib— 532 dashcore + 461 key-wallet + others all passcargo clippy --workspace --all-features --all-targets -- -D warningscleancargo fmt --checkclean🤖 Generated with Claude Code