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

Sixth iteration of the Court pallet #244

Merged
merged 3 commits into from
Aug 19, 2021
Merged

Sixth iteration of the Court pallet #244

merged 3 commits into from
Aug 19, 2021

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Jul 28, 2021

Depends on #233
cc #200

Basically brings back another bunch of storage and types from simple-disputes to PM since they are used on both court and simple-disputes pallets

@c410-f3r c410-f3r changed the title Sixth iteration of the Court pallet f5a7da6 Sixth iteration of the Court pallet Jul 29, 2021
@c410-f3r c410-f3r added the s:review-needed The pull request requires reviews label Aug 1, 2021
@c410-f3r c410-f3r mentioned this pull request Aug 10, 2021
Copy link
Member

@sea212 sea212 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

zrml/prediction-markets/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +1155 to +1158
) -> Result<[usize; 3], DispatchError> {
let mut total_accounts: usize = 0;
let mut total_asset_accounts: usize = 0;
let mut total_categories: usize = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just out of interest, I have always wondered what will happen if the runtime is executed natively on two machines, one uses a usize of 32 bit and one a usize of 64 bit. Now let's assume the numbers stored within that usize variable get greater than 32 bit. Will these two machines come to a different conclusion at this point and therefore fork, since one will overflow and the other won't? Will the wasm variant be run in case a node recognizes it obtains different results?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hum... Not sure what Aura or Babe would do in this situation, I am not sure even if it is something related to block construction or block finalization.
Taking a network of native runtimes as an example, maybe... If an arithmetic operation panics, then the node wouldn't simply participate but since arithmetic operations currently saturate, then I guess the majority of the x86 or x86_64 nodes would apply the canonical block... IDK...
IIRC, the Substrate codebase abuses this usize == 32 bits feature casting usize as u32 in many places but I wonder about a possible wasm64 target in the future.
Luckily, the Parity guys are planning to remove native runtimes -> paritytech/substrate#7288

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for those thought-provoking viewpoints. Didn't knew that a discussion about removing the native runtime exists, a very interesting read!

Block production doesn't actually benefit from the native runtime either since wasm is the canonical runtime and validators use that.

So every block is constructed using the wasm32 runtime currently, at least the validators should always as one unit either accept or reject a block (if everyone follows the same consensus rules). I think I'll try out a usize overflow on x86_64 and look what happens when I have some spare time. Maybe the casting of usize to 32 bits, as you found in many places in the Substrate codebase, will prevent different consensus results in this regards.
It's fun to think about this stuff but I assume in the current state of Zeitgeist no usize value will be that big, certainly the ones marked for this discussion won't overflow on 32bit architecture.

Copy link
Contributor Author

@c410-f3r c410-f3r Aug 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although Substrate will probably never run on an 8 bit micro-controller, the possible effects of indiscriminate casts might appear in the future as silent bugs or as a blocker with an hypothetical wasm64 target.
So this subject should be tackled with more care. I personally, for example, prefer to work exclusively with usize unless a strong reason tells otherwise.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, found your discussion by the backlink in the OG issue. Just wanted to chime in and give my perspective on wasm32 vs. wasm64.

There are several things in play.

We generally avoid using usize. This is not exclusively related to Substrate Runtime. For example, parity-scale-codec does not provide an impl for usize just because it is so easy to get similar problems e.g. on arm (32bit) vs aarch64 vs x86 vs x86_64.

But yes, Wasm Runtime assumes a 32 bit word. wasm64 is on our radar as well and so far it seems we are not going to leverage it. This is because wasm64 only affects memory addressing. It is also doesn't make you use one or another and it is theoretically possible to mix 32 and 64 memories within a single instance. Basically moving from wasm32 to wasm64 will allow us to use a wider pointer. This allows to address more memory but it also leads to more memory consumption, which in turn hurts performance through less optimal CPU cache usage. At the same time, I do not think we want or even can afford to use 4 GiB per wasm instance, and I do not see us relying on virtual memory for tricks, and I do not think tagged pointers or all other stuff gives us much.

Bottom line, I think we can safely assume that Wasm Runtime will stay on wasm32. Sorry if this is too off topic for this place feel free to move this dicussion elsewhere.

Copy link
Contributor Author

@c410-f3r c410-f3r Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @apopiak

It is really nice to see someone from Parity caring about this subject, the more the Polkadot ecosystem unites to help each other, the more successful it will be. Maybe the same attention can not be applied to my Substrate PRs or my unanswered Element questions but that is another history :)

Thanks for asserting that usize will probably always be u32, this brings more confidence to our pallet development and please, feel free to chime in any time! Your comments and expertise are of great value!

@lsaether lsaether merged commit 74c7c7c into main Aug 19, 2021
@c410-f3r c410-f3r deleted the caio-c6 branch August 23, 2021 17:56
@sea212 sea212 added s:accepted This pull request is ready for merge and removed s:review-needed The pull request requires reviews labels Aug 24, 2021
c410-f3r added a commit to c410-f3r/zeitgeist that referenced this pull request Sep 14, 2021
* Sixth iteration of the Court pallet

* Address comment

* Rustfmt
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
s:accepted This pull request is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants