Skip to content
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.

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

[SMST] feat: Use compact SMST proofs #823

Merged
merged 11 commits into from
Sep 24, 2024
Merged

[SMST] feat: Use compact SMST proofs #823

merged 11 commits into from
Sep 24, 2024

Conversation

red-0ne
Copy link
Contributor

@red-0ne red-0ne commented Sep 12, 2024

Summary

This PR changes the proofs being sent in MsgSubmitPRoof and stored by the SubmitProof keeper from SparseMerkleClosestProof to SparseCompactMerkleClosestProof.

Issue

Proofs one of the most block-space consuming primitives. The SMST support compact proofs that could help reduce their sizes.

Type of change

Select one or more from the following:

Testing

  • Unit Tests: make go_develop_and_test
  • LocalNet E2E Tests: make test_e2e
  • DevNet E2E Tests: Add the devnet-test-e2e label to the PR.

Sanity Checklist

  • I have tested my changes using the available tooling
  • I have commented my code
  • I have performed a self-review of my own code; both comments & source code

@red-0ne red-0ne added off-chain Off-chain business logic on-chain On-chain business logic smt Sprase Merkle Tree Related proof Claim & Proof life cycle consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. labels Sep 12, 2024
@red-0ne red-0ne added this to the Shannon Beta TestNet Launch milestone Sep 12, 2024
@red-0ne red-0ne self-assigned this Sep 12, 2024
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazed that everyone just worked!

Any chance you can add a log or a unit test to show the difference in size?

I'm REALLy curious and can be done in a followup.

@red-0ne
Copy link
Contributor Author

red-0ne commented Sep 20, 2024

Any chance you can add a log or a unit test to show the difference in size?

@Olshansk, I'm really grateful that you called this out.

I added this test /pkg/relayer/session/sessiontree_test.go

Which shows that there is barely any improvement in the proof size 😢

I added a (gzip) compressed proofs to the test to have something to compare with.

Running for different leaf sizes, 1000 run each and accumulating size, this a sample result

numLeaf=10: compactionRatio: 0.990917, compressionRatio: 0.804937
numLeaf=100: compactionRatio: 0.989091, compressionRatio: 0.803551
numLeaf=1000: compactionRatio: 0.992390, compressionRatio: 0.805643
numLeaf=10000: compactionRatio: 0.989311, compressionRatio: 0.803930
numLeaf=100000: compactionRatio: 0.992038, compressionRatio: 0.805471
numLeaf=1000000: compactionRatio: 0.990228, compressionRatio: 0.804995

pkg/relayer/session/sessiontree_test.go Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
@Olshansk
Copy link
Member

@red-0ne Thanks for putting in the extra test. This is fine and what I expected.

Want to use this as a learning lesson. Nothing we don't already know but something I want to reiterate:

  1. Compact proof sounds sexy, eh? This is a classic example of "sounds sexy to an engineer" but not actually useful.
    • Probabilistic proof is what will actually ensure scalability
  2. Understanding what's happening under the hood
    • We inherited "Compact Proofs" from the library we forked
    • I didn't implement it, neither did Harry, neither did you
    • None of us looked in the source code
    • I'm 100% sure we can make it more efficient, but again, it "sounds sexy", right?
  3. The importance of visibility / observability
    • This doesn't always need to be a dashboard
    • It doesn't always need to be telemetry
    • It could be a log or a unit test
    • This is a great example of using t.Log
    • This raises the importance of a dashboard for probabilistic proofs

Copy link

The CI will now also run the e2e tests on devnet, which increases the time it takes to complete all CI checks.

You may need to run make trigger_ci to submit an empty commit that'll trigger the tests.

GCP workloads (requires changing the namespace to 823)
Grafana network dashboard for devnet-issue-823

@github-actions github-actions bot added devnet push-image CI related - pushes images to ghcr.io labels Sep 23, 2024
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
x/proof/keeper/proof_validation_test.go Outdated Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
pkg/relayer/session/sessiontree_test.go Outdated Show resolved Hide resolved
@red-0ne red-0ne merged commit 6ed08b4 into main Sep 24, 2024
10 checks passed
bryanchriswhite added a commit that referenced this pull request Sep 24, 2024
…rge/integration-app_x_sup-stake-evts

* issues/799/refactor/integration-app:
  chore: review feedback improvements
  [SMST] feat: Use compact SMST proofs (#823)
  [SessionManager] Skip claims creation if supplier operator balance is too low (#817)
  chore: self-review improvements
bryanchriswhite added a commit that referenced this pull request Sep 24, 2024
… issues/799/tests/params

* issues/799/merge/integration-app_x_sup-stake-evts:
  chore: review feedback improvements
  [SMST] feat: Use compact SMST proofs (#823)
  [SessionManager] Skip claims creation if supplier operator balance is too low (#817)
  chore: self-review improvements

# Conflicts:
#	tests/integration/tokenomics/relay_mining_integration_test.go
bryanchriswhite added a commit that referenced this pull request Sep 24, 2024
…rge/integration-app_x_transfer-period-base

* issues/799/refactor/integration-app:
  fixup! HEAD^
  fix: linter errors
  chore: review feedback improvements
  [SMST] feat: Use compact SMST proofs (#823)
  [SessionManager] Skip claims creation if supplier operator balance is too low (#817)
  chore: self-review improvements
  [Code Health] refactor: rename `ApplicationTransfer` msgs (#788)
  [Docs] Add operations documentation about proof submission fee (#806)
  [Testing] Fix non-idempotency in (and speed up) supplier staking tests (#815)
bryanchriswhite added a commit that referenced this pull request Sep 24, 2024
…' into issues/657/chore/app-transfer-period

* issues/657/merge/integration-app_x_transfer-period-base:
  fixup! HEAD^
  fix: linter errors
  chore: review feedback improvements
  [SMST] feat: Use compact SMST proofs (#823)
  [SessionManager] Skip claims creation if supplier operator balance is too low (#817)
  chore: self-review improvements
  [Code Health] refactor: rename `ApplicationTransfer` msgs (#788)
  [Docs] Add operations documentation about proof submission fee (#806)
  [Testing] Fix non-idempotency in (and speed up) supplier staking tests (#815)
@h5law
Copy link
Contributor

h5law commented Nov 7, 2024

@red-0ne Thanks for putting in the extra test. This is fine and what I expected.

Want to use this as a learning lesson. Nothing we don't already know but something I want to reiterate:

  1. Compact proof sounds sexy, eh? This is a classic example of "sounds sexy to an engineer" but not actually useful.

    • Probabilistic proof is what will actually ensure scalability
  2. Understanding what's happening under the hood

    • We inherited "Compact Proofs" from the library we forked
    • I didn't implement it, neither did Harry, neither did you
    • None of us looked in the source code
    • I'm 100% sure we can make it more efficient, but again, it "sounds sexy", right?
  3. The importance of visibility / observability

    • This doesn't always need to be a dashboard
    • It doesn't always need to be telemetry
    • It could be a log or a unit test
    • This is a great example of using t.Log
    • This raises the importance of a dashboard for probabilistic proofs

Little late to this but just going through my notifications. I actually did implement the CompactClosestProof type to fit the pattern with the other proof types. Had some brief ideas about how they could be compacted properly (was in a rush when implementing this):

  1. Clip the Path field to Depth bits
  2. Replace the FlippedBits byte matrix with a bit mask where 1s represent bits flipped, the [][]byte can be replaced with uint{Depth} as each bit represents an index (up to 4 uint64s for a full 256-bit worst case encoding)
  3. Instead of encoding the Depth as the []byte form of the original int use a single byte (this assumes a 256 bit hasher or figure out a 9-bit type to generalise and include 512-bit hashers)

Realisticallly this method is super optimisable, I didn't really try to optimise it as it was always don't pre-optimise. I'm sure you guys will think of even more, actually doing compression on the encoded bits would be a interesting idea once they are encoded better.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus-breaking IMPORTANT! If the PR with this tag is merged, next release WILL HAVE TO BE an upgrade. devnet devnet-test-e2e off-chain Off-chain business logic on-chain On-chain business logic proof Claim & Proof life cycle push-image CI related - pushes images to ghcr.io smt Sprase Merkle Tree Related
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants