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

Fee upgrade #3236

Merged
merged 27 commits into from
Jun 12, 2024
Merged

Fee upgrade #3236

merged 27 commits into from
Jun 12, 2024

Conversation

ss-es
Copy link
Contributor

@ss-es ss-es commented May 29, 2024

Closes #3253

This PR:

Contains miscellaneous fixes and configuration options for upgrades, including:

  • settings for when HotShot should attempt or give up on an upgrade.
  • providing uniform serialization/deserialization via the VersionedMessage trait, which uses a decided upgrade certificate to correctly handle message versioning.
  • decoupling serialization/deserialization from the ConnectedNetwork trait; network implementations are no longer responsible for versioning their messages, and only handle raw bytes. Versioning and de/serialization are left to the network event and message tasks.

This PR does not:

Places to review

The libp2p-networking crate, the hotshot/src/traits/networking/libp2p_network.rs file and the request.rs/response.rs files are probably the most significant.

In the libp2p_network.rs file, there is also a lingering type mismatch I couldn't track down, but that I resolved with:

                    let res: Message<TYPES> = bincode::deserialize(&msg.0[8..])
                        .map_err(|e| NetworkError::FailedToDeserialize { source: e.into() })?;

This should be the only place bytes are manually dropped from the message; if you have any idea where the leading 8 bytes might be coming from or a better way to fix this, please let me know!

Instructions:

The sequencer must now provide:

start_proposing_view
stop_proposing_view
start_voting_view
stop_voting_view

Suggested values are:

start_proposing_view: 5,
stop_proposing_view: 6,
start_voting_view: 10,
stop_voting_view: 11,

Testing

The example upgrade should now run to completion without error, with:

RUST_LOG=error,hotshot_task_impls=info just cargo run --example all-push-cdn --features example-upgrade -- --config_file ./crates/orchestrator/run-config.toml
RUST_LOG=error,hotshot_task_impls=info just cargo run --example all-libp2p --features example-upgrade -- --config_file ./crates/orchestrator/run-config.toml
RUST_LOG=error,hotshot_task_impls=info just cargo run --example all-combined --features example-upgrade -- --config_file ./crates/orchestrator/run-config.toml

@ss-es ss-es changed the title Upgrade configurability changes for fee upgrade Fee upgrade Jun 10, 2024
@ss-es ss-es marked this pull request as ready for review June 10, 2024 17:00
Copy link
Contributor

@lukaszrzasik lukaszrzasik left a comment

Choose a reason for hiding this comment

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

In general the change looks very good to me. I have only one minor comment at the moment regarding a lock. I also would like to have a closer look at the libp2p changes to fully grasp them. Those 8 bytes bother me a bit so I'll try to get to the bottom of it.

@@ -838,6 +838,8 @@ pub async fn handle_quorum_proposal_validated<TYPES: NodeType, I: NodeImplementa
drop(consensus);

if let Some(cert) = decided_upgrade_cert {
let mut decided_certificate_lock = task_state.decided_upgrade_certificate.write().await;
*decided_certificate_lock = Some(cert.clone());
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be better to drop the write lock before calling an async broadcast. Just to be sure.

Copy link
Contributor

@lukaszrzasik lukaszrzasik left a comment

Choose a reason for hiding this comment

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

I think it's good to go.

@ss-es ss-es merged commit 9d39554 into main Jun 12, 2024
26 checks passed
@ss-es ss-es deleted the ss/fee-upgrade branch June 12, 2024 17:17
dailinsubjam pushed a commit that referenced this pull request Jun 14, 2024
* 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Fee upgrade] - Implement a fee upgrade in HotShot
4 participants