feat(wallet): add support for bip-431 rules 4 and 5#493
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #493 +/- ##
==========================================
+ Coverage 80.96% 81.13% +0.17%
==========================================
Files 24 24
Lines 5489 5544 +55
Branches 247 254 +7
==========================================
+ Hits 4444 4498 +54
Misses 968 968
- Partials 77 78 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
a8c7d37 to
634d476
Compare
|
@yan-pi I updated my branch to remove the bdk_testenv dependency, you can rebase this one to get a green CI (though you might need to update your tests). |
634d476 to
1e9f37e
Compare
|
@yan-pi the CI still failing |
1e9f37e to
1ae1a28
Compare
|
Rebased this on top of the updated #478 branch. I dropped the old |
oleonardolima
changed the title
| /// TRUC (BIP-431) virtual size cap exceeded. | ||
| /// | ||
| /// `cap_vb == 10_000` means Rule 4 (any TRUC tx). | ||
| /// `cap_vb == 1_000` means Rule 5 (TRUC tx with unconfirmed TRUC ancestor). | ||
| TrucSizeExceeded { | ||
| /// The cap that was exceeded, in virtual bytes. | ||
| cap_vb: u64, | ||
| /// The estimated virtual size of the candidate transaction. | ||
| actual_vb: u64, | ||
| }, |
There was a problem hiding this comment.
@yan-pi did you manage to do it as a non-breaking change ? I agree that having the error would be the right approach, though it would only land in the next major release.
There was a problem hiding this comment.
I dropped the new variant and mapped the TRUC size-cap rejection to the existing CreateTxError::CoinSelection(InsufficientFunds { .. }) instead.
This one seems to be the best one to handle the current error without the breaking change.
1ae1a28 to
ebf57d7
Compare
| if estimated_vb > cap_vb { | ||
| let available = coin_selection.selected_amount(); | ||
| return Err(CreateTxError::CoinSelection(InsufficientFunds { | ||
| needed: available + Amount::from_sat(1), | ||
| available, | ||
| })); |
There was a problem hiding this comment.
I'm worried this won't adequately convey the true cause of the error - that the vsize limit was exceeded. Probably the right way is to pass a max-weight parameter to a coin selector which selects inputs while the total weight is within the size limit or fails if the cap is exceeded before the target is funded.
There was a problem hiding this comment.
I agree with the concern.
I talked with @oleonardolima too, and our current thinking is to keep this PR non-breaking. InsufficientFunds is not the ideal error, but adding TrucSizeExceeded would be a breaking API change
Longer term, I agree this should probably be modeled as a coin-selection constraint, not just a post-selection check
And AFAIK bdk_tx is still being built, that maybe would be better for this kind of policy-aware transaction construction
For now, I’d like to keep this PR as the minimal non-breaking guard because Second/Ark users need Rules 4/5 enforced now, and without this BDK can return PSBTs that bitcoind rejects
Happy to track the cleaner design as a follow-up
- add new step in `filter_utxos` to validate BIP-431 rule 2, which validates the proper usage of unconfirmed TRUC/non-TRUC ancestor in a TRUC/non-TRUC tx. - update docs explaining the manual selection caveat when building non-TRUC/TRUC txs.
- introduce `test_create_and_spend_from_tx` to exercise BIP-431 rule-2 filtering in-memory, not relying on `bdk_testenv`/`bdk_electrum`. NOTE: I asked Claude to remove the `bdk_testenv` dependency, keeping the test behavior in-memory. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
- adds `CreateTxError::TrucSizeExceeded { cap_vb, actual_vb }`.
- rejects v3 txs over 10,000 vB (Rule 4), or over 1,000 vB when the
tx has an unconfirmed TRUC ancestor (Rule 5).
- adds private helper `estimate_truc_vsize` using plain weight/4.
- covers rejection and acceptance for both rules, plus a v2 case to ensure non-TRUC builds aren't affected. - end-to-end test funds via regtest electrum, broadcasts a v3 parent, and exercises both the Rule 5 rejection and the small-child acceptance paths through the mempool.
ebf57d7 to
9b35f7f
Compare
|
rebased on top of 7982b6e |
4 participants
partially addresses #477
depends on #478
solves #484
solves #485
Description
Enforces BIP-431 Rules 4 and 5 inside
create_tx, before returning a PSBT:Without this,
build_tx().version(3).finish()can hand back a PSBT that bitcoind rejects at broadcast.The check maps TRUC size-cap failures to the existing
CreateTxError::CoinSelection(InsufficientFunds { .. })to keep this PR non-breaking.A dedicated
CreateTxError::TrucSizeExceeded { cap_vb, actual_vb }would be a better long-term API, but adding it now would be breaking becauseCreateTxErroris public and not#[non_exhaustive].This adds
estimate_truc_vsizeas a private helper and reuses theis_truchelper introduced in #478.This does not address Rules 1–3 (topology / package limits), that's a separate concern tracked in #477 and #478.
Notes to the reviewers
The check runs after
coin_selectand beforecomplete_transaction.CoinSelection::coin_selectconsumesVec<WeightedUtxo>and returnsVec<Utxo>, so the per-inputsatisfaction_weightis gone by the time we'd want it.We pre-collect a
HashMap<OutPoint, Weight>from theWeightedUtxolist before handing it to the selector.The map is only allocated when
version == 3.For the Rule 5 ancestor check, foreign UTXOs are conservatively treated as non-TRUC.
Same posture as the existing TRUC filter in
filter_utxos.We use plain
weight / 4. Bitcoind's TRUC check uses sigop-adjusted vsize (max(weight, sigops * 20) / 4), which matches plain weight/4 for P2WPKH, P2TR and ordinary P2WSH.#477 should have a follow-up to track proper sigop accounting.
Changelog notice
Checklists
All Submissions:
just pbefore pushingNew Features:
Bugfixes: