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

Use nonce for snark work and tx gossips #13535

Merged
merged 2 commits into from
Jul 14, 2023

Conversation

georgeee
Copy link
Member

@georgeee georgeee commented Jul 6, 2023

Problem: with hash-based message ids rebroadcast mechanism became invalid.

Solution: add integer nonce to snark/tx gossip messages in the old topic.

Explain how you tested your changes:

  • Tested on a private cluster, rebroadcast seems to work
  • Requires additional testing to confirm rebroadcast works not worse than on mainnet

Checklist:

  • Dependency versions are unchanged
    • Notify Velocity team if dependencies must change in CI
  • Modified the current draft of release notes with details on what is completed or incomplete within this project
  • Document code purpose, how to use it
    • Mention expected invariants, implicit constraints
  • Tests were added for the new behavior
    • Document test purpose, significance of failures
    • Test names should reflect their purpose
  • All tests pass (CI will check this if you didn't)
  • Serialized types are in stable-versioned modules
  • Does this close issues?

@georgeee georgeee requested a review from a team as a code owner July 6, 2023 15:50
[%%versioned
module Stable = struct
module V1 = struct
type 'a t = { message : 'a; nonce : int } [@@deriving compare, sexp, yojson]
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment here describing why this is needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added the following comment:

Appending a nonce to messages emitted by transaction and snark pools allows to circumvent
libp2p's gossip deduplication logic and allow broadcasts to be broadcasted over and over again.

@deepthiskumar
Copy link
Member

!ci-build-me

@deepthiskumar
Copy link
Member

Version linter failure is ok here as the changes were made to a V2 type


let broadcast_snark_pool_diff t diff =
let broadcast_snark_pool_diff ?nonce t diff =
Copy link
Member

Choose a reason for hiding this comment

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

Why is nonce optional?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not set for initial broadcast (underlying layer will set 0 for it to be used in wire format).

Copy link
Member Author

Choose a reason for hiding this comment

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

For rebroadcasts we generate a random number for a nonce

@georgeee georgeee force-pushed the georgeee/nonce-for-pool-gossips branch from c43b3c3 to c39fa10 Compare July 13, 2023 16:31
@georgeee
Copy link
Member Author

!ci-build-me

@georgeee georgeee force-pushed the georgeee/nonce-for-pool-gossips branch from c39fa10 to 7a4650c Compare July 13, 2023 23:16
@georgeee
Copy link
Member Author

!ci-build-me

Problem: with hash-based message ids rebroadcast mechanism became
invalid.

Solution: add integer nonce to snark/tx gossip messages in the old
topic.
@georgeee georgeee force-pushed the georgeee/nonce-for-pool-gossips branch from 7a4650c to 68c5c98 Compare July 14, 2023 16:00
@georgeee
Copy link
Member Author

!ci-build-me

@deepthiskumar
Copy link
Member

!approved-for-mainnet

@nholland94 nholland94 merged commit c94acef into berkeley Jul 14, 2023
44 of 46 checks passed
@nholland94 nholland94 deleted the georgeee/nonce-for-pool-gossips branch July 14, 2023 21:20
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.

None yet

3 participants