fix(ledger-helpers): harden WalletSeed hex decoding and add_addresses by gilescope · Pull Request #2142 · midnightntwrk/midnight-node

gilescope · 2026-09-11T10:04:06Z

Overview

Follow-up to #1217 (Least Authority audit A2 Suggestion 3, PM-22038). That PR hardened the traits on WalletSeed — removed Copy, added Zeroize/ZeroizeOnDrop, redacted Debug, dropped Clone from Keypair, replaced the indexing panic in add_addresses with a zip. It left the decode paths themselves untouched, and the zip traded a panic for silence. This closes those four gaps.

WalletSeed::try_from_hex_str

Validates the length (32, 64 or 128 hex characters) before decoding. It previously called hex::decode first and checked the byte count afterwards, so a hostile string sized an allocation before anything rejected it.
Odd-length input now reports InvalidHex(OddLength); a well-formed but wrong-sized seed reports InvalidLength(bytes).

Optional 0x prefix

Both try_from_hex_str and try_from_lazy_hex accept one. Keypair::from_str already did, so a 0x-prefixed seed used to parse as a key but not as a seed.

No allocation on either path

hex::decode_to_slice writes straight into the fixed-size array behind the variant. Seed bytes no longer pass through an intermediate Vec<u8> that outlives Zeroize — which was the point of adding ZeroizeOnDrop in the first place.
try_from_lazy_hex also drops its Vec of split parts for split_once, so the oversize check now genuinely happens before any allocation, as its test name already claimed.

MaintenanceUpdateBuilder::add_addresses returns Result

New MaintenanceUpdateError::LengthMismatch { addresses, counters }. The zip removed the unchecked-indexing panic but silently dropped the tail of the longer slice, so a caller that miscounted got a partial update and no signal. Nothing is applied on mismatch.
There are no production callers (tests only), so no downstream churn.

The identical change lands in both ledger/helpers/src/ledger_8/types.rs and ledger_9/types.rs — the two copies created by #2074 are byte-identical apart from their module paths.

Deliberately out of scope

Removing Clone from WalletSeed. #1217's own description defers this: it needs BuildInput / BuildOutput / UtxoOutputInfo / wallet_from_seed to borrow &WalletSeed, threading lifetimes through Box<dyn BuildInput<DefaultDB>>. That is 189 by-value signature sites and 238 clone sites across both ledger trees — an unreviewable diff if bolted onto this one. Worth its own PR.

🗹 TODO before merging

Ready

📌 Submission Checklist

All commits are signed off (git commit -s) for the DCO
Changes are backward-compatible (or flagged if breaking)
Pull request description explains why the change is needed
Self-reviewed the diff
I have included a change file, or skipped for this reason:
If the changes introduce a new feature, I have bumped the node minor version — n/a, no new feature
Update documentation (if relevant) — rustdoc on the changed items
Updated AGENTS.md if build commands, architecture, or workflows changed — n/a
No new todos introduced

🧪 Testing Evidence

cargo test -p midnight-node-ledger-helpers --lib — 52 passed, 0 failed.

New tests, mirrored in both ledger trees:

test	covers
`hex_str_accepts_optional_0x_prefix`	`0x` prefix, direct and via `FromStr`
`lazy_hex_accepts_optional_0x_prefix`	`0x` prefix on the lazy path
`hex_str_rejects_wrong_length_before_decoding`	20 bytes of valid hex rejected on length
`hex_str_rejects_oversized_input`	200 bytes rejected
`hex_str_reports_odd_length_as_a_hex_error`	odd-length input
`hex_str_round_trips_every_seed_size`	Short / Medium / Long
`lazy_hex_rejects_more_than_one_ellipsis`	`00..11..22`
`lazy_hex_places_head_and_tail_at_the_seed_edges`	head/tail placement, zero-filled gap
`add_addresses_rejects_length_mismatch`	mismatch rejected, nothing partially applied
`add_addresses_applies_matched_slices`	happy path

add_addresses_with_zip_truncates_on_mismatch is replaced by add_addresses_rejects_length_mismatch. It asserted the silent truncation as intended behaviour; that is the behaviour this PR changes.

cargo clippy --all-targets clean on midnight-node-ledger-helpers, midnight-ledger-unsafe-helpers and midnight-node-toolkit.

Additional tests are provided (if possible)

🔱 Fork Strategy

Node Runtime Update
Node Client Update
Other
N/A — host-side helper crate only, no runtime or consensus surface.

Links

Follow-up to [PM-22038]: Improve Wallet Seed, Key Pair, and Address Code Quality #1217
Ticket: https://shielded.atlassian.net/browse/PM-22038

🤖 Generated with Claude Code

https://claude.ai/code/session_019K95xJqbnxsbyyRY7pESNJ

Follow-up to #1217 (Least Authority A2 Suggestion 3), which hardened the traits on WalletSeed but left the decode paths as they were. - try_from_hex_str validates the length (32/64/128 hex chars) before decoding rather than decoding first and checking the byte count after, so untrusted input is rejected on size alone. - Both hex paths accept an optional 0x prefix. - Neither allocates: hex::decode_to_slice writes straight into the fixed-size array behind the variant, so seed bytes never sit in an intermediate Vec that outlives Zeroize. - add_addresses returns Result<(), MaintenanceUpdateError> and rejects mismatched slices. The zip added in #1217 removed the indexing panic but silently dropped the tail of the longer slice. No production callers, so nothing downstream changes. Both ledger_8 and ledger_9 copies of types.rs get the identical change. Assisted-by: Claude:claude-opus-5 claude-code Claude-Session: https://claude.ai/code/session_019K95xJqbnxsbyyRY7pESNJ

Assisted-by: Claude:claude-opus-5 claude-code

chatgpt-codex-connector · 2026-09-11T11:20:14Z

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-09-11T11:24:48.192503Z	`6527889`	Draft marked ready

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6527889720

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-09-11T11:24:45Z

+	let mut out = [0u8; N];
+	hex::decode_to_slice(hex_str, &mut out)?;


Zeroize the temporary decode buffers

When a correctly sized seed contains an invalid nibble after a valid prefix, hex::decode_to_slice has already written the preceding decoded bytes into out before returning an error. The ? path then drops this plain array without zeroizing it, leaving partial seed material in stack memory; decode_lazy_hex has the same problem when either half fails. Wrap these temporary arrays in Zeroizing or explicitly zeroize them on every error path in both ledger-version copies.

Useful? React with 👍 / 👎.

Individual remediation per .github/dco.yml (allowRemediationCommits.individual: true). Author of this commit, author of the remediated commits and the remediator named below are all giles-bot, which is what the DCO app requires of the individual form. I, giles-bot <319052204+giles-bot@users.noreply.github.com>, hereby add my Signed-off-by to this commit: d70b09c I, giles-bot <319052204+giles-bot@users.noreply.github.com>, hereby add my Signed-off-by to this commit: 6527889 Signed-off-by: giles-bot <319052204+giles-bot@users.noreply.github.com>

gilescope added skip-changes-check-issue bot:ai-assisted Authored or substantially edited by an AI agent labels Sep 11, 2026

chore: add PR link to change file

6527889

Assisted-by: Claude:claude-opus-5 claude-code

gilescope marked this pull request as ready for review September 11, 2026 11:20

gilescope requested a review from a team as a code owner September 11, 2026 11:20

chatgpt-codex-connector Bot reviewed Sep 11, 2026

View reviewed changes

Navigation Menu

fix(ledger-helpers): harden WalletSeed hex decoding and add_addresses - #2142

fix(ledger-helpers): harden WalletSeed hex decoding and add_addresses#2142
gilescope wants to merge 3 commits into
mainfrom
giles-wallet-seed-hex-hardening

gilescope commented Sep 11, 2026

Uh oh!

chatgpt-codex-connector Bot commented Sep 11, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		let mut out = [0u8; N];
		hex::decode_to_slice(hex_str, &mut out)?;

Conversation

gilescope commented Sep 11, 2026

Overview

Deliberately out of scope

🗹 TODO before merging

📌 Submission Checklist

🧪 Testing Evidence

🔱 Fork Strategy

Links

Uh oh!

chatgpt-codex-connector Bot commented Sep 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codex Review Summary

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Sep 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chatgpt-codex-connector Bot commented Sep 11, 2026 •

edited

Loading