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

feat(block-producer): batch builder #515

Merged
merged 35 commits into from
Oct 22, 2024

Conversation

Mirko-von-Leipzig
Copy link
Contributor

@Mirko-von-Leipzig Mirko-von-Leipzig commented Oct 8, 2024

This PR is intended as a review of the in-progress block-producer redesign. The initial body of work was done on the next-block-producer work, however that initial PR received little scrutiny and thus it makes more sense (to me) to review this one in the context of next proper.

In that context, this PR adds the following pieces:

  • A mempool consisting of
    • InflightState to track state
    • TransactionGraph to form a dependency graph between transactions
    • BatchGraph to form a dependency graph of inflight batches
  • A simple block builder implementation using this new mempool
  • A simple batch builder implementation using this new mempool

The builders reuse the existing DefaultXXX types to minimize the footprint of this PR. Once we've switched over we could refactor these further as needed.

Outstanding work

The work items of #514:

  • input and output note state tracking
  • data propagation from mempool to batch and block builders
  • batch builder with configurable failure/work rate
  • block builder with configurable failure/work rate
  • extensive tests/docs
    • InflightState
    • InflightAccountState
    • TransactionGraph
    • BatchGraph
    • Mempool
  • replacing existing framework
  • Support transaction expiration #508

I'll continue expanding test coverage; though I'd appreciate a review already. In particular, anything that is unclear or looks bad in the components I've marked as tested.

I'm also uncertain about the amount of # Panics involved. I could return more errors and raise the panics up, but there are complications where certain actions just can't (trivially) be made atomic and leave the graphs/state in a broken state. And a broken mempool means that we're dead in the water anyway. Thoughts welcome.

@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as ready for review October 8, 2024 12:35
@Mirko-von-Leipzig
Copy link
Contributor Author

Mirko-von-Leipzig commented Oct 11, 2024

One issue that @polydez has made me aware of is that notes must be ordered by transaction order. See here.

I have some refactoring to do to support this properly. Thinking on this some more, likely the simplest is to remove my new input notes type entirely in favor of just passing around the original transaction/notes along with the note witnesses.

Will draft this PR until I've fixed this; right now this would cause confusion as is.

@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as draft October 12, 2024 06:57
@bobbinth
Copy link
Contributor

One issue that @polydez has made me aware of is that notes must be ordered by transaction order. See here.

Yes, this is how I'm currently thinking about it. But one thing we should do to confirm this is sketch out batch kernel - or at least what we think it'll take as public inputs and what public outputs it'll produce. For example:

  • Public inputs could be a commitment to a list of transaction IDs (+ some additional data needed to verify transaction proofs) for the transactions included in the batch. All detailed transaction could be provided via the advice provider.
  • Public outputs could be a commitment to a list of updated account IDs, a root of the batch note tree, and a commitment to list of created nullifiers.

@Mirko-von-Leipzig Mirko-von-Leipzig changed the base branch from next-block-producer to next October 16, 2024 16:14
@Mirko-von-Leipzig Mirko-von-Leipzig marked this pull request as ready for review October 17, 2024 14:22
Copy link
Contributor

@bobbinth bobbinth 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! Thank you! I left some comments inline - most are minor, but there are a few where I think transaction validation logic needs to be improved.

crates/block-producer/Cargo.toml Outdated Show resolved Hide resolved
crates/block-producer/src/mempool/mod.rs Outdated Show resolved Hide resolved
crates/block-producer/src/mempool/mod.rs Outdated Show resolved Hide resolved
crates/block-producer/src/mempool/mod.rs Outdated Show resolved Hide resolved
crates/block-producer/src/mempool/batch_graph.rs Outdated Show resolved Hide resolved
crates/block-producer/src/block_builder/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth 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! Thank you! I left a few more comments inline.

Once these and the comments from the previous review are addressed, I think we should merge this. If you'd prefer not to merge it into next yet, we can merge into a separate branch. Then we can make more PRs with additional changes against that branch (e.g., the ones discussed here.

crates/block-producer/src/transaction.rs Outdated Show resolved Hide resolved
crates/block-producer/src/transaction.rs Outdated Show resolved Hide resolved
crates/block-producer/src/transaction.rs Outdated Show resolved Hide resolved
crates/block-producer/src/transaction.rs Outdated Show resolved Hide resolved
crates/block-producer/src/transaction.rs Outdated Show resolved Hide resolved
crates/block-producer/src/mempool/inflight_state/mod.rs Outdated Show resolved Hide resolved
@Mirko-von-Leipzig
Copy link
Contributor Author

I've addressed the review comments; I'll merge this PR into next-block-producer and continue with smaller, more revieawable PRs.

@Mirko-von-Leipzig Mirko-von-Leipzig changed the base branch from next to next-block-producer October 22, 2024 13:40
@Mirko-von-Leipzig Mirko-von-Leipzig merged commit 34a45d9 into next-block-producer Oct 22, 2024
8 checks passed
@Mirko-von-Leipzig Mirko-von-Leipzig deleted the mirko-typesafe-tx-notes branch October 22, 2024 14:02
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