New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

removes the merkle root from shreds binary #29427

Merged

behzadnouri merged 1 commit into solana-labs:master from behzadnouri:rm-merkle-root

Feb 15, 2023

Contributor

behzadnouri commented Dec 28, 2022 •

edited

Loading

Problem

The merkle root is unnecessary encoded into shreds binary, reducing shreds capacity to store data.
The root is 32 bytes hash, but since the merkle root is truncated, the signature is on the truncated 20 bytes.

Summary of Changes

#29445 makes it unnecessary to embed merkle roots into shreds binary. This commit removes the merkle root from shreds binary.

This adds 20 bytes to shreds capacity to store more data.
Additionally since we no longer need to truncate the merkle root, the signature would be on the full 32 bytes of hash as opposed to the truncated one.

Also signature verification would now effectively verify merkle proof as well, so we no longer need to verify merkle proof in the sanitize implementation.

behzadnouri force-pushed the rm-merkle-root branch 29 times, most recently from f639e75 to 49c523d Compare

December 30, 2022 17:08

behzadnouri changed the title ~~removes merkle root from shred binary~~ removes merkle root from shreds binary

behzadnouri force-pushed the rm-merkle-root branch 5 times, most recently from 6c1626f to 346be25 Compare

January 5, 2023 17:18

jbiseda previously approved these changes

View reviewed changes

Contributor

jbiseda left a comment

LGTM.

I think it may make it easier to read if you expose shred::OFFSET_OF_SHRED_VARIANT and use that in place of SIZE_OF_SIGNATURE.

ledger/src/shred/merkle.rs

+                      let index = self.erasure_shard_index()?;
+                      let proof_offset = Self::proof_offset(proof_size)?;
+                      let proof = get_merkle_proof(&self.payload, proof_offset, proof_size)?;
+                      let node = get_merkle_node(&self.payload, SIZE_OF_SIGNATURE..proof_offset)?;

Contributor

jbiseda Jan 6, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/ for clarity?

Contributor Author

behzadnouri Jan 8, 2023

#29427 (comment)

ledger/src/shred/merkle.rs Outdated

-                      Ok(verify_merkle_proof(index, node, &self.merkle_branch()?))
+                  fn merkle_node(&self) -> Result<Hash, Error> {
+                      let proof_size = self.proof_size()?;
+                      let offsets = SIZE_OF_SIGNATURE..Self::proof_offset(proof_size)?;

Contributor

jbiseda Jan 6, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/

Contributor Author

behzadnouri Jan 8, 2023

#29427 (comment)

ledger/src/shred/merkle.rs

-                      get_merkle_root(shred, proof_size, index, offset)
+                      let proof_offset = Self::proof_offset(proof_size).ok()?;
+                      let proof = get_merkle_proof(shred, proof_offset, proof_size).ok()?;
+                      let node = get_merkle_node(shred, SIZE_OF_SIGNATURE..proof_offset).ok()?;

Contributor

jbiseda Jan 7, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/

Contributor Author

behzadnouri Jan 8, 2023

#29427 (comment)

ledger/src/shred/merkle.rs

-                      Ok(<&MerkleRoot>::try_from(root).unwrap())
+                  // Where the merkle proof starts in the shred binary.
+                  fn proof_offset(proof_size: u8) -> Result<usize, Error> {
+                      Ok(Self::SIZE_OF_HEADERS + Self::capacity(proof_size)?)

Contributor

jbiseda Jan 7, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/

Contributor Author

behzadnouri Jan 8, 2023

I don't see a SIZE_OF_SIGNATURE here.

ledger/src/shred/merkle.rs

+                      let index = self.erasure_shard_index()?;
+                      let proof_offset = Self::proof_offset(proof_size)?;
+                      let proof = get_merkle_proof(&self.payload, proof_offset, proof_size)?;
+                      let node = get_merkle_node(&self.payload, SIZE_OF_SIGNATURE..proof_offset)?;

Contributor

jbiseda Jan 7, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/

Contributor Author

behzadnouri Jan 8, 2023

#29427 (comment)

ledger/src/shred/merkle.rs Outdated

-                      Some(offset..offset + SIZE_OF_MERKLE_ROOT)
+                  fn merkle_node(&self) -> Result<Hash, Error> {
+                      let proof_size = self.proof_size()?;
+                      let offsets = SIZE_OF_SIGNATURE..Self::proof_offset(proof_size)?;

Contributor

jbiseda Jan 7, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/

Contributor Author

behzadnouri Jan 8, 2023

#29427 (comment)

ledger/src/shred/merkle.rs

-                      get_merkle_root(shred, proof_size, index, offset)
+                      let proof_offset = Self::proof_offset(proof_size).ok()?;
+                      let proof = get_merkle_proof(shred, proof_offset, proof_size).ok()?;
+                      let node = get_merkle_node(shred, SIZE_OF_SIGNATURE..proof_offset).ok()?;

Contributor

jbiseda Jan 7, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/

Contributor Author

behzadnouri Jan 8, 2023

#29427 (comment)

ledger/src/shred/merkle.rs

                       self.payload
-                          .get(SIZE_OF_SIGNATURE..Self::SIZE_OF_HEADERS + data_buffer_size)
+                          .get(SIZE_OF_SIGNATURE..proof_offset)

Contributor

jbiseda Jan 7, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/

Contributor Author

behzadnouri Jan 8, 2023

#29427 (comment)

ledger/src/shred/merkle.rs

                       self.payload
-                          .get(Self::SIZE_OF_HEADERS..Self::SIZE_OF_HEADERS + shard_size)
+                          .get(Self::SIZE_OF_HEADERS..proof_offset)

Contributor

jbiseda Jan 7, 2023

s/SIZE_OF_SIGNATURE/OFFSET_OF_SHRED_VARIANT/

Contributor Author

behzadnouri Jan 8, 2023

I don't see a SIZE_OF_SIGNATURE here.

Contributor Author

behzadnouri commented Jan 8, 2023

I think it may make it easier to read if you expose shred::OFFSET_OF_SHRED_VARIANT and use that in place of SIZE_OF_SIGNATURE.

SIZE_OF_SIGNATURE.. indicates that we are skipping over the signature bytes, because only once we have the erasure coding shards and the merkle tree is constructed, we can sign the merkle root and attach the signature. So the signature bytes won't be included in either erasure coding or merkle tree construction and so are skipped over in all these offsets.
That is also consistent with merkle layout description in the comments which states that "The slice past signature and before merkle branch" is erasure coded or used in merkle tree construction:
https://github.com/solana-labs/solana/blob/8c212f59a/ledger/src/shred/merkle.rs#L52-L55
https://github.com/solana-labs/solana/blob/8c212f59a/ledger/src/shred/merkle.rs#L63-L65

I believe that skipping over signature bytes is better indicated in the code by SIZE_OF_SIGNATURE...
Changing this to OFFSET_OF_SHRED_VARIANT would be less clear why we start off the shred-variant offset.

behzadnouri force-pushed the rm-merkle-root branch from 346be25 to ef9815c Compare

January 8, 2023 16:26

mergify bot dismissed jbiseda’s stale review

January 8, 2023 16:27

Pull request has been modified.

Contributor

jbiseda commented Jan 9, 2023

I believe that skipping over signature bytes is better indicated in the code by SIZE_OF_SIGNATURE... Changing this to OFFSET_OF_SHRED_VARIANT would be less clear why we start off the shred-variant offset.

Ok, I'm fine with SIZE_OF_SIGNATURE.

This was referenced Jan 14, 2023

adds feature to (temporarily) drop merkle shreds from testnet #29711

Merged

Feature Gate: drop merkle shreds from testnet #29712

Closed

Feature Gate: drop merkle shreds from testnet #29713

Closed

behzadnouri added the do-not-close label


          removes the merkle root from shreds binary

75b0430

solana-labs#29445
makes it unnecessary to embed merkle roots into shreds binary. This
commit removes the merkle root from shreds binary.

This adds 20 bytes to shreds capacity to store more data.
Additionally since we no longer need to truncate the merkle root, the
signature would be on the full 32 bytes of hash as opposed to the
truncated one.

Also signature verification would now effectively verify merkle proof as
well, so we no longer need to verify merkle proof in the sanitize
implementation.

behzadnouri force-pushed the rm-merkle-root branch from ef9815c to 75b0430 Compare

February 14, 2023 14:38

behzadnouri requested a review from jbiseda

February 14, 2023 14:55

jbiseda approved these changes

View reviewed changes

behzadnouri merged commit cf0a149 into solana-labs:master

behzadnouri deleted the rm-merkle-root branch

February 15, 2023 21:17

behzadnouri added v1.15 (abandoned) and removed do-not-close labels

mergify bot mentioned this pull request

v1.15: removes the merkle root from shreds binary (backport of #29427) #30344

Merged

mergify bot pushed a commit that referenced this pull request


          removes the merkle root from shreds binary (#29427)

01c166d

#29445
makes it unnecessary to embed merkle roots into shreds binary. This
commit removes the merkle root from shreds binary.

This adds 20 bytes to shreds capacity to store more data.
Additionally since we no longer need to truncate the merkle root, the
signature would be on the full 32 bytes of hash as opposed to the
truncated one.

Also signature verification would now effectively verify merkle proof as
well, so we no longer need to verify merkle proof in the sanitize
implementation.

(cherry picked from commit cf0a149)

mergify bot added a commit that referenced this pull request


          v1.15: removes the merkle root from shreds binary (backport of #29427) (

dd9a7c6

#30344)

removes the merkle root from shreds binary (#29427)

#29445
makes it unnecessary to embed merkle roots into shreds binary. This
commit removes the merkle root from shreds binary.

This adds 20 bytes to shreds capacity to store more data.
Additionally since we no longer need to truncate the merkle root, the
signature would be on the full 32 bytes of hash as opposed to the
truncated one.

Also signature verification would now effectively verify merkle proof as
well, so we no longer need to verify merkle proof in the sanitize
implementation.

(cherry picked from commit cf0a149)

Co-authored-by: behzad nouri <[email protected]>

behzadnouri added the v1.14 label

mergify bot pushed a commit that referenced this pull request


          removes the merkle root from shreds binary (#29427)

7add127

#29445
makes it unnecessary to embed merkle roots into shreds binary. This
commit removes the merkle root from shreds binary.

This adds 20 bytes to shreds capacity to store more data.
Additionally since we no longer need to truncate the merkle root, the
signature would be on the full 32 bytes of hash as opposed to the
truncated one.

Also signature verification would now effectively verify merkle proof as
well, so we no longer need to verify merkle proof in the sanitize
implementation.

(cherry picked from commit cf0a149)

# Conflicts:
#	ledger/src/shred.rs

mergify bot mentioned this pull request

v1.14: removes the merkle root from shreds binary (backport of #29427) #30613

Merged

behzadnouri added a commit that referenced this pull request


          v1.14: removes the merkle root from shreds binary (backport of #29427) (

8c6b6c1

#30613)

* removes the merkle root from shreds binary (#29427)

#29445
makes it unnecessary to embed merkle roots into shreds binary. This
commit removes the merkle root from shreds binary.

This adds 20 bytes to shreds capacity to store more data.
Additionally since we no longer need to truncate the merkle root, the
signature would be on the full 32 bytes of hash as opposed to the
truncated one.

Also signature verification would now effectively verify merkle proof as
well, so we no longer need to verify merkle proof in the sanitize
implementation.

(cherry picked from commit cf0a149)

# Conflicts:
#	ledger/src/shred.rs

* resolves mergify merge conflicts

---------

Co-authored-by: behzad nouri <[email protected]>

nickfrosty pushed a commit to nickfrosty/solana that referenced this pull request


          removes the merkle root from shreds binary (solana-labs#29427)

39b5225

solana-labs#29445
makes it unnecessary to embed merkle roots into shreds binary. This
commit removes the merkle root from shreds binary.

This adds 20 bytes to shreds capacity to store more data.
Additionally since we no longer need to truncate the merkle root, the
signature would be on the full 32 bytes of hash as opposed to the
truncated one.

Also signature verification would now effectively verify merkle proof as
well, so we no longer need to verify merkle proof in the sanitize
implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v1.15 (abandoned)