electrsd: include in workspace and CI

[satsfy]

Closes #552

This PR includes electrsd in corepc workspace. With this change, now test, lints and formats also consider electrsd.

In order for this inclusion to fit in workspace:

• The cargo --all-features test faced several version conflicts becasue these tests enable all electrsd feature flags at the same time. The solution developed was to create a new test-only feature flag called all_features_test that is gets also enabled by default for finer version control, such that only the latest electrsd version runs.
• Version compiler constant is gated through the new flag, making only the latest electrsd version activated.
• All version-specific call sites have handle the new flag.
• Fixes formatting and linting errors as well as adding appropriate rbmt lint configurations for electrsd.
• Updates corresponding lock files for electrsd dependencies.

All of the commits are ordered to the same end, enabling CI to pass and tests to run.

[satsfy]

One thing I thought to include is a default electrsd version, like bitcoind in 0_17_2, I suppose either 0_8_10 or 0_10_6. I have not included a default because it required including std (for bitreq's get() to download), and I'm not sure if it's this PR's scope.

Edit: I suppose we need the default so tests work with an explicit version.

[satsfy]

Latest force push improves the legibility of the solution, sets a default electrs version, reworked the version feature graph so the cfg cascade in versions.rs is unambiguous. Replaced the legacy feature with versions::USE_LEGACY_COOKIE so --all-features tests pass.

[tcharding]

Needs rebase mate. Out of interest what made you go with electrs_0_8_10 instead of electrs_0_10_6?

[satsfy]

Great point, I made a mistake regarding 0_8_10. I had matched bitcoind pattern, but accidentally reversed the order of the versions. The highest version in bitcoind chain is the lowest, meanwhile I put the highest for electrsd. Latest rebase reverses that order.

EDIT: The problem with v0.10.6 in CI was caused by using the legacy --jsonrpc-import arg after 0.8.10. That nuance was lost on me because execution would fail on my Ubuntu 22.04 for requiring GLIBC 2.38 to test 0.10.6 properly.

[satsfy]

On the latest rebase:

• New electrs feature version cascade: electrs_0_10_6 -> 0_9_11 -> 0_9_1 -> 0_8_10 -> esplora.
• Centralized the version-specific behavior in versions.rs with legible constants USE_LEGACY_COOKIE, USE_JSONRPC_IMPORT, USE_VERBOSE_ARG.
• Esplora is tested with --no-default-features so it does not accidentally combine with the default electrs version.
• no-download job now enables bitcoind_download because it still needs a bitcoind binary while using the system-provided ELECTRS_EXEC.
• Corrected the bitcoind 23_2 feature name in ee9443c (is this correct? Should I have done it in a separate PR)?
• Optional change that I think should be merged: make electrsd CI coverage easier to debug by putting RUST_LOG=debug on electrsd jobs.

[tcharding]

Can we separate the fmt changes from actual code changes in c61ea18

[satsfy]

Latest rebase:

• Split rustfmt cleanup (17866ee), lint error fixes (33158b2) and new rbmt lint configuration (9143712).
• Fix Cargo.toml link typo (28022c5)
• Simplifies version conditions:
  • Using any in #[cfg(any(feature = "electrs_0_9_1", not(feature = "electrs_0_8_10")))]
  • or crate::versions::IS_ELECTRS_0_8_10.

[satsfy]

Latest rebase updates lockfiles because of upstream changes to fix ci

[tcharding]

Can you explain what you want to achieve with this feature gate please?

[tcharding]

E.g "I want this code block to run for these versions: a, b, c"

[satsfy]

So bitcoind feature gates, which served as a model for making electrsd features, are organized into a chain:

30_2 = ["30_0"]
30_0 = ["29_0"]
...
0_18_1 = ["0_17_2"]
0_17_2 = []

In bitcoind's file client_versions.rs:48, you can find a similar example of this gate:

#[cfg(all(feature = "0_21_2", not(feature = "22_1")))]
pub use corepc_client::{client_sync::v21::*, types::v21 as vtype};

So the pattern is all(X, not(Y)).
Now imagine I want the negation of a particular version. Say, I need not(version X).
I would express that in feature gates as not(all(X, not(Y)))
But you mentioned above its hard to understand what that means and I agree.
So I did not(all(X, not(Y))) = any(not(X), Y) = any(Y, not(X)) (De Morgan's laws) such as in:

#[cfg(any(feature = "electrs_0_9_1", not(feature = "electrs_0_8_10")))]

Would you prefer if I experimented with some less complex ways of solving the problem? I suppose I'd need to break away from the bitcoind solution and design a new feature flag solution. We do things this way to prevent users from enabling 2 flags at the same time, and the program not knowing which to use.

The alternative would be writing an "O(n²)" solution where every flag is checked against every other flag to ensure the same functionality. On a new electrsd version N, the versions file would be incremented in at least N lines of code.

[jamillambert]

Could this check be done in one place, e.g. build.rs and a new feature created like electrs_0_8_10_only then all of the any not feature gates changes can be feature = "electrs_0_8_10_only" etc. The new const bools can also then be removed.

[satsfy]

That's a good solution, I implemented it on the latest force push.

[tcharding]

No need for the last patch mate, we have #571 ready to release that change. I was planning on merging this after that release.

[jamillambert]

I am not sure the removal of the legacy feature is a good idea. This could break downstream for no real benefit.

Other than that and the few features gates that should be _only the rest looks good.

Reviewed 285011c

[satsfy]

The latest force push reworks the solution a lot, after some suggestions:

• Revert almost all flag changes and simplify the PRs diff. Cascading features were entirely removed.
• Tighten scope, removing some unnecessary changes (the 0.8.10 unrelated fix and CI debug logs) to other PRs.
• Removed the build-time-created flags.
• This is the simple solution I should have done from the start: add a all_features_test flag that gets enabled only when cargo test --all-features gets run. The user is never supposed to use that flag. Feature gating became more complex but much more readable.

NOTE: cannot move lint for format changes to other PRs because workspace now exercises it, causing CI failures. It is an inseparable part of the PR.

I am not sure the removal of the legacy feature is a good idea

I'm new-ish to OSS, and didn't realize how this would affect downstream. The latest push adds it back and keeps the flag situation intact for downstream.

[satsfy]

Latest push simple rebase + lockfile update.

[tcharding]

Looking good. To review I decided to just hack around and see what feel out. Want to look at the last commit on this branch and pull out any changes that you like?

https://github.com/tcharding/corepc/tree/push-zmqllslturol

[satsfy]

Latest force push takes suggestions from @tcharding's commit. Read bitcoind and electrsd side by side to pick what seemed appropriate. Also fixes the README instructions.

[jamillambert]

The lockfiles should be in Include electrsd in workspace 10a2bf0, currently they are not correct for all the patches until then.

[satsfy]

lockfiles should be in Include electrsd in workspace

Done in latest force push

[jamillambert]

ACK d2c019a

Thanks for sticking with it.

[tcharding]

Further follow ups if you want them, just things I noticed during this round of review.

src/lib.rs could be cleaner

//! Electrsd
//!
//! Utility to run a regtest electrsd process, useful in integration testing environment.

mod error;
mod ext;
mod versions;

pub extern crate bitcoind;
pub extern corepc_client;
pub extern crate electrum_client;

use std::env;
use std::ffi::OsStr;
use std::path::PathBuf;
use std::process::{Child, Command, Stdio};
use std::time::Duration;

use bitcoind::anyhow::Context;
use bitcoind::serde_json::Value;
use bitcoind::tempfile::TempDir;
use bitcoind::{anyhow, get_available_port, BitcoinD};
use electrum_client::raw_client::{ElectrumPlaintextStream, RawClient};
use log::{debug, error, warn};

#[rustfmt::skip]                // Keep pubic re-exports separate.
pub use error::Error;

• Remove #![warn(missing_docs)]

[tcharding]

Mate this sort of if/not/not/if feature gating is what we are trying to get away from. The reason is that it is way too hard to read. I had to spend 3 minutes trying to workout what it means and IIUC, and its telling that I'm not even 100% sure, its also broken.

For any build that does not use --all-features the first clause is false which makes the && always false which makes the outer ! always true. But anyway we want it simple, brain-dead-simple is the mantra.

[satsfy]

I see didn't make it simple enough. I'll make intermediary boolean logic variables so that this and other conditions read like:

if !is_all_features_test && should_version_use_jsonrpc_import {

The assumption here is that all feature tests would enable all flags, so we must specifically root it out.

However the boolean logic is correct. You can prove it by running locally:

cargo test --all-features
cargo test --no-default-features --features bitcoind_22_1,bitcoind_download,electrs_
0_9_11
cargo test --no-default-features --features bitcoind_22_1,bitcoind_download,electrs_
0_9_1
cargo test --no-default-features --features bitcoind_22_1,bitcoind_download,electrs_
0_8_10
cargo test --no-default-features --features bitcoind_22_1,bitcoind_download,esplora_a33e97e1

[tcharding]

Goodness, please excuse me. Yesterday I missed totally that there are 2 calls to cfg! - bother. I was reading it as this (which is obviously syntactically incorrect)

if !cfg!(feature = "all_features")
            && any(
                feature = "electrs_0_8_10",
                feature = "esplora_a33e97e1",
                feature = "legacy",
            )

[satsfy]

First force push: made the feature gating more readable and applied the suggestions for lib.rs header and crate re-exports.
Second force push: fix a commit order mistake.

[apoelstra]

d64d557 needs rebase

[tcharding]

Aight, sorry for all the iterations on this. I'm happy with this now. Tiny nit, _TEST in IS_ALL_FEATURES_TEST might be slightly better as _BUILD because one can pass --all-features to cargo check, cargo build, or cargo test.

But anyways, you have to rebase. Ill merge after that whether or not you change the const name. Thanks for you patience and my at times dismal reviews on this one.

[tcharding]

Reviewed: d64d557