fix(wallet): allow fee bump without opt-in RBF signaling

[alienx5499]

Description

Fixes #65.

Wallet::build_fee_bump previously refused to build a replacement when no input signaled opt-in RBF (nSequence ≤ 0xFFFFFFFD). That guard is removed so the wallet can construct a fee-bump transaction even if the original did not opt in. Whether a replacement is accepted is a mempool / relay policy concern (e.g. default full-RBF in recent Bitcoin Core), not something this API can enforce by requiring a specific nSequence on the prior tx.

Also removes BuildFeeBumpError::IrreplaceableTransaction, since that path is no longer reachable.

Notes to the reviewers

• API: BuildFeeBumpError::IrreplaceableTransaction is removed (breaking public enum). If you prefer a deprecation cycle for the variant only, say so and we can adjust.
• Semantics: Callers could previously rely on an error when the original tx did not signal opt-in RBF; they must not assume that anymore.
• Tests: tests/build_fee_bump.rs — the old test expected failure for non–opt-in-RBF; it now asserts a successful fee bump with a higher fee rate.

Changelog notice

fix(wallet): allow build_fee_bump without opt-in RBF on the original tx; remove IrreplaceableTransaction (#65)

Checklists

All Submissions:

• I've signed all my commits
• I followed the contribution guidelines
• I ran just p before pushing

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

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.04%. Comparing base (fb7681a) to head (db7feac).
⚠️ Report is 67 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #430   +/-   ##
=======================================
  Coverage   80.04%   80.04%           
=======================================
  Files          24       24           
  Lines        5336     5326   -10     
  Branches      242      241    -1     
=======================================
- Hits         4271     4263    -8     
+ Misses        987      985    -2     
  Partials       78       78           

Flag	Coverage Δ
rust	80.04% <ø> (+<0.01%)	⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

[alienx5499]

Hi @ValuedMammal, gentle ping for review when you have time. Thanks!

[ValuedMammal]

Approach ACK

[yan-pi]

Approach ACK

Removing the sequence check makes sense to me. build_fee_bump constructs the replacement, while acceptance depends on the node's mempool policy.

The new test passes on db7feac:

cargo test --test build_fee_bump test_bump_fee_tx_without_rbf_signaling

just check on the PR branch hits the deprecations fixed in #455. To make sure those were unrelated to this change, I applied the diff on top of fc88144 and ran:

cargo test --test build_fee_bump
just check test doc

Both pass. But could you rebase the PR onto the current master?

In my local Core 30 regtest, a non-RBF original was replaced successfully with full-RBF enabled.

I left two small comments on the docs and test. Neither is blocking.

[yan-pi]

Could we make this specific to the builder? The first sentence reads like a general replacement rule.

Maybe:

Suggested change

	/// Replacing an unconfirmed transaction does not require the original to signal opt-in RBF
	/// (`nSequence` ≤ `0xFFFFFFFD`). Relay and mining acceptance depend on local policy; Bitcoin
	/// Core 28+ defaults to full-RBF mempool relay, while other implementations or configs may
	/// differ.
	///
	/// This builder does not require the original transaction to signal opt-in RBF
	/// (`nSequence` <= `0xFFFFFFFD`). Whether the resulting replacement is accepted
	/// for relay or mining depends on the local policy of the broadcasting node.

[yan-pi]

Would it be worth asserting the two properties this test is covering: that the original does not explicitly signal RBF, and that the replacement shares a prevout with it?

The current assertions only check that the fee increased and the txid changed.

Maybe something like this before moving tx into the wallet:

let tx = psbt.extract_tx().expect("failed to extract tx");
assert!(!tx.is_explicitly_rbf());
let original_prevouts = tx
    .input
    .iter()
    .map(|txin| txin.previous_output)
    .collect::<Vec<_>>();
let txid = tx.compute_txid();
insert_tx(&mut wallet, tx);

And this after extracting new_tx:

assert!(
    new_tx
        .input
        .iter()
        .any(|txin| original_prevouts.contains(&txin.previous_output)),
);

[alienx5499]

Thanks for the review @yan-pi ! I've updated the docstring in build_fee_bump as suggested and added the assertions to the test to verify !is_explicitly_rbf() and that the replacement shares a prevout. I've also rebased the branch onto the latest master.

[yan-pi]

tACK 4955fea

Thanks, both comments are addressed.
Everything passes.