feat(test): add legacy descriptor tests

[luisschwab]

Description

This PR closes #134 and is a revival of bitcoindevkit/bdk#1130, which adds the following tests:

• test_legacy_bump_fee_no_change_add_input_and_change()
• test_legacy_bump_fee_add_input()
• test_legacy_bump_fee_drain_wallet()
• test_legacy_bump_fee_zero_abs()
• test_legacy_create_tx_custom_sighash()
• test_legacy_create_tx_default_sighash()
• test_legacy_create_tx_absolute_high_fee()
• test_legacy_create_tx_absolute_zero_fee()
• test_legacy_create_tx_absolute_fee()
• test_legacy_create_tx_custom_fee_rate()
• test_legacy_get_funded_wallet_tx_fee_rate()

Changelog:

• Add the above mentioned tests.
• Add a new assert_fee_rate_legacy! macro for legacy transactions.
• The check_fee! macro now returns Amount instead of Result<Amount>.
• All wallet.sent_and_received got destructured to (sent, received) instead of sent_and_received.{0,1}.
• Swap TweakedKeypair::to_inner() for TweakedKeypair::to_keypair().

Checklists

All Submissions:

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

[coveralls]

Pull Request Test Coverage Report for Build 15303301662

Details

• 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.005%) to 85.349%

---

Totals
Change from base Build 15166368710:	0.005%
Covered Lines:	7276
Relevant Lines:	8525

---

💛 - Coveralls

[ValuedMammal]

Side note: I think check_fee! should just return the bitcoin Amount unwrapped instead of being forced to deal with the Option everywhere. If we fail to calculate the fee it should probably just panic to indicate a failing test.

bdk_wallet/wallet/tests/wallet.rs

Lines 778 to 785 in 7cdc985

	macro_rules! check_fee {
	($wallet:expr, $psbt: expr) => {{
	let tx = $psbt.clone().extract_tx().expect("failed to extract tx");
	let tx_fee = $wallet.calculate_fee(&tx).ok();
	assert_eq!(tx_fee, $psbt.fee_amount());
	tx_fee
	}};
	}

[ValuedMammal]

Code review ACK 71ae907

[luisschwab]

Should I fix this clippy warning? It's unrelated.

error: use of deprecated method `bitcoin::key::TweakedKeypair::to_inner`: use to_keypair() instead
   --> wallet/src/wallet/signer.rs:580:14
    |
580 |             .to_inner(),
    |              ^^^^^^^^
    |
    = note: `-D deprecated` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(deprecated)]`

[ValuedMammal]

I would say go ahead and change TweakedKeypair::to_inner to use to_keypair instead. For context see rust-bitcoin/rust-bitcoin#4450.

[luisschwab]

@ValuedMammal fixed.

[ValuedMammal]

It looks good overall, tests are passing for me.

[tvpeter]

@ValuedMammal fixed.

When I updated to a5d1338, the tests failed locally because the compiler was using bitcoin version 0.32.5.

--> wallet/src/wallet/signer.rs:580:14
    |
578 |           None => keypair
    |  _________________-
579 | |             .tap_tweak(secp, psbt_input.tap_merkle_root)
580 | |             .to_keypair(),
    | |             -^^^^^^^^^^ method not found in `TweakedKeypair`
    | |_____________|
    |
    

I temporarily updated the version to 0.32.6, and after that, the compiler used the new version, and the tests passed. I'm uncertain if it's ideal to upgrade to 0.32.6, and I'm curious if anyone else has experienced the same issue.

[ValuedMammal]

In a1d58c3: fix: clippy

Just noting that I don't think this is actually needed until we update the rust-version of the repo to rust 1.86. But I don't see a problem with including the change anyway.

[luisschwab]

Just noting that I don't think this is actually needed until we update the rust-version of the repo to rust 1.86. But I don't see a problem with including the change anyway.

Yes, no harm in just preemptively adding this though.

[luisschwab]

I temporarily updated the version to 0.32.6, and after that, the compiler used the new version, and the tests passed. I'm uncertain if it's ideal to upgrade to 0.32.6, and I'm curious if anyone else has experienced the same issue.

Yeah, my linter and CI were complaining so I updated it.

[luisschwab]

Considering how large the tests/wallet.rs file is, will it be ideal to someday organize the test cases into modules based on functionality? something like persistence, tx creation, etc

Agree, I opened #238 for this.

[ValuedMammal]

In 78eca98: fix clippy

This should be squashed down since it fixes a syntax error in eb50d6b.

[ValuedMammal]

ACK 833ed78

Thanks @luisschwab

h/t @realeinherjar

BOOM

[notmandatory]

ACK 833ed78

[oleonardolima]

tACK 833ed78

[ValuedMammal]

ACK 5dcf128