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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: persistTransaction timeout #1224

Merged
merged 10 commits into from Nov 10, 2023
Merged

Conversation

MantisClone
Copy link
Member

@MantisClone MantisClone commented Oct 27, 2023

Resolves #1145

Context

The persistTransaction() method of the http-data-access implementation works like this:

  1. it calls the request-node to persist a new transaction with POST /persistTransaction
  2. then it waits for the transaction to be confirmed in a retry loop: it calls GET /getConfirmedTransaction multiple times until it does not receive an HTTP error anymore (normally it's NOT FOUND 404). By default, it polls 500 times with a 3-second delay for a total of 25 minutes! 馃槺
  3. Meanwhile, the request-node tries to confirm the transaction via the storage-subgraph. However, if the graph node is running slow, perhaps due to a rate-limited Ethereum Node RPC, this transaction confirmation will timeout after 30 seconds.
  4. IF the confirmation via the storage subgraph times out, the request-node never again tries to confirm it, and the /getConfirmedTransaction will forever return NOT FOUND 404

Changes

  • Reduce the default getConfirmationMaxRetry from 500 to 30 and the default getConfirmationRetryDelay from 3 seconds to 1 second.
  • Improve error logging request-node server side to better explain what happened in the case of a timeout.
  • Improve error logging on the http-data-access client side to explain to the persistTransaction() caller that they must continue polling the getTransactionsByChannelId() if they still need to confirm that their transaction was stored.
  • Improve error handling in the request-node: Store an Error in the confirmedTransactionStore when the POST /persistTransaction endpoint fails. Anyone who calls the GET /getConfirmedTransaction can see the Error.

TODO

- [ ] Add tests

  * Improve error log
  * Default timeout = 3000 ms defer + (30 retries * 1000 ms delay) = 33 seconds
  * Aligns with default timeout fetching from storage-subgraph = 30 seconds
  * This tells the user to stop polling /getConfirmedTransaction because
    the transaction will never be confirmed.
@MantisClone MantisClone marked this pull request as draft October 27, 2023 22:24
@MantisClone
Copy link
Member Author

MantisClone commented Oct 27, 2023

I converted to Draft because I should probably add some tests.

@coveralls
Copy link

coveralls commented Oct 27, 2023

Coverage Status

coverage: 86.918% (-0.01%) from 86.93%
when pulling 2bd8f21 on fix-persist-transaction-timeout
into 9088c79 on master.

@MantisClone MantisClone marked this pull request as ready for review November 10, 2023 21:29
@MantisClone
Copy link
Member Author

I decided to skip writing tests.

@MantisClone MantisClone merged commit b341203 into master Nov 10, 2023
27 checks passed
@MantisClone MantisClone deleted the fix-persist-transaction-timeout branch November 10, 2023 21:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants