Fully encapsulate bitcoinconsensus

[tcharding]

The bitcoinconsensus crate is not fully under our control because it exposes code from Core, so we cannot guarantee its stability across versions. To make our semver compliance easier we can fully encapsulate the bitcoinconsensus crate so it does not appear in our public API.

Please note that with this applied:

• The bitcoinconsenus crate is no longer exported at the crate root
• No bitcoinconsensus types appear in our public API

[tcharding]

I tested this using the following script

#!/usr/bin/env bash
#
# Checks the public API of crates, exits with non-zero if there are currently
# changes to the public API not already committed to in the various api/*.txt
# files.

set -e

export RUSTDOCFLAGS='-A rustdoc::broken-intra-doc-links'
REPO_DIR=$(git rev-parse --show-toplevel)
API_DIR="$REPO_DIR/api"
CMD="cargo +nightly public-api --simplified"

# cargo public-api uses nightly so the toolchain must be available.
if ! cargo +nightly --version > /dev/null; then
    echo "script requires a nightly toolchain to be installed (possibly >= nightly-2023-05-24)" >&2
    exit 1
fi

pushd "$REPO_DIR/bitcoin" > /dev/null
$CMD | sort --unique > "$API_DIR/bitcoin/api-default-features.txt"
$CMD --features=all-stable-features | sort --unique  > "$API_DIR/bitcoin/api-all-stable-features.txt"
$CMD --all-features | sort --unique > "$API_DIR/bitcoin/api-all-features.txt"
popd > /dev/null

pushd "$REPO_DIR/hashes" > /dev/null
$CMD --no-default-features | sort --unique > "$API_DIR/hashes/api-no-features.txt"
$CMD | sort --unique > "$API_DIR/hashes/api-default-features.txt"
$CMD --no-default-features --features=alloc | sort --unique > "$API_DIR/hashes/api-alloc.txt"
$CMD --all-features | sort --unique > "$API_DIR/hashes/api-all-features.txt"
popd > /dev/null

pushd "$REPO_DIR" > /dev/null
if [[ $(git status --porcelain api) ]]; then
    echo "You have introduced changes to the public API, commit the changes to api/ currently in your working directory" >&2
else
    echo "No changes to the current public API"
fi
popd > /dev/null

Grepping the output files we get:

• grep bitcoinconsensus api-all-stable-features.txt returns blank
• grep bitcoinconsensus api-all-features.txt
  pub extern crate bitcoin::bitcoinconsensus

[apoelstra]

ACK 135d143

[tcharding]

Needs #2279

[tcharding]

Rebased to pick up #2279, no other changes.

[apoelstra]

Looks good to me! Would like @Kixunil's opinion on this feature organization to enable gated pub extern crates.

[apoelstra]

ACK 2795b07

[junderw]

Out of curiosity, what are some of the differences between why tokio also requires RUSTFLAGS? (Compared to this PR)

https://docs.rs/tokio/latest/tokio/#unstable-features

[tcharding]

My guess is so that --all-features is stable? Also using --cfg instead of a cargo feature make opting into unstable features more explicit, like we do for bench marks. Perhaps we should do this too?

[junderw]

My guess is so that --all-features is stable? Also using --cfg instead of a cargo feature make opting into unstable features more explicit, like we do for bench marks. Perhaps we should do this too?

Yeah that was my thought process too.

[Kixunil]

They explain it here: https://internals.rust-lang.org/t/feature-request-unstable-opt-in-non-transitive-crate-features/16193#why-not-a-crate-feature-2

Concept ACK having an unstable feature (I had some other ideas too) however, I think the reason for not using cargo features is sound so I'd prefer that way and secondly, I think it's very little value here since people can explicitly depend on a specific version of bitcoinconensus and not have to deal with our flag at all. Thus I think simply not exporting is better here.

[apoelstra]

Yeah, I like their reasoning. Let's use a cfg flag rather than a feature.

[tcharding]

Flagging that this PR does not add any feature, cfg or cargo, because it now just does not re-export the bitcoinconsenus crate. Requires ack from @apoelstra please (who was initially against removing the re-export).

[Kixunil]

ACK 43b1ed1

[Kixunil]

I don't think this is needed at all now.

[apoelstra]

I had the same thought, but didn't check. (In Rust post-2018 non-pub extern crates are no-ops, I believe.)

[Kixunil]

pub are not no-ops (they are reexports) but non-pub are.

[apoelstra]

Flagging that this PR does not add any feature, cfg or cargo, because it now just does not re-export the bitcoinconsenus crate. Requires ack from @apoelstra please (who was initially against removing the re-export).

This is fine, though I'd like to revisit it at some point.

[apoelstra]

ACK 43b1ed1