Skip to content

Direct Sequencer & Progressive Block Building#716

Closed
S1nus wants to merge 42 commits intomainfrom
connor/centralized-sequencer
Closed

Direct Sequencer & Progressive Block Building#716
S1nus wants to merge 42 commits intomainfrom
connor/centralized-sequencer

Conversation

@S1nus
Copy link
Copy Markdown
Contributor

@S1nus S1nus commented Jan 25, 2023

Goal is to improve the UX of roll-ups with centralized sequencers.

  1. Allow sequencers to receive transactions directly, instead of via inefficient p2p gossip Allow centralized sequencers to receive transactions directly #713

  2. Respond to transaction requests with soft confirmations Soft Confirmations for Centralized Sequencers #712

Creates ProgressiveAggregationLoop
Sequencer nodes may now be configured with rollkit.progressive_sequencer. When transactions become available in the mempool, the sequencer starts a 1 second timer. After 1 second, it builds and publishes a block, and responds to all the transaction requests with the blockheight they were included in.

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 29, 2023

Codecov Report

Base: 54.59% // Head: 54.83% // Increases project coverage by +0.23% 🎉

Coverage data is based on head (05eaf2c) compared to base (018e42a).
Patch coverage: 54.04% of modified lines in pull request are covered.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #716      +/-   ##
==========================================
+ Coverage   54.59%   54.83%   +0.23%     
==========================================
  Files          52       53       +1     
  Lines       10372     9978     -394     
==========================================
- Hits         5663     5471     -192     
+ Misses       3840     3699     -141     
+ Partials      869      808      -61     
Impacted Files Coverage Δ
conv/addr.go 100.00% <ø> (ø)
conv/config.go 47.05% <ø> (ø)
da/celestia/celestia.go 50.00% <ø> (ø)
da/celestia/mock/server.go 49.68% <ø> (ø)
da/grpc/grpc.go 73.13% <ø> (ø)
mempool/v1/mempool.go 86.09% <ø> (ø)
node/light.go 0.00% <0.00%> (ø)
node/light_client.go 0.00% <ø> (ø)
node/node.go 52.38% <0.00%> (-26.20%) ⬇️
p2p/client.go 60.43% <ø> (-0.30%) ⬇️
... and 34 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@S1nus S1nus changed the title [wip] centralized sequencer improvements Direct Sequencer & Progressive Block Building Feb 2, 2023
@S1nus S1nus marked this pull request as ready for review February 2, 2023 00:42
@S1nus S1nus requested a review from tzdybal as a code owner February 2, 2023 00:42
@S1nus S1nus requested review from a team, Manav-Aggarwal and gupadhyaya and removed request for a team February 2, 2023 00:42
Comment thread config/config.go Outdated
Comment thread config/config.go Outdated
Comment thread mempool/tx.go Outdated
Comment thread node/full.go Outdated
@gupadhyaya
Copy link
Copy Markdown
Contributor

gupadhyaya commented Feb 3, 2023

@S1nus i don't think you need a separate RPC server. just use the already existing rpc server (rpc/server.go) and extend the rpc/json/service.go with a new method like direct_tx. Doing a new server is redundant and will hit the performance bottleneck. Also, lot of redundant code in your sequencer_server.go. ideally, we should avoid spinning up new servers. do let me know if this is a must. otherwise, please refactor the code to utilize the existing rpc/server and will be happy re-review, as this would be a major change.

also note that, your new config flags won't be needed as rpc config already handled the max connections, CORs, etc.

@S1nus
Copy link
Copy Markdown
Contributor Author

S1nus commented Feb 6, 2023

i don't think you need a separate RPC server

One issue is that the standard RPC server does not have a reference to the FullNode, only a Client interface from tendermint

@S1nus
Copy link
Copy Markdown
Contributor Author

S1nus commented Feb 7, 2023

Which option is cleaner:

  1. Create a second server, exposed by FullNodes when they are configured to be centralized sequencers
  2. Modify the Node interface to export a public function for receiving transactions directly, which will be nil if the node is anything other than a centralized sequencer, and integrate it into the existing RPC server

Comment thread node/full.go
Comment thread block/manager.go Outdated
Comment thread block/manager.go Outdated
Comment thread state/executor.go Outdated
@gupadhyaya
Copy link
Copy Markdown
Contributor

@S1nus does it make sense to separate out the lazy sequencer from direct tx, as par the discussion that direct tx may not be cost efficient over gossip with just 1 peer? we can merge the lazy sequencer fast. let me know.

@S1nus
Copy link
Copy Markdown
Contributor Author

S1nus commented Feb 16, 2023

does it make sense to separate out the lazy sequencer from direct tx, as par the discussion that direct tx may not be cost efficient over gossip with just 1 peer? we can merge the lazy sequencer fast. let me know.

yes I will separate this into two PRs, stay tuned

@S1nus
Copy link
Copy Markdown
Contributor Author

S1nus commented Feb 21, 2023

as par the discussion that direct tx may not be cost efficient over gossip with just 1 peer

I believe the discussion was about configuring the libp2p gossiper to send the transaction directly. Running gossipsub for a 1-to-1 direct message is wasteful

@S1nus S1nus requested a review from nashqueue as a code owner February 21, 2023 20:51
@nashqueue
Copy link
Copy Markdown
Contributor

nashqueue commented Feb 21, 2023

yes I will separate this into two PRs, stay tuned

Is this already separated and ready to be reviewed? :)

@S1nus
Copy link
Copy Markdown
Contributor Author

S1nus commented Feb 21, 2023

Is this already separated and ready to be reviewed? :)

Yes, I just finished removing the Direct Tx stuff, this PR is now just the LazySequencer.

@S1nus
Copy link
Copy Markdown
Contributor Author

S1nus commented Mar 8, 2023

deprecating in favor of the cleaned-up PR
#760

@S1nus S1nus closed this Mar 8, 2023
@tac0turtle tac0turtle deleted the connor/centralized-sequencer branch March 20, 2025 11:56
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.

4 participants