Skip to content

electrsd: include in workspace and CI#570

Open
satsfy wants to merge 9 commits into
rust-bitcoin:masterfrom
satsfy:include-electrsd
Open

electrsd: include in workspace and CI#570
satsfy wants to merge 9 commits into
rust-bitcoin:masterfrom
satsfy:include-electrsd

Conversation

@satsfy
Copy link
Copy Markdown
Contributor

@satsfy satsfy commented Apr 24, 2026

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.

Comment thread electrsd/src/versions.rs Outdated
Comment thread electrsd/src/lib.rs Outdated
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented Apr 24, 2026

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 satsfy force-pushed the include-electrsd branch from 2a42a61 to 4c939ef Compare April 25, 2026 04:01
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented Apr 25, 2026

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.

@satsfy satsfy force-pushed the include-electrsd branch 2 times, most recently from af412f2 to 6e540e4 Compare April 25, 2026 04:23
@satsfy satsfy requested a review from jamillambert April 25, 2026 17:49
@tcharding
Copy link
Copy Markdown
Member

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

@satsfy satsfy force-pushed the include-electrsd branch from 6e540e4 to 8343421 Compare April 28, 2026 03:02
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented Apr 28, 2026

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 satsfy force-pushed the include-electrsd branch from 8343421 to 12645ba Compare April 28, 2026 19:35
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented Apr 28, 2026

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.

@satsfy satsfy force-pushed the include-electrsd branch 2 times, most recently from eca4af4 to 70b269f Compare April 28, 2026 20:29
@tcharding
Copy link
Copy Markdown
Member

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

Comment thread electrsd/src/ext.rs Outdated
@satsfy satsfy force-pushed the include-electrsd branch from 70b269f to 28022c5 Compare April 29, 2026 19:22
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented Apr 29, 2026

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 satsfy force-pushed the include-electrsd branch 5 times, most recently from 3866f93 to cd0ddfe Compare April 29, 2026 21:26
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented Apr 29, 2026

Latest rebase updates lockfiles because of upstream changes to fix ci

Comment thread electrsd/src/ext.rs Outdated
Comment on lines +12 to +13
#[cfg(not(feature = "electrs_0_8_10"))]
#[cfg(any(feature = "electrs_0_9_1", not(feature = "electrs_0_8_10")))]
Copy link
Copy Markdown
Member

@tcharding tcharding Apr 30, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Member

@tcharding tcharding Apr 30, 2026

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor Author

@satsfy satsfy Apr 30, 2026

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

@tcharding
Copy link
Copy Markdown
Member

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

Comment thread electrsd/Cargo.toml Outdated
Comment on lines +82 to +83
'cfg(feature, values("electrs_0_9_11_only", "electrs_0_9_1_only", "electrs_0_8_10_only", "esplora_a33e97e1_only"))',
'cfg(esplora_a33e97e1)',
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We shouldn't need the last one because its a feature, right?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually we don't need any of these do we because of the loop in build.rs?

Copy link
Copy Markdown
Member

@tcharding tcharding May 8, 2026

Choose a reason for hiding this comment

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

+    for v in versions {
+        println!("cargo:rustc-check-cfg=cfg({}_only)", v);
+    }

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think I had put it because of some cargo check or lint warning, but I can't reproduce it anymore, so I'm dropping the change.

Comment thread electrsd/build.rs Outdated
let env_name = format!("CARGO_FEATURE_{}", v.to_uppercase());
if std::env::var_os(&env_name).is_some() {
// Emits cfg for the highest enabled version only and then returns.
println!("cargo:rustc-cfg={}_only", v);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
println!("cargo:rustc-cfg={}_only", v);
println!("cargo:rustc-cfg={}_and_below", v);

I think I finally get it. The 'only' was throwing me off before big time.

Copy link
Copy Markdown
Contributor Author

@satsfy satsfy May 8, 2026

Choose a reason for hiding this comment

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

Sorry, but the _only indeed means only a specific version. The latest change should clear up the confusion, because this older version was not using the only properly. It was an error.

@tcharding
Copy link
Copy Markdown
Member

Sorry got to run half way through review.

@tcharding tcharding closed this May 8, 2026
@tcharding
Copy link
Copy Markdown
Member

Wrong button.

@tcharding tcharding reopened this May 8, 2026
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented May 8, 2026

Force-pushed. Changes:

  • Rebase with master.
  • Update lockfiles to fix CI errors.
  • In versions.rs, replace _only cfg aliases with chained not(...) guards . It was a mistake to make that change because the solution would not compile for electrs versions below 0.10.6. I could not use the _only suffixed flags they are not present at build time, causing a compilation error. But if you do add to Cargo.toml (which I tried), they break -all-features tests because all the _only flags are included when only one of them should.
  • Move RUST_LOG=debug out of inline invocation and into the job env block.
  • Add --no-default-features back to cargo test runs to prevent the default features from leaking into other versions in CI test.
  • Drop download from the default feature set.
  • Revert cfg!(feature = "esplora_a33e97e1_only") back to cfg!(esplora_a33e97e1_only).

@satsfy satsfy force-pushed the include-electrsd branch from 8d274a5 to 285011c Compare May 8, 2026 22:17
Comment thread electrsd/src/versions.rs Outdated
Comment on lines +12 to +18
#[cfg(all(
not(feature = "electrs_0_8_10"),
not(feature = "electrs_0_9_1"),
not(feature = "electrs_0_9_11"),
not(feature = "electrs_0_10_6"),
not(feature = "esplora_a33e97e1"),
))]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
#[cfg(all(
not(feature = "electrs_0_8_10"),
not(feature = "electrs_0_9_1"),
not(feature = "electrs_0_9_11"),
not(feature = "electrs_0_10_6"),
not(feature = "esplora_a33e97e1"),
))]
#[cfg(not(feature = "esplora_a33e97e1"))]

Recently worked out this can be simplified in bitcoind too. This works because of the cascading features. Same change in this file at the bottom.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

There was another gate I thought was wrong then I realised the same, really the feature means esplora_a33e97e1 and later versions

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cascading features were removed entirely in the latest version.

Comment thread electrsd/src/versions.rs Outdated
const VERSION: &str = "v0.10.6";
#[cfg(all(feature = "electrs_0_9_1", not(feature = "electrs_0_9_11")))]
const VERSION: &str = "v0.9.1";
#[cfg(all(feature = "electrs_0_8_10", not(feature = "electrs_0_9_1")))]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not use the _only flags?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I agree, this was one of the reasons to have the new feature.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I have removed these flags. They were too complex. What I did instead if to gate with.

#[cfg(all(feature = "electrs_0_8_10", not(feature = "all_features_test")))]

I think this is reasonable because it reads very clear meaning.

@tcharding
Copy link
Copy Markdown
Member

Reviewed: 285011c

Copy link
Copy Markdown
Collaborator

@jamillambert jamillambert left a comment

Choose a reason for hiding this comment

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

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 satsfy force-pushed the include-electrsd branch 2 times, most recently from 04ae05b to fded3bb Compare May 13, 2026 22:46
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented May 13, 2026

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 satsfy force-pushed the include-electrsd branch from fded3bb to 0f4c5fc Compare May 13, 2026 22:59
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented May 13, 2026

Latest push simple rebase + lockfile update.

@tcharding
Copy link
Copy Markdown
Member

tcharding commented May 14, 2026

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 added 9 commits May 14, 2026 17:16
Introducing electrsd in workspace cause --all-features tests to enable
all electrsd feature flags, this commit creates a new test-only feature that
is only enabled on these particular tests.

Version compiler constant gets appropriately gates through the new flag so only
the latest electsd version is activated.

It introduces flag to all version specific call sites.
Mirror the bitcoind build script.
The previous examples would not run.
@satsfy satsfy force-pushed the include-electrsd branch from 0f4c5fc to 93c52c1 Compare May 14, 2026 20:21
@satsfy
Copy link
Copy Markdown
Contributor Author

satsfy commented May 14, 2026

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Include electrsd in the workspace

3 participants