-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat: worker: Support delegating precommit2 to external binary #11185
Conversation
8b9c17a
to
8c5a5d0
Compare
SupraSeal command looks something like this:
|
sn-pc2 testing with lotus-bench:
So it looks like we either need to come up with a way to create t_aux in Go, or make it be created by sn-pc2. Checking that output matches rust pc2:
Which means we're getting the correct sealed output! Now we just need to figure out the t_aux stuff, and this PR should be good to go! |
triplewz/poseidon#1 got merged! => no more fork is needed :) |
The PC2 binary mentioned in that PR is kind of outdated. Supranational releases a new one at: Currently it only supports 512MiB and 32GiB and it has to be set at compile time. But making it possible to change at runtime is in the works (see supranational/supra_seal#23 (comment)) and should hopefully land soon. |
@vmx @magik6k Support additional sector sizes has been merged. |
@magik6k The proofs side of things sadly still is not ready yet. Though Lotus would want to switch to the binary at https://github.com/supranational/supra_seal/blob/c2ff1acf282dad86812c62f5f431db545d966743/tools/pc2.cu. So perhaps that work can be done in parallel, while the Rust side gets finished. |
@magik6k In order to use proofs and not need a t_aux file use this PR filecoin-project/filecoin-ffi#434 and set the |
Please be aware that not generating t_aux may break backwards compatibility with other remote sealing workers later during sealing process. Sector transfers may also fail if t_aux is missing. |
Yes. Everything would need to be on the same version. |
@rjan90 here's a simpler build script for building the PC2 binary, which doesn't need a SPDK checkout: #!/bin/sh
set -eu
#set -o xtrace
# By default compile for 512MiB and 32GiB sectors only, use `-r` to compile for
# other sector test sector sizes as well.
SECTOR_SIZE=""
while getopts r flag
do
case "${flag}" in
r) SECTOR_SIZE="-DRUNTIME_SECTOR_SIZE";;
*) ;;
esac
done
if [ ! -d "supra_seal" ]; then
git clone https://github.com/supranational/supra_seal.git
fi
cd supra_seal
rm -fr obj
mkdir -p obj
rm -fr bin
mkdir -p bin
mkdir -p deps
if [ ! -d "deps/sppark" ]; then
git clone https://github.com/supranational/sppark.git deps/sppark
fi
if [ ! -d "deps/blst" ]; then
git clone https://github.com/supranational/blst.git deps/blst
(cd deps/blst
./build.sh -march=native)
fi
# Generate .h files for the Poseidon constants
xxd -i poseidon/constants/constants_2 > obj/constants_2.h
xxd -i poseidon/constants/constants_4 > obj/constants_4.h
xxd -i poseidon/constants/constants_8 > obj/constants_8.h
xxd -i poseidon/constants/constants_11 > obj/constants_11.h
xxd -i poseidon/constants/constants_16 > obj/constants_16.h
xxd -i poseidon/constants/constants_24 > obj/constants_24.h
xxd -i poseidon/constants/constants_36 > obj/constants_36.h
nvcc ${SECTOR_SIZE} -DNO_SPDK -DSTREAMING_NODE_READER_FILES \
-arch=sm_80 -gencode arch=compute_70,code=sm_70 -t0 \
-std=c++17 -g -O3 -Xcompiler -march=native \
-Xcompiler -Wall,-Wextra,-Werror \
-Xcompiler -Wno-subobject-linkage,-Wno-unused-parameter \
-x cu tools/pc2.cu -o bin/pc2 \
-Iposeidon -Ideps/sppark -Ideps/sppark/util -Ideps/blst/src -L deps/blst -lblst -lconfig++ It also checks out the supra_seal repo. For all those git checkouts it would make sense to checkout a certain commit/tag instead of the main branches, so that it doesn't change/break in unexpected ways. |
Got the SupraSeal PC2 to work with. The testing that I did:
I got this on the C2-part of the bench though, but unclear if this is the just the lotus-bench code being weird.
^^ Update on the C2-error here. Which was fixed with: #11429 |
Some testing which would be nice to do to gain confidence: On a devnet or calib:
|
Testing in Butterfly-network:
|
With default values it's expected to work. But if you set |
I think the testing here is more targeted towards current storage providers that has been running on mainnet without setting any So just to be entirely sure, that is expected to work right @vmx ? |
Yes that is expected to work. |
Ideally they just run with |
All the test cases has been completed now.. There are some unreliabilites when doing deal sectors calling the SupraSeal PC2 binary through the lotus-worker, but those can be investigated and fixed in a subsequent PR, and should not block this PR from landing. Running with/and without FFI_USE_FIXED_ROWS_TO_DISCARD=1 & the new proofs release is working well across the range of sealing-types and all sectors that has been sealed either with or without it are able to PoSt, and SupraSeal PC2 works well when sealing CC-sectors (both SynthPoRep and non-SynthPoRep) |
Related Issues
This PR implements support for external PC2 binaries #10983
Proposed Changes
lotus-worker run
andlotus-bench simple precommit2
Additional Info
The flag
Can be tested in
lotus-bench
like so:TODO:
lotus-worker
with PC2 backed bylotus-bench simple
lotus-bench
lotus-worker
Checklist
Before you mark the PR ready for review, please make sure that:
<PR type>: <area>: <change being made>
fix: mempool: Introduce a cache for valid signatures
PR type
: fix, feat, build, chore, ci, docs, perf, refactor, revert, style, testarea
, e.g. api, chain, state, market, mempool, multisig, networking, paych, proving, sealing, wallet, deps