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

Tracking: TODOs with closed tasks #6281

Closed
2 of 4 tasks
Tracked by #6277
mpguerra opened this issue Mar 9, 2023 · 5 comments
Closed
2 of 4 tasks
Tracked by #6277

Tracking: TODOs with closed tasks #6281

mpguerra opened this issue Mar 9, 2023 · 5 comments
Assignees
Labels
C-audit Category: Issues arising from audit findings C-cleanup Category: This is a cleanup

Comments

@mpguerra
Copy link
Contributor

mpguerra commented Mar 9, 2023

Motivation

We want to track all of the findings from the zebra audit.

Details

There are a number of TODOs in the codebase that use task numbers that are already closed. For instance, issue #862 states that the Sync process’s state_contains() API only checks the best chain for a given block hash (zebrad/src/components/sync.rs), rather than checking all the available chains in the mempool. The audited version of the library still queries the best chain in state and the task is closed.

Another example is issue #2214 which suggests that the fanout handling should be done by the PeerSet so that all the fanouts wouldn’t use the same peer and block themselves. This seems to only be an issue on testnet where nodes aren’t as well connected as mainnet and the task is closed. This issue is referenced in a TODO in zebrad/src/components/
sync.rs
.

This issue will be used to track any further issues created to address TODOs which refer to issues which have already been closed.

Scope

@mpguerra
Copy link
Contributor Author

Copied from #6347

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
Collaborator

  • Evaluate list of TODOs with closed issues and identify:
    • which TODOs can be removed because the issue is already resolved or no longer relevant
    • which, if any, issues need to be reopened because they are still relevant
    • which TODOs need to be updated with relevant context
    • which TODOs should be updated to WONTFIX

Do we need to have different labels for:

  • this is something we want to do eventually
  • this is not something we will ever spend time on, but we will accept volunteer PRs
  • this is something we don't want to fix, and we won't accept PRs from anyone

I think tickets and TODOs might move between the first two categories as our priorities change, or based on community feedback. But the last category won't change much, because it's usually about an engineering tradeoff where it's too much effort or too risky.

@mpguerra
Copy link
Contributor Author

Do we need to have different labels for:

* this is something we want to do eventually

* this is not something we will ever spend time on, but we will accept volunteer PRs

* this is something we don't want to fix, and we won't accept PRs from anyone

I think tickets and TODOs might move between the first two categories as our priorities change, or based on community feedback. But the last category won't change much, because it's usually about an engineering tradeoff where it's too much effort or too risky.

As long as we keep the TODO label that's fine but I wouldn't replace the TODO with another set of labels as this will make it harder to track.

Also I wonder if we're all bikeshedding a bit too much with this one 😅

@mpguerra
Copy link
Contributor Author

mpguerra commented May 11, 2023

Finally started looking into this.

In order for me to track this more easily I have started this spreadsheet and I'll be adding the various different issues reported by the tool and the action to take here.

Once that's done I may create more issues to clean those up as necessary.

@mpguerra mpguerra self-assigned this May 11, 2023
@mpguerra
Copy link
Contributor Author

I think we've done as much as we're going to do here for now

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 C-cleanup Category: This is a cleanup
Projects
Archived in project
Development

No branches or pull requests

2 participants