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 81.22% 81.91% +0.68%
==========================================
Files 24 25 +1
Lines 5565 6529 +964
Branches 248 301 +53
==========================================
+ Hits 4520 5348 +828
- Misses 969 1081 +112
- Partials 76 100 +24
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 |
| /// 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
ebf57d7 to
9b35f7f
Compare
|
rebased on top of 7982b6e |
|
Since this moved to 4.0, I want to check if I understood the direction. coin-select#51 now has @ValuedMammal is that what you had in mind for #493: change the current coin-selection API, or keep the final Rules 4/5 check here and leave max-weight-aware selection for I also noticed the cap isn't fixed: it's 10k without an unconfirmed ancestor and 1k when the selection has one, so I think we may need two selection attempts. btw, I used AI while researching this, mostly to understand how the current coin-selection work fits together across the repos. |
|
@yan-pi I would say bring back the error you had originally designed for and keep the |
- 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.
9b35f7f to
eff5481
Compare
|
Ive restored the dedicated |
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.When the cap is exceeded,
create_txreturns the newCreateTxError::TrucSizeExceeded { cap_vb, actual_vb }, so callers can tell which rule fired and by how much.Adding a variant to the public
CreateTxErroris a breaking change, this is acceptable since the PR targets the 4.0 release.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: