[VID] AvidM integration #2579

mrain · 2025-02-11T17:14:32Z

Closes #2600

This PR:

Integrate the efficient AvidM scheme.

New VID scheme ships with Epoch upgrade.
Underlying it's currently using keccak256 hash function. Should also consider poseidon2.

Temporarily disable hotshot test_1 b/c #2664

This PR does not:

Update the namespace proof logic. Will deal with it in this issue [VID] update the logic for namespace proof verification #2458

Key places to review:

…into cl/newvid

github-actions · 2025-02-19T16:29:55Z

Unable to build without Cargo.lock file.

This means that after this change 3rd party projects may have
difficulties using crates in this repo as a dependency. If this
isn't easy to fix please open an issue so we can fix it later.

For the first failing build see: https://github.com/EspressoSystems/espresso-sequencer/actions/runs/13417524682

To reproduce locally run

cargo generate-lockfile
cargo check --all-targets

This PR can still be merged.

…into cl/newvid

…ncer into cl/newvid

lukaszrzasik

I reviewed the HS part mainly. It looks good to me but I'd like to wait with the approval until somebody else reviews the sequencer part and crypto.

lukaszrzasik · 2025-02-24T16:41:41Z

hotshot-types/src/consensus.rs

+/// Type alias to avoid complexity
+pub type PayloadWithMetadata<TYPES> = (
+    <TYPES as NodeType>::BlockPayload,
+    <<TYPES as NodeType>::BlockPayload as BlockPayload<TYPES>>::Metadata,
+);


Just a suggestion: could we use a struct with named fields instead of a tuple? I find it easier to reason about the code when I see a name instead of .0 or .1.

+1 to Lucas' suggestion. This seems like it should be a struct with payload and metadata fields

Now wrapped as a struct: 015aa10

ss-es · 2025-02-24T16:57:40Z

.github/workflows/hotshot.yml

@@ -18,7 +18,7 @@ jobs:
    strategy:
      matrix:
        test_suites:
-          - test-ci-1
+          # - test-ci-1 temporarily disabled for (https://github.com/EspressoSystems/espresso-sequencer/issues/2664)


is the test currently hanging or not?

I think we should continue to run this if you moved the calculation out of spawn_blocking and it works. or, at most, I think we should maybe #[ignore] the specific tests that aren't working

the workflows aren't well-organized so this is disabling some integration tests too

spawn_blocking is still there. The conclusion was that the real problem is in the test code. There is a ticket for that: #2664

ss-es · 2025-02-24T17:01:23Z

hotshot-builder-core-refactored/src/service.rs

-                tokio::task::spawn_blocking(move || advz_commitment(&encoded_txns, num_nodes));
+            let join_handle = tokio::task::spawn_blocking(move || {
+                let encoded_tx_len = encoded_txns.len();
+                advz_scheme(num_nodes).commit_only(encoded_txns).map(VidCommitment::V0).unwrap_or_else(|err| panic!("VidScheme::commit_only failure:(num_storage_nodes,payload_byte_len)=({num_nodes},{encoded_tx_len}) error: {err}"))


I don't think it matters either way, but is this keeping the same logic?

the new version seems much less readable to me

Yes this is annoying. Good that VID is removed in the recent merge. a2657f8

ss-es · 2025-02-24T17:03:15Z

hotshot-example-types/src/block_types.rs

-                    .as_ref()
-                    .as_ref(),
+            .var_size_bytes(
+                <TestBlockHeader as BlockHeader<TestTypes>>::payload_commitment(self).as_ref(),


no objection here, but wondering: is this just a cleanup for tests or was the commitment required to be changed?

Ahh this shall be a fixed_size_bytes. 015aa10
Commitments won't change, as those actual types in the sequencer.

ss-es · 2025-02-24T17:11:29Z

hotshot-testing/tests/tests_1.rs

@@ -5,5 +5,6 @@
 // along with the HotShot repository. If not, see <https://mit-license.org/>.

 mod tests_1 {
-    automod::dir!("tests/tests_1");
+    // These tests are temporarily disabled for the following issue: <https://github.com/EspressoSystems/espresso-sequencer/issues/2664>
+    // automod::dir!("tests/tests_1");


I think we should revert this here and just add an #[ignore] for the specific test that was failing (test_da_task?)

ss-es · 2025-02-24T17:13:28Z

hotshot-types/src/consensus.rs

+/// Type alias to avoid complexity
+pub type PayloadWithMetadata<TYPES> = (
+    <TYPES as NodeType>::BlockPayload,
+    <<TYPES as NodeType>::BlockPayload as BlockPayload<TYPES>>::Metadata,
+);


+1 to Lucas' suggestion. This seems like it should be a struct with payload and metadata fields

ss-es · 2025-02-24T17:17:34Z

hotshot-types/src/data/ns_table.rs

+/// Helper function for AvidM scheme to parse a namespace table.
+/// If the namespace table is invalid, it returns a default single entry namespace table.


Can you add even a rough description of what a valid namespace table is and/or what the layout should be?

e.g. I assume that bytes is an array with namespace tables at the start and payload_byte_len bytes at the end, but it would be nice to have this written down here

This is mostly copied from the sequencer repo. Add a link to it 015aa10

I think we should use NsTable::ns_range() which does exactly this already.

ss-es · 2025-02-24T17:23:43Z

types/src/v0/impls/header.rs

@@ -813,7 +815,7 @@ impl BlockHeader<SeqTypes> for Header {
            height = parent_leaf.block_header().block_number() + 1,
            parent_view = ?parent_leaf.view_number(),
            payload_commitment,
-            payload_size = VidSchemeType::get_payload_byte_len(&_vid_common),
+            // payload_size = ADVZScheme::get_payload_byte_len(&_vid_common),


should this be deleted? (not just commented out)

I think there's a few lines that were commented out in this file

deleted: 015aa10

…into cl/newvid

alxiong

@mrain crate vid is directly copied over without new addition, right? anything I should pay special attention to?

vid/Cargo.toml

alxiong · 2025-02-25T11:16:01Z

vid/rustfmt.toml

Q: is it a good idea to have sub-crate rustfmt? or should we just forgo this. (personally I like this config but feels intrusive to impose on the entire monorepo)

cc @rob-maron do you have any suggestion here?

does this work as you'd expect? i.e. does the formatting in this file apply to the vid crate (and only the vid crate) if you run cargo fmt from the workspace root?

if so I feel like it's completely fine

Locally on my side it works well and only applies to the vid crate.

alxiong · 2025-02-25T11:19:22Z

vid/src/lib.rs

+type VidResult<T> = Result<T, VidError>;
+
+/// Trait definition for a Verifiable Information Dispersal (VID) scheme.
+pub trait VidScheme {


Q: we already have trait jf_vid::VidScheme (and used everywhere), would it be confusing to migrate? or can we try to unify the two and stablize this trait to avoid future disruption?

Old trait doesn't take weight distribution. My plan is to migrate them into the new VID crate.

vid/src/utils/bytes_to_field.rs

hotshot-types/src/vid/advz.rs

alxiong · 2025-02-25T11:56:19Z

hotshot-types/src/data/ns_table.rs

+/// Helper function for AvidM scheme to parse a namespace table.
+/// If the namespace table is invalid, it returns a default single entry namespace table.


I think we should use NsTable::ns_range() which does exactly this already.

sequencer/src/persistence/sql.rs

hotshot-query-service/src/data_source/update.rs

vid/src/avid_m/namespaced.rs

…spresso-sequencer into cl/newvid

…into cl/newvid

mrain added 4 commits February 11, 2025 12:05

ongoing work

5374644

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

f60162c

…into cl/newvid

ongoing fix

a079d7f

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

d14ecdb

…into cl/newvid

deepugami pushed a commit to deepugami/espresso-sequencer that referenced this pull request Feb 13, 2025

chore: drop varint (EspressoSystems#2579)

fefb60d

mrain added 3 commits February 18, 2025 12:11

lol it compiles

971d8c4

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

23152d4

…into cl/newvid

metadata for vid_commitment

34dba21

mrain added 12 commits February 19, 2025 12:19

shipping in vid crate

1c8ee77

fixed size ref

3b5c135

fmt & feature tune

fd1464d

cargo lock

c30d4f9

bug fix & fmt

841f2d0

bug fix and features

f4688cd

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

a46bbdd

…into cl/newvid

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

f5e3e71

…into cl/newvid

fix tests

a857a1a

fix sequencer test

83b4b0a

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

45976cf

…into cl/newvid

Merge branch 'main' into cl/newvid

f7c82f9

mrain marked this pull request as ready for review February 21, 2025 15:10

mrain requested review from philippecamacho, ImJeremyHe, sveitser, jbearer, tbro and imabdulbasit as code owners February 21, 2025 15:10

mrain requested review from lukaszrzasik and ss-es February 21, 2025 15:10

mrain requested a review from bfish713 February 21, 2025 15:34

mrain added 3 commits February 21, 2025 12:32

update for hang problem

02f8048

Merge branch 'cl/newvid' of github.com:EspressoSystems/espresso-seque…

4e39f6e

…ncer into cl/newvid

update ci

2ee0796

lukaszrzasik reviewed Feb 24, 2025

View reviewed changes

ss-es reviewed Feb 24, 2025

View reviewed changes

mrain requested a review from alxiong February 24, 2025 17:28

mrain added 3 commits February 24, 2025 16:31

addressing comments

015aa10

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

a2657f8

…into cl/newvid

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

33154f1

…into cl/newvid

alxiong reviewed Feb 25, 2025

View reviewed changes

imabdulbasit reviewed Feb 25, 2025

View reviewed changes

sequencer/src/persistence/sql.rs Show resolved Hide resolved

imabdulbasit reviewed Feb 25, 2025

View reviewed changes

hotshot-query-service/src/data_source/update.rs Show resolved Hide resolved

imabdulbasit reviewed Feb 25, 2025

View reviewed changes

hotshot-query-service/src/data_source/update.rs Show resolved Hide resolved

alxiong reviewed Feb 25, 2025

View reviewed changes

vid/src/avid_m/namespaced.rs Outdated Show resolved Hide resolved

vid/src/avid_m/namespaced.rs Show resolved Hide resolved

mrain added 3 commits February 25, 2025 10:56

tune Cargo.toml. rename interface for NsAvidMShare.

25c2965

Merge branches 'cl/newvid' and 'main' of github.com:EspressoSystems/e…

87dc2a1

…spresso-sequencer into cl/newvid

Merge branch 'main' into cl/newvid

91cfee7

alxiong mentioned this pull request Jan 15, 2025

[VID] update the logic for namespace proof verification #2458

Open

mrain requested review from lukaszrzasik, alxiong, imabdulbasit and ss-es February 26, 2025 14:12

bfish713 approved these changes Feb 26, 2025

View reviewed changes

Merge branch 'main' of github.com:EspressoSystems/espresso-sequencer …

e8e07f2

…into cl/newvid

mrain merged commit a312872 into main Feb 26, 2025
53 checks passed

mrain deleted the cl/newvid branch February 26, 2025 20:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[VID] AvidM integration #2579

[VID] AvidM integration #2579

mrain commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 19, 2025

lukaszrzasik left a comment

lukaszrzasik Feb 24, 2025

ss-es Feb 24, 2025

mrain Feb 24, 2025

ss-es Feb 24, 2025

lukaszrzasik Feb 24, 2025

ss-es Feb 24, 2025

mrain Feb 24, 2025

ss-es Feb 24, 2025

mrain Feb 24, 2025

ss-es Feb 24, 2025

ss-es Feb 24, 2025

ss-es Feb 24, 2025

mrain Feb 24, 2025

alxiong Feb 25, 2025

ss-es Feb 24, 2025

mrain Feb 24, 2025

alxiong left a comment

alxiong Feb 25, 2025

mrain Feb 25, 2025

ss-es Feb 26, 2025

mrain Feb 26, 2025

alxiong Feb 25, 2025

mrain Feb 25, 2025

alxiong Feb 25, 2025

		/// Helper function for AvidM scheme to parse a namespace table.
		/// If the namespace table is invalid, it returns a default single entry namespace table.

[VID] AvidM integration #2579

[VID] AvidM integration #2579

Conversation

mrain commented Feb 11, 2025 • edited Loading

This PR:

This PR does not:

Key places to review:

github-actions bot commented Feb 19, 2025

lukaszrzasik 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

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

alxiong 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

mrain commented Feb 11, 2025 •

edited

Loading