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.

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

feat: add whitelist for reorg handling #88

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 19 additions & 2 deletions contracts/op-finality-gadget/src/contract.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
use crate::error::ContractError;
use crate::exec::admin::set_enabled;
use crate::exec::finality::{handle_finality_signature, handle_public_randomness_commit};
use crate::exec::finality::{
handle_finality_signature, handle_public_randomness_commit, whitelist_forked_blocks,
};
use crate::msg::{ExecuteMsg, InstantiateMsg, QueryMsg};
use crate::queries::{
query_block_voters, query_config, query_first_pub_rand_commit, query_last_pub_rand_commit,
query_block_voters, query_config, query_first_pub_rand_commit, query_forked_blocks,
query_forked_blocks_in_range, query_is_block_forked, query_last_pub_rand_commit,
};
use crate::state::config::{Config, ADMIN, CONFIG, IS_ENABLED};
use crate::state::finality::FORKED_BLOCKS;
use cosmwasm_std::{
to_json_binary, Deps, DepsMut, Env, MessageInfo, QueryResponse, Response, StdResult,
};
Expand All @@ -26,6 +30,9 @@ pub fn instantiate(
};
CONFIG.save(deps.storage, &config)?;

let forked_blocks: Vec<(u64, u64)> = vec![];
FORKED_BLOCKS.save(deps.storage, &forked_blocks)?;

Ok(Response::new().add_attribute("action", "instantiate"))
}

Expand All @@ -42,6 +49,13 @@ pub fn query(deps: Deps, _env: Env, msg: QueryMsg) -> Result<QueryResponse, Cont
QueryMsg::LastPubRandCommit { btc_pk_hex } => Ok(to_json_binary(
&query_last_pub_rand_commit(deps.storage, &btc_pk_hex)?,
)?),
QueryMsg::ForkedBlocks {} => Ok(to_json_binary(&query_forked_blocks(deps)?)?),
QueryMsg::IsBlockForked { height } => {
Ok(to_json_binary(&query_is_block_forked(deps, height)?)?)
}
QueryMsg::ForkedBlocksInRange { start, end } => Ok(to_json_binary(
&query_forked_blocks_in_range(deps, start, end)?,
)?),
QueryMsg::IsEnabled {} => Ok(to_json_binary(&IS_ENABLED.load(deps.storage)?)?),
}
}
Expand Down Expand Up @@ -86,6 +100,9 @@ pub fn execute(
&block_hash,
&signature,
),
ExecuteMsg::WhitelistForkedBlocks { forked_blocks } => {
whitelist_forked_blocks(deps, info, forked_blocks)
}
ExecuteMsg::SetEnabled { enabled } => set_enabled(deps, info, enabled),
ExecuteMsg::UpdateAdmin { admin } => ADMIN
.execute_update_admin(deps, info, Some(api.addr_validate(&admin)?))
Expand Down
2 changes: 2 additions & 0 deletions contracts/op-finality-gadget/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ pub enum ContractError {
NotFoundFinalityProvider(String, String),
#[error("Failed to query the voting power of the finality provider {0}")]
FailedFetchVotingPower(String),
#[error("Empty block range")]
EmptyBlockRange,
#[error("Caller is not the admin")]
Unauthorized,
#[error("Finality gadget is already enabled")]
Expand Down
2 changes: 1 addition & 1 deletion contracts/op-finality-gadget/src/exec/admin.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub fn set_enabled(
}

// Helper function to check caller is contract admin
fn check_admin(deps: &DepsMut, info: MessageInfo) -> Result<(), ContractError> {
pub fn check_admin(deps: &DepsMut, info: MessageInfo) -> Result<(), ContractError> {
// Check caller is admin
if !ADMIN.is_admin(deps.as_ref(), &info.sender)? {
return Err(ContractError::Unauthorized {});
Expand Down
29 changes: 27 additions & 2 deletions contracts/op-finality-gadget/src/exec/finality.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,21 @@ use std::collections::HashSet;
use crate::error::ContractError;
use crate::queries::query_last_pub_rand_commit;
use crate::state::config::CONFIG;
use crate::state::finality::{BLOCK_VOTES, SIGNATURES};
use crate::state::finality::{BLOCK_VOTES, FORKED_BLOCKS, SIGNATURES};
use crate::state::public_randomness::{
get_pub_rand_commit_for_height, PUB_RAND_COMMITS, PUB_RAND_VALUES,
};
use crate::utils::query_finality_provider;

use babylon_apis::finality_api::PubRandCommit;
use babylon_merkle::Proof;
use cosmwasm_std::{Deps, DepsMut, Env, Event, Response};
use cosmwasm_std::{Deps, DepsMut, Env, Event, MessageInfo, Response};
use k256::ecdsa::signature::Verifier;
use k256::schnorr::{Signature, VerifyingKey};
use k256::sha2::{Digest, Sha256};

use super::admin::check_admin;

// Most logic copied from contracts/btc-staking/src/finality.rs
pub fn handle_public_randomness_commit(
deps: DepsMut,
Expand Down Expand Up @@ -303,6 +305,29 @@ pub(crate) fn verify_finality_signature(
Ok(())
}

/// `whitelist_forked_blocks` adds a set of forked blocks to the whitelist. These blocks will be skipped by FG
/// (ie. treated as finalized) duringconsecutive quorum checks to unblock the OP derivation pipeline.
Copy link
Collaborator

Choose a reason for hiding this comment

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

during consecutive

pub fn whitelist_forked_blocks(
deps: DepsMut,
info: MessageInfo,
forked_blocks: Vec<(u64, u64)>,
) -> Result<Response, ContractError> {
// Check caller is admin
check_admin(&deps, info)?;

// Check array is non-empty
if forked_blocks.is_empty() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we also check the first element is less or equal to the second element in the tuple?

return Err(ContractError::EmptyBlockRange);
}

// Append blocks to whitelist
FORKED_BLOCKS.update::<_, ContractError>(deps.storage, |mut blocks| {
blocks.extend(forked_blocks);
Ok(blocks)
})?;
Comment on lines +324 to +327
Copy link
Collaborator

@bap2pecs bap2pecs Nov 28, 2024

Choose a reason for hiding this comment

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

do we want to allow overalp?

e.g. [(1,10) (5, 8) (3, 20)]

should we enforce

  • no overlap
  • always incrementing

so it can only be

e.g. [(1,10) (11, 18), (300,400)]

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, would suggest we do not allow overlap. This simplifies some CRUD around the forked blocks

Ok(Response::new())
}

/// `msg_to_sign` returns the message for an EOTS signature.
///
/// The EOTS signature on a block will be (block_height || block_hash)
Expand Down
16 changes: 16 additions & 0 deletions contracts/op-finality-gadget/src/msg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,16 @@ pub enum QueryMsg {
/// `btc_pk_hex` is the BTC public key of the finality provider, in hex format.
#[returns(Option<PubRandCommit>)]
LastPubRandCommit { btc_pk_hex: String },
/// `ForkedBlocks` returns the list of forked blocks
#[returns(Vec<(u64, u64)>)]
ForkedBlocks {},
/// `IsBlockForked` returns whether a given block is forked.
#[returns(bool)]
IsBlockForked { height: u64 },
/// `ForkedBlocksInRange` returns the list of forked blocks in a given range
#[returns(Vec<(u64, u64)>)]
ForkedBlocksInRange { start: u64, end: u64 },
/// `IsEnabled` returns whether the finality gadget is enabled.
#[returns(bool)]
IsEnabled {},
}
Expand Down Expand Up @@ -75,6 +85,12 @@ pub enum ExecuteMsg {
block_hash: Binary,
signature: Binary,
},
/// Whitelist forked blocks.
///
/// This message can be called by the admin only.
WhitelistForkedBlocks {
forked_blocks: Vec<(u64, u64)>,
},
/// Enable or disable finality gadget.
///
/// This message can be called by the admin only.
Expand Down
48 changes: 47 additions & 1 deletion contracts/op-finality-gadget/src/queries.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
use crate::error::ContractError;
use crate::state::config::{Config, ADMIN, CONFIG, IS_ENABLED};
use crate::state::finality::BLOCK_VOTES;
use crate::state::finality::{BLOCK_VOTES, FORKED_BLOCKS};
use crate::state::public_randomness::get_pub_rand_commit;
use babylon_apis::finality_api::PubRandCommit;
use cosmwasm_std::{Deps, StdResult, Storage};
use cw_controllers::AdminResponse;
use std::cmp::{max, min};
use std::collections::HashSet;

pub fn query_config(deps: Deps) -> StdResult<Config> {
Expand Down Expand Up @@ -46,6 +47,51 @@ pub fn query_last_pub_rand_commit(
Ok(res.into_iter().next())
}

pub fn query_forked_blocks(deps: Deps) -> StdResult<Vec<(u64, u64)>> {
FORKED_BLOCKS.load(deps.storage)
}

pub fn query_is_block_forked(deps: Deps, height: u64) -> StdResult<bool> {
// loop over forked blocks, starting from last entry
let forked_blocks = FORKED_BLOCKS.load(deps.storage)?;
for (start, end) in forked_blocks.iter().rev() {
// if block is in range of any forked block, return true
if height >= *start && height <= *end {
return Ok(true);
}
// terminate early once we reach a forked block range lower than the queried block
if height < *start {
break;
}
Comment on lines +63 to +65
Copy link
Collaborator

Choose a reason for hiding this comment

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

i don't understand the logic here. shouldn't we have a continue statement somewhere

lets say we have [(1,5), (8,13)] and height is 3

the function will return false b/c of the break here but it should return true

Copy link
Collaborator

Choose a reason for hiding this comment

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

actually we can just do a binary search

image

}
Ok(false)
}

pub fn query_forked_blocks_in_range(
deps: Deps,
start: u64,
end: u64,
) -> StdResult<Vec<(u64, u64)>> {
// loop over forked blocks, starting from last entry
let mut block_ranges = Vec::new();
let forked_blocks = FORKED_BLOCKS.load(deps.storage)?;
for (fork_start, fork_end) in forked_blocks.iter().rev() {
// append any overlapping forked block ranges to the list, moving cursor forward
let overlap_start = max(*fork_start, start);
let overlap_end = min(*fork_end, end);
if overlap_start <= overlap_end {
block_ranges.push((overlap_start, overlap_end));
}
// terminate early once we reach a forked block range lower than the queried block
if *fork_start < start {
break;
}
}
// reverse block ranges so they are ordered from highest to lowest
block_ranges.reverse();
Ok(block_ranges)
}

pub fn query_is_enabled(deps: Deps) -> StdResult<bool> {
IS_ENABLED.load(deps.storage)
}
Expand Down
5 changes: 4 additions & 1 deletion contracts/op-finality-gadget/src/state/finality.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
use cw_storage_plus::Map;
use cw_storage_plus::{Item, Map};
use std::collections::HashSet;

/// Map of signatures by block height and fp
pub(crate) const SIGNATURES: Map<(u64, &str), Vec<u8>> = Map::new("fp_sigs");

/// Map of (block height, block hash) tuples to the list of fps that voted for this combination
pub(crate) const BLOCK_VOTES: Map<(u64, &[u8]), HashSet<String>> = Map::new("block_hashes");

/// Ordered list of forked blocks [(start_height_1, end_height_1), (start_height_2, end_height_2), ...]
pub(crate) const FORKED_BLOCKS: Item<Vec<(u64, u64)>> = Item::new("forked_blocks");
Copy link
Member

Choose a reason for hiding this comment

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

This structure means that the vector of start/end heights is considered as a single item. If we track a lot of forks (which might be the case), then the DB ops over it could be some bottleneck.

Would we consider other structures like Map<u64, u64> where key is the start height and value is the end height?

Copy link
Collaborator

@bap2pecs bap2pecs Nov 29, 2024

Choose a reason for hiding this comment

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

then the DB ops over it could be some bottleneck.

i think if it's ordered with no overlap, in practice it's gonna be efficient b/c we can just use a binary search

Copy link
Collaborator

Choose a reason for hiding this comment

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

If we store it as a map, i don't know how to design an efficient algorithm to decide if a block is reorged or not

Copy link
Member

Choose a reason for hiding this comment

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

I think prefix_range or other iterator functions can be useful here. Check out prefixed_range_works under cw-storage-plus` repo

Copy link
Collaborator

Choose a reason for hiding this comment

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

i just took a look. i think the mental model will be cleaner to simply use (u64, u64) and query with a naive binary search like this

image

Copy link
Member

Choose a reason for hiding this comment

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

Actually there is a much simpler way: given a height, use prefix_range to find the first key that is smaller than the height, and then check if the height is between the key and value

Original file line number Diff line number Diff line change
Expand Up @@ -3,44 +3,11 @@ use cosmwasm_vm::testing::{
execute, instantiate, mock_env, mock_info, mock_instance, query, MockApi,
};

use cw_controllers::AdminResponse;
use op_finality_gadget::msg::{ExecuteMsg, InstantiateMsg, QueryMsg};
use op_finality_gadget::state::config::Config;

static WASM: &[u8] = include_bytes!("../../../artifacts/op_finality_gadget.wasm");
const CREATOR: &str = "creator";

#[test]
fn instantiate_works() {
// Setup
let mut deps = mock_instance(WASM, &[]);
let mock_api: MockApi = MockApi::default();
let msg = InstantiateMsg {
admin: mock_api.addr_make(CREATOR),
consumer_id: "op-stack-l2-11155420".to_string(),
is_enabled: false,
};
let info = mock_info(CREATOR, &[]);
let res: ContractResult<Response> = instantiate(&mut deps, mock_env(), info, msg.clone());
let msgs = res.unwrap().messages;
assert_eq!(0, msgs.len());

// Check the config is properly stored in the state and returned
let res: Config =
from_json(query(&mut deps, mock_env(), QueryMsg::Config {}).unwrap()).unwrap();
assert_eq!(msg.consumer_id, res.consumer_id);

// Check the admin is properly stored in the state and returned
let res: AdminResponse =
from_json(query(&mut deps, mock_env(), QueryMsg::Admin {}).unwrap()).unwrap();
assert_eq!(mock_api.addr_make(CREATOR), res.admin.unwrap());

// Check the contract is disabled on instantiation
let enabled: bool =
from_json(query(&mut deps, mock_env(), QueryMsg::IsEnabled {}).unwrap()).unwrap();
assert!(!enabled);
}

#[test]
fn disable_and_reenable_works() {
// Setup
Expand Down
Loading