-
Notifications
You must be signed in to change notification settings - Fork 645
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
Batched transactions (implements NEP#0008) #1140
Conversation
Benches (my macbook pro):
|
Previous benches for comparison:
|
c58ab82
to
a08667e
Compare
@@ -50,10 +49,12 @@ fn kv_to_state_record(key: Vec<u8>, value: DBValue) -> StateRecord { | |||
StateRecord::Account { account_id: to_printable(&key[1..]), account } | |||
} | |||
} | |||
/* |
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.
delete?
@@ -86,9 +87,11 @@ fn print_state_entry(key: Vec<u8>, value: DBValue) { | |||
to_printable(&from_base64(&value).unwrap()) | |||
); | |||
} | |||
/* |
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.
why not?
@@ -16,8 +15,7 @@ pub enum StateRecord { | |||
Contract { account_id: AccountId, code: String }, | |||
/// Access key associated with some account. | |||
AccessKey { account_id: AccountId, public_key: ReadablePublicKey, access_key: AccessKey }, | |||
/// Callback. | |||
Callback { id: Vec<u8>, callback: Callback }, | |||
// TODO: DATA |
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.
we need it for state dump to work, right?
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. Will push it separately
b1b57cb
to
ccba8e1
Compare
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.
Some of the gas-related stuff will later go into near-vm-logic
.
@@ -25,13 +25,10 @@ pub struct BlockHeader { | |||
/// Height of this block since the genesis block (height 0). | |||
pub height: BlockIndex, | |||
/// Hash of the block previous to this in the chain. | |||
#[serde(with = "base_format")] |
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.
Why we don't use base here anymore?
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.
We've implemented it for basic types. It helps to avoid things like Vec to have a type for base_vec_bytes_format
.
@@ -150,6 +192,25 @@ impl TryFrom<&str> for SecretKey { | |||
} | |||
} | |||
|
|||
impl Serialize for SecretKey { |
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.
This looks a bit weird. Where do we need to serialize the PublicKey without going through the proto? When returning through RPC as JSON? When override Serialize
like that we affect any type of serializer, which is kinda fine for now since we probably only serialize it as JSON at the moment, but if someone later in some crate serializes it with bincode (e.g. for cache) it will also go through base encoding which is unnecessary.
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, it's currently for serde serialization as json.
/// Identifier for callbacks, used to store storage and refer in receipts. | ||
pub type CallbackId = Vec<u8>; | ||
/// Gas is a type for storing amount of gas. | ||
pub type Gas = u64; |
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.
Gas
seems fine to me, since we don't have different underlying types for GasUsed
, GasBurnt
etc (for the contrast for storage we had StorageUsage
and StorageUsageChange
because one is u64
and another is i64
). It is not part of the protobuf or the protocol, so we can always change it later if we find it confusing or our code gets more complex.
runtime/runtime/src/actions.rs
Outdated
stake: &StakeAction, | ||
) { | ||
let mut account = account.as_mut().unwrap(); | ||
let increment = if stake.stake > account.staked { stake.stake - account.staked } else { 0 }; |
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.
Is it how we stake? If I staked already X and I want to increase my stake to X+Y then I issue a staking transaction with value X+Y instead of Y, correct?
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
pub add_key: Balance, | ||
pub delete_key: Balance, | ||
pub delete_account: Balance, | ||
pub struct TransactionCosts { |
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.
This structure might need to go entirely into near-vm-logic
.
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.
So this is what I start to wonder. If we decide to charge for every byte of the message (e.g. for args over network). This logic might need to be replicated with the VM, since both of them should produce same Gas cost results.
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 suggest we do the following:
For each binding function of VM we define a cost function: F(args, result)->Gas
. Which represents the fees that will be applied to call this function, including side-effects like creating receipts. We have this function in near-vm-logic
and the runtime
crate receives this information as part of used_gas
.
Pros:
- The logic is encapsulated in
near-vm-logic
which means it can be used for unit tests in smart contracts. People can write a smart contract, cover it with unit tests and if they did good job with coverage they can expect it to work 100% on the blockchain, because anything not covered bynear-vm-*
is guaranteed by the rest of engine (like receipts delivery, etc). - Our economics-related stuff is in one place, so we do a good job testing it to prevent all kinds of attacks, secondary markets, etc. We might even succeed formalizing it into an economic model.
Cons:
F(args, result)->Gas
will not represent the actual cost of sending a receipt over the network, including serialization etc. However, independently on our architecture we cannot devise a function (whether it is inruntime
ornear-vm-logic
) that can encapsulate all side-effects and costs, so it will always be an approximation. For instance we do not account for temporary storage that cross-contract calls create in the trie because we cannot measure how many nodes deep it will be, etc. So I suggest we always think of this fee as an approximation, and since it is an approximation, why not keep it innear-vm-logic
.
FYI this PR did not fix expensive tests, some of them do not even compile, to see run tests like this:
|
Related: #1156 |
Co-Authored-By: Maksym Zavershynskyi <35039879+nearmax@users.noreply.github.com>
Co-Authored-By: Maksym Zavershynskyi <35039879+nearmax@users.noreply.github.com>
7247c18
to
edbd6ab
Compare
…core into batched-tx-1
Some tests still fail. It's more to see what files are modified with some auto-replace.
Fixes: #1032