-
Notifications
You must be signed in to change notification settings - Fork 707
Sequencer calls cmd/transaction-filterer #4294
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
Changes from 37 commits
b93225e
235cfee
e250a63
6d37e9b
638b7ee
e95384c
c42252f
c9645de
45d1dd3
b6eccb4
801ca8f
b244782
40ea668
c5bca58
cdd0916
eb88a51
2559d24
1f53f82
bb75495
5cc32a9
cb5c1ae
2ee93f0
6577316
7ef5272
9857f64
e85704a
4baedff
207928a
ad740b0
9c5f489
73c24bc
fb68449
8200822
d1ae878
f00b17c
687e1d1
8c207bb
d7479ee
10e8cf9
a918047
3cffec2
7f334f1
5221bee
55fbbef
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| ### Added | ||
| - Sequencer calls transaction-filterer command if delayed transaction was filtered |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,6 +5,8 @@ package api | |
|
|
||
| import ( | ||
| "context" | ||
| "sync" | ||
| "testing" | ||
|
|
||
| "github.com/ethereum/go-ethereum/accounts/abi/bind" | ||
| "github.com/ethereum/go-ethereum/common" | ||
|
|
@@ -15,18 +17,22 @@ import ( | |
| "github.com/ethereum/go-ethereum/p2p" | ||
| "github.com/ethereum/go-ethereum/rpc" | ||
|
|
||
| "github.com/offchainlabs/nitro/execution/gethexec" | ||
| "github.com/offchainlabs/nitro/solgen/go/precompilesgen" | ||
| ) | ||
|
|
||
| const namespace = "transactionfilterer" | ||
|
|
||
| type TransactionFiltererAPI struct { | ||
| apiMutex sync.Mutex // avoids concurrent transactions with the same nonce | ||
|
|
||
| arbFilteredTransactionsManager *precompilesgen.ArbFilteredTransactionsManager | ||
| txOpts *bind.TransactOpts | ||
| } | ||
|
|
||
| // Filter adds the given transaction hash to the filtered transactions set, which is managed by the ArbFilteredTransactionsManager precompile. | ||
| func (t *TransactionFiltererAPI) Filter(ctx context.Context, txHashToFilter common.Hash) (common.Hash, error) { | ||
| t.apiMutex.Lock() | ||
| defer t.apiMutex.Unlock() | ||
|
|
||
| txOpts := *t.txOpts | ||
| txOpts.Context = ctx | ||
|
|
||
|
|
@@ -41,19 +47,37 @@ func (t *TransactionFiltererAPI) Filter(ctx context.Context, txHashToFilter comm | |
| } | ||
| } | ||
|
|
||
| // Only used for testing. | ||
| // Sequencer and TransactionFiltererAPI depend on each other, as a workaround for the egg/chicken problem, | ||
| // we set the sequencer client after both are created. | ||
| func (t *TransactionFiltererAPI) SetSequencerClient(_ *testing.T, sequencerClient *ethclient.Client) error { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. using testing.T input in code that's only used for testing helps make sure it's not used anywhere outside of tests by mistakes. Binary size is irrelevant, having a function that can be called from tests and only from tests is useful.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. also, since no member function of testing.T is called I don't believe even binary size will increase
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My worries is not only the binary size, but the test-related runtime or execution. |
||
| arbFilteredTransactionsManager, err := precompilesgen.NewArbFilteredTransactionsManager( | ||
| types.ArbFilteredTransactionsManagerAddress, | ||
| sequencerClient, | ||
| ) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| t.apiMutex.Lock() | ||
| defer t.apiMutex.Unlock() | ||
| t.arbFilteredTransactionsManager = arbFilteredTransactionsManager | ||
| return nil | ||
| } | ||
|
|
||
| var DefaultStackConfig = node.Config{ | ||
| DataDir: "", // ephemeral | ||
| HTTPPort: node.DefaultHTTPPort, | ||
| AuthAddr: node.DefaultAuthHost, | ||
| AuthPort: node.DefaultAuthPort, | ||
| AuthVirtualHosts: node.DefaultAuthVhosts, | ||
| HTTPModules: []string{namespace}, | ||
| HTTPModules: []string{gethexec.TransactionFiltererNamespace}, | ||
| HTTPHost: node.DefaultHTTPHost, | ||
| HTTPVirtualHosts: []string{"localhost"}, | ||
| HTTPTimeouts: rpc.DefaultHTTPTimeouts, | ||
| WSHost: node.DefaultWSHost, | ||
| WSPort: node.DefaultWSPort, | ||
| WSModules: []string{namespace}, | ||
| WSModules: []string{gethexec.TransactionFiltererNamespace}, | ||
| GraphQLVirtualHosts: []string{"localhost"}, | ||
| P2P: p2p.Config{ | ||
| ListenAddr: "", | ||
|
|
@@ -66,31 +90,31 @@ func NewStack( | |
| stackConfig *node.Config, | ||
| txOpts *bind.TransactOpts, | ||
| sequencerClient *ethclient.Client, | ||
| ) (*node.Node, error) { | ||
| ) (*node.Node, *TransactionFiltererAPI, error) { | ||
| stack, err := node.New(stackConfig) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| arbFilteredTransactionsManager, err := precompilesgen.NewArbFilteredTransactionsManager( | ||
| types.ArbFilteredTransactionsManagerAddress, | ||
| sequencerClient, | ||
| ) | ||
| if err != nil { | ||
| return nil, err | ||
| return nil, nil, err | ||
| } | ||
|
|
||
| api := &TransactionFiltererAPI{ | ||
| arbFilteredTransactionsManager: arbFilteredTransactionsManager, | ||
| txOpts: txOpts, | ||
| } | ||
| apis := []rpc.API{{ | ||
| Namespace: namespace, | ||
| Namespace: gethexec.TransactionFiltererNamespace, | ||
| Version: "1.0", | ||
| Service: api, | ||
| Public: true, | ||
| }} | ||
| stack.RegisterAPIs(apis) | ||
|
|
||
| return stack, nil | ||
| return stack, api, nil | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -169,7 +169,8 @@ type ExecutionEngine struct { | |
|
|
||
| runningMaintenance atomic.Bool | ||
|
|
||
| addressChecker state.AddressChecker | ||
| addressChecker state.AddressChecker | ||
| transactionFiltererRPCClient *TransactionFiltererRPCClient | ||
| } | ||
|
|
||
| func NewL1PriceData() *L1PriceData { | ||
|
|
@@ -185,15 +186,22 @@ func init() { | |
| } | ||
| } | ||
|
|
||
| func NewExecutionEngine(bc *core.BlockChain, syncTillBlock uint64, exposeMultiGas bool) (*ExecutionEngine, error) { | ||
| return &ExecutionEngine{ | ||
| bc: bc, | ||
| resequenceChan: make(chan []*arbostypes.MessageWithMetadata), | ||
| newBlockNotifier: make(chan struct{}, 1), | ||
| cachedL1PriceData: NewL1PriceData(), | ||
| exposeMultiGas: exposeMultiGas, | ||
| syncTillBlock: syncTillBlock, | ||
| }, nil | ||
| func NewExecutionEngine( | ||
| bc *core.BlockChain, | ||
| syncTillBlock uint64, | ||
| exposeMultiGas bool, | ||
| transactionFiltererRPCClient *TransactionFiltererRPCClient, | ||
| ) (*ExecutionEngine, error) { | ||
| execEngine := &ExecutionEngine{ | ||
| bc: bc, | ||
| resequenceChan: make(chan []*arbostypes.MessageWithMetadata), | ||
| newBlockNotifier: make(chan struct{}, 1), | ||
| cachedL1PriceData: NewL1PriceData(), | ||
| exposeMultiGas: exposeMultiGas, | ||
| syncTillBlock: syncTillBlock, | ||
| transactionFiltererRPCClient: transactionFiltererRPCClient, | ||
| } | ||
| return execEngine, nil | ||
| } | ||
|
|
||
| func (s *ExecutionEngine) backlogCallDataUnits() uint64 { | ||
|
|
@@ -846,6 +854,20 @@ func (s *ExecutionEngine) createBlockFromNextMessage(msg *arbostypes.MessageWith | |
| } | ||
| // Check if any txs touched filtered addresses but are not in the onchain filter | ||
| if len(filteringHooks.FilteredTxHashes) > 0 { | ||
| if s.transactionFiltererRPCClient != nil { | ||
| s.LaunchThread(func(ctx context.Context) { | ||
| // Call transaction-filterer sequentially. | ||
| // To avoid nonce collisions when adding a tx to ArbFilteredTransactionsManager, | ||
| // transaction-filterer will process only one Filter call at a time. | ||
| for _, filteredTxHash := range filteringHooks.FilteredTxHashes { | ||
| _, err := s.transactionFiltererRPCClient.Filter(filteredTxHash).Await(ctx) | ||
| if err != nil { | ||
| log.Error("error reporting filtered tx to transaction-filterer", "filteredTxHash", filteredTxHash, "err", err) | ||
| } | ||
| } | ||
| }) | ||
| } | ||
|
|
||
| return nil, nil, nil, &ErrFilteredDelayedMessage{ | ||
| TxHashes: filteringHooks.FilteredTxHashes, | ||
| DelayedMsgIdx: msg.DelayedMessagesRead - 1, | ||
|
|
@@ -1129,8 +1151,27 @@ func (s *ExecutionEngine) ArbOSVersionForMessageIndex(msgIdx arbutil.MessageInde | |
| return containers.NewReadyPromise(extra.ArbOSFormatVersion, nil) | ||
| } | ||
|
|
||
| func (s *ExecutionEngine) Start(ctx_in context.Context) { | ||
| s.StopWaiter.Start(ctx_in, s) | ||
| func (s *ExecutionEngine) StopAndWait() { | ||
| if s.transactionFiltererRPCClient != nil { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do this before the parent stopandwait.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fixed ExecutionEngine StopWaiter start/stop
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tsahee I am noop here regarding this pattern, but have question, why the order should be stopChild then stopParent?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. StopAndWait cancels the context, then waits for all threads to be done. |
||
| s.transactionFiltererRPCClient.StopAndWait() | ||
| } | ||
| s.StopWaiter.StopAndWait() | ||
| } | ||
|
|
||
| func (s *ExecutionEngine) Start(ctxIn context.Context) error { | ||
| s.StopWaiter.Start(ctxIn, s) | ||
|
|
||
| ctx, err := s.GetContextSafe() | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if s.transactionFiltererRPCClient != nil { | ||
| err := s.transactionFiltererRPCClient.Start(ctx) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to start transaction filterer RPC client: %w", err) | ||
| } | ||
| } | ||
|
|
||
| s.LaunchThread(func(ctx context.Context) { | ||
| for { | ||
|
|
@@ -1186,6 +1227,8 @@ func (s *ExecutionEngine) Start(ctx_in context.Context) { | |
| } | ||
| }) | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (s *ExecutionEngine) ShouldTriggerMaintenance(trieLimitBeforeFlushMaintenance time.Duration) bool { | ||
|
|
||
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.
use a channel with a single sink to ensure transactions are treated one by one. We should eventually have a service with some more internal state that could e.g. make sure it's filter requests pass through, retries if needed etc.. no need to catch a lock while handling a request.
Uh oh!
There was an error while loading. Please reload this page.
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 following a strategy like this, then adding a message queue layer between the sequencer and cmd/transaction-filterer is a way to move forward.
But it seems over engineering right now TBH.
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.
agree it can be done later