Eng Quality Initiatives

[igorsyl]

Collecting issues and tasks to improve code quality and developer productivity.

Documentation

• add comments to the major structs and fields in the codebase
  • explain anything about each field that needs to be known but isn't clear from the name and type
  • explain the role of each struct, and its relationship to the rest of the system
• create new systems diagrams

  • From Mårten:
    vim-boxdraw is a nice utility to generate these types of diagrams
    https://github.com/gyim/vim-boxdraw
    PlantUML is also able to translate ascii art to proper figures.
    It's a good compromise for documentation where you can have readable charts in the code,
    and also get pretty images in web interfaces https://plantuml.com/ditaa

  • Matthew Little:
    https://github.com/mermaid-js/mermaid
    Mermaidjs is pretty popular too - works on GitHub with Markdown. For example:

    graph TD;
    subgraph User code
      App([Application])-->Logic([Business Logic]);
      UnitTests([Unit Tests])-->Logic;
    end
    UnitTests-->UTF[Unit Test Framework];
    Logic-->UI[UI Framework];
    Logic-->Generic[Generic Framework];
    UI-->IO[System IO];
    

    Loading

  • Brice Dobry:
    I believe there are tools to generate ascii from dot files too (Graphviz)
    https://dot-to-ascii.ggerganov.com

  • Greg Coppola:
    People like figma

  • Another tool to create diagrams: https://text-to-diagram.com/?example=basic

Code quality issues

• Include a .editorconfig.

• Turn on Clippy warnings, and get these fixed.

• Address rustc warnings.

• Move Cargo package from /testnet/stacks-node to /stacks-node

• Move Cargo workspace member "." to blockstack-core directory

• Remove code using unsafe keyword

  • We're in a great place with this in our code, but not in the dependencies. But the use of unsafe in dependencies puts the whole node at risk, as we discovered today with reqwest.

• Investigate issues with libraries reqwest, async-h1 and tokio

  • https://discord.com/channels/621759717756370964/730814571517968436/1043188561349971998
  • Jude prefers just removing them outright if at all possible. Why bring in dependencies that segfault?

• Add the Cargo package stacks-common as a workspace member

• Remove the use of extern crate

  • https://doc.rust-lang.org/edition-guide/rust-2018/path-changes.html

• Add support for Bitcoin v23

• jude: I'm a fan of "all of the core functionality for a single distinct subsystem should be in one file" because then it's straightforward to see the big picture of how the system ought to work.
  the large files come mainly from keeping the unit tests in the same file
  like, src/chainstate/stacks/db/transactions.rs is mostly unit tests (like, 90% of it is just unit tests)
  those could be separated out into a separate tests module
  src/chainstate/stacks/db/blocks.rs without tests is about 7 KLoC; it could probably be separated into four files:

  • one for loading block data out of the filesystem and database
  • one for streaming data
  • one for mempool-related transaction validation
  • one for processing, accepting, and rejecting blocks

Test suite reliability

Some tests are slow (run for over one minute) or fail when run with multiple threads.

• From Jude
  • Yes -- some tests simulate running multiple instances of the peer network in order to test some emergent behaviors (e.g. "the neighbor discovery algorithm eventually discovers all other neighbors"). This not only requires binding on ports (which complicates running many of these tests in parallel), but also can take time.
  • Integration tests are time-consuming in part because they spin up a full Bitcoin node and sometimes multiple Stacks runloops, in order to simulate multiple nodes communicating with one another.

Running local tests

cargo test -r --workspace -- -Z unstable-options --report-time --format=json --test-threads=1 | tee test.log

Running contiguous integration tests

PATH=$PATH:/path-to/bitcoin-22.0/bin
BITCOIND_TEST=1
cargo test -r --workspace -- -Z unstable-options --report-time --format=json --test-threads=1 | tee test-ignored.log


Using the logs files produced above:

Show failing tests

Run local tests with --test-threads=16 to check for tests failing due to concurrent runs

cat test.log | jq -R 'fromjson?' | jq -scr 'map(select(.type=="test" and .event=="failed")) | .[] | "begintest", .name,.reason,.stdout, "endtest"'


Show slow tests

cat test.log | jq -R 'fromjson?' | jq -sc 'map(select(.type=="test" and .event=="ok")) | sort_by(.exec_time) | .[]'

Break up large code files

Jude: These files are large because they have lots of tests in them. We could separate the tests out into their own files. This was already done in next for miner.rs, for example.

find . -name "*.rs" -not -path "./target/*" -exec wc -l {} + | sort -n

7739 ./src/chainstate/stacks/db/transactions.rs
8044 ./src/chainstate/burn/db/sortdb.rs
9934 ./testnet/stacks-node/src/tests/neon_integrations.rs
11136 ./src/chainstate/stacks/db/blocks.rs
11535 ./src/chainstate/stacks/miner.rs
286891 total

[netrome]

Following the test fix in #3425 I've been looking into code quality improvements to reduce the risk of similar concurrency issues in the future. I've looked into the following:

1. Use a macro to get the name of the current function in the tests
General idea: Instead of calling

TestPeerConfig::new("hardcoded_name_of_test")

do

TestPeerConfig::new(function_name!())

Caveat: This macro does not exist in the standard library. These two third-party implementations are interesting:

• stdext
• function_name

2. Remove the test_name parameter completely and rely on generating a unique ID for the tests
We could use UUIDs and accept an astronomically small collision risk, or use something like xid which would guarantee uniqueness for the test cases.

Drawback: We loose some interpretability in debug logs and temporary test directory names.

Recommendation

I recommend using the stdext macro. It's slightly simpler than the dedicated function_name and only requires replacing the hard-coded names to the macro call. We only need to add stdext as a dev-dependency, so the final binary will not be affected by this.

I'd love to get feedback on this. If everyone agrees that this makes sense, I'd be happy to prepare a PR implementing this change.

[jcnelson]

I like option 1. With option 2, it will be a PITA to try and find which directory to go and look at if there are multiple test runs to choose from.

[netrome]

Sweet, I'll then prepare a PR according to option 1 unless someone screams.

[netrome]

PR: #3430

[sergey-shandar]

Suggestion: write pure code, without side-effects.

The most problematic side-effect is IO. Avoid using IO directly, like listening ports. Avoid using functions from third-party libraries which using IO directly. Bind IO at the latest stage. This way we can test and mock IO in our unit tests and try different scenarios without complicated long running integration tests.

[jcnelson]

Agreed, to the extent that is possible. However, there are exceptions:

• The networking stack is built around an edge-triggered epoll(2) loop. We need to be able to test the system's ability to process connections, send data, and receive data with the expected edge-trigger behavior. In particular, we manipulate the send and receive buffers of the underlying sockets to ensure that messages require multiple edge triggers to send and receive.

• A key aspect of debugging failing tests is the ability to use a real Sqlite database. This is because the data can be inspected after the test crashes to determine the root cause. While in many cases we can require that Sqlite databases all reside in RAM, this does not apply to integration tests where the behavior is being tested against a full node implementation (which keeps state on disk).

• Some of the tests on the MARF index are against data on disk, in order to ensure that the behavior being tested is as similar to the production behavior as possible.

• Integration tests necessarily bind on ports and store files on disk because they are designed to exercise all of the code paths that will be used in production.

[sergey-shandar]

Agree. We need all of them, unit tests (without IO), integration tests, and end-to-end tests. Unit tests don't exclude integration tests. However, it would be good to test as much as possible during unit tests. My main concern is huge algorithms with direct IO, which is challenging to mock during unit tests.

[sergey-shandar]

Clear Rust Repository Structure

DON'T mix together project properties and a workspace. A root level Cargo.toml should start with[workspace]. All other Cargo.toml should start with [package].

Example:

A repository x which contains two projects/crates foo and bar:

• Cargo.toml

  [workspace]
  members = [
    "foo",
    "bar"] 

• foo\Cargo.toml

  [package]
  name = "foo"
  version = "0.0.1"
  edition = "2021"

• bar\Cargo.toml

  [package]
  name = "bar"
  version = "0.0.1"
  edition = "2021"

Other projects, outside of this repo, can depend on a specific projects of the mono-repo: https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#specifying-dependencies-from-git-repositories

[dependencies]
b = { git = "https://github.com/x/x", branch = "next" }

[jcnelson]

Where appropriate, have the caller call .clone() or .to_owned() instead of the callee. Putting this here so we can close #3540.

[netrome]

Make stacks_common::types::Address require FromStr. More context in this old issue #3483

[netrome]

Refactor: Remove unneccessary generics from FromRow and FromColumn traits #3513