Skip to content

fix(cnight-observation): complete CIP-19 validation for Cardano reward addresses (#498)#1817

Open
m2ux wants to merge 14 commits into
mainfrom
fix/498-cip19-cardano-reward-address-validation
Open

fix(cnight-observation): complete CIP-19 validation for Cardano reward addresses (#498)#1817
m2ux wants to merge 14 commits into
mainfrom
fix/498-cip19-cardano-reward-address-validation

Conversation

@m2ux

@m2ux m2ux commented Jul 3, 2026

Copy link
Copy Markdown
Contributor

Summary

Consolidate Cardano reward-address (CIP-19) validation into a single validated constructor for CardanoRewardAddressBytes, and route the follower, genesis config, and the mapping-address setter through it so malformed, wrong-length, wrong-type, or wrong-network reward addresses are rejected with specific errors.

🐛 Issue 📐 Engineering

Jira: PM-20267


Motivation

Cardano reward addresses supplied to the cNIGHT observation path are currently validated only partially, and in several independent places. Malformed, wrong-length, wrong-address-type, or wrong-network reward addresses can pass through the follower, the genesis config, and set_mapping_validator_contract_address, and the follower carries roughly three ad-hoc parsing spots each with its own error enum. An address that is not a valid CIP-19 reward address for the expected network can therefore be accepted and stored, misdirecting or silently dropping rewards and making failures hard to diagnose.

This change gives every entry point one strict, shared check that rejects each bad case with a specific, named error, replacing the scattered ad-hoc parsing with a single validated constructor and error type.


Changes

  • Primitive — adds a validated constructor CardanoRewardAddressBytes::try_new(bytes, expected_network) and a dedicated error type with a distinct variant per rejection case (wrong length, wrong address type, wrong network), checking length first then the CIP-19 header nibbles via no_std bit arithmetic; the length-only conversion is retained for test and benchmark use.
  • Follower — the four reward-address sites (registration, deregistration, asset-create, asset-spend) share one infallible constructor that builds the reward address from the stake credential and the configured network. There is no follower-side validation or rejection, because a reward address the node constructs from an already-valid credential cannot be malformed; the now-dead reward-address error variant is removed.
  • Genesis — validates deserialized reward-address keys against address type and expected network during the genesis build, where the network is available, rather than the length-only check used previously.
  • Mapping validator — gives set_mapping_validator_contract_address and the genesis validator-address build a sibling validated path returning a specific error, kept separate from the reward-address constructor because the validator address is a different Cardano address category.
  • Tests — cover each rejection case (wrong length, wrong type, wrong network, malformed header) plus valid type-14 and type-15 round-trips, and repair the existing genesis/extrinsic tests for the new validation.
  • On-chain rejection event — not added. The follower filters invalid reward addresses before the inherent, so the pallet never observes a rejected registration; the per-rejection follower log provides the intended observability without a non-trivial inherent-payload change.

📌 Submission Checklist

  • 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: [reason]
  • If the changes introduce a new feature, I have bumped the node minor version
  • Update documentation (if relevant)
  • No new todos introduced

🔱 Fork Strategy

  • Node Runtime Update
  • Node Client Update
  • Other
  • N/A

🗹 TODO before merging

  • Run and pass cargo check / cargo clippy / cargo nextest (deferred — see Validation status)
  • Ready for review

…work package

Seed commit for issue #498 work package. Implementation to follow.

Signed-off-by: Mike Clay <mike.clay@shielded.io>
@m2ux m2ux self-assigned this Jul 3, 2026
m2ux added 5 commits July 3, 2026 09:56
…-reward-address-validation

Signed-off-by: Mike Clay <mike.clay@shielded.io>
…d addresses

Reward addresses were accepted with only a length check; the four follower
construction sites bypassed even that via to_bytes().try_into().unwrap(), the
genesis path was length-only, and nothing validated the CIP-19 address-type or
network-id nibble. A wrong-length, wrong-type, wrong-network, or malformed
reward address could be accepted and stored.

Add a single validated constructor CardanoRewardAddressBytes::try_new(bytes,
expected_network) on the no_std primitive, with CardanoRewardAddressError
carrying a distinct variant per rejection case (WrongLength, WrongAddressType,
WrongNetwork). Checks run length-first, then the type nibble (14/15), then the
network nibble, using plain bit arithmetic.

Route every production path through it: a shared decode_reward_address helper
wraps try_new at all four follower sites (log-and-skip on rejection, matching
the existing resilience pattern) and the dead CardanoNetworkError variant is
replaced by InvalidRewardAddress. Genesis validates each mappings key against
type + network, deriving the expected network from the mapping-validator Bech32
prefix. The mapping-validator address (a Shelley script/enterprise address, not
a reward address) gets a sibling structural validator returning a specific
InvalidMappingValidatorAddress error rather than the generic length error, used
by both the extrinsic and the genesis build.

Add six unit tests on try_new covering wrong length, wrong type, wrong network,
a malformed header, and valid type-14/type-15 round-trips.

Issue: shieldedtech/shielded-security-engineering#498

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Mike Clay <mike.clay@shielded.io>
…onal verbosity

Manual review of the CIP-19 reward-address validation change flagged that
several new comments were more verbose than the surrounding existing code.
Reduce them so comment density matches the neighbourhood, comments-only with
no logic or behaviour change:

- config.rs: drop the serde deserializer's length-only rationale block.
- pallets/lib.rs: condense the InvalidMappingValidatorAddress variant doc and
  the validate_mapping_validator_address docblock.
- primitives/cnight-observation/lib.rs: drop the CardanoRewardAddressError
  enum docblock and condense the try_new docblock.
- mainchain-follower/cnight_observation.rs: condense the decode_reward_address
  doc.

Issue: shieldedtech/shielded-security-engineering#498

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Mike Clay <mike.clay@shielded.io>
…tests

Post-implementation test-suite review found that routing the genesis
mapping-validator address through validate_mapping_validator_address broke the
existing over-length panic test: it fed a bad-prefix address that now fails the
HRP check before the length branch, and it asserted the old "maximum N" wording
that the new message replaced with "max length N".

Repair the over-length test to use a valid addr_test-prefixed address so it
reaches the length branch, and assert the current wording. Add coverage for the
paths that carry real CIP-19 entropy and previously had no negative test:

- genesis panics with InvalidMappingValidatorAddress on a bad-prefix validator
  address (structural reject before the length branch)
- genesis panics on a mappings key whose network nibble does not match the
  network derived from the validator address prefix
- set_mapping_validator_contract_address rejects a bad-prefix address with
  InvalidMappingValidatorAddress and an over-length valid-prefix address with
  MaxCardanoAddrLengthExceeded, storing neither

Verification of compilation and the test run is deferred to the user
(cargo is not run in this environment).

Issue: shieldedtech/shielded-security-engineering#498

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Mike Clay <mike.clay@shielded.io>
…e file

Add the changes/ fragment for the CIP-19 reward-address validation work so
the change is traceable to issue #498 in release notes and satisfies the CI
changes-check.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Mike Clay <mike.clay@shielded.io>
@datadog-official

This comment has been minimized.

@m2ux
m2ux requested a review from gilescope July 3, 2026 10:28
m2ux added 2 commits July 3, 2026 11:34
…Pallet in genesis build

The genesis build runs in impl BuildGenesisConfig for GenesisConfig<T>, where
Self is GenesisConfig<T>; the helper is an associated fn of Pallet<T>, so the
Self:: path failed to resolve (E0599). Qualify the call as Pallet::<T>::.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Mike Clay <mike.clay@shielded.io>
…ic payload

catch_unwind returns a Result whose Ok arm is the unit type; calling expect on
it yielded that unit value, so downcast_ref was invoked on unit (E0599). Use
expect_err to take the panic payload box instead.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Mike Clay <mike.clay@shielded.io>
Comment thread primitives/mainchain-follower/src/data_source/cnight_observation.rs Outdated
Comment thread primitives/mainchain-follower/src/data_source/cnight_observation.rs Outdated
m2ux and others added 6 commits July 20, 2026 10:44
…genesis

The genesis build began CIP-19-validating the mapping-validator address,
which rejected the empty string that the default CNightAddresses produces.
Every mock runtime and the runtime's own test_genesis_config_builds build
the default genesis, so all 44 of those tests panicked at build().

Restore the prior contract: an empty mapping-validator address is the unset
default and is stored without validation, while a non-empty address is still
CIP-19 validated. Enforcement via the set_mapping_validator_contract_address
extrinsic and for operator-supplied genesis addresses is unchanged.

Also collapse the nested mapping-key if-let into an && let-chain to clear
clippy collapsible_if under -D warnings, and replace the repeat-take idiom
with a 28-byte array in the new test so the all-targets clippy pass does not
trip manual_repeat_n once the test target compiles.

Assisted-by: Claude:claude-opus-4-8
Signed-off-by: Mike Clay <mike.clay@shielded.io>
…ly (#1817)

Signed-off-by: Mike Clay <mike.clay@shielded.io>
…sfy clippy (#1817)

Signed-off-by: Mike Clay <mike.clay@shielded.io>
…on pallet error (#1817)

Signed-off-by: Mike Clay <mike.clay@shielded.io>
…1817)

Bumps axios, brace-expansion, and js-yaml to patched versions to clear
newly-published GHSA advisories flagged by the npm audit CI gate. Lockfile
only; package.json ranges unchanged.

Signed-off-by: Mike Clay <mike.clay@shielded.io>
@m2ux
m2ux marked this pull request as ready for review July 21, 2026 10:41
@m2ux
m2ux requested a review from a team as a code owner July 21, 2026 10:41
@m2ux
m2ux requested a review from LGLO July 21, 2026 10:41
@chatgpt-codex-connector

Copy link
Copy Markdown

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants