Skip to content
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

Disallow usage of from_ref_time for weights after the relevant polkadot version #1589

Closed
Chralt98 opened this issue Sep 15, 2023 · 4 comments
Labels
I10-unconfirmed Issue might be valid, but it's not yet known.

Comments

@Chralt98
Copy link

Hey valuable Polkadot community,

we were able to construct a runtime with from_ref_time here, although weights-v2 are active by our used polkadot version. This resulted in transactions not being able to be processed by the nodes. If a transaction doesn't use from_ref_time we were able to execute those transactions.

Could you please deprecate the usage of from_ref_time so that everyone sees that this function shouldn't be called anymore by the specific polkadot version?

@github-actions github-actions bot added the I10-unconfirmed Issue might be valid, but it's not yet known. label Sep 15, 2023
@ggwpez
Copy link
Member

ggwpez commented Sep 15, 2023

This resulted in transactions not being able to be processed by the nodes. If a transaction doesn't use from_ref_time we were able to execute those transactions.

I dont follow this logic. from_ref_time(x) is exactly equivalent to from_parts(x, 0), it was just a convenience wrapper.

Anyhow, it was deprecated in paritytech/substrate#13475 and removed in paritytech/substrate#14397 already.

@Chralt98
Copy link
Author

Chralt98 commented Sep 15, 2023

We are currently unable to process transactions on battery station here. A transfer call just doesn't get executed. I looked in the code and found out that each transaction with from_ref_time inside the weight function wasn't able to get processed. However, the transaction set_heap_pages() was able to get processed by the local node setup (it just was the first example that I could find without from_ref_time).

Thanks for your quick response! And cool that you already removed it.

EDIT: this commit resulted txs not being processed with from_ref_time inside the weight function (assume a local dev node setup).

@ggwpez
Copy link
Member

ggwpez commented Sep 15, 2023

I just queried System::blockWeight and it reports about this:

  normal: {
    refTime: 0
    proofSize: 0
  }
  operational: {
    refTime: 0
    proofSize: 0
  }
  mandatory: {
    refTime: 7,348,715,184
    proofSize: 69,273,040
  }
}

Which means that some on_initialize or on_finalize hooks are consuming about 70 MB of pov size, while your limit is (correctly) set to 5 MiB.
So that could be why only transactions that have from_ref_time in them are being processed.

It looks like there is a sudo key set, so you can try Sudo(Utility::WithWeight(SetCode(...), (0, 0)) to force a transaction in.
I think there is currently no PoV base weight per transaction, so could be that this works. But better test with a Remark first. There should be a debug log in finalize, in case that you have debug logging enabled somewhere.

ggwpez referenced this issue in zeitgeistpm/zeitgeist Sep 15, 2023
@ggwpez
Copy link
Member

ggwpez commented Sep 15, 2023

Yes pretty much confirmed. Prediction market on_init is using about 70 MB proof per block as introduced here: zeitgeistpm/zeitgeist@bb0e671#r127466669

We are adding some sanity checker for parachains here #1493, but currently the mandatory weight needs to be checked manually or by using something like subweight: https://github.com/ggwpez/substrate-weight-compare

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I10-unconfirmed Issue might be valid, but it's not yet known.
Projects
None yet
Development

No branches or pull requests

2 participants