add new witness scope #2596

Ashuaidehao · 2021-08-31T03:26:05Z

Ashuaidehao · 2021-08-31T03:52:11Z

Add 2 new sign scope to make CheckWitness more safely by checking CallingScriptHash.

CustomCallingContracts set contract with trusted caller contract list. Only Trust Caller or entry call can pass "CheckWitness". For example:

{
	TokenA:[Caller1,Caller2,TokenB,...],
	TokenB:[Caller1,Caller3,...],
	...
}

TokenA,TokenB would call CheckWitness.
Entry=>TokenA, Entry=>Caller1=>TokenA are ok.
Entry=>Caller3=>TokenA will fail.

CustomCallingGroups set contract with trusted caller's group list. Only Trust Group Caller or entry call can pass "CheckWitness".

{
	TokenA:[Group1,Group2,...],
	TokenB:[Group1,Group3,...],
	...
}

src/neo/Network/P2P/Payloads/Signer.cs

roman-khimov

For example:
[
	{TokenA:[Caller1,Caller2,TokenB,...]},
	{TokenB:[Caller1,Caller3,...]},
	...
]
TokenA,TokenB would call CheckWitness.
Entry=>TokenA, Entry=>Caller1=>TokenA are ok.
Entry=>Caller3=>TokenA will fail.

Looks interesting, but it made me wonder about Entry -> Caller1 -> TokenA -> BAD -> Caller1 -> TokenA case. This can happen if for some reason Caller1 needs to move TokenA for user but it makes a move to BAD contract address which is invoked via onNEP17Payment and then does its nasty things. But after some thinking I came to conclusion that in case of CustomCallingContracts this probably is not a problem, after all Caller1 will still do CheckWitness too (or is there any legit case it won't do that?) and it won't work for the second invocation (because properly signed transaction will have CalledByEntry, CustomCallingContracts(TokenA:Calle1) scope which will fail for non-entry invocation of Caller1).

Though we can also imagine Entry -> Caller1 -> Caller2 -> TokenA -> BAD -> Caller2 -> TokenA, here CustomCallingContracts won't help. Maybe it's somewhat of a stretched example, but still, CountedContracts solves this (at least until the count is one). We may also consider invocation stack position instead of counting, like Caller2 2; TokenA 3 witness which will break for Caller2 5 and TokenA 6.

But it's even more complex for groups. Because we actually want to give a signature scoped for any in-group invocations, but stopping right after execution path leaving this group. NeoFS side chain contracts are a perfect example of this, there is a number of them and we trust all of them, so whatever they do internally to do the job calling each other is absolutely irrelevant, we want CheckWitness to work anywhere in Entry -> NeoFS1 -> NeoFS2 -> NeoFS3 -> NeoFS4, but the moment we have Entry -> NeoFS1 -> non-NeoFS on our call stack the witness should be revoked and I don't think CustomCallingGroups allow to express that.

src/neo/Network/P2P/Payloads/Signer.cs

Ashuaidehao · 2021-09-01T03:32:33Z

For Entry=>Caller1=>Caller2=>..=>CallerN=>TokenA, each of Caller1~CallerN should call CheckWitness to make CustomCallingContracts work. If CallerN won't, it will be the weakness in our trust chain.
CustomCallingGroups is just CustomCallingContracts's group implementation now.
If the group scope is:

{
    NeoFS1:[FSGroup],
    NeoFS2:[FSGroup],
    NeoFS3:[FSGroup],
    NeoFS4:[FSGroup],
}

Entry -> NeoFS1 -> NeoFS2 -> NeoFS3 -> NeoFS4 would work, but Entry -> NeoFS1 -> non-NeoFS won't because non-NeoFS doesn't appear in scope Keys.

roman-khimov · 2021-09-01T06:03:17Z

If the group scope is:

{
NeoFS1:[FSGroup],
NeoFS2:[FSGroup],
NeoFS3:[FSGroup],
NeoFS4:[FSGroup],
}

I thought groups are there to simplify things for us and if I need to specify every group contract in the scope then what's use of a group?

Ashuaidehao · 2021-09-01T06:50:33Z

If the group scope is:
{
NeoFS1:[FSGroup],
NeoFS2:[FSGroup],
NeoFS3:[FSGroup],
NeoFS4:[FSGroup],
}

I thought groups are there to simplify things for us and if I need to specify every group contract in the scope then what's use of a group?

Yes, I agree. But I can hardly find a way to make simplify and security at the same time now. I'm trying to find a way let you set "ECPoint" or "UInt160" as a key, maybe use two dictionaries?

roman-khimov · 2021-09-01T07:32:18Z

But I can hardly find a way to make simplify and security at the same time now

CountedGroups solve this.

…to add-new-scope # Conflicts: # src/neo/Network/P2P/Payloads/Signer.cs # tests/neo.UnitTests/Network/P2P/Payloads/UT_Signers.cs # tests/neo.UnitTests/SmartContract/UT_ApplicationEngine.Runtime.cs

Ashuaidehao · 2021-09-01T10:18:56Z

I have refacted the CustomCallingGroups implementation, it can support these input format now(Group Public Key should be converted to AddressHash):

{
    Contract:[Contract, Group],
    Group:[Contract, Group],
}

For Entry -> NeoFS1 -> NeoFS2 -> NeoFS3 -> NeoFS4, just use { FSGroup:[] }.

src/neo/SmartContract/Helper.cs

roman-khimov · 2021-09-01T13:56:07Z

Whoa. Now that's not trivial to explain to people albeit it probably works. Makes me wonder do we need CustomCallingContracts though, looks like CustomCallingGroups covers it completely as well, so it can probably be renamed to CustomCallingEntities or something like that.

Some obligatory nitpicking:

we're usually identifying groups by keys, standard script hash may be a bit unexpected here
related to previous, calling Contract.CreateSignatureRedeemScript in CheckWitness seems to be somewhat expensive

Maybe we can solve it by allowing both keys and script hashes to be specified (and switching mode accordingly)?

Ashuaidehao · 2021-09-02T05:52:51Z

Whoa. Now that's not trivial to explain to people albeit it probably works. Makes me wonder do we need CustomCallingContracts though, looks like CustomCallingGroups covers it completely as well, so it can probably be renamed to CustomCallingEntities or something like that.

Some obligatory nitpicking:

we're usually identifying groups by keys, standard script hash may be a bit unexpected here

related to previous, calling Contract.CreateSignatureRedeemScript in CheckWitness seems to be somewhat expensive

Maybe we can solve it by allowing both keys and script hashes to be specified (and switching mode accordingly)?

Do you have more details about switch mode?

roman-khimov · 2021-09-02T06:28:31Z

I mean we technically can have a map[key_or_hash][keys_or_hashes] or Dictionary<uint160_or_key, uint160_or_key[]>, keys and hashes have different length so it's easy to separate them during decoding phase (if they're VarBytes-encoded). Then you can check for group/hash according to what's inside.

Anyway, CustomCallingContracts becomes a little obsolete with this new CustomCallingGroups.

src/neo/Network/P2P/Payloads/Signer.cs

src/neo/IO/Helper.cs

Ashuaidehao · 2021-09-02T06:56:29Z

phase

Do you mean use byte[] instead of UInt160 and ECPoint(Dictionary<byte[],byte[][]>)? Or add a new custom type combined UInt160 and ECPoint?

shargon · 2021-09-02T08:36:30Z

src/neo/IO/Helper.cs

+        /// <summary>
+        /// Reads an <see cref="ISerializable"/> Dictionary(TKey,TValue[]) from a <see cref="BinaryReader"/>.
+        /// </summary>
+        /// <typeparam name="TKey"></typeparam>


Fill the descriptions please

shargon · 2021-09-02T08:37:18Z

src/neo/Network/P2P/Payloads/Signer.cs

@@ -35,17 +38,22 @@ public class Signer : ISerializable
        /// </summary>
        public ECPoint[] AllowedGroups;

+        public IDictionary<UInt160, UInt160[]> AllowedCallingContracts;


AllowingCall and add a boolean for IsGroup?

roman-khimov · 2021-09-02T09:46:12Z

phase

Do you mean use byte[] instead of UInt160 and ECPoint(Dictionary<byte[],byte[][]>)? Or add a new custom type combined UInt160 and ECPoint?

I guess any of the two works, it's just an implementation detail. My worries are mostly about the protocol, we either allow specifying hash or public key in items or only use hashes and calculate them on the fly for keys. And then we'll need to explain all of that to developers. I'm not sure I've seen groups being identified by script hashes anywhere, so I guess developers would prefer using keys for group identification and if we're to allow using them in the protocol it'd make things somewhat simpler for people trying to construct these transactions.

…to add-new-scope

src/neo/Network/P2P/Payloads/ContractOrGroup.cs

Jim8y · 2021-10-18T14:44:53Z

src/neo/Network/P2P/Payloads/ContractOrGroup.cs

+        public override int GetHashCode()
+        {
+            if (Data == null) return 0;
+            if (Data.Length >= 4) return BitConverter.ToInt32(Data);


Suggested change

if (Data.Length >= 4) return BitConverter.ToInt32(Data);

if (Data.Length >= 4) return BinaryPrimitives.ReadInt32LittleEndian(Data);

Jim8y · 2021-10-18T14:58:55Z

src/neo/Network/P2P/Payloads/ContractOrGroup.cs

+        public static implicit operator ContractOrGroup(UInt160 hash)
+        {
+            return new ContractOrGroup() { _data = hash.ToArray() };
+        }
+
+        public static implicit operator ContractOrGroup(ECPoint point)
+        {
+            return new ContractOrGroup() { _data = point.EncodePoint(true) };
+        }


Suggested change

public static implicit operator ContractOrGroup(UInt160 hash)

{

return new ContractOrGroup() { _data = hash.ToArray() };

}

public static implicit operator ContractOrGroup(ECPoint point)

{

return new ContractOrGroup() { _data = point.EncodePoint(true) };

}

public static implicit operator ContractOrGroup(UInt160 hash)

{

if (hash is null) throw new ArgumentNullException();

return new ContractOrGroup() { _data = hash.ToArray() };

}

public static implicit operator ContractOrGroup(ECPoint point)

{

if (point is null) throw new ArgumentNullException();

return new ContractOrGroup() { _data = point.EncodePoint(true) };

}

Jim8y · 2021-10-18T15:06:09Z

src/neo/Network/P2P/Payloads/ContractOrGroup.cs

+            if (ReferenceEquals(obj, this)) return true;
+            if (obj is ContractOrGroup other)
+            {
+                if (Data == null || other.Data == null) return Data == other.Data;


Suggested change

if (Data == null || other.Data == null) return Data == other.Data;

if (Data is null || other is null || other.Data is null)

return (Data is null && (other is null || other.Data is null));

Jim8y · 2021-10-18T15:12:16Z

src/neo/Network/P2P/Payloads/Signer.cs

+            if (Scopes.HasFlag(WitnessScope.CustomCallingGroups))
+                json["allowedcallinggroups"] = AllowedCallingGroup.Select(p =>
+                {
+                    var obj = new JObject();
+                    if (p.Key.Data?.Length == 20)
+                    {
+                        obj["contract"] = p.Key.ToHashString();
+                    }
+                    else
+                    {
+                        obj["group"] = p.Key.ToString();
+                    }
+                    obj["trusts"] = p.Value.Select(v => (JObject)v.ToHashString()).ToArray();
+                    return obj;
+                }).ToArray();


Suggested change

if (Scopes.HasFlag(WitnessScope.CustomCallingGroups))

json["allowedcallinggroups"] = AllowedCallingGroup.Select(p =>

{

var obj = new JObject();

if (p.Key.Data?.Length == 20)

{

obj["contract"] = p.Key.ToHashString();

}

else

{

obj["group"] = p.Key.ToString();

}

obj["trusts"] = p.Value.Select(v => (JObject)v.ToHashString()).ToArray();

return obj;

}).ToArray();

if (Scopes.HasFlag(WitnessScope.CustomCallingGroups))

{

json["allowedcallinggroups"] = AllowedCallingGroup.Select(p =>

{

var obj = new JObject();

if (p.Key.Data?.Length == 20)

{

obj["contract"] = p.Key.ToHashString();

}

else

{

obj["group"] = p.Key.ToString();

}

obj["trusts"] = p.Value.Select(v => (JObject)v.ToHashString()).ToArray();

return obj;

}).ToArray();

}

Jim8y · 2021-10-18T15:13:37Z

src/neo/Network/P2P/Payloads/Signer.cs

+            if (Scopes.HasFlag(WitnessScope.CustomCallingContracts))
+                json["allowedcallingcontracts"] = AllowedCallingContracts.Select(p =>
+                {
+                    var obj = new JObject();
+                    obj["contract"] = p.Key.ToString();
+                    obj["trusts"] = p.Value.Select(v => (JObject)v.ToString()).ToArray();
+                    return obj;
+                }).ToArray();


Suggested change

if (Scopes.HasFlag(WitnessScope.CustomCallingContracts))

json["allowedcallingcontracts"] = AllowedCallingContracts.Select(p =>

{

var obj = new JObject();

obj["contract"] = p.Key.ToString();

obj["trusts"] = p.Value.Select(v => (JObject)v.ToString()).ToArray();

return obj;

}).ToArray();

if (Scopes.HasFlag(WitnessScope.CustomCallingContracts))

{

json["allowedcallingcontracts"] = AllowedCallingContracts.Select(p =>

{

var obj = new JObject();

obj["contract"] = p.Key.ToString();

obj["trusts"] = p.Value.Select(v => (JObject)v.ToString()).ToArray();

return obj;

}).ToArray();

}

roman-khimov · 2021-10-19T16:22:38Z

src/neo/Network/P2P/Payloads/WitnessScope.cs

+        /// <summary>
+        /// Custom calling contract hash for contract-specific.
+        /// </summary>
+        CustomCallingContracts = 0x02,


Do we need this type now? It seems to me that current CustomCallingGroups allows to express the same setting easily.

I'm not sure it should be removed. In [contract]=>[contract] case, it will have more perfromance than CustomCallingGroups. In my opinion, this is the first choice in most cases.

I think we should keep it and change the key of AllowedCallingGroup to ECPoint.

Should I change AllowedCallingContracts to [contract]=>[contract or ecpoint], and AllowedCallingGroup to [ecpoint]=>[contract or ecpoint]?

Yes. I think it is better.

src/neo/IO/Helper.cs

shargon · 2021-11-05T19:19:43Z

src/neo/IO/Helper.cs

+            for (uint i = 0; i < count; i++)
+            {
+                var key = reader.ReadSerializable<TKey>();
+                var value = reader.ReadSerializableArray<TValue>(max);


max is used for different things, in fact 16 here is too small, isn't it?

shargon · 2021-11-05T19:27:59Z

src/neo/IO/Helper.cs

+        /// <typeparam name="TKey"></typeparam>
+        /// <typeparam name="TValue"></typeparam>
+        /// <param name="dict"></param>
+        /// <returns></returns>


Fill descriptions please

shargon · 2021-11-05T19:29:46Z

src/neo/Network/P2P/Payloads/ContractOrGroup.cs

+
+        public string ToHashString()
+        {
+            if (Data == null) return null;


Better to use _data inside the class?

erikzhang · 2021-11-07T10:48:52Z

@Ashuaidehao @shargon @Liaojinghui @roman-khimov @superboyiii Please check #2622

…to add-new-scope # Conflicts: # src/neo/Network/P2P/Payloads/ContractOrGroup.cs

init

5015c4f

Ashuaidehao force-pushed the add-new-scope branch from 8dd7a2c to 5015c4f Compare August 31, 2021 03:28

erikzhang reviewed Aug 31, 2021

View reviewed changes

src/neo/Network/P2P/Payloads/Signer.cs Outdated Show resolved Hide resolved

roman-khimov reviewed Aug 31, 2021

View reviewed changes

src/neo/Network/P2P/Payloads/Signer.cs Outdated Show resolved Hide resolved

src/neo/Network/P2P/Payloads/Signer.cs Outdated Show resolved Hide resolved

fix

cffb26d

fx ut

d4471f4

shargon and others added 5 commits September 1, 2021 10:06

Merge branch 'master' into add-new-scope

803aa30

Optimize and fixes

937dcaf

Clean empty lines

84f735c

refac group

a337349

Merge branch 'add-new-scope' of https://github.com/Ashuaidehao/neo in…

35e6124

…to add-new-scope # Conflicts: # src/neo/Network/P2P/Payloads/Signer.cs # tests/neo.UnitTests/Network/P2P/Payloads/UT_Signers.cs # tests/neo.UnitTests/SmartContract/UT_ApplicationEngine.Runtime.cs

shargon reviewed Sep 1, 2021

View reviewed changes

src/neo/SmartContract/Helper.cs Outdated Show resolved Hide resolved

update

1b26b88

roman-khimov reviewed Sep 2, 2021

View reviewed changes

src/neo/Network/P2P/Payloads/Signer.cs Outdated Show resolved Hide resolved

src/neo/IO/Helper.cs Outdated Show resolved Hide resolved

Ashuaidehao and others added 2 commits September 2, 2021 15:01

add limit

9b8ed0f

Update Helper.cs

5053df7

shargon reviewed Sep 2, 2021

View reviewed changes

Ashuaidehao added 2 commits September 3, 2021 17:26

refac group

314073a

Merge branch 'add-new-scope' of https://github.com/Ashuaidehao/neo in…

6a2529a

…to add-new-scope

Ashuaidehao and others added 2 commits September 3, 2021 17:51

format

a81cd1e

Merge branch 'master' into add-new-scope

f10c99f

superboyiii mentioned this pull request Sep 9, 2021

Neo v3.0.3 Checklist #2601

Closed

9 tasks

csmuller mentioned this pull request Sep 13, 2021

Update WitnessScopes neow3j/neow3j#640

Closed

Merge branch 'master' into add-new-scope

2e1846b

erikzhang reviewed Oct 17, 2021

View reviewed changes

src/neo/Network/P2P/Payloads/ContractOrGroup.cs Show resolved Hide resolved

add copyright

f6f7fed

Jim8y reviewed Oct 18, 2021

View reviewed changes

update

03e2781

roman-khimov reviewed Oct 19, 2021

View reviewed changes

Ashuaidehao added 3 commits November 2, 2021 11:15

ecpoint

c8252c6

format

21f55d8

call contracts

a45ea67

superboyiii mentioned this pull request Nov 3, 2021

Neo v3.1.0 Checklist #2620

Closed

13 tasks

shargon added 2 commits November 3, 2021 15:12

Merge branch 'master' into add-new-scope

267ddd4

Clean UT

57d9d3f

shargon reviewed Nov 5, 2021

View reviewed changes

Some fixes

99f824e

shargon reviewed Nov 5, 2021

View reviewed changes

erikzhang mentioned this pull request Nov 7, 2021

Add new WitnessScope: Rules #2622

Merged

Ashuaidehao added 2 commits November 8, 2021 17:32

rafac

cfb8b2b

Merge branch 'add-new-scope' of https://github.com/Ashuaidehao/neo in…

e9cbeeb

…to add-new-scope # Conflicts: # src/neo/Network/P2P/Payloads/ContractOrGroup.cs

erikzhang closed this Nov 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add new witness scope #2596

add new witness scope #2596

Ashuaidehao commented Aug 31, 2021

Ashuaidehao commented Aug 31, 2021 •

edited

Loading

roman-khimov left a comment

Ashuaidehao commented Sep 1, 2021

roman-khimov commented Sep 1, 2021

Ashuaidehao commented Sep 1, 2021

roman-khimov commented Sep 1, 2021

Ashuaidehao commented Sep 1, 2021 •

edited

Loading

roman-khimov commented Sep 1, 2021 •

edited

Loading

Ashuaidehao commented Sep 2, 2021

roman-khimov commented Sep 2, 2021

Ashuaidehao commented Sep 2, 2021

shargon Sep 2, 2021

shargon Sep 2, 2021

roman-khimov commented Sep 2, 2021

Jim8y Oct 18, 2021

Jim8y Oct 18, 2021

Jim8y Oct 18, 2021

Jim8y Oct 18, 2021 •

edited

Loading

Jim8y Oct 18, 2021

roman-khimov Oct 19, 2021

Ashuaidehao Oct 20, 2021

erikzhang Oct 20, 2021

Ashuaidehao Nov 1, 2021 •

edited

Loading

erikzhang Nov 2, 2021

shargon Nov 5, 2021

shargon Nov 5, 2021

shargon Nov 5, 2021

erikzhang commented Nov 7, 2021

	if (Data.Length >= 4) return BitConverter.ToInt32(Data);
	if (Data.Length >= 4) return BinaryPrimitives.ReadInt32LittleEndian(Data);

	if (Data == null \|\| other.Data == null) return Data == other.Data;
	if (Data is null \|\| other is null \|\| other.Data is null)
	return (Data is null && (other is null \|\| other.Data is null));

add new witness scope #2596

add new witness scope #2596

Conversation

Ashuaidehao commented Aug 31, 2021

Ashuaidehao commented Aug 31, 2021 • edited Loading

roman-khimov left a comment

Choose a reason for hiding this comment

Ashuaidehao commented Sep 1, 2021

roman-khimov commented Sep 1, 2021

Ashuaidehao commented Sep 1, 2021

roman-khimov commented Sep 1, 2021

Ashuaidehao commented Sep 1, 2021 • edited Loading

roman-khimov commented Sep 1, 2021 • edited Loading

Ashuaidehao commented Sep 2, 2021

roman-khimov commented Sep 2, 2021

Ashuaidehao commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roman-khimov commented Sep 2, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jim8y Oct 18, 2021 • edited Loading

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

Ashuaidehao Nov 1, 2021 • edited Loading

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

erikzhang commented Nov 7, 2021

Ashuaidehao commented Aug 31, 2021 •

edited

Loading

Ashuaidehao commented Sep 1, 2021 •

edited

Loading

roman-khimov commented Sep 1, 2021 •

edited

Loading

Jim8y Oct 18, 2021 •

edited

Loading

Ashuaidehao Nov 1, 2021 •

edited

Loading