Implement timestamp_searcher in blockprod #1747
Conversation
| /// Collect search data that can be subsequently passed into find_timestamps_for_staking | ||
| /// to perform timestamps search. | ||
| /// | ||
| /// For each block height in the specified range, the search will find timestamps where staking | ||
| /// is/was possible. | ||
| /// | ||
| /// `min_height` must not be zero; `max_height` must not exceed the best block height plus one | ||
| /// (this value is assumed if `None` is passed). | ||
| /// | ||
| /// If `check_all_timestamps_between_blocks` is false, `seconds_to_check_for_height + 1` is | ||
| /// the number of seconds that will be checked at each height in the range. | ||
| /// If `check_all_timestamps_between_blocks` is true, `seconds_to_check_for_height` only applies | ||
| /// to the last height in the range; for all other heights the maximum timestamp is the timestamp | ||
| /// of the next block. | ||
| pub async fn collect_timestamp_search_data( | ||
| chainstate_handle: &ChainstateHandle, | ||
| chain_config: Arc<ChainConfig>, | ||
| secret_input_data: PoSTimestampSearchInputData, | ||
| min_height: BlockHeight, | ||
| max_height: Option<BlockHeight>, | ||
| seconds_to_check_for_height: u64, | ||
| check_all_timestamps_between_blocks: bool, | ||
| ) -> Result<TimestampSearchData, BlockProductionError> { |
There was a problem hiding this comment.
2discuss: the check_all_timestamps_between_blocks parameter seems redundant here, probably it should be removed and the code should behave as if it was always true?
There was a problem hiding this comment.
This is an auxiliary functionality, so I'm OK with it not being perfect. It's only gonna be used by us, devs, mostly.
| /// For each block height in the specified range, find timestamps where staking is/was possible | ||
| /// for the given pool. | ||
| /// | ||
| /// `min_height` must not be zero; `max_height` must not exceed the best block height plus one. | ||
| /// | ||
| /// If `check_all_timestamps_between_blocks` is "no", `seconds_to_check_for_height + 1` is the number | ||
| /// of seconds that will be checked at each height in the range. | ||
| /// If `check_all_timestamps_between_blocks` is "yes", `seconds_to_check_for_height` only applies to the | ||
| /// last height in the range; for all other heights the maximum timestamp is the timestamp | ||
| /// of the next block. | ||
| #[clap(name = "node-find-timestamps-for-staking")] | ||
| #[clap(hide = true)] | ||
| FindTimestampsForStaking { | ||
| pool_id: String, | ||
| min_height: BlockHeight, | ||
| max_height: BlockHeight, | ||
| seconds_to_check_for_height: u64, | ||
| check_all_timestamps_between_blocks: YesNo, | ||
| }, |
There was a problem hiding this comment.
2discuss: I'm also not sure about the cli interface; perhaps, pool_id shouldn't be here and the search should always be performed for all pools in all accounts? (or may be we need an additional command for that).
ImplOfAnImpl
requested review from
TheQuantumPhysicist,
azarovh,
erubboli and
iljakuklic
as code owners
ImplOfAnImpl
force-pushed
the
blockprod_misc_cleanup
branch
from
7ba7dcb to
a020ef0
Compare
ImplOfAnImpl
force-pushed
the
blockprod_timestamp_searcher
branch
from
d6fabf2 to
9cc49cf
Compare
| ) -> rpc::RpcResult<()>; | ||
|
|
||
| #[method(name = "node_find_timestamps_for_staking")] | ||
| async fn node_find_timestamps_for_staking( |
There was a problem hiding this comment.
Please document this
There was a problem hiding this comment.
Please document this
Done. But I should note that it's basically a copy-paste of the documentation for WalletCommand::FindTimestampsForStaking.
| .await? | ||
| } | ||
|
|
||
| pub async fn find_timestamps_for_staking( |
There was a problem hiding this comment.
This function also needs documentation
There was a problem hiding this comment.
This function also needs documentation
Functions in this WalletRpc struct don't seem to have much documentation. And since this function just calls find_timestamps_for_staking in wallet-controller, I decided to put the documentation there instead, "for consistency". Again, this is a copy-paste of the documentation for WalletCommand::FindTimestampsForStaking, so it's triplicated now.
TheQuantumPhysicist
left a comment
TheQuantumPhysicist
left a comment
There was a problem hiding this comment.
Looking OK. I asked Boris to review as well.
ImplOfAnImpl
force-pushed
the
blockprod_misc_cleanup
branch
from
a020ef0 to
6de203f
Compare
ImplOfAnImpl
force-pushed
the
blockprod_timestamp_searcher
branch
from
9cc49cf to
e480095
Compare
blockprod/src/detail/mod.rs
Outdated
| pub async fn collect_timestamp_search_data( | ||
| &self, | ||
| secret_input_data: PoSTimestampSearchInputData, |
There was a problem hiding this comment.
FYI: I've just realised that collect_timestamp_search_data doesn't really need the vrf key, it only needs the pool id (passing the whole PoSTimestampSearchInputData here is an artifact of a previous implementation). So, e2e is not needed for this function and I've removed it.
A
timestamp_searchermodule has been added toblockprodthat allows to obtain a list of timestamps where staking is/was possible given a range of block heights and a pool id.Also, the corresponding command was added to wallet-cli, so that this functionality can be invoked from the console.
P.S.
rayon. This will be a problem if the searching is started across an RPC boundary. In the current implementation, this is possible if the wallet-cli acts as a client to wallet-rpc-daemon. This is adresses in the following PR (Call blockprod::find_timestamps_for_staking directly from wallet cli #1748).