-
Notifications
You must be signed in to change notification settings - Fork 1k
feat(state)!: add the ability to submit blobs in parallel #4595
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
base: release/v0.27.0
Are you sure you want to change the base?
Conversation
state/core_access.go
Outdated
response, err := client.SubmitPayForBlobWithAccount(ctx, account.Name(), libBlobs, opts...) | ||
response, err := client.SubmitPayForBlob(ctx, libBlobs, opts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the bulk of the change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly good w me, mostly about namings
nodebuilder/state/config.go
Outdated
// parallel submissions. This means that blobs can be submitted by multiple different | ||
// signers, and that blobs will not be submitted on chain in the original sending order. | ||
// This is highly recommended for high throughput chains. | ||
WorkerAccounts int |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WorkerAccounts int | |
TxWorkerAccounts int |
} | ||
|
||
if cmd.Flag(txWorkerAccountsFlag).Changed { | ||
value, err := strconv.Atoi(cmd.Flag(txWorkerAccountsFlag).Value.String()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can fatal out on err here
state/core_access.go
Outdated
ca.estimatorConn = estimatorConn | ||
} | ||
|
||
if ca.workerAccounts > 0 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe > 1 since default is 1
state/option.go
Outdated
// WithWorkerAccounts configures the CoreAccessor to manage the provided number of | ||
// worker accounts via the TxClient. A value of zero leaves parallel | ||
// submission disabled. | ||
func WithWorkerAccounts(workerAccounts int) Option { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
func WithWorkerAccounts(workerAccounts int) Option { | |
func WithTxWorkerAccounts(workerAccounts int) Option { |
1f32639
to
12ed37e
Compare
…ath thru SubmitPayForBlobWithAccount if non-default account specified in TxConfig
state/core_access.go
Outdated
|
||
response, err := client.SubmitPayForBlobWithAccount(ctx, account.Name(), libBlobs, opts...) | ||
var response *user.TxResponse | ||
if account.Name() == ca.defaultSignerAccount { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that this is ugly, but until we have TxClient refactor, it will remain ugly.
Also, once we get Signer
field in TxResponse
, we should implement unit test for this. For now, manually tested.
For documentation purposes (cc @jcstein)This PR introduces parallel transaction submission lanes #5776. Parallel Transaction Submission LanesNode runners enable this feature by setting [State]
DefaultKeyName = "my_celes_key"
DefaultBackendName = "test"
EstimatorAddress = ""
EnableEstimatorTLS = false
TxWorkerAccounts = 8
Example:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
We should also mark this change as (config) breaking btw
generally correct, but I could see room for confusion
might be worth refactoring this to state the 1 thing first, as the "cannot use this new feature" thing might cause confusion. ofc, we can use this new feature and it actually provides more sequential ordering than before since we don't need to resign. The current TxClient is actually incapable of sequential ordering as is fwiw. The bigger diff here might be that the blobs come from different accounts.
imo it would clearer to state something along the lines of: Setting a value to 0 preservers the previous behavior of submitting multiple blobs from the same account. This is not recommended due to hitting many sequence mismatches and low throughput. Setting a value of 1 is breaking in that blobs are queued and submitted one by one. This avoid sequence mismatches and preserves the original order of the blobs in almost all cases. |
@evan-forbes we do not even allow a TxWorker count of 0 -- we default to 1 worker. We enforce this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't approve my own PR but I approve the changes
This is a draft until we get an official release from app, but it would be super useful for me to get reviews from node folks as I do not know what I'm doing in this repo 😅
Currently, node automatically submits blobs asynchronously. Meaning we can submit multiple per block, but the ordering isn't guaranteed.
This adds the ability to submit blobs in parallel and the ability to submit blobs synchronously.
Synchronous (workers == 1) has a single worker that has a queue of blobs. It submits the blob, waits for it to be confirmed, and then submits the next blob in the queue. This doesn't guarantee order, as its possible a tx fails or gets evicted and the next blob in the queue does not. However most importantly avoids sequence mismatches since we're only ever submitting a single blob from an account at once. The account that is submitting the blob here is the one passed to TxClient.
Parallel submission works in the exact same way, except there are more than one worker. In order to manage these accounts, it creates new ones and grants them a feegrant by submitting a single transaction for all accounts that don't already have this. This ofc means that different accounts will submit blobs. All accounts are in the same keyring that has the main account.