-
Notifications
You must be signed in to change notification settings - Fork 159
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
Offline RPC #3866
Offline RPC #3866
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we refactor the creation of the server?
scripts/tests/calibnet_rpc_check.sh
Outdated
|
||
# Function to get the number of files in the present working directory | ||
num_files_here() { | ||
find . -type f | wc --lines |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this overcounts if I make a file with a newline in the name
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed, now directly using the path returned from snapshot fetch
scripts/tests/calibnet_rpc_check.sh
Outdated
done | ||
|
||
# Check if services on ports 8080 and 8081 have started | ||
while ! (nc -z localhost 8080 && nc -z localhost 8081); do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this could be an until
loop
also hardcoding the ports here defeats the purpose of using the PORTS
array
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
right, fixed
scripts/tests/calibnet_rpc_check.sh
Outdated
result="$($FOREST_TOOL_PATH api compare "$snapshot" --forest /ip4/127.0.0.1/tcp/8080/http --lotus /ip4/127.0.0.1/tcp/8081/http)" | ||
|
||
# Check the result | ||
if echo "$result" | tail -n +3 | grep -E -v "\| *(Valid|Timeout) *\| *(Valid|Timeout) *\|"; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parsing the output is an odd choice - can we not modify forest-tool api compare
to give us useful exit codes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -8,7 +8,7 @@ use fvm_ipld_blockstore::Blockstore; | |||
use jsonrpc_v2::{Data, Error as JsonRpcError, Params}; | |||
|
|||
/// RPC call to create a new JWT Token | |||
pub(in crate::rpc) async fn auth_new<DB: Blockstore>( | |||
pub async fn auth_new<DB: Blockstore>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like it
src/rpc/mod.rs
Outdated
mod state_api; | ||
mod sync_api; | ||
mod wallet_api; | ||
pub mod auth_api; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wibni: a module structure that ditches the common suffix use rpc::api::auth
rather than rpc::auth_api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would like to do this in a separate PR
|
||
#[derive(Debug, Subcommand)] | ||
pub enum ApiCommands { | ||
// Serve | ||
Serve { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using office rpc name instead of node-less rpc
src/tool/subcommands/api_cmd.rs
Outdated
chain: NetworkChain, | ||
rpc_port: u16, | ||
) -> anyhow::Result<()> { | ||
println!("Configuring Node-Less RPC Server"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
surprising that we're not using info!(..)
logs here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changed to using info logger now
src/tool/subcommands/api_cmd.rs
Outdated
.with_method(CHAIN_GET_TIPSET_BY_HEIGHT, chain_get_tipset_by_height::<DB>) | ||
.with_method(CHAIN_GET_GENESIS, chain_get_genesis::<DB>) | ||
.with_method(CHAIN_GET_TIPSET, chain_get_tipset::<DB>) | ||
.with_method(CHAIN_HEAD, chain_head::<DB>) | ||
.with_method(CHAIN_GET_BLOCK, chain_get_block::<DB>) | ||
.with_method(CHAIN_SET_HEAD, chain_set_head::<DB>) | ||
.with_method(CHAIN_GET_MIN_BASE_FEE, chain_get_min_base_fee::<DB>) | ||
.with_method( | ||
CHAIN_GET_MESSAGES_IN_TIPSET, | ||
chain_get_messages_in_tipset::<DB>, | ||
) | ||
.with_method(CHAIN_GET_PARENT_MESSAGES, chain_get_parent_message::<DB>) | ||
.with_method(CHAIN_NOTIFY, chain_notify::<DB>) | ||
.with_method(CHAIN_GET_PARENT_RECEIPTS, chain_get_parent_receipts::<DB>) | ||
// Message Pool API | ||
.with_method(MPOOL_GET_NONCE, mpool_get_nonce::<DB>) | ||
.with_method(MPOOL_PENDING, mpool_pending::<DB>) | ||
.with_method(MPOOL_PUSH, mpool_push::<DB>) | ||
.with_method(MPOOL_PUSH_MESSAGE, mpool_push_message::<DB>) | ||
// Sync API | ||
.with_method(SYNC_CHECK_BAD, sync_check_bad::<DB>) | ||
.with_method(SYNC_MARK_BAD, sync_mark_bad::<DB>) | ||
.with_method(SYNC_STATE, sync_state::<DB>) | ||
// Wallet API | ||
.with_method(WALLET_BALANCE, wallet_balance::<DB>) | ||
.with_method(WALLET_DEFAULT_ADDRESS, wallet_default_address::<DB>) | ||
.with_method(WALLET_EXPORT, wallet_export::<DB>) | ||
.with_method(WALLET_HAS, wallet_has::<DB>) | ||
.with_method(WALLET_IMPORT, wallet_import::<DB>) | ||
.with_method(WALLET_LIST, wallet_list::<DB>) | ||
.with_method(WALLET_NEW, wallet_new::<DB>) | ||
.with_method(WALLET_SET_DEFAULT, wallet_set_default::<DB>) | ||
.with_method(WALLET_SIGN, wallet_sign::<DB>) | ||
.with_method(WALLET_VERIFY, wallet_verify) | ||
.with_method(WALLET_DELETE, wallet_delete::<DB>) | ||
// State API | ||
.with_method(STATE_CALL, state_call::<DB>) | ||
.with_method(STATE_REPLAY, state_replay::<DB>) | ||
.with_method(STATE_NETWORK_NAME, state_network_name::<DB>) | ||
.with_method(STATE_NETWORK_VERSION, state_get_network_version::<DB>) | ||
.with_method(STATE_ACCOUNT_KEY, state_account_key::<DB>) | ||
.with_method(STATE_LOOKUP_ID, state_lookup_id::<DB>) | ||
.with_method(STATE_GET_ACTOR, state_get_actor::<DB>) | ||
.with_method(STATE_MARKET_BALANCE, state_market_balance::<DB>) | ||
.with_method(STATE_MARKET_DEALS, state_market_deals::<DB>) | ||
.with_method(STATE_MINER_INFO, state_miner_info::<DB>) | ||
.with_method(MINER_GET_BASE_INFO, miner_get_base_info::<DB>) | ||
.with_method(STATE_MINER_ACTIVE_SECTORS, state_miner_active_sectors::<DB>) | ||
.with_method(STATE_MINER_SECTOR_COUNT, state_miner_sector_count::<DB>) | ||
.with_method(STATE_MINER_FAULTS, state_miner_faults::<DB>) | ||
.with_method(STATE_MINER_RECOVERIES, state_miner_recoveries::<DB>) | ||
.with_method(STATE_MINER_POWER, state_miner_power::<DB>) | ||
.with_method(STATE_MINER_DEADLINES, state_miner_deadlines::<DB>) | ||
.with_method(STATE_LIST_MINERS, state_list_miners::<DB>) | ||
.with_method( | ||
STATE_MINER_PROVING_DEADLINE, | ||
state_miner_proving_deadline::<DB>, | ||
) | ||
.with_method(STATE_GET_RECEIPT, state_get_receipt::<DB>) | ||
.with_method(STATE_WAIT_MSG, state_wait_msg::<DB>) | ||
.with_method(STATE_SEARCH_MSG, state_search_msg::<DB>) | ||
.with_method(STATE_SEARCH_MSG_LIMITED, state_search_msg_limited::<DB>) | ||
.with_method(STATE_FETCH_ROOT, state_fetch_root::<DB>) | ||
.with_method( | ||
STATE_GET_RANDOMNESS_FROM_TICKETS, | ||
state_get_randomness_from_tickets::<DB>, | ||
) | ||
.with_method( | ||
STATE_GET_RANDOMNESS_FROM_BEACON, | ||
state_get_randomness_from_beacon::<DB>, | ||
) | ||
.with_method(STATE_READ_STATE, state_read_state::<DB>) | ||
.with_method(STATE_CIRCULATING_SUPPLY, state_circulating_supply::<DB>) | ||
.with_method(STATE_SECTOR_GET_INFO, state_sector_get_info::<DB>) | ||
.with_method( | ||
STATE_VM_CIRCULATING_SUPPLY_INTERNAL, | ||
state_vm_circulating_supply_internal::<DB>, | ||
) | ||
.with_method(MSIG_GET_AVAILABLE_BALANCE, msig_get_available_balance::<DB>) | ||
.with_method(MSIG_GET_PENDING, msig_get_pending::<DB>) | ||
// Gas API | ||
.with_method(GAS_ESTIMATE_FEE_CAP, gas_estimate_fee_cap::<DB>) | ||
.with_method(GAS_ESTIMATE_GAS_LIMIT, gas_estimate_gas_limit::<DB>) | ||
.with_method(GAS_ESTIMATE_GAS_PREMIUM, gas_estimate_gas_premium::<DB>) | ||
.with_method(GAS_ESTIMATE_MESSAGE_GAS, gas_estimate_message_gas::<DB>) | ||
// Common API | ||
.with_method(VERSION, move || { | ||
version(block_delay, FOREST_VERSION_STRING.as_str()) | ||
}) | ||
.with_method(SESSION, session) | ||
.with_method(START_TIME, start_time::<DB>) | ||
// Node API | ||
.with_method(NODE_STATUS, node_status::<DB>) | ||
// Eth API | ||
.with_method(ETH_ACCOUNTS, eth_accounts) | ||
.with_method(ETH_BLOCK_NUMBER, eth_block_number::<DB>) | ||
.with_method(ETH_CHAIN_ID, eth_chain_id::<DB>) | ||
.with_method(ETH_GAS_PRICE, eth_gas_price::<DB>) | ||
.with_method(ETH_GET_BALANCE, eth_get_balance::<DB>) | ||
.finish_unwrapped(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have I seen this code before? Could we refactor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it would be really great to have a single "helper" method for all the occurrences of RPC definitions, assuming they are all the same. If not - perhaps whatever is shared is a good start.
Something like:
fn rpc_methods(server: Arc<Server>)
or otherwise we could define a custom trait and implement it for the Server
struct, see extension traits for inspiration.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I can see now that this is RPC v1
. As opposed to v0
or ws
. Curious if there is already a definition for that elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using start_rpc
method now from rpc crate
src/tool/subcommands/api_cmd.rs
Outdated
); | ||
let snapshot_url = | ||
crate::cli_shared::snapshot::stable_url(TrustedVendor::default(), &chain)?; | ||
let tmp_snapshot_path = tempfile::Builder::new().tempdir()?.into_path(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/tmp
typically has 10GB size limit which is insufficient for downloading a mainnet snapshot
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we could just keep the logic simple, ask users to download a snapshot via forest-tool snapshot fetch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or use logic similar to that of the running node? Seems laborious having to download snapshots manually..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with roman, I think we should keep the automatic snapshot download feature if a snapshot is not provided
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but you have to take into account Hailong's comment about tmp
dir size.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using project dir now
src/tool/subcommands/api_cmd.rs
Outdated
let (snapshot_file, snapshot_path) = if let Some(path) = snapshot_path_opt { | ||
(File::open(&path).await?, path) | ||
} else { | ||
warn!( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we add some prompt before downloading? Like in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes i'll add a similar prompt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, then I guess we also need a similar --force
or --yes
option to bypass the prompt
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Summary of changes
Changes introduced in this pull request:
serve
subcommand:forest-tool api serve [CAR_FILE]
command runs an RPC server.2345
which can be configured using--port
flagReference issue to close (if applicable)
Closes #3812
Other information and links
Change checklist