Fix/tx build should not change order of insertion with vector #262
Conversation
When TxBuilder::ordering is TxOrdering::Untouched, the insertion order of recipients and manually selected UTxOs should be preserved in transaction's output and input vectors respectively. Fixes bitcoindevkit#244
Pull Request Test Coverage Report for Build 15566821414Details
💛 - Coveralls |
| self.params.utxos.remove(idx); | ||
| } | ||
| self.params.utxos.push(wutxo); | ||
| } |
There was a problem hiding this comment.
Instead of removing elements can we just not push onto params.utxos if it duplicates an already added outpoint? This seems to be more aligned with the original behavior of filter_duplicates where the first item is kept and any duplicates that come later are ignored.
pub fn add_utxos(&mut self, outpoints: &[OutPoint]) -> Result<&mut Self, AddUtxoError> {
// We want to canonicalize once, instead of once for every call to `get_utxo`.
let unspent: HashMap<OutPoint, LocalOutput> = self
.wallet
.list_unspent()
.map(|output| (output.outpoint, output))
.collect();
// Ensure that only unique outpoints are added.
let mut visited = HashSet::new();
let utxos: Vec<WeightedUtxo> = outpoints
.iter()
.filter(|&&op| {
visited.insert(op)
&& !self
.params
.utxos
.iter()
.any(|wutxo| wutxo.utxo.outpoint() == op)
})
.map(|&op| -> Result<_, AddUtxoError> {
let output = unspent
.get(&op)
.cloned()
.ok_or(AddUtxoError::UnknownUtxo(op))?;
Ok(WeightedUtxo {
satisfaction_weight: self
.wallet
.public_descriptor(output.keychain)
.max_weight_to_satisfy()
.expect("descriptor should be satisfiable"),
utxo: Utxo::Local(output),
})
})
.collect::<Result<_, _>>()?;
self.params.utxos.extend(utxos);
Ok(self)
}There was a problem hiding this comment.
To bring back the original rationale about why favor precedence of new UTxOs over old ones, https://github.com/bitcoindevkit/bdk/pull/1798/files#r1974774295:
Ideally, we would error here if an
OutPointconflicts with a foreign UTXO. However, we will need to add new error variants (which is a MAJOR change since the error type does not havenon-exhaustive).For now, how about we make a note in the docs that if you add the same UTXO as both foreign and local, the latest change has precedence (please word that better).
Same goes for the add-foreign-utxo methods.
In the light of the above comment, do you think we should still roll back to the old precedence @ValuedMammal? @evanlinjin do you have any opinion on this?
I think we still prefer Utxo::Local over Utxo::Foreign, but the code above would not allow that.
There was a problem hiding this comment.
I guess that would work too.
Description
On my attempt to fix bitcoindevkit/bdk#1794 in bitcoindevkit/bdk#1798, I broke the assumption that insertion order is preserved when
TxBuilder::orderingisTxOrdering::Untouched. Some users are relying in this assumption, so here I'm trying to restore it back, without adding a new dependency for this single use case like #252, or creating a new struct just to track this.In this fourth alternative solution I'm going back to use
Vecto store the manually selected UTxOs.I was reluctant to do it in this way because
HashMapseems a better solution giving its property of avoiding duplicates, but as we also want to keep the secuential nature of the insertion of UTxOs inTxBuilder, here is an alternative aligned with that principle.May replace #252
May replace #261 .
Fixes #244
Notes to the reviewers
Also, as I was working on this, I came back to the following tests:
test_prexisting_foreign_utxo_have_no_precedence_over_local_utxo_with_same_outpointtest_prexisting_local_utxo_have_precedence_over_foreign_utxo_with_same_outpointMotivated during the implementation and review of bitcoindevkit/bdk#1798.
Which required the underlying structure to also hold the properties of no duplication for manually selected UTxOs, as the structures were accessed directly for these cases.
The test tries to cover the case when there are two wallets using the same descriptor, one tracks transactions and the other does not. The first passes UTxOs belonging to the second one and this one creates transactions using the
add_foreign_utxointerface.In this case it was expected for any
LocalUtxoof the offline wallet to supersede any conflicting foreign UTxO. But, the simulation of this case went against the borrowing constraints of rust.By how costly was to reproduce this behavior for me in the tests, I would like to have second opinions in the feasibility of the test case.
Changelog notice
No public APIs are changed by these commits.
Checklists
Important
This pull request DOES NOT break the existing API
cargo +nightly fmtandcargo clippybefore committing