custodian: retry proof retrieval indefinitely and avoid fatal receive errors #1927
Conversation
Summary of ChangesHello @ffranr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the robustness of the custodian service by implementing indefinite retries for proof retrieval and making most receive and inspection errors non-fatal. These changes aim to prevent Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces two main improvements for the custodian: it enables indefinite retries for proof retrieval and makes error handling more robust by treating transient errors as non-fatal. This is achieved by adding an unlimitedtries option to the backoff configuration and updating the custodian's event loop to log and continue on non-critical errors. The changes are well-implemented, and I have a few suggestions to improve code style and maintainability.
Ensure proof retrieval has unlimited retries.
ffranr
force-pushed
the
tpm/issues/1923-universe-proof-crash
branch
from
6e1ed7f to
ebd8140
Compare
Pull Request Test Coverage Report for Build 20722871752Details
💛 - Coveralls |
| // UnlimitedTries indicates that we should retry indefinitely until the | ||
| // transfer succeeds or the context is canceled. If true, NumTries must | ||
| // be zero. | ||
| UnlimitedTries bool `long:"unlimitedtries" description:"Retry indefinitely instead of stopping after a fixed number of attempts."` |
There was a problem hiding this comment.
Hmm, not sure we should expose such a setting like this. Could end up being accidental DoS.
| log.Errorf("Unable to check proof availability for "+ | ||
| "event (outpoint=%v): %v", event.Outpoint, err) | ||
|
|
||
| if fn.ErrorAs[*fn.CriticalError](err) { |
There was a problem hiding this comment.
Are we actually using CriticalError anywhere? IMO this is the fundamental fix: don't need to fatalf/criticalf when we fail to fetch as proof.
github-project-automation
bot
moved this from 🆕 New
to 👀 In review
in Taproot-Assets Project Board
|
Replaced by #1941 which is more narrowly focused on the early termination problem and does not add indefinite retry to proof retrieval. |
github-project-automation
bot
moved this from 👀 In review
to ✅ Done
in Taproot-Assets Project Board
Fixes #1923.
Summary
unlimitedtriesto proof backoff and use it for custodian receive paths so missing universe proofs don’t exhaust retriesunlimitedtriesinsample-tapd.confand add courier config validation helpers