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

Enhance Proof Delivery Resilience with ParSliceErrCollect in ChainPorter #1100

Merged
merged 3 commits into from
Aug 27, 2024

Conversation

ffranr
Copy link
Contributor

@ffranr ffranr commented Aug 22, 2024

This PR updates the proof delivery process in ChainPorter by utilizing the newly introduced ParSliceErrCollect function. This change ensures that proof delivery continues even if an individual transfer output proof encounters an error, improving the reliability of the delivery process.

This change ensures that as many transfer output proofs as possible are delivered, even in the presence of errors.

Changes

  • New Function: Introduced ParSliceErrCollect, which collects errors during parallel processing without halting the entire operation.
  • ChainPorter: Modified to use ParSliceErrCollect for parallel proof delivery, allowing the process to continue despite individual delivery failures.

Related to: #1082

@ffranr ffranr self-assigned this Aug 22, 2024
@ffranr ffranr force-pushed the robust-multi-proof-delivery branch from 94362fd to 55c9d3b Compare August 22, 2024 12:12
@ffranr ffranr changed the title Enhance Proof Delivery Resilience with ParSliceErrorCollect in ChainPorter Enhance Proof Delivery Resilience with ParSliceErrCollect in ChainPorter Aug 22, 2024
@coveralls
Copy link

coveralls commented Aug 22, 2024

Pull Request Test Coverage Report for Build 10579893055

Details

  • 0 of 54 (0.0%) changed or added relevant lines in 3 files are covered.
  • 22 unchanged lines in 4 files lost coverage.
  • Overall coverage decreased (-0.05%) to 40.279%

Changes Missing Coverage Covered Lines Changed/Added Lines %
fn/func.go 0 5 0.0%
fn/concurrency.go 0 23 0.0%
tapfreighter/chain_porter.go 0 26 0.0%
Files with Coverage Reduction New Missed Lines %
asset/asset.go 2 81.61%
tapdb/universe.go 4 80.91%
commitment/tap.go 4 83.64%
universe/interface.go 12 50.22%
Totals Coverage Status
Change from base Build 10534107048: -0.05%
Covered Lines: 23964
Relevant Lines: 59495

💛 - Coveralls

@dstadulis
Copy link
Collaborator

dstadulis commented Aug 22, 2024

PR Ensures proofs are delivered given partial proof-courier addresses availability

@dstadulis dstadulis added this to the v0.4.2 milestone Aug 22, 2024
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Very nice improvement!

Just nits and suggestions.

fn/concurrency.go Outdated Show resolved Hide resolved
instanceErrors := make(map[int]error, len(s))

for idx, v := range s {
v := v
Copy link
Member

Choose a reason for hiding this comment

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

nit: this is no longer required, as we're now using Go 1.22.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've gone with s[idx] in the latest commits. Are you saying that v := v isn't required because of Go 1.22? Why is that, can you say more please?

Copy link
Member

Choose a reason for hiding this comment

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

fn/concurrency.go Show resolved Hide resolved
fn/concurrency.go Outdated Show resolved Hide resolved
fn/concurrency.go Show resolved Hide resolved
tapfreighter/chain_porter.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

Looks good, a useful func! I think we have some spots in the universe code, specifically federations, where this could be used as well.

Would be good to add a test for this alongside the test for ParSlice.

fn/concurrency.go Show resolved Hide resolved
tapfreighter/chain_porter.go Outdated Show resolved Hide resolved
tapfreighter/chain_porter.go Outdated Show resolved Hide resolved
Add ParSliceErrCollect function to handle errors without halting
parallel processing. ParSliceErrCollect is similar to ParSlice but
allows processing to continue even when errors occur. Instead of halting
on the first error, it collects all errors for later handling, providing
more robust parallel execution.
This commit adds the `PeekMap` function. This function
non-deterministically selects and returns a single key-value pair from
the given map.
This commit leverages the new ParSliceErrCollect function to process
transfer output proofs in parallel. This change ensures that processing
continues even if a transfer output proof delivery instance fails.
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

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

Nice, LGTM 🎉

fn/concurrency.go Show resolved Hide resolved
Copy link
Collaborator

@jharveyb jharveyb left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏽

@guggero guggero merged commit 30e7166 into main Aug 27, 2024
17 checks passed
@guggero guggero deleted the robust-multi-proof-delivery branch August 27, 2024 18:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

5 participants