fix(wallet)!: make TxBuilder::current_height type-safe via absolute::Height

[none34829]

Description

Closes #49.

TxBuilder::current_height and the fallback to chain.tip().height() in create_tx both called absolute::LockTime::from_height(h).expect(...), which panics whenever h >= 500_000_000. The Wizardsardine audit flagged this because it lets a remote Electrum/Esplora server crash the wallet by reporting a tip height at or above that threshold — panics on externally-provided input are unsafe for a library.

This PR replaces the panics with a typed error:

• TxParams.current_height now stores a raw u32 instead of an already-validated absolute::LockTime, so the builder setter no longer has to validate in a position where it can't return a Result.
• Wallet::create_tx performs the u32 -> LockTime conversion in one place, mapping the ConversionError to a new CreateTxError::InvalidCurrentHeight(u32) variant via ?.
• Both the explicit-caller path (TxBuilder::current_height) and the implicit-fallback path (chain tip) now flow through the same validation and surface the same error.

Notes to the reviewers

• The change to TxParams.current_height is crate-private (pub(crate)) so there's no external API break there. The TxBuilder::current_height public signature is unchanged.
• Adding a new variant to CreateTxError is technically an additive change, but it is an enum so downstream exhaustive matches would need to be updated. CreateTxError is not marked #[non_exhaustive], which is tracked separately in Consider making all error enum variants #[non_exhaustive] #239.
• Follows the audit recommendation: "it's safer to only panic on inconsistent internal state and not on externally provided inputs."

Changelog notice

• Fixed: Wallet::create_tx now returns CreateTxError::InvalidCurrentHeight instead of panicking when TxBuilder::current_height or the fallback chain tip height is >= 500_000_000.

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran just p before pushing (140 lib tests + 118 wallet + 22 fee_bump + 10 persisted + 13 descriptor_macro + 54 doctests all pass; clippy clean; fmt clean; cargo doc clean)

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.21%. Comparing base (35a87f3) to head (6dac1a0).

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #449   +/-   ##
=======================================
  Coverage   80.21%   80.21%           
=======================================
  Files          24       24           
  Lines        5348     5348           
  Branches      242      242           
=======================================
  Hits         4290     4290           
  Misses        980      980           
  Partials       78       78           

Flag	Coverage Δ
rust	80.21% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[none34829]

Thanks for the review- the type-safe approach is definitely cleaner.

• TxBuilder::current_height now takes absolute::Height instead of u32, so the conversion to LockTime is infallible via impl From<Height> for LockTime. No expect, no new error variant.
• The chain tip fallback in create_tx keeps the expect, but now with a message noting that an invalid LocalChain tip height is an upstream invariant violation rather than silently swallowing it.
• CreateTxError::InvalidCurrentHeight removed entirely, along with the test that exercised it (the error case is gone).
• Existing test call sites (test_create_tx_custom_locktime, test_spend_coinbase) updated to construct the height via absolute::Height::from_consensus(h).unwrap().

[none34829]

CI failures here are unrelated to this PR- bitcoin-units 0.1.3 was published and deprecated FeeRate::from_sat_per_vb_unchecked, which breaks clippy -D warnings on master too (47 errors locally when I reproduce against master).

I've opened #455 with a focused mechanical rename to from_sat_per_vb_u32. Once that lands I'll rebase this PR on top and CI should go green.

[yan-pi]

NACK (6dac1a0) on the current patch as a resolution for #49.

Accepting absolute::Height in the explicit setter is a good API improvement, but the fallback still panics for a height accepted through Wallet::apply_update and Wallet::load. I reproduced both paths at height 500_000_000 on the current head.

I think you can keep the typed setter, make the fallback conversion return CreateTxError::InvalidCurrentHeight, and add the regression test.

The PR description and checklist also need to be updated: they describe the old force-pushed implementation, the current signature is API-breaking, and the branch needs a rebase.

[yan-pi]

The invariant described here is not enforced by LocalChain.

CheckPoint::new accepts a BlockId with any u32 height, CheckPoint::push only checks ordering, and LocalChain::from_changeset/apply_update do not check the locktime threshold.

The update below succeeds, after which transaction creation reaches this expect:

let tip = wallet.latest_checkpoint().push(BlockId {
    height: 500_000_000,
    hash: BlockHash::all_zeros(),
}).unwrap();

wallet.apply_update(Update {
    chain: Some(tip),
    ..Default::default()
}).unwrap();

let mut builder = wallet.build_tx();
builder.add_recipient(script, amount);
let _ = builder.finish();

I reproduced the same panic after taking the staged ChangeSet and loading it with Wallet::load().load_wallet_no_persist(...).

I'd keep absolute::Height for the explicit setter and make the fallback conversion return CreateTxError::InvalidCurrentHeight instead of relying on this invariant.

Please also add regression tests for the update and reload paths. The current tests only wrap existing valid constants in Height::from_consensus(...).unwrap() and do not exercise the audit finding.