Skip to content

Conversation

@GabrielMartinezRodriguez
Copy link

@GabrielMartinezRodriguez GabrielMartinezRodriguez commented Jan 30, 2025

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

@GabrielMartinezRodriguez GabrielMartinezRodriguez changed the title feat: prioritize randomness transactions Prioritize randomness transactions Jan 30, 2025
delete(pendingBlobTxs, randomnessOwner)
}

if len(privilegedPlainTxs) > 0 || len(privilegedBlobTxs) > 0 {
Copy link

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?

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?

Copy link

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)
Copy link

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 {
Copy link

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 {
Copy link

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{
Copy link

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()
Copy link

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)
Copy link

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
Copy link

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.

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.

3 participants