contracts: only require BLS key to remove staker (#2420)

* contracts: only require BLS key to remove staker
* don't shadow functions with variables
* Update contract bindings
* Fix tests, add test to load update from toml file
* Call the from_toml_file functions in tests

Ensure that the implementations of these functions to load initial stake
table and stake table update are actually exercised in tests.

* Fix solhint linting

contracts/rust/adapter/src/stake_table.rs

@@ -133,6 +133,10 @@ impl NodeInfoJf {
             da: rng.gen(),
         }
     }
+
+    pub fn stake_table_key_sol(&self) -> permissioned_stake_table::G2Point {
+        bls_jf_to_sol(self.stake_table_key)
+    }
 }
 
 impl From<NodeInfoJf> for NodeInfo {
@@ -173,25 +177,7 @@ impl From<NodeInfo> for NodeInfoJf {
             schnorr_vk,
             is_da,
         } = value;
-        let stake_table_key = {
-            let g2 = diff_test_bn254::ParsedG2Point {
-                x0: bls_vk.x_0,
-                x1: bls_vk.x_1,
-                y0: bls_vk.y_0,
-                y1: bls_vk.y_1,
-            };
-            let g2_affine = short_weierstrass::Affine::<ark_bn254::g2::Config>::from(g2);
-            let mut bytes = vec![];
-            // TODO: remove serde round-trip once jellyfin provides a way to
-            // convert from Affine representation to VerKey.
-            //
-            // Serialization and de-serialization shouldn't fail.
-            g2_affine
-                .into_group()
-                .serialize_compressed(&mut bytes)
-                .unwrap();
-            BLSPubKey::deserialize_compressed(&bytes[..]).unwrap()
-        };
+        let stake_table_key = bls_sol_to_jf(bls_vk);
         let state_ver_key = {
             let g1_point: ParsedEdOnBN254Point = schnorr_vk.into();
             let state_sk_affine = twisted_edwards::Affine::<EdwardsConfig>::from(g1_point);
@@ -221,6 +207,36 @@ impl From<PeerConfigKeys<BLSPubKey>> for NodeInfoJf {
     }
 }
 
+pub fn bls_jf_to_sol(bls_vk: BLSPubKey) -> permissioned_stake_table::G2Point {
+    let ParsedG2Point { x0, x1, y0, y1 } = bls_vk.to_affine().into();
+    permissioned_stake_table::G2Point {
+        x_0: x0,
+        x_1: x1,
+        y_0: y0,
+        y_1: y1,
+    }
+}
+
+pub fn bls_sol_to_jf(bls_vk: permissioned_stake_table::G2Point) -> BLSPubKey {
+    let g2 = diff_test_bn254::ParsedG2Point {
+        x0: bls_vk.x_0,
+        x1: bls_vk.x_1,
+        y0: bls_vk.y_0,
+        y1: bls_vk.y_1,
+    };
+    let g2_affine = short_weierstrass::Affine::<ark_bn254::g2::Config>::from(g2);
+    let mut bytes = vec![];
+    // TODO: remove serde round-trip once jellyfin provides a way to
+    // convert from Affine representation to VerKey.
+    //
+    // Serialization and de-serialization shouldn't fail.
+    g2_affine
+        .into_group()
+        .serialize_compressed(&mut bytes)
+        .unwrap();
+    BLSPubKey::deserialize_compressed(&bytes[..]).unwrap()
+}
+
 #[cfg(test)]
 mod test {
     use super::*;

contracts/src/PermissionedStakeTable.sol

@@ -10,7 +10,7 @@ import { EdOnBN254 } from "./libraries/EdOnBn254.sol";
  * @dev An stake table mapping with owner-only access control.
  */
 contract PermissionedStakeTable is Ownable {
-    event StakersUpdated(NodeInfo[] removed, NodeInfo[] added);
+    event StakersUpdated(BN254.G2Point[] removed, NodeInfo[] added);
 
     error StakerAlreadyExists(BN254.G2Point);
     error StakerNotFound(BN254.G2Point);
@@ -33,7 +33,7 @@ contract PermissionedStakeTable is Ownable {
 
     // public methods
 
-    function update(NodeInfo[] memory stakersToRemove, NodeInfo[] memory newStakers)
+    function update(BN254.G2Point[] memory stakersToRemove, NodeInfo[] memory newStakers)
         public
         onlyOwner
     {
@@ -55,12 +55,12 @@ contract PermissionedStakeTable is Ownable {
         }
     }
 
-    function remove(NodeInfo[] memory stakersToRemove) internal {
+    function remove(BN254.G2Point[] memory stakersToRemove) internal {
         // TODO: revert if array empty
         for (uint256 i = 0; i < stakersToRemove.length; i++) {
-            bytes32 stakerID = _hashBlsKey(stakersToRemove[i].blsVK);
+            bytes32 stakerID = _hashBlsKey(stakersToRemove[i]);
             if (!stakers[stakerID]) {
-                revert StakerNotFound(stakersToRemove[i].blsVK);
+                revert StakerNotFound(stakersToRemove[i]);
             }
             stakers[stakerID] = false;
         }

contracts/test/PermissionedStakeTable.t.sol

@@ -13,12 +13,12 @@ contract PermissionedStakeTableTest is Test {
 
     function setUp() public {
         vm.prank(owner);
-        PermissionedStakeTable.NodeInfo[] memory initialStakers = nodes(0, 1);
+        PermissionedStakeTable.NodeInfo[] memory initialStakers = createNodes(0, 1);
         stakeTable = new PermissionedStakeTable(initialStakers);
     }
 
     // Create `numNodes` node IDs from `start` for testing.
-    function nodes(uint64 start, uint64 numNodes)
+    function createNodes(uint64 start, uint64 numNodes)
         private
         returns (PermissionedStakeTable.NodeInfo[] memory)
     {
@@ -38,37 +38,55 @@ contract PermissionedStakeTableTest is Test {
         return ps;
     }
 
+    // Convert NodeInfo array to BLS keys array
+    function toBls(PermissionedStakeTable.NodeInfo[] memory nodes)
+        private
+        pure
+        returns (BN254.G2Point[] memory)
+    {
+        BN254.G2Point[] memory bls = new BN254.G2Point[](nodes.length);
+        for (uint64 i = 0; i < nodes.length; i++) {
+            bls[i] = nodes[i].blsVK;
+        }
+        return bls;
+    }
+
+    // Empty array of NodeInfo
+    // solhint-disable-next-line no-empty-blocks
+    function emptyNodes() private pure returns (PermissionedStakeTable.NodeInfo[] memory nodes) { }
+
+    // Empty array of BLS keys
+    // solhint-disable-next-line no-empty-blocks
+    function emptyKeys() private pure returns (BN254.G2Point[] memory keys) { }
+
     function testInsert() public {
         vm.prank(owner);
-        PermissionedStakeTable.NodeInfo[] memory stakers = nodes(1, 1);
-        PermissionedStakeTable.NodeInfo[] memory empty = nodes(1, 0);
+        PermissionedStakeTable.NodeInfo[] memory stakers = createNodes(1, 1);
 
         vm.expectEmit();
-        emit PermissionedStakeTable.StakersUpdated(empty, stakers);
+        emit PermissionedStakeTable.StakersUpdated(emptyKeys(), stakers);
 
-        stakeTable.update(empty, stakers);
+        stakeTable.update(emptyKeys(), stakers);
 
         assertTrue(stakeTable.isStaker(stakers[0].blsVK));
     }
 
     function testInsertMany() public {
         vm.prank(owner);
-        PermissionedStakeTable.NodeInfo[] memory stakers = nodes(1, 10);
-        PermissionedStakeTable.NodeInfo[] memory empty = nodes(1, 0);
+        PermissionedStakeTable.NodeInfo[] memory stakers = createNodes(1, 10);
 
         vm.expectEmit();
-        emit PermissionedStakeTable.StakersUpdated(empty, stakers);
+        emit PermissionedStakeTable.StakersUpdated(emptyKeys(), stakers);
 
-        stakeTable.update(empty, stakers);
+        stakeTable.update(emptyKeys(), stakers);
 
         assertTrue(stakeTable.isStaker(stakers[0].blsVK));
     }
 
     function testInsertRevertsIfStakerExists() public {
         vm.prank(owner);
-        PermissionedStakeTable.NodeInfo[] memory stakers = nodes(1, 1);
-        PermissionedStakeTable.NodeInfo[] memory empty = nodes(1, 0);
-        stakeTable.update(empty, stakers);
+        PermissionedStakeTable.NodeInfo[] memory stakers = createNodes(1, 1);
+        stakeTable.update(emptyKeys(), stakers);
 
         // Try adding the same staker again
         vm.expectRevert(
@@ -77,53 +95,52 @@ contract PermissionedStakeTableTest is Test {
             )
         );
         vm.prank(owner);
-        stakeTable.update(empty, stakers);
+        stakeTable.update(emptyKeys(), stakers);
     }
 
     function testRemove() public {
-        PermissionedStakeTable.NodeInfo[] memory stakers = nodes(1, 1);
-        PermissionedStakeTable.NodeInfo[] memory empty = nodes(1, 0);
+        PermissionedStakeTable.NodeInfo[] memory stakersToInsert = createNodes(1, 1);
+        BN254.G2Point[] memory keysToRemove = toBls(stakersToInsert);
         vm.prank(owner);
-        stakeTable.update(empty, stakers);
+
+        // Insert the stakers we want to remove later.
+        stakeTable.update(emptyKeys(), stakersToInsert);
 
         vm.prank(owner);
 
         vm.expectEmit();
-        emit PermissionedStakeTable.StakersUpdated(stakers, empty);
+        emit PermissionedStakeTable.StakersUpdated(keysToRemove, emptyNodes());
 
-        stakeTable.update(stakers, empty);
+        stakeTable.update(keysToRemove, emptyNodes());
 
-        assertFalse(stakeTable.isStaker(stakers[0].blsVK));
+        assertFalse(stakeTable.isStaker(keysToRemove[0]));
     }
 
     function testRemoveRevertsIfStakerNotFound() public {
         vm.prank(owner);
-        PermissionedStakeTable.NodeInfo[] memory stakers = nodes(1, 1);
-        PermissionedStakeTable.NodeInfo[] memory empty = nodes(1, 0);
+        BN254.G2Point[] memory keysToRemove = toBls(createNodes(1, 1));
         vm.expectRevert(
-            abi.encodeWithSelector(PermissionedStakeTable.StakerNotFound.selector, stakers[0].blsVK)
+            abi.encodeWithSelector(PermissionedStakeTable.StakerNotFound.selector, keysToRemove[0])
         );
         // Attempt to remove a non-existent staker
-        stakeTable.update(stakers, empty);
+        stakeTable.update(keysToRemove, emptyNodes());
     }
 
     function testNonOwnerCannotInsert() public {
         vm.prank(address(2));
         vm.expectRevert(
             abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, address(2))
         );
-        PermissionedStakeTable.NodeInfo[] memory stakers = nodes(1, 1);
-        PermissionedStakeTable.NodeInfo[] memory empty = nodes(1, 0);
-        stakeTable.update(empty, stakers);
+        PermissionedStakeTable.NodeInfo[] memory stakers = createNodes(1, 1);
+        stakeTable.update(emptyKeys(), stakers);
     }
 
     function testNonOwnerCannotRemove() public {
         vm.prank(address(2));
         vm.expectRevert(
             abi.encodeWithSelector(Ownable.OwnableUnauthorizedAccount.selector, address(2))
         );
-        PermissionedStakeTable.NodeInfo[] memory stakers = nodes(1, 1);
-        PermissionedStakeTable.NodeInfo[] memory empty = nodes(1, 0);
-        stakeTable.update(stakers, empty);
+        BN254.G2Point[] memory keys = toBls(createNodes(1, 1));
+        stakeTable.update(keys, emptyNodes());
     }
 }

sequencer/src/bin/update-permissioned-stake-table.rs

@@ -61,9 +61,6 @@ struct Options {
     /// stakers_to_remove = [
     ///   {
     ///     stake_table_key = "BLS_VER_KEY~...",
-    ///     state_ver_key = "SCHNORR_VER_KEY~...",
-    ///     da = false,
-    ///     stake = 1, # this value is ignored, but needs to be set
     ///   },
     /// ]
     ///

types/src/v0/impls/l1.rs

@@ -1197,20 +1197,20 @@ mod test {
             SignerMiddleware::new(provider.clone(), wallet.with_chain_id(anvil.chain_id()));
         let client = Arc::new(client);
 
-        let v: Vec<NodeInfo> = Vec::new();
         // deploy the stake_table contract
-        let stake_table_contract = PermissionedStakeTable::deploy(client.clone(), v.clone())
-            .unwrap()
-            .send()
-            .await?;
+        let stake_table_contract =
+            PermissionedStakeTable::deploy(client.clone(), Vec::<NodeInfo>::new())
+                .unwrap()
+                .send()
+                .await?;
 
         let address = stake_table_contract.address();
 
         let mut rng = rand::thread_rng();
         let node = NodeInfoJf::random(&mut rng);
 
         let new_nodes: Vec<NodeInfo> = vec![node.into()];
-        let updater = stake_table_contract.update(v, new_nodes);
+        let updater = stake_table_contract.update(vec![], new_nodes);
         updater.send().await?.await?;
 
         let block = client.get_block(BlockNumber::Latest).await?.unwrap();

types/src/v0/impls/stake_table.rs

@@ -6,8 +6,8 @@ use super::{
 use async_trait::async_trait;
 use contract_bindings::permissioned_stake_table::StakersUpdatedFilter;
 use ethers::types::{Address, U256};
-use hotshot::types::SignatureKey as _;
-use hotshot_contract_adapter::stake_table::NodeInfoJf;
+use hotshot::types::{BLSPubKey, SignatureKey as _};
+use hotshot_contract_adapter::stake_table::{bls_sol_to_jf, NodeInfoJf};
 use hotshot_types::{
     data::EpochNumber,
     stake_table::StakeTableEntry,
@@ -54,25 +54,25 @@ impl StakeTables {
                 event
                     .removed
                     .into_iter()
-                    .map(|node_info| StakeTableDelta::remove(node_info.into()))
+                    .map(|key| StakeTableChange::Remove(bls_sol_to_jf(key)))
                     .chain(
                         event
                             .added
                             .into_iter()
-                            .map(|node_info| StakeTableDelta::add(node_info.into())),
+                            .map(|node_info| StakeTableChange::Add(node_info.into())),
                     )
             })
-            .group_by(|delta| delta.node_info.stake_table_key);
+            .group_by(|change| change.key());
 
         // If the last event for a stakers is `Added` the staker is currently
         // staking, if the last event is removed or (or the staker is not present)
         // they are not staking.
         let currently_staking = changes_per_node
             .into_iter()
             .map(|(_pub_key, deltas)| deltas.last().expect("deltas non-empty").clone())
-            .filter_map(|delta| match delta.change {
-                StakeTableChange::Add => Some(delta.node_info),
-                StakeTableChange::Remove => None,
+            .filter_map(|change| match change {
+                StakeTableChange::Add(node_info) => Some(node_info),
+                StakeTableChange::Remove(_) => None,
             });
 
         let mut consensus_stake_table: Vec<StakeTableEntry<PubKey>> = vec![];
@@ -105,30 +105,19 @@ pub struct EpochCommittees {
 
 #[derive(Debug, Clone, PartialEq)]
 enum StakeTableChange {
-    Add,
-    Remove,
+    Add(NodeInfoJf),
+    Remove(BLSPubKey),
 }
 
-#[derive(Debug, Clone)]
-struct StakeTableDelta {
-    change: StakeTableChange,
-    node_info: NodeInfoJf,
-}
-
-impl StakeTableDelta {
-    fn add(node_info: NodeInfoJf) -> Self {
-        Self {
-            change: StakeTableChange::Add,
-            node_info,
-        }
-    }
-    fn remove(node_info: NodeInfoJf) -> Self {
-        Self {
-            change: StakeTableChange::Remove,
-            node_info,
+impl StakeTableChange {
+    pub(crate) fn key(&self) -> BLSPubKey {
+        match self {
+            StakeTableChange::Add(node_info) => node_info.stake_table_key,
+            StakeTableChange::Remove(key) => *key,
         }
     }
 }
+
 /// Holds Stake table and da stake
 #[derive(Clone, Debug)]
 struct Committee {
@@ -527,7 +516,7 @@ mod tests {
         let mut new_da_node = consensus_node.clone();
         new_da_node.da = true;
         updates.push(StakersUpdatedFilter {
-            removed: vec![consensus_node.clone().into()],
+            removed: vec![consensus_node.stake_table_key_sol()],
             added: vec![new_da_node.clone().into()],
         });
         let st = StakeTables::from_l1_events(updates.clone());
@@ -544,7 +533,7 @@ mod tests {
 
         // Simulate removing the second node
         updates.push(StakersUpdatedFilter {
-            removed: vec![new_da_node.clone().into()],
+            removed: vec![new_da_node.stake_table_key_sol()],
             added: vec![],
         });
         let st = StakeTables::from_l1_events(updates);