Conversation
|
elf sizes
Stack consumption summary
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #494 +/- ##
===========================================
+ Coverage 72.36% 72.78% +0.41%
===========================================
Files 71 75 +4
Lines 10347 11196 +849
Branches 1876 2124 +248
===========================================
+ Hits 7488 8149 +661
- Misses 2113 2169 +56
- Partials 746 878 +132
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:
|
407e2b5 to
308dab3
Compare
2b4b96c to
ac59da0
Compare
927271f to
6719dec
Compare
0d73bc4 to
3722207
Compare
|
There was a problem hiding this comment.
Started looking at and playing with.
Few Claude's comments I will need to decode yet:
cleartext Implementation Review
Review of the cleartext wallet-policy feature on the cleartext branch of
app-bitcoin-new (the C base app), compared against the Rust reference
implementation in vanadium/apps/bitcoin/bip388.
Findings are ranked from most severe to least severe.
1. Tap-leaf ordering can diverge from the reference canonical order
Severity: high — correctness, user-visible
leaf_cmp (src/common/cleartext.c:471-520) compares bindings positionally in
pattern order. For a Multisig / SortedMultisig leaf the bindings are
[threshold, keys], so C sorts by threshold first, then key count. The
reference display_cmp
(vanadium/apps/bitcoin/bip388/src/cleartext/mod.rs:194-197) sorts by
key count first, then threshold:
k1.len().cmp(&k2.len()).then(t1.cmp(t2)).then_with(|| cmp_keys(k1,k2))These disagree whenever two same-class multisig leaves have threshold order and
key-count order inverted. For
tr(@0/**,{multi_a(3,@1..@4),multi_a(2,@5..@9)}) the reference produces
(verified):
Any 3 of @1, @2, @3 and @4 must sign
Any 2 of @5, @6, @7, @8 and @9 must sign
but C's leaf_cmp emits the 2-of-5 first (threshold 2 < 3). This is a
fully-cleartext taproot, so it is user-visible, and it breaks the
canonical-ordering assumption the Rust reverse-decoder relies on. No current
vector exercises it, so the unit test passes.
Fix: for the multisig classes, compare key count → threshold → keys
explicitly rather than relying on positional binding order. Add a regression
vector.
2. No CI guard that generated C matches the TOML
Severity: high — maintainability, can ship wrong code silently
cleartext_match.c, cleartext_specs.c, and
unit-tests/cleartext_vectors.inc.c are committed generated artifacts, but
nothing fails the build if someone edits specs/bip388/cleartext.toml without
rerunning gen.py, or hand-edits the generated C.
Fix: a one-line CI step — run gen.py, then git diff --exit-code. Highest
value-to-cost ratio of anything here.
3. (unknown) leaves weaken the stated guarantee
Severity: high — security-relevant; hard blocker for the opt-in feature
cleartext_encode (src/common/cleartext.c:989-999) renders every
unrecognized leaf as the fixed "(unknown)", where the reference prints
"Raw policy: <descriptor>". The framework's premise is that a cleartext
backup suffices to recover the template; (unknown) does not — two different
unknown leaves are indistinguishable.
It is safe today only because such policies always have
has_cleartext == false, so the descriptor template is always shown and the
cleartext screen is suppressed. But:
- the C output is untestable against the shared vectors for these cases
(unit-tests/test_cleartext.c:119skips text comparison when
!cleartext_flag) — that path has zero text coverage; - it is a landmine for the planned "hide descriptor template" opt-in.
Fix: add even a minimal AST unparser (multisig / pk only), or at minimum
document this as a hard prerequisite for the opt-in.
4. Two diverging copies of the spec
Severity: medium — drift risk
This repo's specs/bip388/cleartext.toml + test_vectors.toml are independent
from vanadium's. They are the "single source of truth" for two implementations
that can silently drift apart.
Fix: vendor one canonically (submodule or sync script), and/or a CI diff
between the two trees.
5. leaf_cmp's CT_BV_KEY musig branch is a silent no-op
Severity: medium — latent trap
src/common/cleartext.c:481-485 only tie-breaks when both keys are
KEY_EXPRESSION_NORMAL; for two musig keys they fall through comparing nothing
(reference cmp_key compares len then indices). Dead today (musig leaves
classify as Multisig, so $key only ever binds a plain key), but a trap if a
future spec binds a musig to a $key.
Fix: assert plainness, or implement the musig comparison.
6. Policy is classified ~2× and tap-leaves matched many times
Severity: low/medium — efficiency, on-device
src/handler/register_wallet.c:229-243 calls cleartext_confusion_score then
cleartext_encode; both run match_top_level, collect leaves, and
match_tapleaf per leaf, and both run key_orderings_count (encode
(void)-discards the orderings it computes at src/common/cleartext.c:916).
Inside encode, match_tapleaf is re-invoked on the same sub-nodes by leaf_cmp
(:503-504), render_spec_at (:779), and leaf_score (:838).
Fix: classify once into a small cached structure and thread it through
score + encode + sort + render. (Minor on small policies; real for large/deep
taptrees.)
7. CT_MAX_LINES == 8 silently caps describable taproots
Severity: low — product limit
src/common/cleartext.h:15: a taproot with >7 recognized leaves falls back to
raw even though it is fully describable. Probably fine, but worth a comment at
the cap so it is an intentional decision, not a surprise.
8. CT_MAX_TAP_LEAVES 64 vs. comment saying depth-9 allows up to 256
Severity: low — comment/constant mismatch
src/common/cleartext.c:807: the cap is a safe-fail (returns UINT64_MAX →
treated as too-confusing), but the mismatch between comment and constant invites
confusion.
9. Duplicated threshold/keys scan
Severity: low — minor cleanup
admitting_pattern_count (src/common/cleartext.c:89-108) and
bindings_is_n_of_n (:719-728) run the same scan loop. Extract one helper.
Suggested first actions
These are the two highest-impact items.
Finally first few seem to be relevant, are not they ?
|
Going through the comments from above:
Added test cases upstream and fixed the canonical ordering in eb387e4.
Addressed with 7b80557.
It's a fair point, but it's not easy to fix without an AST unparser (that is, producing the descriptor template from the parsed AST), which is currently missing and non-trivial to build. Since we don't show the cleartext unless all leaves have the cleartext, this is not a problem.
Fair point, but this can only be addressed once the upstream Rust crate has a permanent house, possibly separate from the Vanadium repository.
Done in ee5f859.
I'm not worried about performance for this part of the code.
That's fine, it's an arbitrary limitation by design.
I don't see the problem, and 64 is plenty in practice, unlikely we'll ever need more.
Not worth the refactor imho. |
d0219e0 to
3d44796
Compare
Writing the struct directly is fragile, and risks incomplete initializations or non-zeroed fields 'leaking' into future calls. We rather write a simple helper that creates the full struct with a literal, guaranteeing that everything that is not explicitly set is zeroed.
5193698 to
1e23b81
Compare
Implements the confusion score and the cleartext representation for a
large subset of commonly used wallet policies. This is the standalone
engine layer: it compiles into the app but is not yet wired into any
handler.
The classifier tables and AST matchers are generated from the TOML specs
in specs/bip388/ by gen.py:
- specs/bip388/cleartext.toml -> cleartext_specs.{h,c}, cleartext_match.c
- specs/bip388/test_vectors.toml -> unit-tests/cleartext_vectors.inc.c
The hand-written runtime (cleartext.c, cleartext_match.h) provides the
scorer/encoder and the primitives the generated classifier calls into.
are_key_placeholders_identical() is exposed from policy.h for use by the
encoder.
A CI job (cleartext-gen-check.yml) runs `gen.py --check` to ensure the
committed generated files stay in sync with the TOML specs.
Mostly ported by Claude from the Rust reference implementation, with
several iterations of review and refinement.
Wires the cleartext engine into the register-wallet flow: the handler computes the confusion score and, when below the threshold, renders the cleartext spending-path lines shown before the raw descriptor template. For multisig wallet policies specifically, this also simplifies the UX by omitting the raw descriptor template altogether. This is safe for such simple policies, with very little ambiguity. In the long term, we might be able to extend this to more complex wallet policies, but that needs to be done intentionally, and possibly be an opt-in feature that is explicitly requested by software wallets that adapt their own UX accordingly. Unit tests of register_wallet had to be adapted slightly to take into account that the descriptor is not always shown.
Code coverage reportPer-file coverage
|
TODOs:
confusion_scoreis computed (maybe in the Rust crate)nbgl_useCaseReviewLightis changed to allow wrapping; that's good for the cleartext description (so it only wraps on words), but it makes descriptor templates wrap unnecessarily; it would be great to not wrap the descriptor template, but it is not allowed by Nbgl with ReviewLight (all pairs must share the same wrapping behavior).question: should we show the address_index/change pair when verifying a receiving address? The user should make sure that when they receive for the first time on a registered account, they receive on address index 0. Otherwise, bruteforcing from the cleartext becomes a lot harder as one would have to try address indexes to verify if money was sent there, which makes the problem a lot harder.Not implemented in this release. It would be more important for a future release where the descriptor template is hidden, since the user can't verify that it matches the backup. In this release, the user is still expected to check that the backup matches, so this is not a security concern.simplification: currently, the same pattern-matching machinery is used for both the top-level descriptors, and for taproot leaves. It might be much simpler to manually implement the matching for top-level descriptors (since it seems likely that it will be a fixed list that doesn't grow), and limit the pattern matching logic to the taproot leaf patterns. This doesn't require any functional change.Let's keep the full generality for now.Enhances the UX for the registration of multisig and (taproot) miniscript wallet policies by identifying and explaining in human terms the meaning of the descriptor template.
Framework
The cleartext representation has the following properties:
build.rsfile in the reference Rust crate. As long as the language's grammar doesn't change, adding some new patterns only requires updating the TOML files, and regenerating the code.The model introduces a
confusion_scorethat allows to provide an upper bound on the number of different descriptor templates that could possibly generate the same cleartext description. For any descriptor template with a too highconfusion_score, or with no cleartext representation for any other reason, the descriptor template is shown as usual, in order to not prevent more advanced users from benefiting from the generality of miniscript.This guarantees that, even if the user does not have a backup of the descriptor template, but has an exact backup of the cleartext description, it is possible to programmatically enumerate all the possible descriptor templates, and eventually find the correct one. In this way, the user is protected from ransom attacks even if they only check that they have a backup for the cleartext, rather than the descriptor template.
NOTE: in this PR, the descriptor template is still always shown (except in few very simple multisigs as explained below). Therefore, there is no ransom attack risk if the user checked that the descriptor template matches their backup, which remains the expected workflow for the time being.
C implementation
The implementation in C only needs:
The full reversible process (validating that the grammar is unambiguous, and that bruteforcing a cleartext description is feasible) is implemented in this rust crate.
In this PR, the descriptor template is still shown to the user in addition to the descriptor template. In the future, we could have as an opt-in feature to hide the descriptor_template (if the software wallet is prepared to show the identical clear-text to the user).
Clear text miniscript wallet policies
For any taproot-based miniscript wallet policy that admits a cleartext description, it is shown before the descriptor template when registering the account on the device:
When the cleartext description is not available (either a pattern not covered by the specs, or a confusion score that is too high), only the descriptor template is shown (same UX as today).
Many other examples can be found in test_vectors.toml.
Simple multisig policies
Only for simple multisig descriptor templates, the app automatically hides the descriptor template, as the ambiguity is very low. For example, this is for a 2-of-2:
The wording is "Each of ..." for any n-of-n multisignature (regardless if it's legacy, segwit, or even taproot musig), while it says "Any of ..." for a k-of-n multisig (with k < n)
Future extensions