Introduce canonicalization parameters

[evanlinjin]

Partially Fixes bitcoindevkit/bdk_wallet#40

Description

Add the ability to modify the canonicalization algorithm. Right now, the only modifier is assume_canonical which takes in a Vec (ordered list) of txids and superimposes it on the canonicalization algorithm. Txs later in the list (higher index) have a higher priority (in case of conflicts).

Notes to the reviewers

None at the moment.

Changelog notice

• Added CanonicalizationParams to allow the caller to modify the canonicalization algorithm. This in a new parameter on CanonicalIter::new.
• Changed TxGraph::insert_tx to allow for updating a transaction's witness field. This is useful for initially introducing an unsigned tx and adding witnesses later on.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran cargo fmt and cargo clippy before committing

New Features:

• I've added tests for the new feature
• I've added docs for the new feature

Bugfixes:

• This pull request breaks the existing API
• I've added tests to reproduce the issue which are now passing
• I'm linking the issue being fixed by this PR

[notmandatory]

Would it make sense to add CanonicalizationMods as a field of TxGraph ?

This way the existing TxGraph method signatures don't change, you'd just need a new function to add TxIds to the CanonicalizationMods in the TxGraph.

And if you could only add new TxIds to your CanonicalizationMods this should still be monotone right?

Then you could also add the CanonicalizationMods data to the tx_graph::Changeset so it's persisted. Of course you'd need to update the persistence crates but wouldn't users need/want to persist these mods anyway so a reloaded wallet shows the same balance and utxos?

For Wallet I'm imaging some higher level functions for the things a wallet user needs. For instance (based on suggestions in bitcoindevkit/bdk_wallet#40):

1. change TxBuilder::finish to insert new unbroadcast tx and add a canonical mod so their inputs won't be double spent
2. add a canonical mod for a new RBF tx so it has precedence over the unconfirmed tx it's replacing
3. list unbroadcast tx (found in CanonicalizationMods but not seen)
4. add an option to TxBuilder to include inputs from unbroadcast tx(s), the new tx has higher canonicalization
5. remove tx from CanonicalizationMods after seen in mempool (or would this make TxGraph not monotone?)

[LLFourn]

ConceptACK. Would be nice to have draft PR built on this that resolves bitcoindevkit/bdk_wallet#40 so we know that this is all that's needed at the chain level before merging this.

[oleonardolima]

cACK 0f00cdb

[notmandatory]

Took this out of the bdk_wallet 1.1 milestone since tagging the release tomorrow.

[evanlinjin]

Would it make sense to add CanonicalizationMods as a field of TxGraph ?

@notmandatory I do not think so. Different calls to canonicalize will be for different purposes. Especially when we add more fields to CanonicalizationMods (i.e. in #1809).

[evanlinjin]

And if you could only add new TxIds to your CanonicalizationMods this should still be monotone right?

We want ordering for these txids. In case of conflict between them, we need a way to prioritize ones over others. This order should be controlled by the caller.

[evanlinjin]

@LLFourn I have a draft PR (bitcoindevkit/bdk_wallet#220) which showcases that this API is enough for bdk_wallet to keep track of unbroadcasted transactions (and not accidentally replace them).

[LagginTimes]

Just a few consistency nits:

• Commit message 53d6fa1 needs to be updated to rename CanonicalizationMods → CanonicalizationParams.
• Same change needs to be done in the PR description changelog.

Besides these, this LGTM!

[LagginTimes]

ACK 53afb35

[ValuedMammal]

ACK 53afb35

All comments addressed

[ValuedMammal]

Looks good to me.

@LLFourn Please re-review if you find any issues