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.

Sign up for GitHub

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

Jump to bottom

Multiple builders #3203

Merged
merged 10 commits into from
Jun 12, 2024
Merged

Multiple builders #3203

merged 10 commits into from
Jun 12, 2024

Conversation

QuentinI
Copy link
Contributor

@QuentinI QuentinI commented May 21, 2024 •
edited
Loading

Closes #3181

This PR:

  • Adds support for multiple builders
  • Adds support for multiple builders in testing harness
  • Adds ability to simulate builder failures in tests

This PR does not:

Key places to review:

crates/task-impls/src/transactions.rs
crates/testing/tests/tests_3/test_with_builder_failures.rs

@QuentinI QuentinI changed the title [WIP] Multiple builders MVP [WIP] Multiple builders Jun 3, 2024
nyospe
nyospe previously approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@nyospe nyospe left a comment

Choose a reason for hiding this comment

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

Yes, I see that this is still a draft, but...

The implementation is both functional and elegant, and assuming the remaining testing holds up, I'm happy with this solution.

crates/task-impls/src/transactions.rs Outdated
// First we complete the query to the fastest fraction of the builders
let threshold = (self.builder_clients.len() * BUILDER_MAIN_BATCH_THRESHOLD_DIVIDEND)
.div_ceil(BUILDER_MAIN_BATCH_THRESHOLD_DIVISOR);
let mut tasks = tasks.take(threshold);
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh! Very nice solution!

@QuentinI QuentinI changed the title [WIP] Multiple builders Multiple builders Jun 5, 2024
@QuentinI QuentinI marked this pull request as ready for review June 5, 2024 16:16
jparr721
jparr721 requested changes Jun 5, 2024
View reviewed changes
Copy link
Contributor

@jparr721 jparr721 left a comment

Choose a reason for hiding this comment

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

Some questions, happy to discuss anything further.

crates/examples/infra/mod.rs Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated Show resolved Hide resolved
crates/task-impls/src/transactions.rs Outdated
) {
bail!("Failed to verify available block info response message signature");
}
// Create a future for every builder
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is quite long, is it possible to pare it down some? It looks like we're doing a lot of logic that's not necessarily tightly related.

crates/testing/src/block_builder/simple.rs Show resolved Hide resolved
crates/testing/src/overall_safety_task.rs Outdated Show resolved Hide resolved
crates/testing/src/test_builder.rs Show resolved Hide resolved
crates/testing/src/test_builder.rs Show resolved Hide resolved
crates/testing/tests/tests_3/test_with_builder_failures.rs Outdated
// has time to initialize

// First builder will always respond with available blocks, but will periodically fail claim calls
#[allow(clippy::unnecessary_filter_map)] // False positive
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a false positive? Does clippy not provide a working replacement option? It looks like you could fill the map then just swap the value for key 2 and 0 in this and the below builder without needing to map over it at all.

Copy link
Contributor Author

@QuentinI QuentinI Jun 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Is this a false positive?

Oh, wow. The reason I though it was a false positive was because this was supposed to be

                0 => Some((view_num, BuilderChange::Down)),
                1 => Some((view_num, BuilderChange::Up)),
                _ => None, // We don't want to send Up when builder is up already, that'd
                           // restart it

and I somehow convinced myself that that's what I wrote... Thank you for the catch!

It looks like you could fill the map then just swap the value for key 2 and 0 in this and the below builder without needing to map over it at all.

Not sure what you mean by this though - the keys range from 0 to the number of views we're testing for, it's not only 0 and 2.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, hah pardon my blindness on the second point. This sounds fine.

@QuentinI
Copy link
Contributor Author

QuentinI commented Jun 7, 2024

@jparr721 addressed all comments

jparr721
jparr721 previously approved these changes Jun 7, 2024
View reviewed changes
Copy link
Contributor

@jparr721 jparr721 left a comment

Choose a reason for hiding this comment

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

Great improvements to the comments. LGTM

@jparr721 jparr721 self-assigned this Jun 7, 2024
nyospe
nyospe previously approved these changes Jun 10, 2024
View reviewed changes
Copy link
Contributor

@nyospe nyospe left a comment

Choose a reason for hiding this comment

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

LGTM

dailinsubjam
dailinsubjam reviewed Jun 10, 2024
View reviewed changes
Copy link
Contributor

@dailinsubjam dailinsubjam left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a few benchmark-related questions: Is there an instruction on starting multiple builders? And who will run these builders?

crates/orchestrator/src/config.rs Outdated Show resolved Hide resolved
@QuentinI QuentinI merged commit f00e9b9 into main Jun 12, 2024
26 checks passed
@QuentinI QuentinI deleted the ag/multiple-builders branch June 12, 2024 18:52
@QuentinI QuentinI restored the ag/multiple-builders branch June 12, 2024 18:52
dailinsubjam pushed a commit that referenced this pull request Jun 14, 2024
@QuentinI @jparr721
@github-actions @dependabot @ss-es @tudorpintea999 @shenkeyao
Benchmarking adjustments (#3324)
91aba4f 
* bump (#3291)

* flake.lock: Update (#3294)

Flake lock file updates:

• Updated input 'fenix':
    'github:nix-community/fenix/9a025daf6799e3af80b677f0af57ef76432c3fcf?narHash=sha256-ih8NPk3Jn5EAILOGQZ%2BKS5NLmu6QmwohJX%2B36MaTAQE%3D' (2024-06-01)
  → 'github:nix-community/fenix/ab655c627777ab5f9964652fe23bbb1dfbd687a8?narHash=sha256-ixopuTeTouxqTxfMuzs6IaRttbT8JqRW5C9Q/57WxQw%3D' (2024-06-08)
• Updated input 'fenix/rust-analyzer-src':
    'github:rust-lang/rust-analyzer/d6d735e6f20ef78b16a79886fe28bd69cf059504?narHash=sha256-qBruki5NHrSqIw5ulxtwFmVsb6W/aOKOMjsCJjfalA4%3D' (2024-05-31)
  → 'github:rust-lang/rust-analyzer/48bbdd6a74f3176987d5c809894ac33957000d19?narHash=sha256-%2BlRAmz92CNUxorqWusgJbL9VE1eKCnQQojglRemzwkw%3D' (2024-06-05)
• Updated input 'nixpkgs':
    'github:NixOS/nixpkgs/6132b0f6e344ce2fe34fc051b72fb46e34f668e0?narHash=sha256-7R2ZvOnvd9h8fDd65p0JnB7wXfUvreox3xFdYWd1BnY%3D' (2024-05-30)
  → 'github:NixOS/nixpkgs/d226935fd75012939397c83f6c385e4d6d832288?narHash=sha256-HV97wqUQv9wvptiHCb3Y0/YH0lJ60uZ8FYfEOIzYEqI%3D' (2024-06-07)

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

* Bump clap from 4.5.6 to 4.5.7 (#3302)

* Bump url from 2.5.0 to 2.5.1 (#3304)

* Fix Leaf Ascension (#3307)

* fix tests, halfway improve impl

* move the last of the variables

* fix upgrade tests

* move to helper method

* lint

* fix last suite

* use debugged function, remove dead tests

* fix tests

* fix lint

* comments, fix last test

* Minor test fixes (#3256)

* Revert "Minor test fixes (#3256)" (#3316)

This reverts commit 1932e42.

* Docs improvements (#3295)

* Update General.md

* Update QuorumProposalRecv.md

* Update Timeout.md

* Update ViewSyncTrigger.md

* Update VoteOnQuorumProposal.md

* [DEPENDENCY_REFACTOR] - Remove proposal validation from the quorum proposal task (#3275)

* Move validation

* Fix fmt and import

* Fix build and recv test

* Remove a todo

* Restore an event

* Fix non-dependency lints

* Fix tests

* Fix CI

* Fix fmt and imports

* Fix proposal and proposal recv tasks tests

* 3 more tests

* Fix doc

* Address comments

* Add error

* Fix Conficts in keyao/remove-proposal-validation (#3308)

* fix

* remove dead test

* fix lint

* fix test

* Fix vote tests

---------

Co-authored-by: Keyao Shen <[email protected]>

* Fix upgrade test

---------

Co-authored-by: Jarred Parr <[email protected]>

* Fee upgrade (#3236)

* Multiple builders (#3203)

* Multiple builders support

* Bump vec1 from 1.12.0 to 1.12.1 (#3319)

* Bump libp2p-identity from 0.2.8 to 0.2.9 (#3318)

* [DEPENDENCY_REFACTOR] - Modify `update_validated_state_map` to not override a view with more info (#3315)

* Move validation

* Fix fmt and import

* Fix build and recv test

* Remove a todo

* Restore an event

* Fix non-dependency lints

* Fix tests

* Fix CI

* Fix fmt and imports

* Fix proposal and proposal recv tasks tests

* 3 more tests

* Fix doc

* Address comments

* Add error

* Fix Conficts in keyao/remove-proposal-validation (#3308)

* fix

* remove dead test

* fix lint

* fix test

* Fix vote tests

---------

Co-authored-by: Keyao Shen <[email protected]>

* Save changes

* Fix upgrade test

* Fix build

* Add error handling to update_validated_state_map

* Fix tests

* use ref

---------

Co-authored-by: Jarred Parr <[email protected]>

* Choose builder url on orchestrator

* Multiple builders

* Fix lints

* Fix malformed URLs

---------

Co-authored-by: Jarred Parr <[email protected]>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: ss-es <[email protected]>
Co-authored-by: iwantanode <[email protected]>
Co-authored-by: Keyao Shen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dailinsubjam dailinsubjam dailinsubjam approved these changes

@nyospe nyospe nyospe left review comments

@jparr721 jparr721 jparr721 left review comments

@ss-es ss-es Awaiting requested review from ss-es ss-es is a code owner

@elliedavidson elliedavidson Awaiting requested review from elliedavidson

@bfish713 bfish713 Awaiting requested review from bfish713 bfish713 is a code owner

Assignees

@jparr721 jparr721

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUILDER] - Multiple builder support
4 participants
@QuentinI @dailinsubjam @nyospe @jparr721