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

fix: bench: Set ticket and seed to a non-all zero value #11429

Merged
merged 1 commit into from
Nov 20, 2023

Conversation

rjan90
Copy link
Contributor

@rjan90 rjan90 commented Nov 17, 2023

Related Issues

The latest proofs-release expects the seed-value to be a non zero value. In the lotus-bench we currently had set this to all zeros, which causes the Commit2 bench to fail with:

./lotus-bench simple commit2 /home/misty/benchdir/c1.json
2023-11-16T17:51:54.256 DEBUG filcrypto::util::types > seal_commit_phase2: start
2023-11-16T17:51:54.257 INFO filecoin_proofs::api::seal > seal_commit_phase2:start: SectorId(1)
2023-11-16T17:51:54.257 DEBUG filcrypto::util::types > seal_commit_phase2: end
2023-11-16T17:51:54.257Z	WARN	lotus-bench	lotus-bench/main.go:129	Invalid porep challenge seed

(Ref: #11185 (comment))

Proposed Changes

This PR sets the ticket and seed value to a non zero value.

Testing

./lotus-bench simple commit2 --synthetic=true /Users/phi/lotus/c1.json
2023-11-17T15:17:58.138+0100	INFO	lotus-bench	lotus-bench/main.go:110	Starting lotus-bench
------
2023-11-17T15:18:14.299 INFO filecoin_proofs::api::seal > seal_commit_phase2:finish: SectorId(1)
2023-11-17T15:18:14.299 DEBUG filcrypto::util::types > seal_commit_phase2: end
Commit2: 14.127566709s (144 B/s)
proof: 903ed0fea2dd9fa1ca86219390970db2b1157274beacc5a8c29fe29cbc08749a5332392593c3fb701d60f0d17308ae4f978fa25efdf0b4842bb5a8dd8cb624227b7f66cc0366ca8368be08b684f413babe915857c8c0821caf9dd81ed0ac62c1044e5c17cdd44486aa1b5450f668d4118423bf286c8c64ff8007025624eb116a6e4e5c0449ea09fa1f6d668cd0a26c49a5fb6593b28922a84964c7951a6fd5597efd18ec9581fba0983c78c7bdca2f0c8b945010e6109217c066fb3ef1edd876

Checklist

Before you mark the PR ready for review, please make sure that:

  • Commits have a clear commit message.
  • PR title is in the form of of <PR type>: <area>: <change being made>
    • example: fix: mempool: Introduce a cache for valid signatures
    • PR type: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, test
    • area, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps
  • If the PR affects users (e.g., new feature, bug fix, system requirements change), update the CHANGELOG.md and add details to the UNRELEASED section.
  • New features have usage guidelines and / or documentation updates in
  • Tests exist for new functionality or change in behavior
  • CI is green

Set ticket and seed to a non-all zero value
@@ -266,7 +266,10 @@ var simplePreCommit1 = &cli.Command{
ProofType: spt(sectorSize, cctx.Bool("synthetic")),
}

var ticket [32]byte // all zero
ticket := [32]byte{}
Copy link
Contributor

Choose a reason for hiding this comment

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

It might make sense that the ticket could be any arbitrary value, except all zeros.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Like a user-defined value? Or just any random arbitrary value?

Copy link
Contributor

Choose a reason for hiding this comment

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

From a proofs perspective it could be anything, hence a random value or something like all 1s would be fine. IIRC the value for the ticket comes from Drand when the code is normally run.

Copy link
Contributor

@magik6k magik6k left a comment

Choose a reason for hiding this comment

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

Probably fine to merge if in makes things work, we can allow user-provided/more elaborate values in a separate PR

@rjan90 rjan90 merged commit 59873c7 into master Nov 20, 2023
87 checks passed
@rjan90 rjan90 deleted the phi-fix-seedticket-bench branch November 20, 2023 11:10
@vmx
Copy link
Contributor

vmx commented Nov 20, 2023

Probably fine to merge if in makes things work, we can allow user-provided/more elaborate values in a separate PR

Oh, I misunderstood @rjan90 question then. For this purpose I would just hard-code the value (as it currently is), but as mentioned leave a comment for clarity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants