-
Notifications
You must be signed in to change notification settings - Fork 0
Prioritize randomness transactions #1
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: happy-current-version
Are you sure you want to change the base?
Prioritize randomness transactions #1
Conversation
| delete(pendingBlobTxs, randomnessOwner) | ||
| } | ||
|
|
||
| if len(privilegedPlainTxs) > 0 || len(privilegedBlobTxs) > 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.
do we need to "prioritize" blob type transactions too?
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.
Actually, it is not needed, but if you read the code below, you’ll see that they split the transaction into two groups: one for local and one for remote. In each of them, they process the blobs for every address together with the local ones. I can change it, but in my opinion, it slightly breaks the pattern. What do you think?
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.
Yeah I think leave as you have implemented for now
| for attempt := 1; attempt <= attempts; attempt++ { | ||
| // Gas prices: priority < non-priority (reversed from normal scenario) | ||
| priorityGasPrice := big.NewInt(params.InitialBaseFee * 5) | ||
| nonPriorityGasPrice := big.NewInt(params.InitialBaseFee * 50) |
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 cool - i also tried changing the nonPriorityGasPrice to be lower than the priorityGasPrice and the test still passes which makes sense, the transactions are ordered in priority regardless of gasPrice (which is what we want), though i think it makes most sense to test for this case
| } | ||
| } | ||
|
|
||
| if minedBlock == nil { |
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 think this check is redundant?
| } | ||
| } | ||
|
|
||
| func newCustomTestWorkerBackend(t *testing.T, chainConfig *params.ChainConfig, engine *clique.Clique, db ethdb.Database, chain *core.BlockChain) *testWorkerBackend { |
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 think we dont need params t and engine
| Config: &config, | ||
| Timestamp: 1, | ||
| ExtraData: extra, | ||
| Alloc: core.GenesisAlloc{ |
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.
my vscode tells me core.GenesisAlloc is deprecated, changing to the suggested types.GenesisAlloc works in place
|
|
||
| priorityAddressHash := common.BytesToHash(common.LeftPadBytes(priorityAddress.Bytes(), 32)) | ||
|
|
||
| otherKey, _ := crypto.GenerateKey() |
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.
nit: renaming suggestion: nonPriorityKey and nonPriorityAddress
| } | ||
|
|
||
| // Create a worker | ||
| w := newWorker(priorityConfig, &config, engine, backend, new(event.TypeMux), nil, false) |
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.
event.TypeMux also marked as deprecated, but the suggested replacement doesnt work seemingly without changing the newWorker signature so just leave as is
| time.Sleep(1 * time.Second) | ||
| continue | ||
| } | ||
| minedBlock = block |
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.
fwiw blockchain.InsertChain() also works here but i dont think its necessary.
This PR aims to modify op-geth so that the randomness service transaction is always executed first. To accomplish this, the PR adds a new command parameter to specify the randomness service contract. Then, during block building, the owner account of the randomness contract is checked in order to prioritize transactions from that address