Skip to content

sequencer, oracle integration with L1 to L2 messages#683

Closed
rahul-kothari wants to merge 1 commit into
masterfrom
rk/sequencerl1Tol2
Closed

sequencer, oracle integration with L1 to L2 messages#683
rahul-kothari wants to merge 1 commit into
masterfrom
rk/sequencerl1Tol2

Conversation

@rahul-kothari

@rahul-kothari rahul-kothari commented May 24, 2023

Copy link
Copy Markdown
Contributor

Description

Fixes #529:
Builds on from #642

  • Archiver now stores a map of message key to the message (for all messages ever sent to inbox)
  • Archiver also stores these in a max heap (sorted by fees) so that sequencer can pull an arbitrary amount of messages quickly.
  • Once the sequencer requests messages, they are removed from the heap. Sequencer inserts these messages into the block (& sends to the circuits). If the block publishes, nothing needs to be done (since the messages were already removed from the heap). If the block fails to publish then the messages would need to be added back to the archiver store.
  • Archiver also implements a getter for a message - useful for oracle calls from noir
  • Call this getter from the RPC Server (which responds to oracle queries)

TODO: Add some tests around the archiver, fix the l1 publisher integration
Please sanity check that there are no other places I have missed
Also any test suggestions are welcome!

Checklist:

  • I have reviewed my diff in github, line by line.
  • Every change is related to the PR description.
  • I have linked this pull request to the issue(s) that it resolves.
  • There are no unexpected formatting changes, superfluous debug logs, or commented-out code.
  • The branch has been merged or rebased against the head of its merge target.
  • I'm happy for the PR to be merged at the reviewer's next convenience.

void msgKey; // this line can be removed its to appease the linter on this stub
const message = L1ToL2Message.empty().toFieldArray();
const message = (await this.node.getL1ToL2Message(msgKey)).toFieldArray();
// TODO: note index will be requested from the database, stubbed as 0 for the meantime

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.

@Maddiaa0 what is this btw?

@PhilWindle PhilWindle left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The approach of removing from the archiver and then reinserting upon publish failure seems very brittle. What if a sequencer stops/crashes after retrieving the messages but before publishing them? I think a much better approach is to leave the messages in the archiver store and remove them upon receipt of a new block in the archiver.

It also breaks down in a multi-sequencer world. The archiver should be a read-only store of latest chain state for all consumers to query. Pending messages should remain pending until the chain says they aren't.


// there are only 2 l1ToL2 messages in the store
expect((await archiver.getPendingL1ToL2Messages(10)).length).toEqual(2);
expect((await archiver.consumePendingL1ToL2Messages(10)).length).toEqual(2);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This seems like it is "consuming" the pending messages (popping), but that should happen when the L2 block that includes them is broadcast, not when you want to just fetch them?

* @returns Array of the top L1 to L2 message keys sorted by fee
* (of maximum size `take` - smaller if not enough messages)
*/
public consumePendingL1ToL2Messages(take: number = NUMBER_OF_L1_L2_MESSAGES_PER_ROLLUP): Promise<Fr[]> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This don't seem to match the task, #529 don't require removing things from the archivers pending queue (that should be handled in #520 as L1 sees the blocks).

@LHerskind

Copy link
Copy Markdown
Contributor

As @PhilWindle mentioned and outlined in #520 and #529, the archiver depends only on what happens on L1 and don't need to know about the existance of sequencers. Pending L1 to L2 messages should be "consumed" when they are included in an L2 block. I have created #691 since the ever-growing "non-pending" L1 to L2 message set was not explicitly outlined in #520 before. Also reopened #520 as the last point was not addressed in #642.

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.

Sequencer pulls L1 -> L2 messages from archiver

3 participants