fix(platform)!: apply the review of contract group key bounds

docs/protocol/contract-bound-authentication-keys.md

@@ -58,9 +58,10 @@ level do not already allow.
 - A group-bound key may sign only Batch transitions. A member on contract `C` is inside
   the bounds when `C` is a whole-contract member of the group, when the member's document
   type is a member of the group, or when the member's token is a member of the group.
-  Consensus reads `C`'s group memberships once per batch member and bills the read; a
-  member outside the group fails as a paid `ContractBoundedKeyOutOfBoundsError`, as for
-  a contract bound.
+  Consensus reads the group memberships of each distinct contract the batch touches
+  once, however many members name that contract, and bills each read once; a member
+  outside the group fails as a paid `ContractBoundedKeyOutOfBoundsError`, as for a
+  contract bound.
 - Memberships are append-only, so what a group-bound key may sign grows whenever the
   group's owner or an admin adds a contract, document type or token. Binding a key to a
   group trusts the group's owner and admins with that growth.

docs/protocol/contract-groups.md

@@ -132,8 +132,8 @@ the responses into `ContractGroupInfo`, `ContractGroupMembersPage` and
 
 An identity's AUTHENTICATION key may carry `contractBounds` of type `contractGroup`
 naming a group. The key may then sign batch members whose contract, document type or
-token is a member of the group; consensus reads the member contract's memberships once
-per batch member and bills the read. The group must exist when the key is registered,
-any identity may bind a key to any group, and encryption and decryption keys cannot be
-group-bound. `IdentityCreateFromShieldedPool` refuses group-bound keys. The rules and
-errors are in `contract-bound-authentication-keys.md`.
+token is a member of the group; consensus reads the memberships of each distinct
+contract the batch touches once and bills each read once. The group must exist when the
+key is registered, any identity may bind a key to any group, and encryption and
+decryption keys cannot be group-bound. `IdentityCreateFromShieldedPool` refuses
+group-bound keys. The rules and errors are in `contract-bound-authentication-keys.md`.

packages/rs-dpp/src/identity/identity_public_key/contract_bounds/mod.rs

@@ -1,3 +1,4 @@
+#[cfg(feature = "state-transitions")]
 use crate::contract_group::ContractGroupMember;
 use crate::identifier::Identifier;
 use crate::identity::identity_public_key::contract_bounds::ContractBounds::{
@@ -416,7 +417,7 @@ mod tests {
     }
 
     #[test]
-    fn contract_bounds_contract_group_json_round_trip() {
+    fn should_round_trip_contract_group_bounds_through_json() {
         let id = Identifier::from([0xEFu8; 32]);
         let bounds = ContractBounds::ContractGroup { id };
 

packages/rs-dpp/src/shielded/builder/identity_create_from_shielded_pool.rs

@@ -3,7 +3,6 @@ use grovedb_commitment_tree::{Anchor, FullViewingKey, SpendAuthorizingKey};
 use crate::address_funds::OrchardAddress;
 use crate::address_funds::PlatformAddress;
 use crate::fee::Credits;
-use crate::identity::contract_bounds::ContractBounds;
 use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0;
 use crate::identity::signer::Signer;
 use crate::identity::IdentityPublicKey;
@@ -187,12 +186,9 @@ where
     let mut bound_identity_id: Option<Identifier> = None;
     // Consensus refuses a key bound to a contract group in this transition (its Orchard sighash
     // layout predates group bounds); refuse it here before a proof is generated.
-    if let Some(key) = in_creation_keys.iter().find(|key| {
-        matches!(
-            key.contract_bounds(),
-            Some(ContractBounds::ContractGroup { .. })
-        )
-    }) {
+    if let Some(key) =
+        IdentityPublicKeyInCreation::first_bound_to_a_contract_group(&in_creation_keys)
+    {
         return Err(ProtocolError::ShieldedBuildError(format!(
             "key {} is bound to a contract group, which an identity created from the shielded \
              pool cannot register; add it with an identity update",

packages/rs-dpp/src/state_transition/mod.rs

@@ -70,7 +70,6 @@ use crate::fee::Credits;
 use crate::identity::identity_public_key::accessors::v0::IdentityPublicKeyGettersV0;
 #[cfg(feature = "state-transition-signing")]
 use crate::identity::identity_public_key::contract_bounds::BatchedTransitionBoundsCheck;
-use crate::identity::identity_public_key::contract_bounds::ContractBounds;
 #[cfg(feature = "state-transition-signing")]
 use crate::identity::signer::Signer;
 use crate::identity::state_transition::OptionallyAssetLockProved;
@@ -152,7 +151,6 @@ use crate::state_transition::identity_update_transition::{
 };
 use crate::state_transition::masternode_vote_transition::MasternodeVoteTransition;
 use crate::state_transition::masternode_vote_transition::MasternodeVoteTransitionSignable;
-use crate::state_transition::public_key_in_creation::accessors::IdentityPublicKeyInCreationV0Getters;
 use crate::state_transition::public_key_in_creation::IdentityPublicKeyInCreation;
 use crate::state_transition::shield_from_asset_lock_transition::{
     ShieldFromAssetLockTransition, ShieldFromAssetLockTransitionSignable,
@@ -860,12 +858,7 @@ fn active_version_range_for_keys_in_creation(
     keys: &[IdentityPublicKeyInCreation],
     otherwise: RangeInclusive<ProtocolVersion>,
 ) -> RangeInclusive<ProtocolVersion> {
-    if keys.iter().any(|key| {
-        matches!(
-            key.contract_bounds(),
-            Some(ContractBounds::ContractGroup { .. })
-        )
-    }) {
+    if IdentityPublicKeyInCreation::first_bound_to_a_contract_group(keys).is_some() {
         14..=LATEST_VERSION
     } else {
         otherwise
@@ -3475,6 +3468,38 @@ mod tests {
     // A version 1 data contract create carries contract groups, which only exist from
     // protocol version 14. Below that a node must reject it rather than create the
     // contract and drop the group data.
+    #[test]
+    fn test_data_contract_create_v1_is_not_active_before_protocol_version_14() {
+        use crate::serialization::PlatformSerializable;
+
+        let original = sample_data_contract_create_v1_st();
+        assert_eq!(original.active_version_range(), 14..=LATEST_VERSION);
+
+        let bytes =
+            PlatformSerializable::serialize_to_bytes(&original).expect("serialize succeeds");
+
+        let version_13 = PlatformVersion::get(13).expect("platform version 13 exists");
+        let err = StateTransition::deserialize_from_bytes_untrusted_in_version(&bytes, version_13)
+            .expect_err("expected StateTransitionIsNotActiveError at protocol version 13");
+        match err {
+            ProtocolError::StateTransitionError(
+                crate::state_transition::errors::StateTransitionError::StateTransitionIsNotActiveError {
+                    active_version_range,
+                    current_protocol_version,
+                    ..
+                },
+            ) => {
+                assert_eq!(current_protocol_version, 13);
+                assert_eq!(*active_version_range.start(), 14);
+            }
+            other => panic!("expected StateTransitionIsNotActiveError, got {other:?}"),
+        }
+
+        let version_14 = PlatformVersion::get(14).expect("platform version 14 exists");
+        StateTransition::deserialize_from_bytes_untrusted_in_version(&bytes, version_14)
+            .expect("a version 1 create is active at protocol version 14");
+    }
+
     #[test]
     fn should_gate_identity_transitions_carrying_a_contract_group_bound_key_to_protocol_version_14()
     {
@@ -3555,38 +3580,6 @@ mod tests {
             .expect("a contract group bound key is active at protocol version 14");
     }
 
-    #[test]
-    fn test_data_contract_create_v1_is_not_active_before_protocol_version_14() {
-        use crate::serialization::PlatformSerializable;
-
-        let original = sample_data_contract_create_v1_st();
-        assert_eq!(original.active_version_range(), 14..=LATEST_VERSION);
-
-        let bytes =
-            PlatformSerializable::serialize_to_bytes(&original).expect("serialize succeeds");
-
-        let version_13 = PlatformVersion::get(13).expect("platform version 13 exists");
-        let err = StateTransition::deserialize_from_bytes_untrusted_in_version(&bytes, version_13)
-            .expect_err("expected StateTransitionIsNotActiveError at protocol version 13");
-        match err {
-            ProtocolError::StateTransitionError(
-                crate::state_transition::errors::StateTransitionError::StateTransitionIsNotActiveError {
-                    active_version_range,
-                    current_protocol_version,
-                    ..
-                },
-            ) => {
-                assert_eq!(current_protocol_version, 13);
-                assert_eq!(*active_version_range.start(), 14);
-            }
-            other => panic!("expected StateTransitionIsNotActiveError, got {other:?}"),
-        }
-
-        let version_14 = PlatformVersion::get(14).expect("platform version 14 exists");
-        StateTransition::deserialize_from_bytes_untrusted_in_version(&bytes, version_14)
-            .expect("a version 1 create is active at protocol version 14");
-    }
-
     // -----------------------------------------------------------------------
     // Additional coverage: variants not yet exercised.
     //

packages/rs-dpp/src/state_transition/state_transitions/identity/public_key_in_creation/mod.rs

@@ -1,8 +1,10 @@
+use crate::identity::contract_bounds::ContractBounds;
 use crate::identity::IdentityPublicKey;
 #[cfg(feature = "json-conversion")]
 use crate::serialization::JsonConvertible;
 #[cfg(feature = "value-conversion")]
 use crate::serialization::ValueConvertible;
+use crate::state_transition::public_key_in_creation::accessors::IdentityPublicKeyInCreationV0Getters;
 use crate::state_transition::public_key_in_creation::v0::IdentityPublicKeyInCreationV0;
 use crate::state_transition::public_key_in_creation::v0::IdentityPublicKeyInCreationV0Signable;
 use crate::ProtocolError;
@@ -54,6 +56,17 @@ impl IdentityPublicKeyInCreation {
             }),
         }
     }
+
+    /// The first of `keys` bound to a contract group, if any. A transition carrying such a key
+    /// is active from protocol version 14, and an identity created from the shielded pool
+    /// cannot register one.
+    pub fn first_bound_to_a_contract_group(keys: &[Self]) -> Option<&Self> {
+        keys.iter().find(|key| {
+            key.contract_bounds()
+                .and_then(ContractBounds::contract_group_id)
+                .is_some()
+        })
+    }
 }
 
 impl From<&IdentityPublicKeyInCreation> for IdentityPublicKey {

packages/rs-drive-abci/src/execution/validation/state_transition/common/validate_identity_public_key_contract_bounds/mod.rs

@@ -91,7 +91,9 @@ mod tests {
     use crate::execution::types::state_transition_execution_context::{
         StateTransitionExecutionContext, StateTransitionExecutionContextMethodsV0,
     };
-    use crate::test::helpers::setup::TestPlatformBuilder;
+    use crate::rpc::core::MockCoreRPCLike;
+    use crate::test::helpers::contract_groups::{register_group, single_owner_info};
+    use crate::test::helpers::setup::{TempPlatform, TestPlatformBuilder};
     use dpp::block::block_info::BlockInfo;
     use dpp::block::epoch::Epoch;
     use dpp::consensus::basic::BasicError;
@@ -619,31 +621,16 @@ mod tests {
 
     fn platform_with_contract_group(
         contract_group_id: Identifier,
-    ) -> crate::test::helpers::setup::TempPlatform<crate::rpc::core::MockCoreRPCLike> {
-        use dpp::contract_group::{ContractGroupInfo, ContractGroupRegistration};
+    ) -> TempPlatform<MockCoreRPCLike> {
         let platform = TestPlatformBuilder::new()
             .build_with_mock_rpc()
             .set_genesis_state();
-        let info: ContractGroupInfo = (
-            Identifier::from([0x60; 32]),
-            ContractGroupRegistration {
-                admins: Default::default(),
-                name: None,
-                description: None,
-            },
-        )
-            .into();
-        platform
-            .drive
-            .insert_contract_group(
-                contract_group_id,
-                &info,
-                &BlockInfo::default(),
-                true,
-                None,
-                PlatformVersion::latest(),
-            )
-            .expect("expected to register the group");
+        register_group(
+            &platform,
+            contract_group_id,
+            &single_owner_info(Identifier::from([0x60; 32]), None, None),
+            PlatformVersion::latest(),
+        );
         platform
     }
 

packages/rs-drive-abci/src/execution/validation/state_transition/processor/traits/shielded_proof.rs

@@ -14,7 +14,6 @@ use dpp::consensus::basic::BasicError;
 use dpp::consensus::state::shielded::insufficient_shielded_fee_error::InsufficientShieldedFeeError;
 use dpp::consensus::state::state_error::StateError;
 use dpp::consensus::ConsensusError;
-use dpp::identity::contract_bounds::ContractBounds;
 use dpp::serialization::{PlatformMessageSignable, Signable};
 use dpp::state_transition::public_key_in_creation::accessors::IdentityPublicKeyInCreationV0Getters;
 use dpp::state_transition::public_key_in_creation::IdentityPublicKeyInCreation;
@@ -678,17 +677,9 @@ fn contract_group_bound_key_in_shielded_creation(
         return None;
     };
     let IdentityCreateFromShieldedPoolTransition::V0(v0) = st;
-    v0.public_keys
-        .iter()
-        .find(|key| {
-            matches!(
-                key.contract_bounds(),
-                Some(ContractBounds::ContractGroup { .. })
-            )
-        })
-        .map(|key| {
-            ContractGroupBoundKeyNotAllowedInShieldedIdentityCreationError::new(key.id()).into()
-        })
+    IdentityPublicKeyInCreation::first_bound_to_a_contract_group(&v0.public_keys).map(|key| {
+        ContractGroupBoundKeyNotAllowedInShieldedIdentityCreationError::new(key.id()).into()
+    })
 }
 
 #[cfg(test)]

packages/rs-drive-abci/src/execution/validation/state_transition/state_transitions/batch/state/v2/mod.rs

@@ -78,10 +78,13 @@ impl DocumentsBatchStateTransitionStateValidationV2 for BatchTransition {
             execution_context,
         )?;
 
+        // A result with errors never reaches the bounds check, so nothing is resolved for it.
+        let bounds_will_be_checked =
+            signed_by_a_group_bound_key && validation_result.errors.is_empty();
         if let Some(action) = validation_result
             .data
             .as_mut()
-            .filter(|_| signed_by_a_group_bound_key)
+            .filter(|_| bounds_will_be_checked)
         {
             let platform_version = platform.state.current_platform_version()?;
             let contract_ids: BTreeSet<Identifier> = self