fix: apply BIP-69 sorting to asset lock credit outputs#622
Open
QuantumExplorer wants to merge 1 commit intov0.42-devfrom
Open
fix: apply BIP-69 sorting to asset lock credit outputs#622QuantumExplorer wants to merge 1 commit intov0.42-devfrom
QuantumExplorer wants to merge 1 commit intov0.42-devfrom
Conversation
The payload's credit_outputs were not sorted, making the transaction non-deterministic and potentially fingerprintable. Changes: - TransactionBuilder::build_asset_lock() now sorts credit outputs by BIP-69 (amount ascending, then scriptPubKey lexicographically) - Returns (Transaction, sorted_indices) so callers can re-map keys to the new sorted positions - ManagedWalletInfo::build_asset_lock() reorders derived keys to match the sorted output positions, so keys[i] always corresponds to payload.credit_outputs[i] Co-Authored-By: Claude Opus 4.6 <[email protected]>
Contributor
📝 WalkthroughWalkthroughThe changes modify the Changes
Sequence DiagramsequenceDiagram
participant Caller
participant AssetLockBuilder
participant TransactionBuilder
participant Sorter as BIP-69 Sorter
Caller->>AssetLockBuilder: build_asset_lock(credit_outputs)
AssetLockBuilder->>TransactionBuilder: build_asset_lock(credit_outputs)
TransactionBuilder->>Sorter: Sort outputs by value, then script_pubkey
Sorter-->>TransactionBuilder: sorted_outputs, original_indices
TransactionBuilder->>TransactionBuilder: Create AssetLockPayload with sorted_outputs
TransactionBuilder->>TransactionBuilder: Build transaction
TransactionBuilder-->>AssetLockBuilder: (transaction, original_indices)
AssetLockBuilder->>AssetLockBuilder: Collect derived keys from credit_outputs
AssetLockBuilder->>AssetLockBuilder: Reorder keys using original_indices
AssetLockBuilder-->>Caller: keys[sorted_pos] matches payload sorted position
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v0.42-dev #622 +/- ##
=============================================
- Coverage 67.60% 67.57% -0.03%
=============================================
Files 321 321
Lines 68258 68275 +17
=============================================
- Hits 46147 46140 -7
- Misses 22111 22135 +24
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs (1)
244-273: 🛠️ Refactor suggestion | 🟠 MajorPlease add a regression test for the new
sorted_indices→keyscontract.This is the first consumer of the new mapping, but the local tests below still only exercise error paths. A small success-path test with equal-value outputs and different
script_pubkeys would pin both the BIP-69 tie-breaker and the guarantee thatkeys[i]lines up withpayload.credit_outputs[i].As per coding guidelines, "Write unit tests for new functionality".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs` around lines 244 - 273, Add a regression unit test that exercises the success path of build_asset_lock/asset_lock_builder logic to verify the new sorted_indices→keys contract: construct a tx_builder_with_inputs scenario with multiple equal-value credit outputs but different script_pubkeys, call build_asset_lock to obtain sorted_indices and the resulting payload.credit_outputs, derive keys via the same flow (using credit_output_fundings and resolve_funding_account/next_private_key), and assert that for each i the derived keys[i] corresponds to the private key used for payload.credit_outputs[i] (i.e., keys ordering matches sorted_indices/BIP-69 tie-breaker). Target the asset_lock_builder.rs tests for a test that sets up equal-value outputs, uses credit_output_fundings, asserts no errors, and checks the keys-to-payload.credit_outputs alignment to prevent regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs`:
- Around line 637-659: The build_asset_lock() currently sets
self.special_payload only at the final build step, so callers like
asset_lock_builder::select_inputs() and calculate_fee*() never see the payload
bytes and can underselect; fix this by exposing the sorted AssetLockPayload and
indices prior to input selection—either (A) add a preparatory method on the
builder (e.g., prepare_asset_lock_payload(&mut self, credit_outputs: Vec<TxOut>)
-> Result<Vec<usize>, BuilderError>) that performs the BIP-69 sorting, installs
self.special_payload (or otherwise stores the payload) and returns
sorted_indices so select_inputs()/calculate_fee*() can include payload size, or
(B) change build_asset_lock() to factor out the sorting into a public helper
(e.g., sort_asset_lock_outputs(credit_outputs) -> (AssetLockPayload,
Vec<usize>)) that asset_lock_builder can call before select_inputs(); update
callers in asset_lock_builder.rs (the code around lines 227-245) to call the new
preparatory API so payload bytes are counted during fee/input selection.
---
Outside diff comments:
In `@key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs`:
- Around line 244-273: Add a regression unit test that exercises the success
path of build_asset_lock/asset_lock_builder logic to verify the new
sorted_indices→keys contract: construct a tx_builder_with_inputs scenario with
multiple equal-value credit outputs but different script_pubkeys, call
build_asset_lock to obtain sorted_indices and the resulting
payload.credit_outputs, derive keys via the same flow (using
credit_output_fundings and resolve_funding_account/next_private_key), and assert
that for each i the derived keys[i] corresponds to the private key used for
payload.credit_outputs[i] (i.e., keys ordering matches sorted_indices/BIP-69
tie-breaker). Target the asset_lock_builder.rs tests for a test that sets up
equal-value outputs, uses credit_output_fundings, asserts no errors, and checks
the keys-to-payload.credit_outputs alignment to prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50186ded-3f4c-4cfc-9aa9-fb044c3d029f
📒 Files selected for processing (2)
key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rskey-wallet/src/wallet/managed_wallet_info/transaction_builder.rs
Comment on lines
+637
to
+659
| pub fn build_asset_lock( | ||
| self, | ||
| credit_outputs: Vec<TxOut>, | ||
| ) -> Result<(Transaction, Vec<usize>), BuilderError> { | ||
| // Track original indices so callers can re-map keys | ||
| let mut indexed: Vec<(usize, TxOut)> = credit_outputs.into_iter().enumerate().collect(); | ||
|
|
||
| // BIP-69: sort by amount ascending, then scriptPubKey lexicographically | ||
| indexed.sort_by(|(_, a), (_, b)| match a.value.cmp(&b.value) { | ||
| std::cmp::Ordering::Equal => a.script_pubkey.as_bytes().cmp(b.script_pubkey.as_bytes()), | ||
| other => other, | ||
| }); | ||
|
|
||
| let sorted_indices: Vec<usize> = indexed.iter().map(|(orig, _)| *orig).collect(); | ||
| let sorted_outputs: Vec<TxOut> = indexed.into_iter().map(|(_, out)| out).collect(); | ||
|
|
||
| let payload = AssetLockPayload { | ||
| version: 0, | ||
| credit_outputs, | ||
| credit_outputs: sorted_outputs, | ||
| }; | ||
| self.set_special_payload(TransactionPayload::AssetLockPayloadType(payload)).build() | ||
| let tx = | ||
| self.set_special_payload(TransactionPayload::AssetLockPayloadType(payload)).build()?; | ||
| Ok((tx, sorted_indices)) |
Contributor
There was a problem hiding this comment.
This still hides asset-lock payload bytes from select_inputs().
special_payload is only created here, but key-wallet/src/wallet/managed_wallet_info/asset_lock_builder.rs, Lines 227-245, call select_inputs() and calculate_fee*() before invoking build_asset_lock(). Because the builder only counts payload bytes once self.special_payload is set, asset-lock builds can still underselect near the margin and return a fee that excludes the payload. Please expose the sorted payload/indices before selection, or add a preparatory API that lets callers install the payload earlier.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@key-wallet/src/wallet/managed_wallet_info/transaction_builder.rs` around
lines 637 - 659, The build_asset_lock() currently sets self.special_payload only
at the final build step, so callers like asset_lock_builder::select_inputs() and
calculate_fee*() never see the payload bytes and can underselect; fix this by
exposing the sorted AssetLockPayload and indices prior to input selection—either
(A) add a preparatory method on the builder (e.g.,
prepare_asset_lock_payload(&mut self, credit_outputs: Vec<TxOut>) ->
Result<Vec<usize>, BuilderError>) that performs the BIP-69 sorting, installs
self.special_payload (or otherwise stores the payload) and returns
sorted_indices so select_inputs()/calculate_fee*() can include payload size, or
(B) change build_asset_lock() to factor out the sorting into a public helper
(e.g., sort_asset_lock_outputs(credit_outputs) -> (AssetLockPayload,
Vec<usize>)) that asset_lock_builder can call before select_inputs(); update
callers in asset_lock_builder.rs (the code around lines 227-245) to call the new
preparatory API so payload bytes are counted during fee/input selection.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The asset lock payload's
credit_outputswere not BIP-69 sorted, making the transaction output ordering non-deterministic and potentially fingerprintable.Changes
TransactionBuilder::build_asset_lock()(Transaction, Vec<usize>)wheresorted_indices[i]= original index of the output now at sorted positioniManagedWalletInfo::build_asset_lock()result.keys[i]always corresponds topayload.credit_outputs[i]Why
BIP-69 mandates deterministic output ordering. The transaction's regular outputs were already sorted, but the payload's credit outputs were in caller-provided order. This inconsistency could fingerprint transactions built by this library.
Test plan
cargo clippy --workspace --all-targets -- -D warningsclean🤖 Generated with Claude Code
Summary by CodeRabbit