Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

contracts: only require BLS key to remove staker #2420

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

contracts: only require BLS key to remove staker #2420

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
cf2d88c
contracts: only require BLS key to remove staker
sveitser Dec 19, 2024
51e8d12
don't shadow functions with variables
sveitser Dec 19, 2024
ec726f7
Update contract bindings, non-test code
sveitser Dec 19, 2024
8ad585d
Fix tests, add test to load update from toml file
sveitser Jan 15, 2025
a52d048
forge fmt
sveitser Jan 15, 2025
2e6cfd5
Call the from_toml_file functions in tests
sveitser Jan 16, 2025
423864f
Fix solhint linting
sveitser Jan 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 19 additions & 67 deletions contract-bindings/src/permissioned_stake_table.rs
View file
Open in desktop

Large diffs are not rendered by default.

54 changes: 35 additions & 19 deletions contracts/rust/adapter/src/stake_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -221,6 +207,36 @@ impl From<PeerConfigKeys<BLSPubKey>> for NodeInfoJf {
}
}

pub fn bls_jf_to_sol(bls_vk: BLSPubKey) -> permissioned_stake_table::G2Point {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does this mean? bls_jf_to_sol

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Converting the jellyfish type of BLSPubKey to the solidity type.

Copy link
Collaborator Author

@sveitser sveitser Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since neither of these types are owned by this crate we can't implement the From due to the orphan rule. I think it gets somewhat confusing if we create too many type wrappers so just opted for a function here.

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::*;
Expand Down
10 changes: 5 additions & 5 deletions contracts/src/PermissionedStakeTable.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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
{
Expand All @@ -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;
}
Expand Down
77 changes: 47 additions & 30 deletions contracts/test/PermissionedStakeTable.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand All @@ -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(
Expand All @@ -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());
}
}
3 changes: 0 additions & 3 deletions sequencer/src/bin/update-permissioned-stake-table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
/// },
/// ]
///
Expand Down
12 changes: 6 additions & 6 deletions types/src/v0/impls/l1.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
47 changes: 18 additions & 29 deletions types/src/v0/impls/stake_table.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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![];
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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());
Expand All @@ -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);
Expand Down
Loading
Loading