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

[NCC-E005955-GHX] Incorrectly Disabled Consistency Check #6617

Closed
Tracked by #6277
mpguerra opened this issue May 5, 2023 · 1 comment
Closed
Tracked by #6277

[NCC-E005955-GHX] Incorrectly Disabled Consistency Check #6617

mpguerra opened this issue May 5, 2023 · 1 comment
Assignees
Labels
C-audit Category: Issues arising from audit findings

Comments

@mpguerra
Copy link
Contributor

mpguerra commented May 5, 2023

Impact

Disabling checks to pass incorrect tests rather than fixing the tests themselves may prevent the detection of bugs or other incorrect behavior.

Description

The function fetch_sprout_final_treestates() in zebra-state/src/service/check/anchors.rs contains a block of commented code that performs an assertion that “that roots match the fetched tree during tests”. The annotations suggest that this check was disabled due to bad test data:

/* TODO:
- fix tests that generate incorrect root data
- assert that roots match the fetched tree during tests
- move this CPU-intensive check to sprout_anchors_refer_to_treestates()
assert_eq!(
input_tree.root(),
joinsplit.anchor,
"anchor and fetched input tree root did not match:\n\
anchor: {anchor:?},\n\
input tree root: {input_tree_root:?},\n\
input_tree: {input_tree:?}",
anchor = joinsplit.anchor
);
*/

As is, the code suggests that a required safety check has been disabled to prevent tests from failing, which prevents the test from being run in production as well. The preferred solution would be one of:

  1. Fix the tests such that the check can remain enabled.
  2. Disable this code only when tests are running.

Initial discussions with the Zebra team suggested that the latter approach would be preferable. Furthermore, the comment suggests that this check should be refactored and located elsewhere in the code. A task to track this change should be documented.

Recommendation

  1. Re-enable the check and either add a flag to disable it during tests, or update tests to correctly pass when the check is in place.
  2. Refactor the code as recommended in the comment, or add a link to a tracked issue detailing this work item.
@mpguerra mpguerra added this to Zebra May 5, 2023
@github-project-automation github-project-automation bot moved this to 🆕 New in Zebra May 5, 2023
@teor2345
Copy link
Contributor

teor2345 commented May 7, 2023

This check was verified as redundant and the comment was deleted in PR #6450.

@teor2345 teor2345 closed this as completed May 7, 2023
@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Done in Zebra May 7, 2023
@teor2345 teor2345 self-assigned this May 7, 2023
@mpguerra mpguerra added P-Medium ⚡ C-audit Category: Issues arising from audit findings labels May 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-audit Category: Issues arising from audit findings
Projects
Archived in project
Development

No branches or pull requests

2 participants