Skip to content

[consensus] codec migration (Attempt 2)#735

Merged
patrick-ogrady merged 27 commits into
mainfrom
consensus-codec-migration
Apr 18, 2025
Merged

[consensus] codec migration (Attempt 2)#735
patrick-ogrady merged 27 commits into
mainfrom
consensus-codec-migration

Conversation

@patrick-ogrady
Copy link
Copy Markdown
Contributor

@patrick-ogrady patrick-ogrady commented Apr 15, 2025

Replaces: #675
Resolves: #444
Replaces: #462
Related: #707
Resolves: #712

TODO

Comment thread consensus/src/threshold_simplex/types.rs Outdated
@patrick-ogrady patrick-ogrady marked this pull request as ready for review April 17, 2025 22:56
@patrick-ogrady patrick-ogrady requested a review from Copilot April 17, 2025 22:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request migrates several consensus components to use the new codec “types” and updates the generics by replacing the Array constraints with Digest where appropriate. Key changes include refactoring of the simplex resolver actor, removal of legacy proto-based modules (wire.proto, parsed, serializer, prover, namespace), and a renaming of committers to reporters in the ordered broadcast module and associated mocks and configuration.

Reviewed Changes

Copilot reviewed 72 out of 72 changed files in this pull request and generated no comments.

Show a summary per file
File Description
consensus/src/simplex/actors/resolver/actor.rs Updated to use structured types, replaced deprecated namespace functions, and adjusted generic bounds (Array → Digest).
consensus/src/ordered_broadcast/* Removed legacy proto definitions and related modules; updated usage to reference new types and shifted from Scheme to Verifier where applicable.
Various mocks and config files Transition from using Committer to Reporter along with corresponding test updates.
consensus/src/ordered_broadcast/ack_manager.rs Updated the ack aggregation to use the new signature field and adjusted generic parameter ordering.
consensus/build.rs & Cargo.toml Removed build dependencies for the obsolete proto files and adjusted dependency declarations.
Comments suppressed due to low confidence (3)

consensus/src/ordered_broadcast/ack_manager.rs:75

  • The change from using 'ack.partial.index' to 'ack.signature.index' assumes that all Ack objects are constructed with a valid signature field. Please verify that all Ack producers have been updated accordingly to prevent potential mismatches.
if (!p.shares.insert(ack.signature.index)) {

consensus/src/ordered_broadcast/wire.proto:1

  • The entire 'wire.proto' file has been removed. Please ensure that no lingering dependencies or build scripts rely on this file to avoid integration issues.
syntax = "proto3";

consensus/src/ordered_broadcast/mocks/reporter.rs:1

  • [nitpick] With the migration from Committer to Reporter in mocks, please double-check that all tests cover the new Reporter flow and any behavioral differences that may impact consensus reporting.
use crate::{ordered_broadcast::types::{Activity, Chunk, Epoch, Lock, Proposal}, Reporter as Z};

@patrick-ogrady patrick-ogrady added the bug Something isn't working label Apr 17, 2025
@patrick-ogrady patrick-ogrady added the enhancement New feature or request label Apr 17, 2025
Comment thread consensus/src/ordered_broadcast/types.rs
Comment on lines +327 to +337
impl Debug for Scalar {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", hex(&self.as_slice()))
}
}

impl Display for Scalar {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "{}", hex(&self.as_slice()))
}
}
Copy link
Copy Markdown
Collaborator

@BrendanChou BrendanChou Apr 17, 2025

Choose a reason for hiding this comment

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

To prevent writing these functions twice, should we have one be the primary (say Debug) and have Display call Debug::fmt(self, f)?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is a pattern I adapted from elsewhere in cryptography. Happy to migrate to a shared implementation but think we should do all at once.

Comment thread examples/bridge/src/application/mod.rs Outdated
Comment thread examples/bridge/src/application/actor.rs Outdated
Comment thread consensus/src/ordered_broadcast/types.rs Outdated
Comment thread consensus/src/ordered_broadcast/types.rs Outdated
Comment thread consensus/src/simplex/types.rs Outdated
Comment thread consensus/src/threshold_simplex/types.rs Outdated
Comment thread consensus/src/threshold_simplex/types.rs
Comment thread consensus/src/ordered_broadcast/types.rs Outdated
Comment thread consensus/src/threshold_simplex/actors/voter/actor.rs
BrendanChou
BrendanChou previously approved these changes Apr 18, 2025
Copy link
Copy Markdown
Collaborator

@BrendanChou BrendanChou left a comment

Choose a reason for hiding this comment

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

None of my comments are blocking, feel free to merge

@patrick-ogrady
Copy link
Copy Markdown
Contributor Author

Nits addressed here: #772

@patrick-ogrady patrick-ogrady merged commit ea036be into main Apr 18, 2025
7 checks passed
@patrick-ogrady patrick-ogrady deleted the consensus-codec-migration branch April 18, 2025 17:06
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 18, 2025

Codecov Report

Attention: Patch coverage is 94.47589% with 283 lines in your changes missing coverage. Please review.

Project coverage is 89.37%. Comparing base (5a14fd6) to head (0b9ae71).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
consensus/src/simplex/types.rs 88.55% 128 Missing ⚠️
consensus/src/simplex/actors/voter/actor.rs 93.50% 28 Missing ⚠️
...sensus/src/threshold_simplex/actors/voter/actor.rs 93.75% 24 Missing ⚠️
consensus/src/ordered_broadcast/types.rs 97.97% 19 Missing ⚠️
...onsensus/src/threshold_simplex/mocks/supervisor.rs 89.47% 16 Missing ⚠️
consensus/src/simplex/mod.rs 97.30% 14 Missing ⚠️
consensus/src/threshold_simplex/mod.rs 97.24% 14 Missing ⚠️
consensus/src/simplex/mocks/supervisor.rs 94.37% 9 Missing ⚠️
consensus/src/ordered_broadcast/mocks/reporter.rs 89.06% 7 Missing ⚠️
consensus/src/simplex/actors/resolver/actor.rs 86.11% 5 Missing ⚠️
... and 6 more
@@            Coverage Diff             @@
##             main     #735      +/-   ##
==========================================
+ Coverage   88.84%   89.37%   +0.53%     
==========================================
  Files         168      165       -3     
  Lines       40198    41293    +1095     
==========================================
+ Hits        35714    36907    +1193     
+ Misses       4484     4386      -98     
Files with missing lines Coverage Δ
consensus/src/ordered_broadcast/ack_manager.rs 100.00% <100.00%> (ø)
consensus/src/ordered_broadcast/mocks/automaton.rs 94.87% <ø> (ø)
consensus/src/ordered_broadcast/mocks/monitor.rs 100.00% <ø> (ø)
...onsensus/src/ordered_broadcast/mocks/sequencers.rs 75.00% <ø> (+6.81%) ⬆️
...onsensus/src/ordered_broadcast/mocks/validators.rs 86.66% <ø> (+5.41%) ⬆️
consensus/src/ordered_broadcast/mod.rs 99.33% <100.00%> (+0.01%) ⬆️
consensus/src/ordered_broadcast/tip_manager.rs 100.00% <100.00%> (ø)
consensus/src/simplex/actors/resolver/ingress.rs 100.00% <100.00%> (ø)
consensus/src/simplex/actors/voter/ingress.rs 100.00% <100.00%> (ø)
consensus/src/simplex/config.rs 68.57% <ø> (+0.15%) ⬆️
... and 29 more

... and 6 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a14fd6...0b9ae71. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[consensus] Error on Catchup (after alto restart) [consensus] Emit Nullify and Nullification Proofs

3 participants