fix(cnight-observation): complete CIP-19 validation for Cardano reward addresses (#498)#1817
Draft
m2ux wants to merge 8 commits into
Draft
fix(cnight-observation): complete CIP-19 validation for Cardano reward addresses (#498)#1817m2ux wants to merge 8 commits into
m2ux wants to merge 8 commits into
Conversation
…work package Seed commit for issue #498 work package. Implementation to follow. Signed-off-by: Mike Clay <mike.clay@shielded.io>
…-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>
…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>
LGLO
reviewed
Jul 6, 2026
| let reward_address = RewardAddress::new(cardano_network, &credential); | ||
| // Unwrap here is OK - we know the reward_address is always 29 bytes | ||
| let cardano_address = reward_address.to_address().to_bytes().try_into().unwrap(); | ||
| let cardano_reward_address = match decode_reward_address( |
Contributor
There was a problem hiding this comment.
Minor: creating RewardAddress can be hidden in decode_reward_address. Make decode_reward_address accept Credential
LGLO
reviewed
Jul 6, 2026
| ) { | ||
| Ok(addr) => addr, | ||
| Err(e) => { | ||
| log::error!("Rejected deregistration reward address: {e} ({header:?})"); |
Contributor
There was a problem hiding this comment.
I think that unwrap was correct. Perhaps hidden in renamed decode_reward_address.
We have two correct inputs at line 293. It is insane to reject at this point, because on chain data was correct and our algorithm is invalid.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
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 viano_stdbit arithmetic; the length-only conversion is retained for test and benchmark use.set_mapping_validator_contract_addressand 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.📌 Submission Checklist
🔱 Fork Strategy
🗹 TODO before merging
cargo check/cargo clippy/cargo nextest(deferred — see Validation status)