fix(rust/rbac-registration): Fix some RBAC logging (#459)

* Fix some RBAC logging

* Use map_err

* Remove 'compare_key_hash' function

* Fix some comments

* Fix validate_stake_public_key formatting

* Fix formatting inside convert_payment_key function

* Fix tests

* Use option instead of result for RegistrationChain::new and RegistrationChain::update

* Remove debug formatting

Author: stanislav-tkach

rust/rbac-registration/src/cardano/cip509/cip509.rs

@@ -465,7 +465,7 @@ fn payment_history(
     report: &ProblemReport,
 ) -> HashMap<ShelleyAddress, Vec<Payment>> {
     let hash = MultiEraTx::Conway(Box::new(Cow::Borrowed(txn))).hash();
-    let context = format!("Populating payment history for Cip509, transaction hash = {hash:?}");
+    let context = format!("Populating payment history for Cip509, transaction = {hash}");
 
     let mut result: HashMap<_, _> = track_payment_addresses
         .iter()

rust/rbac-registration/src/cardano/cip509/types/role_data.rs

@@ -11,9 +11,7 @@ use pallas::ledger::{
 };
 
 use crate::cardano::cip509::{
-    rbac::role_data::CborRoleData,
-    utils::cip19::{compare_key_hash, extract_key_hash},
-    KeyLocalRef,
+    rbac::role_data::CborRoleData, utils::cip19::extract_key_hash, KeyLocalRef,
 };
 
 /// A role data.
@@ -132,20 +130,31 @@ fn convert_payment_key(
             return None;
         },
     };
-    validate_payment_output(address, &witness, context, report);
 
-    match Address::from_bytes(address) {
-        Ok(Address::Shelley(a)) => Some(a),
-        Ok(a) => {
+    let address = match Address::from_bytes(address) {
+        Ok(a) => a,
+        Err(e) => {
             report.other(
-                &format!("Unsupported address type ({a:?}) in payment key index ({index})"),
+                &format!("Invalid address in payment key index ({index}): {e}"),
+                context,
+            );
+            return None;
+        },
+    };
+    validate_payment_output(&address, &witness, context, report);
+
+    match address {
+        Address::Shelley(a) => Some(a),
+        Address::Byron(_) => {
+            report.other(
+                &format!("Unsupported Byron address type in payment key index ({index})"),
                 context,
             );
             None
         },
-        Err(e) => {
+        Address::Stake(_) => {
             report.other(
-                &format!("Invalid address in payment key index ({index}): {e:?}"),
+                &format!("Unsupported Stake address type in payment key index ({index})"),
                 context,
             );
             None
@@ -155,23 +164,24 @@ fn convert_payment_key(
 
 /// Helper function for validating payment output key.
 fn validate_payment_output(
-    output_address: &[u8],
+    address: &Address,
     witness: &TxnWitness,
     context: &str,
     report: &ProblemReport,
 ) {
-    let Some(key) = extract_key_hash(output_address) else {
+    let bytes = address.to_vec();
+    let Some(key) = extract_key_hash(&bytes) else {
         report.other("Failed to extract payment key hash from address", context);
         return;
     };
 
     // Set transaction index to 0 because the list of transaction is manually constructed
     // for TxWitness -> &[txn.clone()], so we can assume that the witness contains only
     // the witness within this transaction.
-    if let Err(e) = compare_key_hash(&[key], witness, 0.into()) {
+    if !witness.check_witness_in_tx(&key, 0.into()) {
         report.other(
             &format!(
-                "Unable to find payment output key ({key:?}) in the transaction witness set: {e:?}"
+                "Payment Key {address} (0x{key}) is not present in the transaction witness set, and can not be verified as owned and spendable."
             ),
             context,
         );

rust/rbac-registration/src/cardano/cip509/utils/cip19.rs

@@ -1,33 +1,9 @@
 //! Utility functions for CIP-19 address.
 
-use anyhow::bail;
-use cardano_blockchain_types::{TxnIndex, TxnWitness, VKeyHash};
+use cardano_blockchain_types::VKeyHash;
 
 /// Extract the first 28 bytes from the given key
 /// Refer to <https://cips.cardano.org/cip/CIP-19> for more information.
 pub(crate) fn extract_key_hash(key: &[u8]) -> Option<VKeyHash> {
     key.get(1..29).and_then(|v| v.try_into().ok())
 }
-
-/// Compare the given public key bytes with the transaction witness set.
-pub(crate) fn compare_key_hash(
-    pk_addrs: &[VKeyHash],
-    witness: &TxnWitness,
-    txn_idx: TxnIndex,
-) -> anyhow::Result<()> {
-    if pk_addrs.is_empty() {
-        bail!("No public key addresses provided");
-    }
-
-    pk_addrs.iter().try_for_each(|pk_addr| {
-        // Key hash not found in the transaction witness set
-        if !witness.check_witness_in_tx(pk_addr, txn_idx) {
-            bail!(
-                "Public key hash not found in transaction witness set given {:?}",
-                pk_addr
-            );
-        }
-
-        Ok(())
-    })
-}

rust/rbac-registration/src/cardano/cip509/validation.rs

@@ -23,13 +23,11 @@ use pallas::{
 };
 use x509_cert::{der::Encode as X509Encode, Certificate as X509};
 
-use super::{
-    extract_key::{c509_key, x509_key},
-    utils::cip19::compare_key_hash,
-};
 use crate::cardano::cip509::{
-    rbac::Cip509RbacMetadata, types::TxInputHash, C509Cert, Cip0134UriSet, LocalRefInt, RoleData,
-    SimplePublicKeyType, X509DerCert,
+    rbac::Cip509RbacMetadata,
+    types::TxInputHash,
+    utils::extract_key::{c509_key, x509_key},
+    C509Cert, Cip0134UriSet, LocalRefInt, RoleData, SimplePublicKeyType, X509DerCert,
 };
 
 /// Context-specific primitive type with tag number 6 (`raw_tag` 134) for
@@ -109,7 +107,9 @@ pub fn validate_aux(
     let hash = Blake2b256Hash::new(raw_aux_data);
     if hash != auxiliary_data_hash {
         report.other(
-            &format!("Incorrect transaction auxiliary data hash = '{hash:?}', expected = '{auxiliary_data_hash:?}'"),
+            &format!("Incorrect transaction auxiliary data hash = '0x{hash}', expected = '0x{auxiliary_data_hash}'. \
+            This metadata does not belong with this transaction. \
+            Catalyst metadata may not be transcribed to any other transaction body, and is therefore invalid."),
             context,
         );
     }
@@ -142,17 +142,22 @@ pub fn validate_stake_public_key(
         return;
     }
 
-    if let Err(e) = compare_key_hash(&pk_addrs, &witness, 0.into()) {
-        report.other(
-            &format!("Failed to compare public keys with witnesses: {e:?}"),
-            context,
-        );
+    for (hash, address) in pk_addrs {
+        if !witness.check_witness_in_tx(&hash, 0.into()) {
+            report.other(
+                &format!(
+                    "Payment Key '{address}' (0x{hash}) is not present in the transaction witness set, and can not be verified as owned and spendable."
+                ),
+                context,
+            );
+        }
     }
 }
 
 /// Extracts all stake addresses from both X509 and C509 certificates containing in the
-/// given `Cip509` and converts their hashes to bytes.
-fn extract_stake_addresses(uris: Option<&Cip0134UriSet>) -> Vec<VKeyHash> {
+/// given `Cip509`. Returns a list of pairs containing verifying public key hash and
+/// `bech32` string representation of address.
+fn extract_stake_addresses(uris: Option<&Cip0134UriSet>) -> Vec<(VKeyHash, String)> {
     let Some(uris) = uris else {
         return Vec::new();
     };
@@ -163,7 +168,13 @@ fn extract_stake_addresses(uris: Option<&Cip0134UriSet>) -> Vec<VKeyHash> {
         .flat_map(|(_index, uris)| uris.iter())
         .filter_map(|uri| {
             if let Address::Stake(a) = uri.address() {
-                a.payload().as_hash().as_slice().try_into().ok()
+                let bech32 = uri.address().to_string();
+                a.payload()
+                    .as_hash()
+                    .as_slice()
+                    .try_into()
+                    .ok()
+                    .map(|hash| (hash, bech32))
             } else {
                 None
             }
@@ -547,7 +558,7 @@ mod tests {
         let report = registration.consume().unwrap_err();
         assert!(report.is_problematic());
         let report = format!("{report:?}");
-        assert!(report.contains("Public key hash not found in transaction witness set"));
+        assert!(report.contains("is not present in the transaction witness set, and can not be verified as owned and spendable"));
     }
 
     #[test]
@@ -617,7 +628,7 @@ mod tests {
 
         let addresses = extract_stake_addresses(cip509.certificate_uris());
         assert_eq!(1, addresses.len());
-        assert_eq!(addresses.first().unwrap(), &hash);
+        assert_eq!(addresses.first().unwrap().0, hash);
     }
 
     // Verify that we are able to parse `Cip509` with legacy transaction output type.