Skip to content
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

Remove contract event topic limits #958

Closed
jayz22 opened this issue Jul 19, 2023 · 4 comments · Fixed by #959
Closed

Remove contract event topic limits #958

jayz22 opened this issue Jul 19, 2023 · 4 comments · Fixed by #959
Assignees

Comments

@jayz22
Copy link
Contributor

jayz22 commented Jul 19, 2023

We have a few limits in the contract event topics:

  • No. topics <= 4
  • Topic byte size < 64

These were some arbitrary limits put there initially, but are no longer needed. They are also not enforced consistently (diagnostic event contains a ContractEvent but we don't check that limit). Contract events are hashed into the transaction result, both contract and diagnostic contract events are included in the meta and is subject to limits and fees.

@jayz22 jayz22 self-assigned this Jul 19, 2023
@jayz22
Copy link
Contributor Author

jayz22 commented Jul 24, 2023

On second thought, these limits should not apply to diagnostic events (the current behavior is correct), since diagnostic events should be infallible (see #962 to validate that).

@jayz22
Copy link
Contributor Author

jayz22 commented Jul 24, 2023

I think it is necessary to more clearly spell out the event limits as network config parameters.

  • number of topics
  • single topic size limit
  • data size limit

Without these limits, contracts may DOS the downstream system by submitting many large events (each up to the metadata size limit).

They need to be specified in the CAP (stellar/stellar-protocol#1371) and the host need to adapt to them.

@jayz22 jayz22 changed the title Remove contract event topic limits Clearly specify event limits Jul 24, 2023
@sisuresh
Copy link
Contributor

The 4 limit was chosen arbitrarily since that's the limit of topics in the evm. We don't need to follow that though because the topics are not indexed in the protocol like in Ethereum.

@jayz22
Copy link
Contributor Author

jayz22 commented Jul 27, 2023

Based on recent discussions, downstream are more concerned with the meta data size to maintain than the number of topics. Thus we've decided to move the topics limit.
The meta size limit has been repurposed to event size limit in https://github.com/stellar/stellar-xdr/pull/127/files, and will be set to a much lower value ~10k to limit the downstream impact.

@jayz22 jayz22 changed the title Clearly specify event limits Remove contract event topic limits Jul 27, 2023
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 a pull request may close this issue.

2 participants