-
Notifications
You must be signed in to change notification settings - Fork 705
Add ArbFilteredTransactionsManager precompile #4174
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: master
Are you sure you want to change the base?
Add ArbFilteredTransactionsManager precompile #4174
Conversation
1adc79f to
81f4e5e
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4174 +/- ##
==========================================
- Coverage 33.12% 29.48% -3.64%
==========================================
Files 462 464 +2
Lines 55958 56132 +174
==========================================
- Hits 18534 16549 -1985
- Misses 34168 36582 +2414
+ Partials 3256 3001 -255 |
❌ 6 Tests Failed:
View the top 3 failed tests by shortest run time
📣 Thoughts on this report? Let Codecov know! | Powered by Codecov |
45fb413 to
67c13f2
Compare
7b7a735 to
f56f36c
Compare
Tristan-Wilson
left a comment
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.
Looks good, just a minor comment about consistency of the interface with other precompiles.
| return filteredState.IsFiltered(txHash) | ||
| } | ||
|
|
||
| func (con ArbFilteredTransactionsManager) hasAccess(c *Context) (bool, error) { |
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 signature is slightly different to ArbNativeTokenManager which has
func (con ArbNativeTokenManager) hasAccess(c ctx) bool {
manager, err := c.State.NativeTokenOwners().IsMember(c.caller)
return manager && err == nil
}
and therefore the behavior when there is an error is slightly different in this implementation in that if there is an error, then the gas is not burned out.
Unsure if this difference is intentional or not.
|
We should consider making |
@Tristan-Wilson, wdyt about going further: remove ArbNativeToken-style check and return an error right in CensorPrecompile, like in ArbOwner? |
| "github.com/offchainlabs/nitro/solgen/go/precompilesgen" | ||
| ) | ||
|
|
||
| func TestManageTransactionCensors(t *testing.T) { |
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.
Can you please also test that the transactions that should be free are free? You can look at TestArbNativeTokenManager and getGasUsed inside it for inspiration.
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 don't really get how to use getGasUsed in this case, since the transaction will still cost something, only the add/delete call is free. I can suggest to use multigas like this:
tx, err = arbFilteredTxs.AddFilteredTransaction(&censorTxOpts, txHash)
require.NoError(t, err)
receipt, err := builder.L2.EnsureTxSucceeded(tx)
require.NoError(t, err)
require.Equal(t, uint64(0), receipt.MultiGasUsed.Get(multigas.ResourceKindStorageAccess))
This somehow proves that tx did not perform any storage operations like set and clear
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.
Could you check the censor account's balance is unchanged after adding/deleting filtered txs?
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 precompile wrapper does not make the transaction completely free, but I still believe that this test is useful because it allows to verify that the call goes through the wrapper
2fad658 to
85d7766
Compare
85d7766 to
9d83f9c
Compare
precompiles/wrapper.go
Outdated
| evm, | ||
| ) | ||
| if err != nil { | ||
| return output, gasSupplied, multigas.ZeroGas(), err |
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.
Gas charging decision based on if the caller is a filterer should be done independently of this err.
With current implementation, if the caller is not authorized, then this err will not be nil, and multigas.ZeroGas() will be charged.
This is not the wanted behavior.
Also, how about adding a test, that would have caught this?
Not sure yet if it is straightforward to do this test though, so we can discuss it out of github if you find it will take too much time to implement, and evaluate if it is worth to add it.
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.
Good catch, I overlooked it. I don’t see how this exact issue is reachable from a system test, so I added a precompile-level test
|
|
||
| // Initially neither owner nor user can modify filtered transactions, | ||
| // but both can read (get) filtered status | ||
| _, err = arbFilteredTxs.IsTransactionFiltered(ownerCallOpts, txHash) |
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.
nitpick: check that returned value is 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.
fixed
| } | ||
| ev, parseErr := arbOwner.ParseTransactionFiltererAdded(*lg) | ||
| if parseErr != nil { | ||
| continue |
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.
nitpick: why continuing instead of requiring that parseErr is 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.
fixed
Fixes NIT-4152 and NIT-4253
pulls in OffchainLabs/nitro-precompile-interfaces#25
pulls in OffchainLabs/go-ethereum#604
pulls in OffchainLabs/nitro-testnode#169
Changes:
ArbFilteredTransactionsManagerto manage filtered transactionsArbFilteredTransactionsManager