-
Notifications
You must be signed in to change notification settings - Fork 9
feat: parallel package for precompile pre-processing
#228
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: main
Are you sure you want to change the base?
Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Arran Schlosberg <519948+ARR4N@users.noreply.github.com>
| // non-blocking, however it is NOT possible to overwrite the value without an | ||
| // intervening call to [eventual.getAndKeep]. | ||
| func (e eventual[T]) set(v T) { | ||
| e.ch <- v |
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.
If set() is called twice without an intervening getAndKeep(), the second call blocks forever (channel buffer size is 1). This would deadlock the entire block processing.
I did notice that the finishBlock method drains all eventuals to reset them for the next block, so reuse is required, which means guarding with a sync.Once is not an option.
What I would suggest as an optional improvement without changing the method's signature is:
func (e eventual[T]) set(v T) {
select {
case e.ch <- v:
default:
panic("eventual: set called on non-empty eventual (double-set without intervening getAndKeep)")
}
}I deem this as appropriate because:
- Double-set is always a programming error in the orchestration logic, never expected behavior.
- Failing fast with a clear message is better than a silent deadlock.
- The existing
goleak.VerifyTestMainwould catch any test that triggers this.
But I don't defend this strongly if there is another design consideration that would either treat it as a tradeoff or something else.
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.
Double-set is always a programming error in the orchestration logic, never expected behavior.
Exactly, which means this must be detected in development. Is the goal of your suggested signature to make it easier to diagnose deadlocks?
| // We can reuse the channels already in the data and results slices because | ||
| // they're emptied by [wrapper.process] and [wrapper.finishBlock] | ||
| // respectively. | ||
| for i := len(w.results); i < w.totalTxsInBlock; i++ { | ||
| w.data = append(w.data, eventually[D]()) | ||
| w.results = append(w.results, eventually[result[R]]()) | ||
| } |
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 understanding on bounds here is the following:
- This is bounded by the largest block ever processed, not truly unbounded.
- Each unused
eventualis maybe ~100 bytes (struct + channel overhead), so 10,000 unused elements would roughly equal 1MB. - You did mention that the reuse is intentional per the comment: We can reuse the channels already in the data and results slices.
Optionally we can play around with some slice trimming logic if we want to avoid potential memory pressure. Something like if w.totalTxsInBlock < cap(w.results)/4 && cap(w.results) > 1000, then we can trim excess capacity from w.data and w.results.
Having said that, maybe it's not worth doing unless we can prove with a benchmark test that this will be a noticeable issue, but just throwing this as a suggestion.
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 bounded by the largest block ever processed, not truly unbounded.
Correct
Having said that, maybe it's not worth doing unless we can prove with a benchmark test that this will be a noticeable issue, but just throwing this as a suggestion.
I'm leaning towards this. 10k transactions in a block is biiiig1 and even then it's only 1MB.
Footnotes
-
210M gas for regular transfers using
params.TxGas(21k). ↩
| // goroutine guaranteed to have completed by the time a respective | ||
| // getter unblocks (i.e. in any call to [wrapper.prefetch]). | ||
| w.common.set(w.BeforeBlock(sdb, types.CopyHeader(b.Header()))) |
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.
Mhm, I think that potentially there is a risk here that could halt block processing:
- If
BeforeBlockpanics, the goroutine dies silently,w.commonis never set, and subsequent calls tow.common.getAndReplace()inshouldProcess()will block forever. - Since
shouldProcessis called synchronously inStartBlock's transaction loop, this causesStartBlockto hang indefinitely. Handler.BeforeBlockis user-provided code, making panics possible. A single handler panic would silently halt all block processing.
A proposed solution here would be to add a panic-recover that intercepts the panic and sets w.common to unblock the waiters.
Let me know your thoughts here.
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.
If
BeforeBlockpanics, the goroutine dies silently
It won't be silent:
type anxious struct {
Handler[struct{}, struct{}, struct{}, struct{}]
}
func (anxious) BeforeBlock(libevm.StateReader, *types.Header) (_ struct{}) { panic(99) }
func TestBuildBlockPanic(t *testing.T) {
p := New(8, 8)
t.Cleanup(p.Close)
AddHandler(p, &anxious{})
_, _, sdb := ethtest.NewEmptyStateDB(t)
rules := params.MergedTestChainConfig.Rules(big.NewInt(0), true, 0)
b := types.NewBlock(
&types.Header{Number: big.NewInt(1)},
types.Transactions{},
nil, nil, trie.NewStackTrie(nil),
)
require.NoError(t, p.StartBlock(sdb, rules, b))
p.FinishBlock(sdb, b, nil)
}panic: 99
goroutine 3 [running]:
github.com/ava-labs/libevm/libevm/precompiles/parallel.anxious.BeforeBlock(...)
Why this should be merged
EVM parallel co-processors that interface with the regular transaction path via precompiles.
How this works
Introduces the
parallel.Processor, which orchestrates a set ofparallel.Handlers. EachHandlerperforms arbitrary, strongly typed processing of any sub-set of transactions in a block and makes its results available to a precompile and/or a post-block method for persisting state. Although stateful,Handlers can only read the pre-block and post-block state, which isolates them from conflicts with the regular transaction path.There is deliberately no support for a precompile to "write" to a
Handler, only to "read". This is because the transaction might still revert, which would also have to be communicated to theHandler, resulting in unnecessary complexity. Logs/events are the recommended approach for precompile ->Handlercommunication, to be read from thetypes.Receiptsat the end of the block.How this was tested
Integration tests covering:
Handler.