[sui-verifier] Added support for characteristic types #3563

awelc · 2022-07-28T02:07:02Z

This PR is the first step towards implementing so called one-time-witness in the form of so called characteristic type. The idea is to guarantee that a given witness type can be used only once.

An example use is to guarantee that there can be only one instance of a treasure capability for a given coin (a "permission-less coin)- treasure capability uses a witness type to create a new coin.

This is realized by having a very specific type (optionally) defined in a module, and guarantee that this type can be instantiated only once and the instance cannot be stored or copied. This type must fulfill the following properties which are checked in a verifier pass implemented in this PR:

it has only one ability: drop
it has only one arbitrarily named field of type boolean (since Move structs cannot be empty)
its definition does not involve type parameters
its only instance in existence is passed as an argument to the module initializer
it is never instantiated anywhere in its defining module

A followup to this PR will be another one that implements a native function checking if a given type is a characteristic type and providing an example of how all this can be used to implement a permission-less coin.

tnowacki

Overall looks great!

Mostly just layout/naming nits :) (sorry)

We should have one other test where you do something like

module a::m {
  struct N has drop {}
}
module a::n {
  fun init(_witness: N, ctx: &mut TxContext) { ... }
}

Where you are using the upper case, but from a different module

tnowacki · 2022-07-29T02:17:13Z

crates/sui-verifier/src/char_type_verifier.rs

+// Copyright (c) 2022, Mysten Labs, Inc.
+// SPDX-License-Identifier: Apache-2.0
+
+//! A module can define a "characteristic type", that is a type that is instantiated only once, and


Bit of a naming nit, I think we are going with "one-time-witness type" for anything user facing. As it sort of describes what it is supposed to be used for.

So a general s/characteristic/one-time-witness/

With or without the hyphens, doesn't matter too much

And open to discussion here :) I just think calling this thing a one-time witness everywhere might help people understand what is going on and why they want to use it (assuming they understand witnesses)

To me a characteristic type was a way to implement one-time witness. For now I have updated the comment to reflect that but I also open for a discussion :-)

I would agree with that description, but I think it is the only way to implement a one-time witness. At least the only safe way.
And I've been pushing more for naming in the Sui Framework for things to be named what they are, not how they work. Really, the name should reflect the most important and distinguishing part of the thing.

That being said, I could see one-time witness being confusing as there are lots of ways to implement one-time witness, though all those other ways would be not as safe.

Thoughts given this perspective?

As you say (and as the comment now say), there are different ways to implement one-time-witness so I think I'd stick to what we have now in the verifier but use one-time-witness in the native function name. We can always change it later if people find this confusing.

Fine with landing the naming as is. I will be curious what @sblackshear thinks, as he came up with the initial name

crates/sui-verifier/src/char_type_verifier.rs

tnowacki · 2022-07-29T02:19:21Z

crates/sui-verifier/src/char_type_verifier.rs

+    let mut char_type_candidate = None;
+    for def in struct_defs {
+        let struct_handle = module.struct_handle_at(def.struct_handle);
+        let struct_name = view.identifier_at(struct_handle.name).as_str();
+        if struct_name.to_ascii_uppercase() == struct_name
+            && mod_name.to_ascii_uppercase() == struct_name
+        {
+            verify_char_type(module, struct_name, struct_handle, def)
+                .map_err(verification_failure)?;
+            // if we reached this point, it means we have a legitimate characteristic type candidate
+            // and we have to make sure that both the init function's signature reflects this and
+            // that this type is not instantiated in any function of the module
+            char_type_candidate = Some((struct_name, struct_handle, def));
+            break; // no reason to look any further
+        }
+    }


All this would be a bit cleaner with struct_defs.iter().find

Possibly, but then we'd have to deal with a return value being a vector, explain why it would only have one or zero elements, etc. Overall, I am not sure it's a huge win, but happy to do the rewrite if requested one more time :-)

find gives the first element that satisfies the predicate, so what you have here is equivalent to find, no vectors or anything

Good point! I was thinking about find_map (there are so many of these little utility functions...). While attempting the rewrite, I realized that (I think...) there is no easy way to propagate error from verify_char_type if it's used inside of `find's closure.

I didn't realize the Result in here. Really I think you need try_find, which is still in nightly

crates/sui-verifier/src/entry_points_verifier.rs

crates/sui-verifier/src/char_type_verifier.rs

tnowacki · 2022-07-29T02:26:58Z

crates/sui-verifier/src/char_type_verifier.rs

+    // unwrap below is safe as it will always be successful if declared_field_count call above is
+    // successful
+    if field_count != 1
+        || struct_def.field(0).unwrap().signature != TypeSignature(SignatureToken::Bool)


signature.0 to avoid the TypeSignature(...) wrapper

You explicitly tried to avoid it as if I remember correctly you don't love .0 syntax :-) Changed now!

I'm here and there, maybe this is going back to my love for my little sp!() macro in the source language? I do tend to dislike field access instead of pattern matching

crates/sui-verifier/src/char_type_verifier.rs

sui_programmability/examples/fungible_tokens/sources/regulated_coin.move

tnowacki · 2022-07-29T02:50:07Z

One other thing, and we don't have to decide on it in this PR, but should we optionally allow the witness type to have store? I know @damirka wanted it.

And do we want the signature of is_one_time_witness to be:
<T>(_witness: T): bool
or
<T>(_witness: &T): bool

The former forces you to use it correctly, but the second gives you a tiny bit more leeway. However we cannot do the by-ref version with store so our options are:

A) optional store and <T>(_witness: T): bool
B) no store and <T>(_witness: T): bool
C) no store and <T>(_witness: &T): bool

awelc · 2022-07-29T20:22:16Z

One other thing, and we don't have to decide on it in this PR, but should we optionally allow the witness type to have store? I know @damirka wanted it.

And do we want the signature of is_one_time_witness to be: <T>(_witness: T): bool or <T>(_witness: &T): bool

The former forces you to use it correctly, but the second gives you a tiny bit more leeway. However we cannot do the by-ref version with store so our options are:

A) optional store and <T>(_witness: T): bool B) no store and <T>(_witness: T): bool C) no store and <T>(_witness: &T): bool

Agreed! I went with the most conservative option for now to avoid this discussion before this PR lands. Will likely do the same when implementing the native method as it's easy to change (though we should decide soon!).

tnowacki · 2022-08-01T17:36:00Z

crates/sui-verifier/src/char_type_verifier.rs

+    struct_name: &str,
+    struct_def: &StructDefinition,


tnowacki · 2022-08-01T17:36:21Z

crates/sui-verifier/src/char_type_verifier.rs

+fn is_char_type(
+    view: &BinaryIndexedView,
+    tok: &SignatureToken,
+    struct_handle: &StructHandle,


tnowacki · 2022-08-01T17:36:27Z

crates/sui-verifier/src/char_type_verifier.rs

+    struct_name: &str,
+    struct_handle: &StructHandle,


tnowacki · 2022-08-01T17:36:39Z

crates/sui-verifier/src/char_type_verifier.rs

+    struct_name: &str,
+    struct_handle: &StructHandle,
+    struct_def: &StructDefinition,


tnowacki · 2022-08-01T17:38:30Z

crates/sui-verifier/src/char_type_verifier.rs

+    if let SignatureToken::Struct(idx) = tok {
+        if view.struct_handle_at(*idx) == struct_handle {


if you want to be fancy, `matches(tok, SignatureToken::Struct(idx) if view.struct_handle_at(*idx) == struct_handle)

It is fancy indeed!

tnowacki · 2022-08-01T17:40:14Z

crates/sui-verifier/src/char_type_verifier.rs

+            let fn_handle = module.function_handle_at(fn_def.function);
+            let fn_name = module.identifier_at(fn_handle.name);
+            return Err(format!(
+                        "characteristic type {}::{} is instantiated in the {}::{} function and must never be",


is this what rustfmt gave? Is the string maybe too long to not format this?

tnowacki · 2022-08-01T17:41:07Z

crates/sui-verifier/src/char_type_verifier.rs

+        let fn_handle = module.function_handle_at(fn_def.function);
+        let fn_name = module.identifier_at(fn_handle.name);
+        if fn_name == INIT_FN_NAME {
+            if let Some((candidate_name, candidate_handle, _)) = char_type_candidate {


Maybe someday we should just move the init verification over here as it is a bit strange to have it split. But probably fine for now

tnowacki

Any other naming discussions can happen after this lands. Thanks for the changes!

awelc requested review from tnowacki and damirka July 28, 2022 02:07

awelc added 4 commits July 28, 2022 13:18

[sui-verifier] Added support for characteristic types

f6170c1

Fixed a problem related to another struct being defined

8350d8f

Adjusted tests and examples to work with the new verifier pass

172ed4f

Fixed another problem plus adjusted more test outputs

825a829

awelc force-pushed the aw/char-type branch from f7e254a to 825a829 Compare July 28, 2022 20:18

Another bug fix

2e5330e

awelc self-assigned this Jul 28, 2022

Yet another fix

1781875

awelc marked this pull request as ready for review July 28, 2022 22:53

awelc mentioned this pull request Jul 28, 2022

One time witness problem #3591

Open

tnowacki reviewed Jul 29, 2022

View reviewed changes

awelc added 2 commits July 29, 2022 13:23

Addressed review comments

40801bb

Addressed more review comments

cb6d4a9

tnowacki reviewed Aug 1, 2022

View reviewed changes

Addressed even more review comments

41f617a

tnowacki approved these changes Aug 1, 2022

View reviewed changes

awelc merged commit 7942def into main Aug 2, 2022

awelc deleted the aw/char-type branch August 2, 2022 00:05

awelc mentioned this pull request Aug 4, 2022

[Move] Implemented one-type witness checking #3771

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sui-verifier] Added support for characteristic types #3563

[sui-verifier] Added support for characteristic types #3563

awelc commented Jul 28, 2022

tnowacki left a comment

tnowacki Jul 29, 2022

tnowacki Jul 29, 2022

tnowacki Jul 29, 2022

awelc Jul 29, 2022

tnowacki Jul 29, 2022

awelc Jul 30, 2022

tnowacki Aug 1, 2022

tnowacki Jul 29, 2022

awelc Jul 29, 2022

tnowacki Jul 29, 2022

awelc Jul 30, 2022

tnowacki Aug 1, 2022

tnowacki Jul 29, 2022

awelc Jul 29, 2022

tnowacki Jul 29, 2022

tnowacki commented Jul 29, 2022

awelc commented Jul 29, 2022

tnowacki Aug 1, 2022

tnowacki Aug 1, 2022

tnowacki Aug 1, 2022

tnowacki Aug 1, 2022

tnowacki Aug 1, 2022

awelc Aug 1, 2022

tnowacki Aug 1, 2022

tnowacki Aug 1, 2022

tnowacki left a comment

		if let SignatureToken::Struct(idx) = tok {
		if view.struct_handle_at(*idx) == struct_handle {

[sui-verifier] Added support for characteristic types #3563

[sui-verifier] Added support for characteristic types #3563

Conversation

awelc commented Jul 28, 2022

tnowacki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnowacki commented Jul 29, 2022

awelc commented Jul 29, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tnowacki left a comment

Choose a reason for hiding this comment