[Infra] System Contract Dictionary by quantumagi · Pull Request #490 · stratisproject/StratisFullNode

quantumagi · 2021-03-30T05:54:50Z

Note that this approach, relying on signatures, does not require additional external permission management.

Example Authorization:

- Anyone calls the "WhiteList" method on the "Dictionary" contract. The method responds with an authorization challenge:
  - E.g. "WhiteList(Nonce:0,CodeHash:123,LastAddress:,Name:)"
- Each signatory calls the legacy "SignMessage" API to obtain a signature for the challenge.
- The submitter calls the "WhiteList" method again with all the signatures.
- The contract is now successfully white-listed and can now be deployed.
- Following contract deployment a contract address will be available. 
- This process is repeated to provide the contract address and a friendly name (e.g. "Multisig").
- Now all nodes that rely on the information from the "Multisig" contract will be able to locate the new contract.

Note: The contract hash of the "System Contract Dictionary" itself will be hard-coded (as being white-listed) in the code base. Initially only the first deployment will be used to determine the contract address unless it is self-listed with a different address in which case we will "follow the chain" to find the most recent version.

Merge PR #509 first.

noescape00 · 2021-03-31T17:44:26Z

I prefer this one over 489
Please let me know when it's ready for full review

quantumagi · 2021-04-08T08:28:16Z

@noescape00 , I've added some more functionality if you would like to have a look now.

quantumagi · 2021-04-10T09:00:46Z

Sign in to view

+        for (int i = 0; i < signatories.Length; i++)
+            Assert(signatories[i] != address, "The signatory already exists.");
+
+        Assert((signatories.Length + 1) == newSize, "The expected size is incorrect.");


Equality check should not be negated in here?

It is correct afaik. The error is displayed when the assertion fails.

YakupIpek

I added my feedbacks and i did not leave same feedbacks on duplicated issues. So i will review again after you submited changes.

I suggest you to review and follow style of DAO contract
https://github.com/stratisproject/CirrusSmartContracts/blob/master/Testnet/DAOContract/DAOContract/DAOContract.cs

YakupIpek · 2021-04-15T11:25:27Z

@@ -0,0 +1,205 @@
+using Stratis.SmartContracts;
+using ECRecover=Stratis.SCL.Crypto.ECRecover;


Can you import just namespace in here ?

Is there a reason?

These type of using statements for fixing conflicts when one type is exist in more than 2 namespace so this is not a case in here.

Hmm... It would protect against any conflicts that may arise in the future?

Seems fine.

YakupIpek · 2021-04-15T11:26:48Z

+
+    public SystemContractsDictionary(ISmartContractState state) : base(state)
+    {
+        this.State.SetArray($"Signatories:{primaryGroup}", new[] { new Address(0, 0, 0, 0, 0), new Address(0, 0, 0, 0, 1), new Address(0, 0, 0, 0, 2) });


These value should be hard coded ?

These "default" values should be updated at some stage with the correct default values. The AddSignatory and RemoveSignatory will allow the defaults to be changed. I am worried about passing the defaults to the constructor as maybe this will lead to reduced or single person security and maybe make a mockery of the multiple person security this wants to offer.

State should be access by getters/setters in everywhere

YakupIpek · 2021-04-15T11:36:14Z

+    private void VerifySignatures(byte[] signatures, string authorizationChallenge)
+    {
+        string[] sigs = this.Serializer.ToArray<string>(signatures);
+        Assert(ECRecover.TryVerifySignatures(sigs, System.Text.Encoding.ASCII.GetBytes(authorizationChallenge), this.Signatories, out Address[] verifiedSignatures) && verifiedSignatures.Length >= this.Quorum,


Can you move namespace to top of page ?

IMO ECRecover.TryVerifySignatures should be GetVerifiedSignatories and return addresses otherwise Try prefix and out param is quite confusing to me. I suggest code like below.

var challange = Encoding.ASCII.GetBytes(authorizationChallenge); var verifieds = ECRecover.GEtVerifiedSignatures(sigs, challange, this.Signatories); Assert(verifieds.Length >= this.Quorum, $"Please provide {this.Quorum} valid signatures for '{authorizationChallenge}'.");

@rowandh , this was your suggestion, wdyt? I'm on the fence.

When TryVerifySignatures will return false ?
When there is some verified signatories ? No
When there is no any verified signatories ? Still no
These are very unexpected behavior from this method.

It only returns false if provided parameters are null. It is clear that expected behavior of the method is return verified signatories.

That is how it was and frankly I had similar thoughts as Yakup. @rowandh should I change this back?

YakupIpek · 2021-04-15T11:47:51Z

+
+    public Address[] GetSignatories(string group)
+    {
+        Assert(!string.IsNullOrEmpty(group));


This assert is not necessary so you can remove it.

It does make a difference by throwing an error versus simply returning the default. Intuitively throwing an error makes more sense but perhaps there is some reason you don't want to do that for smart contracts. @rowandh , wdyt?

You might save a small amount of gas by doing this. Seems fine.

YakupIpek · 2021-04-15T11:58:14Z

+        return this.State.GetUInt32($"Quorum:{group}");
+    }
+
+    public void AddSignatory(byte[] signatures, string group, Address address, uint newSize, uint newQuorum)


Who is going to call this method ? Owner of the contract or anyone ?

Anyone that's willing to pay the gas cost to execute it. However signatures are checked before any updates are made.

YakupIpek · 2021-04-15T12:05:20Z

+
+        Address[] signatories = this.GetSignatories(group);
+        for (int i = 0; i < signatories.Length; i++)
+            Assert(signatories[i] != address, "The signatory already exists.");


Adding, removing or checking existence of signatories cause iterative codes and resizing array costs. Alternatively we can index signatories in state like

"Signatories:{signature}" => group

After that you can quickly verify it is existence in dictionary

Another key pair alternative would be

"Signatories:{group}:{signature}" => bool(true)

@rowandh wdty?

That would shift the problem of being able to obtain a list of all signatories.

Proably "Signatories:{signature}" => group wouldn't work because same address can be added to different groups so second solution is only solution.

@YakupIpek , given your approach how would you list all the signatories of a group? I've optimized for this method since the other methods would occur far less frequently:

public Address[] GetSignatories(string group) { return this.State.GetArray<Address>($"Signatories:{group}"); }

I am not quite sure which path we have to follow right now. You can still separate index like $"Signatories:{group}:{index}" but you have to iterate it at the end if you want to obtain all of them.
Lets wait @rowandh feedback.

YakupIpek · 2021-04-15T12:06:47Z

+    }
+
+    public void RemoveSignatory(byte[] signatures, string group, Address address, uint newSize, uint newQuorum)
+    {


These methods can be called by anyone ?

Anyone that's willing to pay the gas cost to execute it. However signatures are checked before any updates are made.

It is really needed to specify newSize and newQuorum, why not to automate it ?

I really want the user to be very exact about the change being accomplished especially due to how important the quorum count is in relation to the signatory count. The user must know exactly what is being accomplished in that regards and show that knowledge by specifying the values explicitly. All the signatories will also see that information in the security challenge when signing.

Hımm but the transaction is going to fail and cost fee if anyone going to call these methods in same block or in prev blocks.

Not sure I understand what you're saying. Will mull it over.

YakupIpek · 2021-04-15T12:07:56Z

+
+    public UInt256 GetCodeHash(string name)
+    {
+        Assert(!string.IsNullOrEmpty(name));


Imo these asserts are not necessary. If someone send null,empty or none existence key then we return default value already from state.

It does make a difference by throwing an error versus simply returning the default. Intuitively throwing an error makes more sense but perhaps there is some reason you don't want to do that for smart contracts. @rowandh , wdyt?

As caller of the method, i should to verify that there is hash code or not for given name parameter in any case and i do it by checking its value is default or not. But you added assertion, in the case of it is null so i have to verify exception + default value check.

We should not optimize code for exceptional cases.

This is common way we did in other contracts ussually.

If there is insufficient checks something like "Quorum:" could obtain a value, then when "Quorum:" is accessed later a junk value could be returned.

so i have to verify exception + default value check.

You don't have to verify exception if you're not passing null values. If you are unexpectedly passing null values then it better to get an error than some junk value you may proceed processing with.

YakupIpek · 2021-04-15T12:10:53Z

+        string authorizationChallenge;
+        if (whiteListEntry.CodeHash == default(UInt256))
+        {
+            authorizationChallenge = $"{nameof(WhiteList)}(Nonce:{nonce},CodeHash:{codeHash},LastAddress:{lastAddress},Name:{name})";


Why not as below ?

authorizationChallenge = $"{nameof(WhiteList)}:{nonce}:{codeHash},{lastAddress},{name})"

It's meant to be as human readable as possible to ensure signatories can read it and know what they are signing.

YakupIpek · 2021-04-16T02:49:59Z

+    private void VerifySignatures(byte[] signatures, string authorizationChallenge)
+    {
+        string[] sigs = this.Serializer.ToArray<string>(signatures);
+        Assert(ECRecover.TryVerifySignatures(sigs, System.Text.Encoding.ASCII.GetBytes(authorizationChallenge), this.Signatories, out Address[] verifiedSignatures) && verifiedSignatures.Length >= this.Quorum,


When TryVerifySignatures will return false ?
When there is some verified signatories ? No
When there is no any verified signatories ? Still no
These are very unexpected behavior from this method.

It only returns false if provided parameters are null. It is clear that expected behavior of the method is return verified signatories.

YakupIpek · 2021-04-16T03:29:28Z

+
+        Address[] signatories = this.GetSignatories(group);
+        for (int i = 0; i < signatories.Length; i++)
+            Assert(signatories[i] != address, "The signatory already exists.");


I am not quite sure which path we have to follow right now. You can still separate index like $"Signatories:{group}:{index}" but you have to iterate it at the end if you want to obtain all of them.
Lets wait @rowandh feedback.

YakupIpek · 2021-04-16T05:16:48Z

+
+    public UInt256 GetCodeHash(string name)
+    {
+        Assert(!string.IsNullOrEmpty(name));


As caller of the method, i should to verify that there is hash code or not for given name parameter in any case and i do it by checking its value is default or not. But you added assertion, in the case of it is null so i have to verify exception + default value check.

We should not optimize code for exceptional cases.

YakupIpek · 2021-04-16T05:17:05Z

+
+    public UInt256 GetCodeHash(string name)
+    {
+        Assert(!string.IsNullOrEmpty(name));


This is common way we did in other contracts ussually.

quantumagi · 2021-04-16T08:10:15Z

#490 (comment)

noescape00 · 2021-04-16T13:07:51Z

Please add xmldoc for all methods of SystemContractsDictionary that are not pretty much self explanatory, plus general description of the class

rowandh · 2021-04-17T03:14:13Z

-        public static bool TryVerifySignatures(string[] signatures, byte[] message, Address[] addresses, out Address[] verifiedAddresses)
+        /// <param name="addresses">The addresses returned are intersected with these addresses (if not <c>null</c>).</param>
+        /// <returns>The list of addresses that produced the signatures and constrained to the list provided in <paramref name="addresses"/>.</returns>
+        public static Address[] GetVerifiedSignatures(string[] signatures, string message, Address[] addresses)


This is an API change to the SCL library. I don't think these should be made randomly in PRs like this but rather as part of a structured API design for the initial SCL release.

quantumagi · 2021-04-29T07:37:44Z

Moved to other PRs.

Candidate 2

2ffef77

quantumagi marked this pull request as draft March 30, 2021 05:54

quantumagi mentioned this pull request Mar 30, 2021

[Infra] System Contract Dictionary (Candidate 1) #489

Closed

Fix tests

b286f79

quantumagi mentioned this pull request Apr 6, 2021

[Infra] Enable passing signatures to SC method calls #502

Merged

quantumagi added 2 commits April 6, 2021 21:38

Copy contract to output

5abe3b8

Update comments

8d02279

quantumagi mentioned this pull request Apr 8, 2021

[Infra] Add VerifySignatures stratisproject/Stratis.SmartContracts#14

Closed

quantumagi added 9 commits April 8, 2021 13:30

Add signatory management

c0264b1

Remove unused struct

bf6e443

Update tests

5018f77

Fix typo

b2f092d

Add VerifySignatures private method

d5f27cd

Refactor

d7e5d4d

Remove SetQuorum

61cddba

Fix commented code

d550216

Add null checks

d12435d

quantumagi marked this pull request as ready for review April 8, 2021 05:45

quantumagi requested review from noescape00 and rowandh April 8, 2021 07:51

YakupIpek reviewed Apr 10, 2021

View reviewed changes

quantumagi added 7 commits April 12, 2021 13:31

Add signatures argument

77e0ad2

Refactor

b7036e4

Update comments

95df108

Update contract

874f26b

Merge branch 'infracontracts' into candidate2

5bb2101

Merge branch 'infracontracts' into candidate2

62641be

Add VerifySignatures and update test cases

a7f457c

quantumagi changed the title ~~[Infra] System Contract Dictionary (Candidate 2)~~ [Infra] System Contract Dictionary Apr 15, 2021

YakupIpek reviewed Apr 15, 2021

View reviewed changes

Changes based on feedback

aa9794d

YakupIpek reviewed Apr 16, 2021

View reviewed changes

quantumagi added 2 commits April 16, 2021 17:24

Changes based on feedback

bd66981

Changes based on feedback

7b76017

YakupIpek reviewed Apr 16, 2021

View reviewed changes

Comment thread src/Stratis.SmartContracts.CLR.Tests/SmartContracts/SystemContractsDictionary.cs Outdated

quantumagi added 3 commits April 16, 2021 17:56

Changes based on feedback

dcded24

Add namespace

bec8fff

Update test

5695579

Changes based on feedback

d2f02ad

rowandh reviewed Apr 17, 2021

View reviewed changes

Modify signaturure verification method

d08549c

quantumagi mentioned this pull request Apr 17, 2021

[Infra] Modify signaturure verification method #522

Merged

quantumagi added 10 commits April 17, 2021 17:24

Revert changes

02357c2

Revert changes

523d4bd

Changes based on feedback

6b8e48d

Refactor

6e5e1f7

Merge master

5333622

Merge branch 'infracontracts' into candidate2

8d0d17e

Merge branch 'infracontracts' into candidate2

ce850ee

Add comments to contract

a535af1

Add comment

3ad0954

Update constructor

b659b86

quantumagi closed this Apr 29, 2021

		@@ -0,0 +1,205 @@
		using Stratis.SmartContracts;
		using ECRecover=Stratis.SCL.Crypto.ECRecover;

Uh oh!

Conversation

quantumagi commented Mar 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noescape00 commented Mar 31, 2021

Uh oh!

quantumagi commented Apr 8, 2021

Uh oh!

This comment was marked as resolved.

Uh oh!

quantumagi Apr 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YakupIpek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quantumagi Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YakupIpek Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quantumagi Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YakupIpek Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

YakupIpek Apr 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

quantumagi Apr 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

quantumagi commented Mar 30, 2021 •

edited

Loading

quantumagi Apr 10, 2021 •

edited

Loading

quantumagi Apr 15, 2021 •

edited

Loading

YakupIpek Apr 15, 2021 •

edited

Loading

quantumagi Apr 16, 2021 •

edited

Loading

YakupIpek Apr 15, 2021 •

edited

Loading

YakupIpek Apr 15, 2021 •

edited

Loading

quantumagi Apr 16, 2021 •

edited

Loading

quantumagi Apr 15, 2021 •

edited

Loading

quantumagi Apr 16, 2021 •

edited

Loading

YakupIpek Apr 16, 2021 •

edited

Loading

quantumagi Apr 16, 2021 •

edited

Loading

quantumagi Apr 15, 2021 •

edited

Loading