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

feat(script): Add binary for finding references to closed issues #6347

Merged
merged 18 commits into from
Mar 28, 2023

Conversation

arya2
Copy link
Contributor

@arya2 arya2 commented Mar 16, 2023

Motivation

We want a full list of comments in the repo referencing issues that are closed.

Closes #6278.

Solution

  • Add a search-issue-refs binary to zebra-utils
    • Search all .rs, .yml, .yaml, and .toml files in the local directory for possible issue refs
    • Filter out any possible issue refs that don't have closed issues in Zebra
    • Print out a list of closed issues and their locations

Review

Anyone can review.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

@arya2 arya2 requested a review from a team as a code owner March 16, 2023 17:56
@arya2 arya2 self-assigned this Mar 16, 2023
@arya2 arya2 requested review from upbqdn and removed request for a team March 16, 2023 17:56
@github-actions github-actions bot added C-enhancement Category: This is an improvement C-feature Category: New features labels Mar 16, 2023
@arya2 arya2 force-pushed the find-todos-with-closed-issues branch from 1b79bdb to 1d551b3 Compare March 16, 2023 17:59
@arya2 arya2 changed the title change(script): Add binary for finding refs to closed issues. change(script): Add binary for finding references to closed issues. Mar 16, 2023
@codecov
Copy link

codecov bot commented Mar 16, 2023

Codecov Report

Merging #6347 (f2eee16) into main (a97a975) will decrease coverage by 0.04%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6347      +/-   ##
==========================================
- Coverage   77.78%   77.74%   -0.04%     
==========================================
  Files         304      304              
  Lines       39528    39585      +57     
==========================================
+ Hits        30745    30775      +30     
- Misses       8783     8810      +27     

@arya2 arya2 changed the title change(script): Add binary for finding references to closed issues. feat(script): Add binary for finding references to closed issues. Mar 17, 2023
@arya2 arya2 added C-cleanup Category: This is a cleanup P-Medium ⚡ A-script Area: Script handling C-audit Category: Issues arising from audit findings labels Mar 17, 2023
upbqdn
upbqdn previously approved these changes Mar 17, 2023
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

This is awesome.

I noticed a potential bug in the regex and left some comments.

zebra-utils/src/bin/search-issue-refs/main.rs Outdated Show resolved Hide resolved
zebra-utils/src/bin/search-issue-refs/main.rs Outdated Show resolved Hide resolved
zebra-utils/src/bin/search-issue-refs/main.rs Outdated Show resolved Hide resolved
zebra-utils/src/bin/search-issue-refs/main.rs Outdated Show resolved Hide resolved
@arya2 arya2 force-pushed the find-todos-with-closed-issues branch from 0012243 to 30c5d87 Compare March 21, 2023 18:11
@mpguerra mpguerra mentioned this pull request Mar 21, 2023
36 tasks
@teor2345 teor2345 changed the title feat(script): Add binary for finding references to closed issues. feat(script): Add binary for finding references to closed issues Mar 21, 2023
@mpguerra
Copy link
Contributor

@upbqdn are you able to review this one? Otherwise @oxarbitrage can you please take over this review?

@mpguerra mpguerra requested review from oxarbitrage and removed request for upbqdn March 27, 2023 12:09
@arya2 arya2 requested review from upbqdn and removed request for upbqdn March 27, 2023 16:52
oxarbitrage
oxarbitrage previously approved these changes Mar 27, 2023
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.

This looks really good, i made a few optional comments.

There are a few things that i think we have to consider:

  • We now need to use it to fix some/all of the findings (there are currently 184). Should we open a ticket ?
  • I am a bit concerned that we do more rust utilities in general, which will mean more dependencies, more CI, etc. We hide them behind rust features but i think that is still attached to the main project. Should we consider making utilities outside of the zebra repo or encourage to do what we did here in the future? Any opinions?

zebra-utils/src/bin/search-issue-refs/main.rs Outdated Show resolved Hide resolved
zebra-utils/src/bin/search-issue-refs/main.rs Outdated Show resolved Hide resolved
zebra-utils/src/bin/search-issue-refs/main.rs Show resolved Hide resolved
Co-authored-by: Alfredo Garcia <oxarbitrage@gmail.com>
@arya2 arya2 requested a review from oxarbitrage March 27, 2023 21:38
mergify bot added a commit that referenced this pull request Mar 27, 2023
@mergify mergify bot merged commit aa1e407 into main Mar 28, 2023
@mergify mergify bot deleted the find-todos-with-closed-issues branch March 28, 2023 01:40
@mpguerra
Copy link
Contributor

This looks really good, i made a few optional comments.

There are a few things that i think we have to consider:

* We now need to use it to fix some/all of the findings (there are currently 184). Should we open a ticket ?

There's a tracking issue #6281 to deal with/list these already

@mpguerra
Copy link
Contributor

mpguerra commented Mar 31, 2023

Edit: @arya2 can you please add the output of this script as new comment on #6281 so someone can go through them and create issues if necessary?

@arya2
Copy link
Contributor Author

arya2 commented Mar 31, 2023

Edit: @arya2 can you please add the output of this script as new comment on #6281 so someone can go through them and create issues if necessary?

Yes! (script lightly modified for permalinks and markdown output):

GITHUB_TOKEN=XXX cargo run --package zebra-utils --bin search-issue-refs --all-features --features search-issue-refs

Searching files in this repo with a ".rs", ".yml", ".yaml", or ".toml" file extension for issue references..

Found 209 possible references to 104 issues, checking statuses on Github..

{results - see below}

Confirmed 179 references to 91 closed issues.

References to Closed Issues


// Based on Result::map from https://doc.rust-lang.org/src/core/result.rs.html#765


// temporary error type until #1186 is fixed

see more:

// TODO: implement graceful shutdown for InventoryRegistry (#1678)


// TODO: Avoid new calls to `insert_fake_orchard_shielded_data` for each check #2409.


/// TODO: replace with a custom DateTime64 type (#2171)


// TODO: move debug_stop_at_height into a task in the start command (#3442)

// TODO: move the stop height check to the syncer (#3442)

/// TODO: move the stop height check to the syncer (#3442)

/// TODO: make private after the stop height check has moved to the syncer (#3442)


// TODO: perform listener DNS lookups asynchronously with a timeout (#1631)


// so we always choose different peers (#3235)


/// <https://github.com/ZcashFoundation/zebra/issues/3027>


// TODO: Replace with calls to `watch::Sender::borrow` once Tokio is updated to 1.0.0 (#2573)


// https://github.com/ZcashFoundation/zebra/issues/2909


// TODO: hide this command from users in release builds (#3279)


// "load shed directly" pattern from #1618.


// TODO: use const generics https://github.com/ZcashFoundation/zebra/issues/2042


/// See <https://github.com/ZcashFoundation/zebra/issues/1021> for more details.

/// See <https://github.com/ZcashFoundation/zebra/issues/1021> for more details.


/// - [Advertising a different external IP address #1890](https://github.com/ZcashFoundation/zebra/issues/1890), or


// we start generating our own blocks (see #483).

// transactions (see #483).


# TODO: replace with environmental variables that skip the tests when not set (part of #2995)


// Windows problems with this match will be worked on at #1654


/// until bug #4794 is fixed.)


// TODO: move fanouts into the PeerSet, so we always choose different peers (#2214)

// TODO: move fanouts into the PeerSet, so we always choose different peers (#2214)

// TODO: move fanouts into the PeerSet, so we always choose different peers (#2214)

/// TODO: Restore the fanout to 3, once fanouts are limited to the number of ready peers (#2214)

// TODO: move fanouts into the PeerSet, so we always choose different peers (#2214)


// - verify orchard shielded pool (ZIP-224) (#2105)


// TODO: add tests for finalized and non-finalized resets (#2654)


/// - [Auto-discovering its own external IP address #1893](https://github.com/ZcashFoundation/zebra/issues/1893).

// TODO: detect external address (#1893)


// Based on Result::map_err from https://doc.rust-lang.org/src/core/result.rs.html#850


// TODO: This channel will be bounded by the connection limit (#1850, #1851, #2902).


// to avoid deadlocks (see #1976)

// so all operations are performed on a blocking thread (see #1976).

// to avoid deadlocks (see #1976).

// TODO: only mark the peer as AttemptPending when it is actually used (#1976)

// Correctness: Spawn address book accesses on a blocking thread, to avoid deadlocks (see #1976).

// to avoid deadlocks (see #1976).


// https://github.com/ZcashFoundation/zebra/issues/2079



// TODO: add tests for Error conversion and Reject message serialization (#4633)


//! TODO: test that inner service errors are handled correctly (#3204)


/// <https://github.com/ZcashFoundation/zebra/issues/3127>


// TODO: use the latest network upgrade (#1974)

// TODO: dynamically select any future network upgrade (#1974)

// TODO: add future network upgrades (#1974)


// TODO: Remove when the code base migrates to Rust 2021 edition (#2709).



// responses to the inv collector. (#2156, #1768)

// TODO: reduce this log level after testing #2156 and #2726


// See #5126 for details.


// TODO: treat `TryRecvError::Closed` in `Inbound::poll_ready` as a fatal error (#1655)


// TODO: In the context of #4734:


// TODO: check coinbase transaction remaining value (#338, #1162)


continue-on-error: true # TODO: remove after fixig https://github.com/ZcashFoundation/zebra/issues/5933


//! Unused key types are not implemented, see PR #5476.

//! Unused key types are not implemented, see PR #5476.

//! Unused key types are not implemented, see PR #5476.


/// Only checked when the command outputs each new line (#1140).

/// the child (#1140).

// TODO: create a similar test that pauses after output (#1140)

/// This test fails due to bugs in TestDirExt, see #1140 for details.

// TODO: create a similar test that pauses after output (#1140)

/// This test fails due to bugs in TestDirExt, see #1140 for details.

/// TODO: test the failure regex on timeouts with no output (#1140)


/// In #3110, we changed the fanout to 1, to make sure we actually use cached address responses.


// - shielded input and output limits? (#2379)


// TODO: check that the nullifiers were correctly inserted (#2230)


/// TODO: replace with `MAX_CLOSE_TO_TIP_BLOCKS` after fixing slow syncing near tip (#3375)

// after fixing slow syncing near tip (#3375)

// TODO: warn after fixing slow syncing near tip (#3375)


// TODO: replace Utc.timestamp with DateTime32 (#2211)

// TODO: replace Utc.timestamp with DateTime32 (#2211)


// TODO: use a generic error constructor (#5548)

// TODO: use a generic error constructor (#5548)

// use a generic error constructor (#5548)

// - use a generic error constructor (#5548)

// TODO: use a generic error constructor (#5548)

// TODO: use a generic error constructor (#5548)

// use a generic error constructor (#5548)

// - use a generic error constructor (#5548)

// TODO: use a generic error constructor (#5548)

// TODO: use a generic error constructor (#5548)

// TODO: use a generic error constructor (#5548)

// TODO: use a generic error constructor (#5548)


// SECURITY TODO: check if the timestamp field can be zeroed, to remove another distinguisher (#3300)

// SECURITY TODO: should this just be zeroed anyway? (#3300)

/// SECURITY TODO: check if the timestamp field can be zeroed, to remove another distinguisher (#3300)


// responses to the inv collector. (#2156, #1768)


// canonical SocketAddr (#2357)


// but they were removed because the testnet is unreliable (#1222).


// This validates the 𝜋_{ZKJoinSplit} element. In #3179 we plan to validate

// [`groth16::Item::try_from`]. In #3179 we plan to validate here instead.

// [`groth16::Item::try_from`]. In #3179 we plan to validate here instead.

// [`groth16::Item::try_from`]. In #3179 we plan to validate here instead.

// [`groth16::Item::try_from`]. In #3179 we plan to validate here instead.

// [`groth16::Item::try_from`]. In #3179 we plan to validate here instead.


// TODO: this would be cleaner with poll_map (#2693)

// TODO: this would be cleaner with poll_map (#2693)

// TODO: this would be cleaner with poll_map (#2693)


// In #2244 we will fix this by replacing response Vecs with HashSets.

// In #2244 we will fix this by replacing response Vecs with HashSets.

// In #2244 we will fix this by replacing response Vecs with HashSets.

// TODO: make this into a HashMap<SocketAddr, MetaAddr> - a unique list of peer addresses (#2244)

// TODO: make this into an IndexMap - an ordered unique list of hashes (#2244)

// TODO: make this into an IndexMap - an ordered unique list of headers (#2244)

// TODO: make this into a HashSet - a unique list (#2244)

// TODO: make this into a HashMap<block::Hash, InventoryResponse<Arc<Block>, ()>> - a unique list (#2244)

// TODO: make this into a HashMap<UnminedTxId, InventoryResponse<UnminedTx, ()>> - a unique list (#2244)

// TODO: make this into an IndexMap - an ordered unique list of hashes (#2244)

// TODO: make this into an IndexMap - an ordered unique list of hashes (#2244)


// See #1781.

/// processes that have panicked. See #1781.

/// have panicked. See #1781.


// TODO: make it more accurate #2869


/// This bug might be fixed by moving database operations to blocking threads (#2188),

// TODO: move opening the database to a blocking thread (#2188)

/// move shutting down the database to a blocking thread (#2188)


.field(&format_args!("{:#010x}", self.0))


// TODO: provide a different hint if the disk is full, see #1623


// during contextual validation (#2336).


// (See https://docs.rs/time/0.1.44/src/time/duration.rs.html#166-173)


// TODO: serialize the index into a smaller number of bytes (#3953)

// serialize the index in big-endian order (#3953)


/// TODO: after PR #2275 merges, add a similar proptest,


// ZIP 208 don't specify this conversion either.) See #1277 for details.


// https://github.com/ZcashFoundation/zebra/issues/2592#issuecomment-897304684


// Based on Option::map from https://doc.rust-lang.org/src/core/option.rs.html#844


// - Zebra should ignore peers that are older than 3 weeks (part of #1865)


// We should re-add them after we have more testnet instances (#1791).


// Missing RPCs in zebrad logs (this log is from PR #3860)


// TODO: This channel will be bounded by the connection limit (#1850, #1851, #2902).


# or remove this workaround once the build is more efficient (#3005).

# TODO: Remove this workaround once the build is more efficient (#3005).


/// Old state versions are [not automatically deleted](https://github.com/ZcashFoundation/zebra/issues/1213).


/// The second node will panic with the Zcash listener conflict hint added in #1535.

/// conflict hint added in #1535.

/// conflict hint added in #1535.

/// The second node will panic with the Zcash state conflict hint added in #1535.


// TODO: split out block snapshots into their own function (#3151)

// TODO: split out transaction snapshots into their own function (#3151)

// TODO: snapshot TransactionLocations without Some (#3151)


// threshold, but ZIP-205 and ZIP-208 are ambiguous. See bug #1276.


// TODO: reduce this log level after testing #2156 and #2726


/// [#2694](https://github.com/ZcashFoundation/zebra/issues/2694)


/// [`super::inbound::downloads::MAX_INBOUND_CONCURRENCY`] and #1880 for details.

/// (See #1880 for more details.)

/// (See #1880 for more details.)


//! It should be refactored into a cleaner set of request/response pairs (#1515).


// tests bug #2789


// once we upgrade to tokio 1.0 (#2200)



// TODO: This argument can be removed and just use Nu5 after we have an activation height #1841

// TODO: update this to Nu5 after we have a height #1841


// See #1593 for details.

// `poll_ready` has a corresponding `call`. See #1593.


/// fields. (After PR #2276 merges.)


// split gossiped and handshake services? (#2324)

// consider sanitizing untrusted services to NODE_NETWORK (#2324)

// TODO: create a "services implemented by Zebra" constant (#2324)

// TODO: split untrusted and direct services (#2324)

// TODO: order services by usefulness, not bit pattern values (#2324)


// TODO: This channel will be bounded by the connection limit (#1850, #1851, #2902).


// TODO: check coinbase transaction remaining value (#338, #1162)


// Use separate verifiers so shared batch tasks aren't killed when the test ends (#2390)

// Use separate verifiers so shared batch tasks aren't killed when the test ends (#2390).

// Use separate verifiers so shared batch tasks aren't killed when the test ends (#2390).

// Use separate verifier so shared batch tasks aren't killed when the test ends (#2390)

// Use separate verifier so shared batch tasks aren't killed when the test ends (#2390)

@teor2345
Copy link
Contributor

teor2345 commented Apr 3, 2023

Note that some of these tasks have been closed as "wontfix", which is different from actually being fixed.

We might want to keep references to "wontfix" tasks in the code for context, but maybe we could change "TODO" to "WONTFIX".

@mpguerra mpguerra mentioned this pull request Apr 11, 2023
4 tasks
@arya2 arya2 mentioned this pull request Apr 18, 2023
36 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-script Area: Script handling C-audit Category: Issues arising from audit findings C-cleanup Category: This is a cleanup C-enhancement Category: This is an improvement C-feature Category: New features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Write script to find TODOs mentioning closed github issues
5 participants