Integrate libsnark #3587

chriseth · 2017-02-22T16:18:20Z

Implements ethereum/EIPs#212 and ethereum/EIPs#213
Fixes #3619 (comment)

This is still work in progress.

change point encoding
remove unnecessary stuff
make this work for truncated input
we can probably patch libsnark to remove all cpp files outside the directory src/algebra/curves/alt_bn128/ - this requires patching the makefile.
add source hash for external dependency
move unittest file to the right place
clean up git history
check edge cases for pairing

chriseth · 2017-02-28T19:21:31Z

@chfast please take over from here :-)

winsvega · 2017-03-02T21:40:32Z

please rebase

codecov-io · 2017-03-07T16:25:56Z

Codecov Report

Merging #3587 into develop will increase coverage by 0.38%.
The diff coverage is 99.6%.

@@             Coverage Diff             @@
##           develop    #3587      +/-   ##
===========================================
+ Coverage    65.89%   66.28%   +0.38%     
===========================================
  Files          303      305       +2     
  Lines        23046    23299     +253     
===========================================
+ Hits         15186    15443     +257     
+ Misses        7860     7856       -4

Impacted Files	Coverage Δ
libethashseal/genesis/mainNetwork.cpp	`100% <ø> (ø)`	⬆️
libethashseal/genesis/metropolisTest.cpp	`100% <ø> (ø)`	⬆️
test/tools/libtesteth/ImportTest.cpp	`49.61% <100%> (ø)`	⬆️
test/unittests/libdevcrypto/LibSnark.cpp	`100% <100%> (ø)`
test/tools/libtesteth/boostTest.cpp	`55.1% <100%> (ø)`	⬆️
test/tools/jsontests/StateTests.cpp	`95.23% <100%> (+0.05%)`	⬆️
libethcore/Precompiled.cpp	`94.91% <100%> (+0.79%)`	⬆️
libdevcrypto/LibSnark.cpp	`98.85% <98.85%> (ø)`
test/tools/fuzzTesting/fuzzHelper.cpp	`31.86% <0%> (-1.97%)`	⬇️
... and 5 more

winsvega · 2017-03-30T08:39:25Z

please rebase on develop so to point this PR to a test branch
hm. cant refill zeroKnowledge tests. dedlock of some kind

chfast · 2017-04-21T15:23:59Z

Progress: it builds on Windows, but tests fail.

pirapira · 2017-05-19T10:02:45Z

libethashseal/genesis/metropolisTest.cpp

@@ -57,7 +57,10 @@ R"E(
 		"0000000000000000000000000000000000000002": { "wei": "1", "precompiled": { "name": "sha256", "linear": { "base": 60, "word": 12 } } },
 		"0000000000000000000000000000000000000003": { "wei": "1", "precompiled": { "name": "ripemd160", "linear": { "base": 600, "word": 120 } } },
 		"0000000000000000000000000000000000000004": { "wei": "1", "precompiled": { "name": "identity", "linear": { "base": 15, "word": 3 } } },
-		"0000000000000000000000000000000000000005": { "wei": "1", "precompiled": { "name": "modexp" } }
+		"0000000000000000000000000000000000000005": { "wei": "1", "precompiled": { "name": "modexp" } } },


This line has one too many }. Really.

pirapira · 2017-05-19T17:49:17Z

There is an input that crashes libff (Pairing with the zero-element of G2). The fix is pirapira@958a69c

pirapira · 2017-05-22T08:37:01Z

I prepared some tests for snark integration https://github.com/ethereum/tests/pull/171/files

I pushed a fix for my comment.

pirapira · 2017-05-22T16:16:33Z

libdevcrypto/LibSnark.cpp

+{
+	// This is hackish, but we really want to use `static` variable for lock
+	// free thread-safe initialization.
+	static bool initialized = (libff::alt_bn128_pp::init_public_params(), true);


So is init_public_params() supposed to be called just once?

Maybe we should first say if (!initialized) in the beginning. Maybe we need to compare-and-swap initialized.

Yes, good idea.
You can use std::call_ones as a start.

That's something new to me. Let me try this.

Alternatively, you can wrap the function to add some logs to make sure it is called, and it is called once.

pirapira · 2017-05-23T09:31:19Z

libdevcrypto/LibSnark.cpp

+
+libff::alt_bn128_G1 decodePointG1(dev::bytesConstRef _data)
+{
+	libff::alt_bn128_Fq X = decodeFqElement(_data.cropped(0));


I think it's better to specify the length as cropped(0, 32).

No, now I see that the code is supposed to work even when _data is shorter than 64 bytes.

pirapira · 2017-05-23T09:34:00Z

libdevcrypto/LibSnark.cpp

+	// is too short.
+	h256 xbin(_data, h256::AlignLeft);
+	if (u256(xbin) >= u256(fromLibsnarkBigint(libff::alt_bn128_Fq::mod)))
+		BOOST_THROW_EXCEPTION(InvalidEncoding());


I'll add a more informative message here (especially the value of the modulo).

chriseth

Looks good!

pirapira · 2017-05-23T16:38:48Z

libdevcrypto/LibSnark.cpp

+	constexpr auto L = sizeof(x.data[0]);
+	for (unsigned i = 0; i < N; i++)
+		for (unsigned j = 0; j < L; j++)
+			x.data[N - 1 - i] |= uint64_t(_x[i * L + j]) << (8 * (L - 1 - j));


This might be a problem when x.data[0] is wider than 64bit, but I'm not sure if we should go for

x.data[N - 1 - i] |= (std::remove_reference<decltype(x.data[0])>::type)(_x[i * L + j]) << (8 * (L - 1 - j));

I guess I should just say libff::mp_limb_t.

pirapira · 2017-06-09T09:49:49Z

libdevcrypto/LibSnark.cpp

+		libff::alt_bn128_pp::init_public_params();
+		return true;
+	}();
+	(void)s_initialized;


I'm wondering why we need this boolean that we never read. Maybe doing this is enough?

[]() noexcept { libff::inhibit_profiling_info = true; libff::inhibit_profiling_counters = true; libff::alt_bn128_pp::init_public_params(); }();

Or, do we even need the closure?

We need any static variable to use thread-safe static initialization (aka magic statics). We can use call_once here explicitly, but static vars are handled better by compilers.
What do you think? Go back to call_once?

No need to go back; I just wondered.

pirapira · 2017-06-09T11:35:11Z

In the EIP PR, I asked for some clarification about the input length. ethereum/EIPs#212 (comment)

I checked some other clients, and all of them required input_length % 192 == 0 for the pairing checker.

pirapira · 2017-06-12T10:25:41Z

libdevcrypto/LibSnark.cpp

+	constexpr size_t L = sizeof(_b.data[0]);
+	static_assert(sizeof(mp_limb_t) == L, "Unexpected limb size in libff::bigint.");
+	h256 x;
+	for (size_t i = 0; i < N; i++)


I get the following error. Maybe it helps to specify the type of N.

libdevcrypto/LibSnark.cpp:68:23: error: comparison of integers of different signs: 'size_t' (aka 'unsigned long') and 'const long' [-Werror,-Wsign-compare] for (size_t i = 0; i < N; i++)

pirapira · 2017-06-12T13:33:51Z

libdevcrypto/LibSnark.cpp

+libff::alt_bn128_G2 decodePointG2(dev::bytesConstRef _data)
+{
+	libff::alt_bn128_Fq2 x = decodeFq2Element(_data);
+	libff::alt_bn128_Fq2 y = decodeFq2Element(_data.cropped(64));


x and y can be const.

pirapira · 2017-06-12T13:34:31Z

libdevcrypto/LibSnark.cpp

+	if (x == libff::alt_bn128_Fq2::zero() && y == libff::alt_bn128_Fq2::zero())
+		return libff::alt_bn128_G2::zero();
+	libff::alt_bn128_G2 p(x, y, libff::alt_bn128_Fq2::one());
+	if (!p.is_well_formed())


p can also be const.

pirapira · 2017-06-12T13:35:16Z

libdevcrypto/LibSnark.cpp

+	{
+		// Invalid length.
+		return {false, bytes{}};
+	}


Style: we don't have braces around a single-line body of if.

pirapira · 2017-06-12T13:36:44Z

libdevcrypto/LibSnark.cpp

+		for (size_t i = 0; i < pairs; ++i)
+		{
+			dev::bytesConstRef pair = _in.cropped(i * pairSize, pairSize);
+			libff::alt_bn128_G2 p = decodePointG2(pair.cropped(2 * 32));


pair and p can be const.

pirapira · 2017-06-12T13:38:18Z

libdevcrypto/LibSnark.cpp

+	}
+	catch (...)
+	{
+		cwarn << "Internal exception from libsnark. Forwarding as precompiled contract failure.";


Perhaps, we should look into the library, and see which exception is supposed to happen.

It does not throw any exceptions.

Should we silence unexpected exceptions here?

I prefer not to. It's better to terminate the node in this case than running it in invalid state mode. Agree?

chfast · 2017-06-12T14:09:42Z

Ok, I will handle these.

Handle only expected exceptions around snark primitives precompiled, pass anything else higher.

chfast · 2017-06-13T13:36:00Z

Great! Waiting for CI tests...

pirapira · 2017-06-13T17:40:05Z

All green.

chriseth added the in progress label Feb 22, 2017

chriseth force-pushed the snark branch 4 times, most recently from 1746645 to 70b6758 Compare February 28, 2017 19:14

chfast self-assigned this Mar 1, 2017

chfast added this to the Metropolis milestone Mar 3, 2017

chriseth force-pushed the snark branch from 70b6758 to 3db341b Compare March 7, 2017 12:52

chfast force-pushed the snark branch from 66b2879 to 2bafb09 Compare March 7, 2017 16:04

chfast force-pushed the snark branch from e70dec2 to c25ae98 Compare March 16, 2017 12:01

chfast force-pushed the snark branch 2 times, most recently from f175811 to 356b9c0 Compare April 21, 2017 12:40

chfast force-pushed the snark branch 3 times, most recently from 77369b3 to e221f3e Compare May 8, 2017 20:51

pirapira previously requested changes May 19, 2017

View reviewed changes

chfast mentioned this pull request May 19, 2017

[META] Byzantium implementation progress #4050

Closed

17 tasks

pirapira reviewed May 22, 2017

View reviewed changes

pirapira reviewed May 23, 2017

View reviewed changes

chriseth commented May 23, 2017

View reviewed changes

pirapira reviewed May 23, 2017

View reviewed changes

pirapira reviewed Jun 9, 2017

View reviewed changes

chfast force-pushed the snark branch 2 times, most recently from ebd5a90 to dca57cd Compare June 9, 2017 11:54

pirapira suggested changes Jun 12, 2017

View reviewed changes

pirapira reviewed Jun 12, 2017

View reviewed changes

chfast force-pushed the snark branch 2 times, most recently from d824b64 to 5b383c5 Compare June 12, 2017 15:21

pirapira approved these changes Jun 12, 2017

View reviewed changes

chriseth and others added 9 commits June 13, 2017 12:33

Add snark primitives and pairing support

df9386b

ProjectSnark: switch to libff

8991828

testeth: save one thread

90ad447

Integrate MPIR for libff

8d1726c

libff: Fix configuration

88a0786

snark: Fix conversions and static initialization

e924e18

snark: Fixes and cleanups

9cbc85d

snark: Cleanups and tests

a1b0264

snark: Handle only expected exceptions

969f83b

Handle only expected exceptions around snark primitives precompiled, pass anything else higher.

pirapira force-pushed the snark branch from 98ba0fd to 969f83b Compare June 13, 2017 10:33

Update tests

8eee78c

pirapira merged commit 8f8edab into develop Jun 13, 2017

pirapira deleted the snark branch June 13, 2017 17:40

pirapira removed the needs review label Jun 13, 2017

Integrate libsnark #3587

Integrate libsnark #3587

Conversation

chriseth commented Feb 22, 2017 • edited by chfast Loading

chriseth commented Feb 28, 2017

winsvega commented Mar 2, 2017

codecov-io commented Mar 7, 2017 • edited Loading

Codecov Report

winsvega commented Mar 30, 2017 • edited Loading

chfast commented Apr 21, 2017

Choose a reason for hiding this comment

pirapira commented May 19, 2017

pirapira commented May 22, 2017

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

chriseth 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

pirapira commented Jun 9, 2017

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

chfast commented Jun 12, 2017

chfast commented Jun 13, 2017

pirapira commented Jun 13, 2017

chriseth commented Feb 22, 2017 •

edited by chfast

Loading

codecov-io commented Mar 7, 2017 •

edited

Loading

winsvega commented Mar 30, 2017 •

edited

Loading