-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
@@ -127,6 +137,14 @@ pub fn buy_gas<T: Trait>( | |||
transactor: &T::AccountId, | |||
gas_limit: T::Gas, | |||
) -> Result<GasMeter<T>, &'static str> { | |||
// Check if the specified amount of gas is available in the current block. | |||
// This cannot underflow since `gas_spent` is never greater than `block_gas_limit`. | |||
let gas_available = <Module<T>>::block_gas_limit() - <Module<T>>::gas_spent(); |
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 store gas_left
instead of the two?
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.
You mean instead of block_gas_limit
and gas_spent
make just once gas_left
?
Hm, I don't see how that could work. At the end of the block we need to replenish gas_left
to the initial per block limit. We can hardcode it or... add block_gas_limit
storage variable : )
By the way, I considered using block_gas_limit
+ gas_left
combo. The reason why I chose gas_spent
is because you can reset it to zero at the finalization stage. With gas_left
you need to replenish it before executing the first transaction in the block and it is not obvious to me how to make it cleanly.
// Increase total spent gas. | ||
// This cannot overflow, since `gas_spent` is never greater than `block_gas_limit`, which | ||
// also has T::Gas type. | ||
let gas_spent = <Module<T>>::gas_spent() + gas_meter.spent(); |
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 this correct? Are we sure to never add the same gas_meter.spent()
twice here?
My point is that:
- You spend some gas and store it here (so you have
B + spent
in the storage) - You spend some more, say
X
and you add it here, so you will have(B + spent + (spent + X))
in the storage
I suppose it's not really a case, since we consume gas_meter
and probably take care of proper accounting on some other layer, but wouldn't it be simpler to just store global gas_left
in the storage?
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, that's not really the case and yes, I've tried hard to achieve the design where you have the construct like:
1. gas_meter = buy_gas(price)
... do stuff with gas meter
n. refund_unused_gas(gas_meter)
so it is an invariant that we do refund and spent gas accounting only once per transaction and the mechanism for that, as you noticed, is consuming the GasMeter
.
Let's move our discussion why I used gas_spent
instead of gas_left
in this thread!
32b4b52
to
7909105
Compare
7909105
to
2fba520
Compare
Staking::free_balance(&CONTRACT_SHOULD_TRANSFER_TO), | ||
0, | ||
); | ||
assert_eq!(Staking::free_balance(&1), 11,); |
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.
trailing ,
on inline list is superfluous
0, | ||
); | ||
assert_eq!(Staking::free_balance(&1), 11,); | ||
assert_eq!(Staking::free_balance(&CONTRACT_SHOULD_TRANSFER_TO), 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.
same
Staking::free_balance(&CONTRACT_SHOULD_TRANSFER_TO), | ||
11, | ||
); | ||
assert_eq!(Staking::free_balance(&CONTRACT_SHOULD_TRANSFER_TO), 11,); |
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.
same
// Setup two accounts with free balance above than exsistential threshold. | ||
{ | ||
Staking::set_free_balance(&1, 110); | ||
Staking::increase_total_stake_by(110); |
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.
gah! spaces!
<StorageOf<Test>>::insert(1, b"bar".to_vec(), b"2".to_vec()); | ||
|
||
Staking::set_free_balance(&2, 110); | ||
Staking::increase_total_stake_by(110); |
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.
gah! spaces!
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.
style...
8677fd4
to
a58a6e5
Compare
a58a6e5
to
72ba19b
Compare
Fixed and rebased, @gavofyork please take a look! |
//! Gas is bought upfront. Any unused is refunded after the transaction (regardless of the | ||
//! execution outcome). If all gas is used, then changes made for the specific call or create | ||
//! are reverted (including balance transfers). | ||
//! Gas is bought upfront up to the, specified in transaction, limit. Any unused gas is refunded |
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.
should read "upfront for the limit, which is specified in transaction"
looks reasonable. |
* master: Contracts: Per block gas limit (#506) Make sure to ban invalid transactions. (#615) (#620) Forward-port BFT fixes from v0.2 and restructure agreement cancelling (#619) Allow specifying listening multiaddresses (#577) Introduce Runtime Events (#607) update substrate/extrinsic-pool (#616) add a new unit test for extrinsic pool (#611) set the current repo in Cargo.toml (#610) add cli for purge chain (#609)
…rs-generic-over-hasher-and-rlpcodec * origin/master: (26 commits) Contract runtime polishing (#601) WIP on chain heap (#639) Events to track extrinsic success (#640) Install llvm-tools-preview component (#643) fix wasm executor compile error (#631) random fixes (#638) Empty becomes (), reflecting convention (#637) Allow to build_upon skipped entries, but don't walk back (#635) Separate out staking module into balances and payment (#629) Update .gitlab-ci.yml (#633) Do not attempt to rustup if in CI. This is taken care of by the base (#621) Avoid need for ident strings in storage (#624) rename to panic_handler as panic_implementation is deprecated in nightly (#626) 5 random fixes (#2) (#623) fix one typo in README (#627) Misspelled words (#625) Contracts: Per block gas limit (#506) Make sure to ban invalid transactions. (#615) (#620) Forward-port BFT fixes from v0.2 and restructure agreement cancelling (#619) Allow specifying listening multiaddresses (#577) ...
* Fix locking, from an updated Substrate (paritytech#506) * Fix locking, from an updated Substrate * Bump runtime version * Lock file. * Bump version and extra fix.
Decrease storage fee percentage that goes into escrow and taken out of it each block
Introduce per block gas limit checking.
Before buying gas we check how much gas available to spend in this block. The available gas is computed by substracting spent gas from the block limit. Spent gas is increased at the same time when we do refunds and reset at the block finilization stage.