feat(avm): constrain sha256 #11048

IlyasRidhuan · 2025-01-03T22:15:14Z

Most of the sha256 constraints. The main things missing are:

Lookup constraints for XOR / AND
Memory loading constraints

We'll delay this until vm2 re-design

IlyasRidhuan · 2025-01-03T22:15:29Z

feat(avm): constrain sha256 #11048 👈 (View in Graphite)
fix: hardcode value in constants #11442
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

jeanmon · 2025-01-09T14:58:17Z

barretenberg/cpp/pil/avm/gadgets/sha256.pil

+    sel * (1 - sel) = 0;
+    // For the XOR lookups
+    pol commit xor_sel;
+    perform_round * (xor_sel - 2) = 0;


We should introduce constant for op_id of OR, AND, XOR and use it instead of hardcoded '2'.

jeanmon · 2025-01-09T15:00:57Z

barretenberg/cpp/pil/avm/gadgets/sha256.pil

+    perform_round * (xor_sel - 2) = 0;
+    // For the AND lookups
+    pol commit and_sel;
+    and_sel = 0;


Same as above.
Maybe add a note that, once we support constant in a tuple member of a lookup, we will not need to define this column anymore.

jeanmon · 2025-01-09T15:04:31Z

barretenberg/cpp/pil/avm/gadgets/sha256.pil

+
+    // Counter
+    pol commit rounds_remaining;
+    start * (rounds_remaining - 64) + perform_round * (rounds_remaining - rounds_remaining' - 1) = 0;


Please define a constant for 64.

jeanmon · 2025-01-09T15:16:48Z

barretenberg/cpp/src/barretenberg/vm/avm/trace/gadgets/fixed_sha256_constants.hpp

+};
+
+template <typename DestRow>
+void merge_into(DestRow& dest, FixedSha256ConstantsTable::Sha256ConstantsRow const& src, uint32_t multiplicity)


Suggested change

void merge_into(DestRow& dest, FixedSha256ConstantsTable::Sha256ConstantsRow const& src, uint32_t multiplicity)

void merge_into(DestRow& dest, const FixedSha256ConstantsTable::Sha256ConstantsRow& src, uint32_t multiplicity)

jeanmon · 2025-01-09T15:16:58Z

barretenberg/cpp/src/barretenberg/vm/avm/trace/gadgets/fixed_sha256_constants.hpp

+};
+
+template <typename DestRow>
+void merge_into(DestRow& dest, FixedSha256ConstantsTable::Sha256ConstantsRow const& src, uint32_t multiplicity)


Suggested change

void merge_into(DestRow& dest, FixedSha256ConstantsTable::Sha256ConstantsRow const& src, uint32_t multiplicity)

void merge_into(DestRow& dest, const FixedSha256ConstantsTable::Sha256ConstantsRow& src, uint32_t multiplicity)

jeanmon · 2025-01-09T15:18:11Z

barretenberg/cpp/src/barretenberg/vm/avm/trace/gadgets/sha256.cpp

    return output;
 }

+static std::tuple<uint32_t, uint32_t> bit_limbs(uint32_t const& a, uint8_t const b)


Suggested change

static std::tuple<uint32_t, uint32_t> bit_limbs(uint32_t const& a, uint8_t const b)

static std::tuple<uint32_t, uint32_t> bit_limbs(const uint32_t a, const uint8_t b)

jeanmon · 2025-01-09T15:18:45Z

barretenberg/cpp/src/barretenberg/vm/avm/trace/gadgets/sha256.cpp

+{
+
+    for (size_t i = 0; i < sha256_trace.size(); i++) {
+        auto const& src = sha256_trace.at(i);


Suggested change

auto const& src = sha256_trace.at(i);

const auto& src = sha256_trace.at(i);

jeanmon

LGTM but have a look at my feedback. Cool work!

fcarreiro · 2025-01-24T14:16:19Z

barretenberg/cpp/pil/vm2/precomputed.pil

+
+// SHA256 Round Params Lookup
+pol constant sel_sha256_compression;


I think you need to do tracegen for these

add to the precomputed trace

add it in the tracegen helper

fcarreiro · 2025-01-24T14:18:01Z

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/sha256.test.cpp

+    builder.process(sha256_event_container, trace);
+
+    TestTraceContainer::RowTraceContainer rows = trace.as_rows();
+    rows[0].precomputed_first_row = 1;


do you need this? doesnt seem used in the pil file?

fcarreiro · 2025-01-24T14:18:59Z

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/sha256.test.cpp

+using C = Column;
+using sha256 = bb::avm2::sha256<FF>;
+
+// This test imports a bunch of external code since hand-generating the sha256 trace is a bit laborious atm.


Please add an "empty row test": just a single row with precomputed_first_row = 1 and run the whole relation. This is to check that the relation passes on empty rows (and kind of test skippable). See David's PR.

fcarreiro · 2025-01-24T14:19:29Z

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/sha256.test.cpp

+
+    TestTraceContainer::RowTraceContainer rows = trace.as_rows();
+    rows[0].precomputed_first_row = 1;
+    rows[0].sha256_latch = 1;


why isnt tracegen doing this?

fcarreiro · 2025-01-24T14:20:03Z

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/sha256.test.cpp

+
+    check_relation<sha256>(rows);
+}
+


No negative tests now or no negative tests ever? :)

fcarreiro · 2025-01-24T14:24:38Z

barretenberg/cpp/src/barretenberg/vm2/tracegen/sha256_trace.cpp

+}
+
+// Computes and returns the message schedule (w) value for that round, and inserts witness data into the trace.
+uint32_t Sha256TraceBuilder::compute_w_with_witness(std::array<uint32_t, 16> prev_w_helpers, TraceContainer& trace)


I will consider taking the trace in the constructor so that we can eventually avoid passing it everywhere.

fcarreiro · 2025-01-24T14:25:39Z

barretenberg/cpp/src/barretenberg/vm2/tracegen/sha256_trace.cpp

+        state[6], /*h = g*/
+    };
+}
+


I will trust you for most of this. But indeed you effort paid off. It's looking much better now!

fcarreiro · 2025-01-24T14:25:59Z

barretenberg/cpp/pil/vm2/sha256.pil

@@ -0,0 +1,375 @@
+include "precomputed.pil";
+namespace sha256;


newline before

fcarreiro · 2025-01-24T14:26:21Z

barretenberg/cpp/pil/vm2/sha256.pil

+namespace sha256;
+
+    // Memory ops
+    pol commit state_offset;


We are not indenting in the other files, since there is only 1 namespace expected, does it make sense?

fcarreiro · 2025-01-24T14:27:41Z

barretenberg/cpp/pil/vm2/sha256.pil

+    // binary.start {binary.acc_ia, binary.acc_ib, binary.acc_ic, binary.op_id};
+
+    pol commit ch;
+    // #[LOOKUP_CH_XOR]


(not this one)
You can add lookups into precomputed I think. See how David did it.

fcarreiro · 2025-01-24T14:29:10Z

barretenberg/cpp/src/barretenberg/vm2/simulation/sha256_compression.cpp

+    // Load 8 elements representing the state from memory.
+    for (uint32_t i = 0; i < 8; ++i) {
+        auto memory_value = memory.get(state_addr + i).value;
+        /*Check Tag is U32 */


missing " ", same below in the other check tag

fcarreiro · 2025-01-24T14:29:37Z

barretenberg/cpp/src/barretenberg/vm2/simulation/sha256_compression.cpp

+    // Load 16 elements representing the input from memory.
+    for (uint32_t i = 0; i < 16; ++i) {
+        auto memory_value = memory.get(input_addr + i).value;
+        /*Check Tag is U32 */


Is this an error handling TODO? Do you need to check the tags here? if yes please make explicit

fcarreiro · 2025-01-24T14:42:22Z

barretenberg/cpp/src/barretenberg/vm2/simulation/sha256_compression.hpp

+                     MemoryAddress output_addr) override;
+
+  private:
+    EventEmitterInterface<Sha256CompressionEvent>& events;


~~Please add a TODO that we should be using "deduplicating events" here. I'll build the infra at some point.~~

Sorry, that was wrong. This is memory aware.

IlyasRidhuan force-pushed the ir/01-03-feat_constrain_sha256 branch from edd1c61 to 1f4a5bf Compare January 9, 2025 10:38

IlyasRidhuan marked this pull request as ready for review January 9, 2025 10:57

IlyasRidhuan requested review from jeanmon, fcarreiro and Maddiaa0 as code owners January 9, 2025 10:57

IlyasRidhuan changed the title ~~feat: constrain sha256~~ feat(avm): constrain sha256 Jan 9, 2025

IlyasRidhuan requested review from fcarreiro and dbanks12 and removed request for fcarreiro and Maddiaa0 January 9, 2025 10:58

IlyasRidhuan force-pushed the ir/01-03-feat_constrain_sha256 branch from 1f4a5bf to 87cef77 Compare January 9, 2025 12:51

jeanmon reviewed Jan 9, 2025

View reviewed changes

jeanmon approved these changes Jan 9, 2025

View reviewed changes

IlyasRidhuan force-pushed the ir/01-03-feat_constrain_sha256 branch from 87cef77 to eb03f46 Compare January 10, 2025 11:54

IlyasRidhuan marked this pull request as draft January 15, 2025 12:54

IlyasRidhuan force-pushed the ir/01-03-feat_constrain_sha256 branch from eb03f46 to 8c6e5a8 Compare January 23, 2025 12:38

IlyasRidhuan changed the base branch from master to ir/01-21-fix_hardcode_value_in_constants January 23, 2025 12:38

IlyasRidhuan mentioned this pull request Jan 23, 2025

fix: hardcode value in constants #11442

Merged

IlyasRidhuan force-pushed the ir/01-03-feat_constrain_sha256 branch 3 times, most recently from 1f891fb to c6ffba9 Compare January 23, 2025 13:53

IlyasRidhuan marked this pull request as ready for review January 23, 2025 13:54

IlyasRidhuan force-pushed the ir/01-03-feat_constrain_sha256 branch from c6ffba9 to c3f7087 Compare January 23, 2025 14:16

Base automatically changed from ir/01-21-fix_hardcode_value_in_constants to master January 23, 2025 14:52

feat: constrain sha256

6f9fbd0

IlyasRidhuan force-pushed the ir/01-03-feat_constrain_sha256 branch from c3f7087 to 6f9fbd0 Compare January 23, 2025 15:34

fcarreiro requested changes Jan 24, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(avm): constrain sha256 #11048

feat(avm): constrain sha256 #11048

IlyasRidhuan commented Jan 3, 2025 •

edited

Loading

IlyasRidhuan commented Jan 3, 2025 •

edited

Loading

jeanmon Jan 9, 2025

jeanmon Jan 9, 2025

jeanmon Jan 9, 2025

jeanmon Jan 9, 2025

jeanmon Jan 9, 2025

jeanmon Jan 9, 2025

jeanmon Jan 9, 2025

jeanmon left a comment

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025

fcarreiro Jan 24, 2025 •

edited

Loading

	void merge_into(DestRow& dest, FixedSha256ConstantsTable::Sha256ConstantsRow const& src, uint32_t multiplicity)
	void merge_into(DestRow& dest, const FixedSha256ConstantsTable::Sha256ConstantsRow& src, uint32_t multiplicity)

	static std::tuple<uint32_t, uint32_t> bit_limbs(uint32_t const& a, uint8_t const b)
	static std::tuple<uint32_t, uint32_t> bit_limbs(const uint32_t a, const uint8_t b)

	auto const& src = sha256_trace.at(i);
	const auto& src = sha256_trace.at(i);


		// SHA256 Round Params Lookup
		pol constant sel_sha256_compression;

		@@ -0,0 +1,375 @@
		include "precomputed.pil";
		namespace sha256;

feat(avm): constrain sha256 #11048

Are you sure you want to change the base?

feat(avm): constrain sha256 #11048

Conversation

IlyasRidhuan commented Jan 3, 2025 • edited Loading

IlyasRidhuan commented Jan 3, 2025 • 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

jeanmon 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

fcarreiro Jan 24, 2025 • edited Loading

Choose a reason for hiding this comment

IlyasRidhuan commented Jan 3, 2025 •

edited

Loading

IlyasRidhuan commented Jan 3, 2025 •

edited

Loading

fcarreiro Jan 24, 2025 •

edited

Loading