Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

feat: improve block hashes fetching #62

Closed
wants to merge 16 commits into from

Conversation

vladimir-trifonov
Copy link
Contributor

feat: improve block hashes fetching

Issue: 0xPolygonZero/zk_evm#297

leader/src/cli.rs Outdated Show resolved Hide resolved
leader/src/jerigon.rs Outdated Show resolved Hide resolved
tools/prove_blocks.sh Show resolved Hide resolved
leader/src/cli.rs Show resolved Hide resolved
leader/src/cli.rs Outdated Show resolved Hide resolved
rpc/src/rpc.rs Outdated Show resolved Hide resolved
if [ $retVal -ne 0 ]; then
# Some error occured.
echo "Blocks $1..=$2 errored. See ${OUT_LOG_PATH} for more details."
exit $retVal
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we shouldn't abort early in case we're proving a range. The error log should be enough while we pursue proving other blocks.

Copy link
Contributor Author

@vladimir-trifonov vladimir-trifonov Apr 24, 2024

Choose a reason for hiding this comment

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

I don't think it is feasible with the current implementation of jerigon_main since it exits when block proving fails.

Copy link
Collaborator

@Nashtare Nashtare left a comment

Choose a reason for hiding this comment

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

LGTM!

@vladimir-trifonov
Copy link
Contributor Author

@praetoriansentry simple_test.sh is failing for this PR, do you know what could be the reason... I try to run it locally and witness.json.bz2 has zero size.

@praetoriansentry
Copy link
Contributor

@praetoriansentry simple_test.sh is failing for this PR, do you know what could be the reason... I try to run it locally and witness.json.bz2 has zero size.

I would guess there was some issue with pulling the witness temparily. I re-ran and it seems ok for now:
image

@Nashtare
Copy link
Collaborator

Curious about the double log Proof verified successfully 👀 (though orthogonal to this PR)

@vladimir-trifonov
Copy link
Contributor Author

Curious about the double log Proof verified successfully 👀 (though orthogonal to this PR)

can't make it pass locally, maybe because of mac m1, and figure it out why is that :(

@Nashtare
Copy link
Collaborator

@vladimir-trifonov I think you just need to change nproc, it's not supported on Apple chips (cf my comment in #69)

@vladimir-trifonov
Copy link
Contributor Author

@vladimir-trifonov I think you just need to change nproc, it's not supported on Apple chips (cf my comment in #69)

nproc returns 8 on my Apple M1 machine, so I think it works fine, but the simple_test.sh is still failing locally.

@Nashtare
Copy link
Collaborator

Nashtare commented May 7, 2024

What's your error?

@vladimir-trifonov
Copy link
Contributor Author

What's your error?

 ./simple_test.sh                        
Pulling sample witness

bunzip2: Compressed file ends unexpectedly;
        perhaps it is corrupted?  *Possible* reason follows.
bunzip2: No such file or directory
        Input file = witness.json.bz2, output file = witness.json

It is possible that the compressed file(s) have become corrupted.
You can use the -tvv option to test integrity of such files.

You can use the `bzip2recover' program to attempt to recover
data from undamaged sections of corrupted files.

bunzip2: Deleting output file witness.json, if it exists.
   Compiling rpc v0.1.0 (/Users/vladimirtrifonov/src/rnd/polygon-zkevm/zero-bin/rpc)
   Compiling leader v0.1.0 (/Users/vladimirtrifonov/src/rnd/polygon-zkevm/zero-bin/leader)
    Finished release [optimized] target(s) in 2m 47s
./simple_test.sh: line 63: witness.json: No such file or directory
Error: EOF while parsing a value at line 1 column 0

Stack backtrace:
   0: std::backtrace::Backtrace::capture
   1: verifier::main
   2: std::sys_common::backtrace::__rust_begin_short_backtrace
   3: _main
there was an issue with proof verification

it looks like it is mac specific, or the way it saves the data and reads it, I'm looking in it...

@vladimir-trifonov
Copy link
Contributor Author

@Nashtare I managed to run it locally and this is the output:

2024-05-08T10:54:35.809263Z  INFO common::prover_state: initializing verifier state...
2024-05-08T10:54:35.809281Z  INFO common::prover_state: attempting to load preprocessed verifier circuit from disk...
2024-05-08T10:54:35.809677Z  INFO common::prover_state: successfully loaded preprocessed verifier circuit from disk
2024-05-08T10:54:35.814027Z  INFO verifier: Proof verified successfully!

INFO verifier: Proof verified successfully! exists just once...

@Nashtare
Copy link
Collaborator

Nashtare commented May 8, 2024

INFO verifier: Proof verified successfully! exists just once...

What do you mean by this? The output looks fine to me

@vladimir-trifonov
Copy link
Contributor Author

INFO verifier: Proof verified successfully! exists just once...

What do you mean by this? The output looks fine to me

I meant that because of:

Curious about the double log Proof verified successfully 👀 (though orthogonal to this PR)

I saw that it is doubled in the github output, but locally it is not.

@Nashtare
Copy link
Collaborator

Nashtare commented May 8, 2024

Ah sorry I had forgotten about this old comment. Hmm yeah maybe logging is a bit messed up on the CI. Not a big deal probably!

@vladimir-trifonov
Copy link
Contributor Author

vladimir-trifonov commented May 31, 2024

@Nashtare @BGluth do you guys think this PR is still relevant, after looking into #78? cc: @atanmarko

@atanmarko
Copy link
Member

atanmarko commented May 31, 2024

@Nashtare @BGluth do you guys think this PR is still relevant, after looking into #78? cc: @atanmarko

CC: @muursh, @cpubot

Also taking into account #86 maybe we need more general approach to block ranges

@vgnom vgnom added this to the Cancun - Q2 2024 milestone Jun 11, 2024
@atanmarko
Copy link
Member

atanmarko commented Jun 13, 2024

Multi block proving is covered with #90. Block hash retrieval optimization needs to be ported to alloy

@Nashtare
Copy link
Collaborator

Nashtare commented Jun 13, 2024

EDIT: I wrote this while you were creating the issue, all good then :)

@atanmarko The issue is different here. It was about reducing the number of RPC calls to get the 256 previous hashes needed for a block payload. This isn't tackled by #90, which calls block_prover_input() for every single block in the sequence, while once we fetched the initial 256 previous block hashes, we only need one more RPC call per following block, instead of the current 128. But this is a minor optimization.

@atanmarko
Copy link
Member

atanmarko commented Jun 13, 2024

EDIT: I wrote this while you were creating the issue, all good then :)

@atanmarko The issue is different here. It was about reducing the number of RPC calls to get the 256 previous hashes needed for a block payload. This isn't tackled by #90, which calls block_prover_input() for every single block in the sequence, while once we fetched the initial 256 previous block hashes, we only need one more RPC call per following block, instead of the current 128. But this is a minor optimization.

@Nashtare Right, this PR also introduced block ranges, that part has different implementation with #90. Hash retrial optimization is yet to be implemented(ported)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Improve block hashes fetching
7 participants