Closed aakoshh closed 1 year ago
The syscalls that definitely don't need to be repriced are mostly miner/crypto related ones:
I went through the syscalls :eyes:
Here are some rough notes I took: syscall-notes.txt
Things I thought might be worth looking into:
vm::abort
: As noted by @Stebalien it allows the user to set an arbitrarily long message
which DefaultCallManager::send_resolved
will put in the backtrace
, which finally is copied into the ApplyRet
in DefaultExecutor::finish_message
. Maybe worth limiting the size of that message, or make them pay for it.ipld::block_stat
: This one is notable because seemingly all it does is find a block in the BlockRegistry
and return its size, charging a fixed fee for it. Other methods that also access the block registry don't have a similar fixed fee. ipld::block_read
: It charges in proportion to the amount of data read, ~but shouldn't it charge a fixed fee as well for the registry lookup, like ipld::block_stat
?~ EDIT: It does, called block_read_base
.ipld::block_link
: It charges in proportion to the amount of data written to the store, ~but shouldn't it charge a fixed fee as well for the registry lookup, like ipld::block_stat
?~ EDIT: It does, called block_link_base
.sself::root
: No gas charge. If the data isn't found in the snapshot caches, it will read it from the HAMT which has a variable cost depending on the size of the data structure and fetch from the Blockstore
. Since this isn't going through ipld
it won't be charged, but the amount of data read can be non-trivial as seen in the EVM. It's also outside the control of the actor since it depends on the number other actors in the state tree, but this is one of the reason the EVM opcodes need to be repriced as the state grows. sself::set_root
: No gas charge. Similar to sself::root
it will look data up in the HAMT if it has to. Then it will insert new data into the snapshot layers which get flushed at the end to the block store. These reads/writes are not accounted for. Their size is again outside the control of the actor I think, but their cost will grow as the state gets bigger. sself::current_balance
: No gas charge. Similar to sself::root
it might end up loading data into the HAMT.sself::self_destruct
: Charges a fixed price. Calls current_balance
which is free. iff the balance is non-zero, it will also do: 1) address lookup which might involve the HAMT and the Blockstore then 2) a transfer, which involves two actor lookups and two actor states written to the snapshots, and finally 3) it marks the actor for deletion in the snapshots. 2) and 3) both result in deferred writes to the HAMT and flushing, which involves varying amount of data depending on the size of the state. Can all this be expressed by that fixed price?actor::resolve_address
: No gas charge. Involves lookups in the ever growing state tree HAMT.actor::get_actor_code_cid
: No gas charge. Involves actor lookup in the ever growing state tree HAMT.actor::new_actor_address
: No gas charge. Calls resolve_to_key_addr
but tells it not to charge for gas.actor::create_actor
: Charges a fixed fee, but creation involves a lookup and a deferred insert into the ever growing state tree HAMT.actor::get_builtin_actor_type
:
No gas charge. Just an in-memory lookup. Should there be some fixed fee?actor::get_code_cid_for_type
: No gas charge. Just an in-memory lookup. Should there be some fixed fee?crypto::verify_signature
: Charges a fixed fee. Shouldn't it also charge in proportion to the size of the plaintext it has to hash? crypto::hash
does it.crypto::compute_unsealed_sector_cid
: Receives a vector of things that get aggregated but charges a fixed fee.crypto::verify_consensus_fault
: Receives three byte arrays with unlimited size which it passes on, but it charges a fixed fee.crypto::batch_verify_seals
: No gas charge. Even though it verifies an unlimited amount of inputs in parallel, there's no gas as far as I see.gas::charge_gas
: Should we charge some fixed fee for calling this? It also accepts unlimited sized name; although it's probably disabled in production, that will only be apparent after all the copying from memory and calling the tracker.debug::store_artifact
: Just a note that I found the error handling flow confusing. First I thought it won't even write any data. On second read, it should, but it will log that it successfully exported the artifact even after logging that it failed.send::send
: Doesn't charge gas on its own, but it will look up the parameters in the block registry - should it charge in proportion to the size, or a fixed fee? (I also noticed that the Block
actually shares the underlying data with other instances through an Rc
. I thought ticket #1074 was talking about these block counting against memory, but I see they aren't stored with duplication during this message passing. Maybe the ticket is about something else).
vm::abort
The message is "for debugging only" and shouldn't have any affect on consensus. It think we just need to:
Thanks for writing this up!
@Stebalien @aakoshh what are next steps here?
@maciejwitowski I can think of the following:
@maciejwitowski following up on my previous comment:
While we're here, chain state storage is dramatically underpriced relative to message bytes. This was bad but not terrible without user-programmed actors but could become really bad with user-programmed actors, as it will encourage optimisation that will end up being more expensive for the network. Can we make some progress towards rectifying that here? cc @Kubuxu
chain state storage is dramatically underpriced relative to message bytes
@anorth could you elaborate? How were the price of message bytes established?
Perhaps unrelated, but we need a conversion factor from in-memory to on-disk storage access times.
Similar effort to https://github.com/filecoin-project/ref-fvm/pull/465
Capture the time-to-gas ratio of syscalls, to facilitate an analysis as in https://arxiv.org/pdf/1905.00553v1.pdf
Another, more deterministic approach is explained in https://github.com/near/nearcore/blob/78ae3b1696b308e25a61a6e896e484886829837c/runtime/runtime-params-estimator/emu-cost/README.md I'm not sure how IO is accounted for there.