Reattempt proof delivery on node restart #1055
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit also adds a field doc comment.
ffranr
force-pushed
the
retry-proof-delivery
branch
from
d1087a9
to
d4573cb
Compare
ffranr
changed the title
TransferOutput.ShouldDeliverProof such that we don't retry proof deliver if complete
ffranr
force-pushed
the
retry-proof-delivery
branch
3 times, most recently
from
987a93e
to
bd73f91
Compare
guggero
reviewed
Aug 7, 2024
There was a problem hiding this comment.
Code looks good, nice work.
Just to clarify: There will be a follow-up PR that actually addresses the issue that a transfer isn't completed (e.g. a call to ConfirmProofDelivery) while the proof transfer wasn't successful?
Or was the goal to address this here (might need some re-ordering of actions)?
@guggero Thanks for the review! I will put up another PR for the |
- Set the `ProofDeliveryComplete` field in the transfer output struct during its initialization. - The `insertAssetTransferOutput` function now only marshals the `ProofDeliveryComplete` status for database storage. It does not determine the status at the time of storing in the database. - `TransferOutput.ShouldDeliverProof` will return `false` if `ProofDeliveryComplete == fn.Some(true)`. - Improved logging for scenarios where proof delivery is skipped, specifically when `out.ShouldDeliverProof()` returns `false`.
Remove inaccurate comments. Rephrase and reword comments for clarity.
Refactor to perform asset minting and node setup before subscribing to the send notification events from the sending node. This change aims to improve test clarity and readability.
This commit moves code into a new `ChainPorter` method called `resumePendingParcels`.
This commit introduces a new logging method to the `ChainPorter`. The method captures and logs transfer outputs with delivery pending proofs.
This commit adds a test which ensures that a failed attempt at transferring a transfer output proof to a proof courier will be reattempted by the sending tapd node upon restart.
ffranr
force-pushed
the
retry-proof-delivery
branch
from
bd73f91
to
332a550
Compare
jharveyb
approved these changes
Aug 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some of the commits in this PR refactor the code for clarity and don't modify functionality.
The overall goal of this PR is to ensure that we don't try to deliver proofs which have already been delivered. And that we reattempt proof delivery on node restart.
Related to #1009