feat: Transfer event removal #701
Conversation
|
Do we need to fully ignore Transfer events? e.g. they appear and impact log indexes, is it okay to be this way or it is better to somehow change this behaviour? |
Artemka374
changed the title
# Conflicts: # core/lib/dal/sqlx-data.json # core/lib/dal/src/events_web3_dal.rs # core/lib/dal/src/transactions_web3_dal.rs
…nto afo/remove-transactions-event
# Conflicts: # core/lib/dal/src/models/storage_event.rs # core/lib/zksync_core/src/api_server/web3/mod.rs # core/lib/zksync_core/src/api_server/web3/pubsub.rs # core/lib/zksync_core/src/api_server/web3/tests/mod.rs # core/lib/zksync_core/src/api_server/web3/tests/snapshots.rs # core/lib/zksync_core/src/api_server/web3/tests/ws.rs # core/lib/zksync_core/src/state_keeper/io/tests/mod.rs # core/lib/zksync_core/src/sync_layer/tests.rs # prover/Cargo.lock # yarn.lock
|
A really late question, sorry for not raising it before. What if the event filtering is controlled via an additional field in |
core/lib/zksync_core/src/state_keeper/io/event_indexes_migration.rs
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| /// Method checks, whether any event in the miniblock has `index_without_eth_transfer` equal to NULL, which means that indexes are not migrated. | ||
| pub async fn are_event_indexes_migrated( |
There was a problem hiding this comment.
Is it strictly necessary to check all miniblocks in the range? Note that in #688, it's sufficient to check only the starting miniblock of the range provided that ranges are guaranteed to be identical across node restarts. Would a similar assumption not apply here? AFAIU, checking the entire range would put significantly more load on the DB if the range is reasonably large.
@slowli I believe most people use SDKs (ethers and such) for that, and in most cases modifying the logic of interacting with filters is hidden. Probably it won't work. |
|
@popzxc For ethers in particular, the interceptor abstraction I've talked about is encapsulated in the interface FilterWithEthTransfers extends Filter {
filterEthTransfers?: boolean;
}
class CustomizableProvider extends zksync.Provider {
private filterEthTransfers: boolean;
override async _getFilter(filter: Filter | /* ... */): Promise<FilterWithEthTransfers | /* ... */> {
const resolvedFilter = <FilterWithEthTransfers>await super._getFilter(filter);
resolvedFilter.filterEthTransfers = this.filterEthTransfers; // passed to JSON-RPC call
return resolvedFilter;
}
}IMO, controlling filtering on the client side makes the approach more maintainable and easier to reason about, both for us and clients. OTOH, I may be unaware of complexities associated with modifying and maintaining client code. |
|
@slowli I think that if anything more complex than just "swap URL" would be required, people would just not bother. Though I may be wrong, cc @RomanBrodetski |
| export async function extractFee(receipt: zksync.types.TransactionReceipt, wallet: zksync.Wallet): Promise<Fee> { | ||
| const trace = await wallet!.provider.send('debug_traceTransaction', [receipt.transactionHash]); | ||
|
|
||
| if (!trace) { |
There was a problem hiding this comment.
why do we need this check? Having the extractFee as async function is not bad, but most of the diff in tests comes from it, so just curious
There was a problem hiding this comment.
With this method we are checking whether all the balances were updated correctly(along with fees) and since in this new mode we can't just ask for receipt for native transfers - this is the only way to get fee
# Conflicts: # .github/workflows/ci-core-reusable.yml # core/lib/zksync_core/src/api_server/web3/tests/mod.rs
| ALTER TABLE events | ||
| ADD COLUMN IF NOT EXISTS event_index_in_block_without_eth_transfer INT DEFAULT NULL; | ||
| ALTER TABLE events | ||
| ADD COLUMN IF NOT EXISTS event_index_in_tx_without_eth_transfer INT DEFAULT NULL; |
There was a problem hiding this comment.
According to quick research
In Postgres 10 or older (such as OP's 9.6), any DEFAULT clause even DEFAULT NULL will require a full rewrite of the table, which might take essentially forever for large tables[1].
so this is not safe. We can try it on stage2 (although please copy a subset of table first, don't amend eevents table there) , but I think it won't work.
We may find a better migration approach (just remove DEFAULT?) but even then we might negatively impact insert time to this table which will result in lower TPS. Is there an alternative solution?
There was a problem hiding this comment.
We don't use postgres 10 or older, don't we? I thought we're at 14 or at least 12?
There was a problem hiding this comment.
E.g. from the same response (I guess) below:
This was changed with Postgres 11, Vao Tsun's answer is only correct for these, per the Postgres 11 documentation:
When a column is added with ADD COLUMN and a non-volatile DEFAULT is specified, the default is evaluated at the time of the statement and the result stored in the table's metadata. That value will be used for the column for all existing rows. If no DEFAULT is specified, NULL is used. In neither case is a rewrite of the table required.
So as long as DEFAULT is a static value or a non-volatile expression the table will not need to be rewritten.
There was a problem hiding this comment.
oh sorry I misread it, you are right. Still wanted to ask if anyone sees an alternative solution so that we don't need to change schema, store more data per event and migrate that data as well
# Conflicts: # core/bin/external_node/src/main.rs # core/lib/dal/src/events_dal.rs # core/lib/zksync_core/src/api_server/web3/tests/mod.rs # core/lib/zksync_core/src/state_keeper/io/tests/mod.rs # core/lib/zksync_core/src/sync_layer/tests.rs
No performance difference detected (anymore) |
|
@Artemka374 is this PR still relevant or can it be closed? |
|
I don't know to be honest. The discussion about it was brought up again not that long ago, but I don't remember any decision was made. Maybe @RomanBrodetski or @StanislavBreadless can say more? |
|
I believe it will be needed in the long term anyway, so better to finish it up and get into production |
What ❔
Divide web3 server API into 2:
API that will be run per instance is defined via config:
Added new columns to
eventsdatabase to reassign indexes of logs correctly(added SQL migrations and Rust migrations, mostly inspired by #688):event_index_in_block_without_eth_transferevent_index_in_tx_without_eth_transferWhy ❔
To be EVM compatible.
Checklist
zk fmtandzk lint.cargo spellcheck --cfg=./spellcheck/era.cfg --code 1.