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

v1.17: gossip: process duplicate proofs for merkle root conflicts (backport of #34066) #34292

Closed
wants to merge 1 commit into from

Conversation

mergify[bot]
Copy link
Contributor

@mergify mergify bot commented Nov 30, 2023

This is an automatic backport of pull request #34066 done by Mergify.
Cherry-pick of ca6ab08 has failed:

On branch mergify/bp/v1.17/pr-34066
Your branch is up to date with 'origin/v1.17'.

You are currently cherry-picking commit ca6ab08555.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --skip" to skip this patch)
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:
	modified:   gossip/src/duplicate_shred.rs

Unmerged paths:
  (use "git add <file>..." to mark resolution)
	both modified:   gossip/src/cluster_info.rs

To fix up this pull request, you can check it out locally. See documentation: https://docs.github.com/en/github/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/checking-out-pull-requests-locally


Mergify commands and options

More conditions and actions can be found in the documentation.

You can also trigger Mergify actions by commenting on this pull request:

  • @Mergifyio refresh will re-evaluate the rules
  • @Mergifyio rebase will rebase this PR on its base branch
  • @Mergifyio update will merge the base branch into this PR
  • @Mergifyio backport <destination> will backport this PR on <destination> branch

Additionally, on Mergify dashboard you can:

  • look at your merge queues
  • generate the Mergify configuration with the config editor.

Finally, you can contact us on https://mergify.com

@mergify mergify bot added the conflicts label Nov 30, 2023
@AshwinSekar AshwinSekar force-pushed the mergify/bp/v1.17/pr-34066 branch 2 times, most recently from b9415a5 to c5a1c50 Compare December 1, 2023 04:30
@AshwinSekar
Copy link
Contributor

need to merge #34291 first

@AshwinSekar AshwinSekar closed this Dec 6, 2023
@mergify mergify bot deleted the mergify/bp/v1.17/pr-34066 branch December 6, 2023 00:04
@AshwinSekar AshwinSekar restored the mergify/bp/v1.17/pr-34066 branch December 7, 2023 16:54
@AshwinSekar
Copy link
Contributor

necessary for the backport chain to #34270

@AshwinSekar AshwinSekar reopened this Dec 7, 2023
* gossip: process duplicate proofs for merkle root conflicts

* pr comments + abi

(cherry picked from commit ca6ab08)

# Conflicts:
#	gossip/src/cluster_info.rs
@behzadnouri
Copy link
Contributor

What was the merge conflict here?

In the future, please don't squash the commits so we can see the merge conflict and how it was resolved.

@AshwinSekar
Copy link
Contributor

sorry bad habit. The merge conflict was the ABI on cluster info

@behzadnouri
Copy link
Contributor

Probably I should have asked this on the original commit, but what happens if during the upgrade some % of the cluster considers 2 shreds as a duplicate proof but the rest of the cluster without this code does not recognize those as duplicates?

@AshwinSekar
Copy link
Contributor

The part of the cluster without the code will be unable to deserialize the duplicate proof thinking that it is invalid. I assume this will also hurt the propagation of such proofs in gossip.

Note that we don't actually consume this proof in fork choice yet, #32963. So there shouldn't be a divergence there.

For rollout I think we should wait to consume the proof until the majority of the cluster can process it through gossip.

@AshwinSekar AshwinSekar closed this Dec 8, 2023
@mergify mergify bot deleted the mergify/bp/v1.17/pr-34066 branch December 8, 2023 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants