Skip to content

Add os_derive_slip21_key helper for SLIP-21 derivations.#1381

Draft
bigspider wants to merge 2 commits intomasterfrom
slip21_helper
Draft

Add os_derive_slip21_key helper for SLIP-21 derivations.#1381
bigspider wants to merge 2 commits intomasterfrom
slip21_helper

Conversation

@bigspider
Copy link
Copy Markdown
Contributor

@bigspider bigspider commented Jan 14, 2026

Description

While os_derive_bip32_with_seed_no_throw supports SLIP-21, the way parameters were retrofitted from BIP-32 makes it rather hard to use.

This adds a helper with a simple interface that takes care of translating the parameters to the expected format.
See for example here for an analogous (but more limited) helper currently used in the bitcoin app.

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Tests
  • Documentation
  • Other (for changes that might not fit in any category)

Auto cherry-pick in API_LEVEL

If requested to port the commits from this PR on a dedicated API_LEVEL branch,
select the targeted one(s), or add new references if not listed:

[x] TARGET_API_LEVEL: API_LEVEL_25

This will only create the PR with cherry-picks, ready to be reviewed and merged.

Remember:

  • The merge will ALWAYS be a manual operation.
  • It is possible the cherry-picks don't apply correctly, mainly if previous commits have been forgotten.
  • In case of failure, there is no other solution than redo the operation manually...

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 14, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.98%. Comparing base (e88076a) to head (f1507fe).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1381   +/-   ##
=======================================
  Coverage   79.98%   79.98%           
=======================================
  Files          43       43           
  Lines        5067     5067           
  Branches      745      745           
=======================================
  Hits         4053     4053           
  Misses        926      926           
  Partials       88       88           
Flag Coverage Δ
unittests 79.98% <ø> (ø)

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.

Comment thread include/os_seed.h Outdated
Comment thread include/os_seed.h Outdated
// Aligned to 4 bytes as this is later passed as a (const unsigned int *) pointer
// Make sure that the buffer is large enough (account for 1 extra byte per label for the \x00
// prefix, and the 1 byte for the '/' separator after the first label).
unsigned char path_buffer[256 + 2 * 10 - 1] __attribute__((aligned(4)));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Difficult to understand this calculation. Why 256?
IMHO it should be:

  • either a big "independent" number, e.g. unsigned char path_buffer[300] as this is the total buffer/path length we can afford
  • or we should deduct all from the max label count and a label length, e.g. unsigned char path_buffer[(LABEL_LEN+1+1)*LABEL_COUNT_MAX - 1]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

256 is an arbitrary limit that I chose, and reported in the documentation as a constraint.

Perhaps I can add a constant

#define LABEL_TOTAL_LENGTH_MAX

So the computation becomes:

    unsigned char path_buffer[LABEL_TOTAL_LENGTH_MAX + 2 * LABEL_COUNT_MAX - 1] __attribute__((aligned(4)));

What do you think?

Copy link
Copy Markdown
Contributor Author

@bigspider bigspider Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In f1507fe I introduced SLIP21_MAX_TOTAL_LENGTH, and rewrote the calculation for this buffer in terms of SLIP21_MAX_TOTAL_LENGTH and SLIP21_MAX_LABELS.

Copy link
Copy Markdown
Contributor

@iartemov-ledger iartemov-ledger Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My point was even that if "256 is an arbitrary limit that I chose" than we would not need the calculation, just:

unsigned char path_buffer[LABEL_TOTAL_LENGTH_MAX] __attribute__((aligned(4)));

;)

Copy link
Copy Markdown
Contributor Author

@bigspider bigspider Jan 16, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then you'd have to do the reverse calculation to document what is the maximum total length of labels before they are assembled (which would be size of the buffer - (2 * LABEL_COUNT_MAX - 1)).

Comment thread include/os_seed.h Outdated
While os_derive_bip32_with_seed_no_throw supports SLIP-21, the
way parameters were retrofitted from BIP-32 makes it rather hard
to use.

This adds a helper with a simple interface that takes care of
translating the parameters to the expected format.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants