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

fix(rpc): Check for panics in the transaction queue #4046

Merged
merged 3 commits into from
Apr 6, 2022

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Apr 6, 2022

Motivation

In PR #4015, we want to check that the transaction queue code does not panic.

Solution

  • Check for panics in the RPC transaction queue in the start task
  • Check for panics in the RPC transaction queue in the tests

Related fixes:

  • Add missing pin and abort in the start task

Review

@oxarbitrage wrote PR #4015.

This PR is based on PR #4015.

Reviewer Checklist

  • Code implements Specs and Designs
  • Tests for Expected Behaviour
  • Tests for Errors

@teor2345 teor2345 added C-enhancement Category: This is an improvement P-Low ❄️ A-rpc Area: Remote Procedure Call interfaces lightwalletd any work associated with lightwalletd labels Apr 6, 2022
@teor2345 teor2345 requested a review from oxarbitrage April 6, 2022 04:38
@teor2345 teor2345 self-assigned this Apr 6, 2022
@teor2345 teor2345 requested a review from a team as a code owner April 6, 2022 04:38
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, i see what you mean now and makes sense. Thank you for working on it.

@oxarbitrage oxarbitrage merged commit ecfb7ed into issue3654 Apr 6, 2022
@oxarbitrage oxarbitrage deleted the issue3654-check-panics branch April 6, 2022 13:19
teor2345 added a commit that referenced this pull request Apr 12, 2022
* Add a rpc queue

* Implement the rpc queue

* Add rpc queue tests

* Remove mutex, use broadcast channel

* Have order and limit in the queue

* fix multiple transactions channel

* Use a network argument

* Use chain tip to calculate block spacing

* Add extra time

* Finalize the state check test

* Add a retry test

* Fix description

* fix some docs

* add additional empty check to `Runner::run`

* remove non used method

* ignore some errors

* fix some docs

* add a panic checker to the queue

* add missing file changes for panic checker

* skip checks and retries if height has not changed

* change constants

* reduce the number of queue test cases

* remove suggestion

* change best tip check

* fix(rpc): Check for panics in the transaction queue (#4046)

* Check for panics in the RPC transaction queue

* Add missing pin! and abort in the start task

* Check for transaction queue panics in tests

* Fixup a new RPC test from the main branch

Co-authored-by: teor <teor@riseup.net>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rpc Area: Remote Procedure Call interfaces C-enhancement Category: This is an improvement lightwalletd any work associated with lightwalletd
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants