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

feat: block interval and parallel block proving #90

Merged
merged 17 commits into from
Jun 10, 2024

Conversation

atanmarko
Copy link
Member

@atanmarko atanmarko commented Jun 3, 2024

Resolves #87
Resolves #88
Resolves #89

@atanmarko atanmarko self-assigned this Jun 3, 2024
@atanmarko atanmarko force-pushed the feat/block-interval branch 5 times, most recently from 9197924 to d260a41 Compare June 3, 2024 16:51
@atanmarko atanmarko force-pushed the feat/block-interval branch 2 times, most recently from 6f5d0e7 to bc2ff6e Compare June 5, 2024 12:27
@atanmarko atanmarko changed the title WIP: block interval feat: block interval Jun 5, 2024
@atanmarko atanmarko marked this pull request as ready for review June 5, 2024 13:01
@atanmarko atanmarko requested a review from 0xaatif June 5, 2024 14:00
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.

First pass looking good, though I'll probably do a second round later. Also leaving the async stuff and related TODO questions to people more familiar (@0xaatif @cpubot)

common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
leader/src/jerigon.rs Outdated Show resolved Hide resolved
leader/src/main.rs Outdated Show resolved Hide resolved
prover/src/lib.rs Outdated Show resolved Hide resolved
tools/prove_blocks.sh Show resolved Hide resolved
tools/prove_blocks.sh Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
prover/src/lib.rs Outdated Show resolved Hide resolved
prover/src/lib.rs Outdated Show resolved Hide resolved
leader/src/jerigon.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/parsing.rs Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
tools/witness.json Outdated Show resolved Hide resolved
@atanmarko atanmarko requested review from cpubot and Nashtare June 6, 2024 11:06
Copy link
Contributor

@0xaatif 0xaatif left a comment

Choose a reason for hiding this comment

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

nits

Cargo.toml Outdated Show resolved Hide resolved
common/src/block_interval.rs Outdated Show resolved Hide resolved
common/src/parsing.rs Outdated Show resolved Hide resolved
cpubot and others added 3 commits June 6, 2024 16:11
* feat: implement concurrent block proving #88 #89

* Update prover/src/lib.rs

Co-authored-by: 0xaatif <169152398+0xaatif@users.noreply.github.com>

* fix suggestion

* Fix duplicated import when #[cfg(feature = "test_only")]

---------

Co-authored-by: 0xaatif <169152398+0xaatif@users.noreply.github.com>
Co-authored-by: Marko Atanasievski <markoatana@gmail.com>
common/src/block_interval.rs Outdated Show resolved Hide resolved
@atanmarko atanmarko changed the title feat: block interval feat: block interval and parallel block proving Jun 7, 2024
@atanmarko atanmarko requested review from muursh and 0xaatif June 7, 2024 09:43
} => Ok(try_stream! {
let mut current = start_block;
loop {
let last_block_number = provider.get_block_number().await.map_err(|e: alloy::transports::RpcError<_>| {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised you can't elide the type. Also .context could work here

Copy link
Member Author

Choose a reason for hiding this comment

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

Compiler insisted for some reason

current += 1;
yield current;
} else {
info!("Waiting for the new blocks to be mined, requested block number: {current}, \
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: I prefer structured logging for stats

}
}
}),
_ => Err(anyhow!(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: could be bail!

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.

Out of curiosity, have we tested the new behavior against actual multiple blocks? To make sure workers are picking up jobs properly, there's no weird synchronicity issues, we don't get overlasting delays on the first blocks upon increased work pressure, etc...

@atanmarko
Copy link
Member Author

Out of curiosity, have we tested the new behavior against actual multiple blocks? To make sure workers are picking up jobs properly, there's no weird synchronicity issues, we don't get overlasting delays on the first blocks upon increased work pressure, etc...

@Nashtare Yes, I did tests with 5 blocks range, both test_only and real mode. Seems to work without issues

@muursh
Copy link
Contributor

muursh commented Jun 7, 2024

Out of curiosity, have we tested the new behavior against actual multiple blocks? To make sure workers are picking up jobs properly, there's no weird synchronicity issues, we don't get overlasting delays on the first blocks upon increased work pressure, etc...

@Nashtare Yes, I did tests with 5 blocks range, both test_only and real mode. Seems to work without issues

Once this is in develop we can ask @praetoriansentry to bully it for us.

@praetoriansentry
Copy link
Contributor

Once this is in develop we can ask @praetoriansentry to bully it for us.

@atanmarko atanmarko merged commit 69b170c into develop Jun 10, 2024
4 checks passed
@atanmarko atanmarko deleted the feat/block-interval branch June 10, 2024 15:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Status: Done
6 participants