disputes: attestation update to latest format #188

abarmat · 2020-05-07T00:42:44Z

Related to https://github.com/graphprotocol/network-rfcs/pull/24

- update the attestation format to match the one sent by index node
- add an attestation parser
- set graph protocol to zero in domain separator hash
- emit the signed attestation instead of the bare one

That3Percent · 2020-05-07T14:48:11Z

contracts/DisputeManager.sol

-        bytes32 hash;
-        uint16 hashFunction;
+    // Attestation data
+    struct Attestation {


This was called Receipt in the latest RFC. The name of the struct factors into the hashKey of EIP-712, so we need to have consistency. I don't mind if we switch to Attestation and SignedAttestation if you prefer, but we need to choose just 1.

Let's use the same as in the RFC.

The latest here is confusing and differs from the RFC. There is an Attestation struct, which matches the RFC I think. But then, in this file there is the ATTESTATION_TYPE_HASH which holds the schema Attestation(bytes32 requestCID,bytes32 responseCID,bytes32 subgraphID). This doesn't match the Attestation struct, but has the fields of the Receipt struct and the name of the Attestation struct.

@That3Percent this is the one you mention right?

struct Receipt { hash requestCID, hash responseCID, hash subgraphID, }

So the type_hash says ATTESTATION when it should say RECEIPT as the type hash does not include the signature.

You are right I will do that change!

For the struct I used Attestation because that is what I parse from the incoming transaction, the receipt + signature = attestation.

And the type hash should be ""Receipt(bytes32 requestCID,bytes32 responseCID,bytes32 subgraphID)" @That3Percent

Yes, I think that the type hash should be "Receipt(..., but also that the variable name should be renamed to RECEIPT_TYPE_HASH so as not to be confused with the Attestation struct.

👍 updated in the last commit

That3Percent · 2020-05-07T14:51:49Z

contracts/DisputeManager.sol

-        bytes32 s;
+    struct SignedAttestation {
+        Attestation data;
+        bytes sig;


This also differs from the details in the latest RFC. I had copied the details from here, before these changes. So instead of bytes sig (which has variable length), we have fields for v r and s in the RFC.

I'm unpacking v,r,s into the struct now.

That3Percent · 2020-05-07T15:10:43Z

contracts/DisputeManager.sol

-        bytes32 r;
-        bytes32 s;
+    struct SignedAttestation {
+        Attestation data;


The RFC does not nest the structs. I'm amenable to nesting, so long as it doesn't increase the size of the struct due to the padding rules. As far as I can tell, we're in the clear here since it looks like the Attestationtype takes up exactly 3 storage slots - but I'm no solidity expert so you'll want to double-check. In any case, we need to have consistency so let me know if you want me to update the RFC.

I updated structs to match the RFC.

That3Percent · 2020-05-07T15:11:22Z

contracts/DisputeManager.sol

    }

-    uint256 private constant ATTESTATION_SIZE_BYTES = 192;
+    // Includes attestation + signature (96 + 65)


This comment is wrong (or misplaced). I think you mean to put it over PAYLOAD_SIZE_BYTES

That3Percent · 2020-05-07T15:25:28Z

contracts/DisputeManager.sol

    );
    bytes32 private constant DOMAIN_NAME_HASH = keccak256("Graph Protocol");
-    bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0.1");
+    bytes32 private constant DOMAIN_VERSION_HASH = keccak256("0");


(Sorry, GitHub wouldn't let me comment on line 174). The domainSeparator calculation seems wrong to me. Note that in the EIP domainSeparator is hashStruct(eip712Domain) but within the EIP712Domain struct the version field is of type string, not bytes32. So, pre-hashing this value is not correct. Same for the name field.

string is variable length and bytes32 is fixed, I think you found an issue, I'll review it

Using variable-length values in EIP712Domain should not be an issue, since the domainSeparator is meant to be pre-calcuated and always has an output size of bytes32.

I've been reviewing the EIP712 and found an example on how the eip712 domain is created. They hash all string data.

https://github.com/ethereum/EIPs/blob/master/assets/eip-712/Example.sol

function hash(EIP712Domain eip712Domain) internal pure returns (bytes32) { return keccak256(abi.encode( EIP712DOMAIN_TYPEHASH, keccak256(bytes(eip712Domain.name)), keccak256(bytes(eip712Domain.version)), eip712Domain.chainId, eip712Domain.verifyingContract )); }

In the code I'm doing:

// EIP-712 domain separator DOMAIN_SEPARATOR = keccak256( abi.encode( DOMAIN_TYPE_HASH, DOMAIN_NAME_HASH, DOMAIN_VERSION_HASH, _getChainID(), address(this), DOMAIN_SALT ) );

Yes, you are correct in pre-hashing the strings. I reviewed encodeData to see if there was any explanation for this and there is:

Each encoded member value is exactly 32-byte long.

...and then later...

The dynamic values bytes and string are encoded as a keccak256 hash of their contents.

That3Percent · 2020-05-07T15:27:51Z

contracts/DisputeManager.sol

+        require(_payload.length == PAYLOAD_SIZE_BYTES, "Signed attestation must be 161 bytes long");
+
+        // Decode attestation
+        bytes32 requestCID = _payload.slice(0, 32).toBytes32(0);


Does solidity seriously not have some utility for this? You have to do this by hand every time?

Solidity is super limited, we are using a BytesLib library to do bytes manipulation.
I have an item in my internal TODO to see if we can use a newer feature from Solidity 0.6 (the original code was written for Solidity 0.5.0) to slice calldata bytes. https://solidity.readthedocs.io/en/v0.6.0/types.html#array-slices

Will give quick look before we merge this PR

I'm now using a feature from Solidity 0.6 to slice arrays and removed a dependency we had on a BytesLib.

That3Percent · 2020-05-11T18:38:40Z

contracts/DisputeManager.sol


        // Decode attestation
-        bytes memory attestation = _data.slice(32, ATTESTATION_SIZE_BYTES);
-        require(attestation.length == ATTESTATION_SIZE_BYTES, "Signature must be 192 bytes long");
+        bytes32 requestCID = abi.decode(_data[0:32], (bytes32));


Hmm. I wasn't clear on my question before about what APIs are offered by solidity. What I want to know is whether this kind of manual deserialization is necessary. Is there not some sort of transmute/cast from bytes to Attestation? Or is it always necessary for solidity developers to roll their own deserialization all the time?

I misunderstood your initial comment but it helped me to rewrite it in a more efficient way.

You need to roll your own deserialization, it would be great to cast from bytes to the struct directly. The closest discussion about that I found is this one ethereum/solidity#7134

That3Percent · 2020-05-11T18:46:49Z

contracts/DisputeManager.sol

+            _attestation.responseCID,
+            _attestation.subgraphID
+        );
+        bytes32 disputeID = getDisputeID(receipt);


I am concerned that this uniquely identifies a dispute from only these fields, and does not include the indexNode in the hash in any way. Since multiple indexers may provide the same incorrect responses, separate disputes need to be created for them.

If anything, the disputeId should be created from r, s, and v once all other checks have passed since these uniquely identify the request/response/indexer tuple.

Good observation. Yes, this code is now rejecting when two receipts are similar and are submitted by different indexers. I'll ask Brandon as he had some thoughts about conflicting attestations.

That3Percent · 2020-05-11T18:50:05Z

contracts/DisputeManager.sol

+            _attestation.subgraphID,
+            _attestation.r,
+            _attestation.s,
+            _attestation.v


The field order here does not match the Attestation struct. It has v,r,s but this has r,s,v. Is that a problem?

Let's use:

struct Attestation { bytes32 requestCID; bytes32 responseCID; bytes32 subgraphID; bytes32 r; bytes32 s; uint8 v; }

The libraries used to sign (web3, ethers) send the signature like r,s,v

That3Percent · 2020-05-11T18:55:22Z

test/lib/attestation.js

-  ) // IEP712 : domain separator + signed attestation
+    receipt.substring(2) + // Attestation
+    messageSig.substring(2) // Signature
+  ) // receipt + signature = attestation in EIP712 format


I think this comment is wrong. EIP712 is not a "format" for interchange. It's a method for hashing and signing - not serialization. There is an encodeStruct method, but this encoding does not match what these methods do to read/write structs from bytes. encodeStruct is not lossless, and therefore cannot in principle be used for serialization even if you wanted to. So, the string returned by this method cannot be of an attestation in "EIP712 format".

I understand, I think I did a bad use of the language. I'd been using EIP712 "format" when it really is a standard, and I'd been calling the format the end result of encode/hashing the struct with the type_hash and each content field processed.

I'm not sure that we're on the same page. This looks like some kind of ABI encoded SignedAttestation, whereas the comment says it's EIP-712.

In my comment I was referring to when I used EIP712 in the code.

About that particular section, the comment is not ok, it is just the attestation what that function returns, the receipt + signature appended.

That3Percent · 2020-05-11T19:06:21Z

test/lib/attestation.js

-
-  // Message
-  const message = createMessage(domainSeparatorHash, attestationHash)
+  // Attestation signing wrapped in EIP721 format


This has a typo and it says EIP721 when it meant EIP712, but to reiterate, there is no EIP-712 format.

abarmat · 2020-05-13T13:54:05Z

@That3Percent I pushed changes including:

DisputeID considering the indexer.
Use ethers library (better) for signing.
Refactor decoding the attestation struct.

- update the attestation format to match the one sent by index node - add an attestation parser - set graph protocol to zero in domain separator hash - emit the signed attestation instead of the bare one

- remove BytesLib dependency and use Solidity 0.6 array slice - remove ECDSA-openzeppelin dependency for ecrecover() - refactor Attestation struct to follow RFC - change language around attestation and receipts - ignore linting of the DisputeManager till support for array slices is added

…ature sent by clients

…d signatures

abarmat changed the base branch from master to feat/curation-events-and-lang May 7, 2020 00:43

abarmat requested review from davekaj, Jannis and That3Percent May 7, 2020 00:43

abarmat self-assigned this May 7, 2020

abarmat added the enhancement New feature or request label May 7, 2020

That3Percent suggested changes May 7, 2020

View reviewed changes

abarmat force-pushed the feat/disputes-attestation-update branch from 9746d13 to b1bec82 Compare May 8, 2020 14:33

abarmat force-pushed the feat/curation-events-and-lang branch from 2e7c7fc to 165852f Compare May 8, 2020 14:46

abarmat changed the base branch from feat/curation-events-and-lang to master May 11, 2020 14:24

abarmat force-pushed the feat/disputes-attestation-update branch from b1bec82 to a66a0f0 Compare May 11, 2020 14:43

That3Percent suggested changes May 11, 2020

View reviewed changes

That3Percent approved these changes May 13, 2020

View reviewed changes

abarmat added 9 commits May 13, 2020 18:04

disputes:

b694b49

- update the attestation format to match the one sent by index node - add an attestation parser - set graph protocol to zero in domain separator hash - emit the signed attestation instead of the bare one

dispute: update variable name

6c210e0

disputes: use receipt instead of attestation according to the RFC

bcfbb2c

disputes: reorder attestation struct attributes to match RFC and sign…

367c5d8

…ature sent by clients

disputes: improve comments

366654e

disputes: refactor signature parsing and update client library to sen…

827d915

…d signatures

disputes: create dispute ID by hashing receipt + indexer address

f7d894e

disputes: update package-lock

0956b1b

abarmat force-pushed the feat/disputes-attestation-update branch from f0d8fcd to 0956b1b Compare May 13, 2020 23:45

abarmat merged commit f8a3669 into master May 14, 2020

abarmat deleted the feat/disputes-attestation-update branch May 14, 2020 00:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

disputes: attestation update to latest format #188

disputes: attestation update to latest format #188

abarmat commented May 7, 2020

That3Percent May 7, 2020

abarmat May 7, 2020

That3Percent May 11, 2020

abarmat May 11, 2020

abarmat May 11, 2020

That3Percent May 11, 2020

abarmat May 11, 2020

That3Percent May 7, 2020

abarmat May 8, 2020

That3Percent May 7, 2020

abarmat May 8, 2020

That3Percent May 7, 2020

That3Percent May 7, 2020 •

edited

Loading

abarmat May 7, 2020

That3Percent May 7, 2020

abarmat May 8, 2020

That3Percent May 11, 2020

That3Percent May 7, 2020

abarmat May 7, 2020

abarmat May 8, 2020

That3Percent May 11, 2020

abarmat May 11, 2020

That3Percent May 11, 2020

abarmat May 11, 2020

That3Percent May 11, 2020

abarmat May 11, 2020

That3Percent May 11, 2020

abarmat May 11, 2020

That3Percent May 11, 2020

abarmat May 11, 2020

That3Percent May 11, 2020

abarmat commented May 13, 2020

disputes: attestation update to latest format #188

disputes: attestation update to latest format #188

Conversation

abarmat commented May 7, 2020

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

That3Percent May 7, 2020 • 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

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

abarmat commented May 13, 2020

That3Percent May 7, 2020 •

edited

Loading