Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

Proposal: Allow Dynamic Weights #3730

Closed
kianenigma opened this issue Oct 1, 2019 · 6 comments
Closed

Proposal: Allow Dynamic Weights #3730

kianenigma opened this issue Oct 1, 2019 · 6 comments
Labels
J0-enhancement An additional feature request. J2-unconfirmed Issue might be valid, but it’s not yet known.

Comments

@kianenigma
Copy link
Contributor

Regards to this conversation about weights and this part of it:

and calculating it would take about as long as actually doing it

I was thinking maybe, maybe, we can allow this with a special type of weight (e.g. #[weight = Dynamic(Foo)]) which is allowed to be updated after the call has been dispatched. The requirement comes from what I have been discussing with @pepyakin recently about how he can use the weight system for his contract module and at least for his situation: a dispatch's operations can be separated into two parts:

  • Fixed: preparing contract, gas metering primitives etc. equivalent of calling an noop contract.
  • Dynamic: dependent directly on some input or internal state which cannot be predicted without actually dispatching the call first.

I was thinking we could make it a pattern of passing some data from the Ok(()) of the .dispatch() and then pass it into post_dispatch to make updates current block weight, charge more/less fee etc. .post_dispatch would automatically call Foo::update_weight() with some parameter T returned from .dispatch().

I am not sure yet if this is general enough to be considered in substrate, or if we want to stick to the current simple approach of weight must be calculated before the dispatch, so this is to discuss this before any prototyping takes place.

@kianenigma kianenigma added J0-enhancement An additional feature request. J2-unconfirmed Issue might be valid, but it’s not yet known. labels Oct 1, 2019
@xlc
Copy link
Contributor

xlc commented Oct 1, 2019

Any chain use tx fee as a security mechanism and have dispatch methods that are not O(1) will need this ability.
One tricky thing is how to (re)charge for the additional amount of the tx fee. It can either over-charge upfront (like max gas fee) and refund the unused part, or simply charge additional fee per increase weight (this means weight can never decrease, which is fine).

@kianenigma
Copy link
Contributor Author

Any chain use tx fee as a security mechanism and have dispatch methods that are not O(1) will need this ability.

Yeah -- now the approach is that if a particular transaction can cost more, it should handle it manually somehow.

One tricky thing is how to (re)charge for the additional amount of the tx fee. It can either over-charge upfront (like max gas fee) and refund the unused part, or simply charge additional fee per increase weight (this means weight can never decrease, which is fine).

Yeah indeed, this is exactly how the smart contract was discussed to work with the proposal of this issue.

@kianenigma kianenigma changed the title Allow dynamic weights Proposal: Allow Dynamic Weights Oct 1, 2019
@kianenigma
Copy link
Contributor Author

kianenigma commented Oct 3, 2019

We have been talking quite a lot with @pepyakin off-band about this and the gist of it, at least from my PoV is:

Primitives

Why do we have weights?: The main purpose is to prevent a block from being filled too much with transactions. weights should embody all limited resources in a block. It is easy, yet accurate, to think of it just in terms of

  • how much you write to persistent state.
  • how much of the block time you consume, since a block production being overrun can cause consensus issues and is deemed as a pretty back f-up.

What does the weight system now represent?: Just execution time. For now, transactions that could potentially write to state are (to be best of our knowledge) safeguarded manually by charging more money or taking bonds or some other manual inspection form within the function definition itself. Moreover, the current system has one very important condition: weights must be known pre-dispatch from outside the runtime. This makes sense. The client (tx queue in particular) wants to know how much time it is going to spend on a transaction before starting to execute it.

For this reason, the best-effort approach is to make any static weight annotation, a pure representation of the worse case scenario, if possible.

I will use the term gas and weight interchangeably below but we can imagine both are the same (or can be trivially converted as both are just a fictional representation of resource), and they can be both mapped to a fee via some Fn(w: weight) -> Fee.

New discussion.

The core fo the discussion is rooted in the fact that a typical smart contract call is consisted of

  1. minor static part (boilerplate for contract exec env., gas metering etc.)
  2. and a major dynamic part which is entirely unpredictable pre-dispatch (depends on the contract code and arguments).

What the contract module is doing now is

  1. it is individually charging a max_gas, stopping execution if that max gas is overrun.
  2. refunding if you have some left.

Now, I never expect all runtime calls to be so restricted. We rant a lot in docs that a runtime dev should be responsible and prevent such mistakes and attack vectors. It would merely be nice to give them more flexibility for doing so, but we won't hard-code such approaches. In this sense, and so far, I don't really see substrate needing to make any effort regarding helping contracts module.

But there is one problem that the current smart-contract module is causing. smart-contracts module, as of now, can not, and does not, properly update the global, per-block, accumulated weight, which is being examined by the tx-queue (indirectly) in the above example. This, as mentioned, can be a major problem as blocks could become big in terms of resource, while no one knows why.

we both agree that the use cases of smart-contracts, while being unique to itself, is not the only one and even in the current substrate node runtime we are overlooking similar scenarios. Sudo, some dispatches that take a Vec<_> as input, and and any dispatch which is nested (proposals), have the same scenario.

Now, @pepyakin can go on and solve everything by an ugly hack of making this accumulated weight in system public, and manually updating it. But, as mentioned, since some other modules (not to mention future modules) share the same, I see it worthwhile to create some primitives to help such scenarios. Best thing to do, that I think all modules can benefit from is to enable a general way for post-dispatch weight update.

There will be a special type of weights that imply: it is still static, it should still be a worse case approximation, but you can update it after you your dispatch is done. As mentioned, this is not a holy grail to solve any attackable dispatch. That's merely the responsibility of the developer. This just makes it easier to deal with such stuff. Ideally, we can even emit this information to the transaction queue so that it can make decisions based on knowing:

  • priority (which afaik is the only transaction inclusion ordering as of now).
  • weight type aka. enum Weight { Static(u32), Dynamic(u32) }.

(cc @tomusdrw)

So that

1- queue would now the risk ahead of executing something which is Dynamic
2- substrate runtime automatically allows such calls to update the weight post-dispatch

  • Not surprisingly at all, I think this can be easily implemented using the already existing post_dispatch hook. We just have to allow some data from the dispatch itself to flow into post_dispatch.

(In the rest of our discussion, we got into some more details that I will summarise here later.)

@kianenigma
Copy link
Contributor Author

I won't close this for now, but an update that it partially happened by adding register_additional_weight() into system #4058

@athei
Copy link
Member

athei commented Apr 9, 2020

Parts of the (dynamically) annotated max weight can soon be refunded: #5458 #5584

Is this issue resolved by this?

@kianenigma
Copy link
Contributor Author

Yes, this is closed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J0-enhancement An additional feature request. J2-unconfirmed Issue might be valid, but it’s not yet known.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants