rest: fix unreachable provably-unspendable script classification

[zloglevel]

Summary

Fix REST output script classification for provably unspendable scripts.

The existing classification contained two identical is_op_return() branches.

The second branch was unreachable, causing non-OP_RETURN provably unspendable scripts to be reported as unknown.

Changes

• Replace the unreachable duplicate is_op_return() check with is_provably_unspendable().
• Preserve the dedicated op_return classification by checking it first.
• Add regression coverage for:
  • OP_RETURN scripts
  • non-OP_RETURN provably unspendable scripts
  • unknown but potentially spendable scripts
• Cover both Bitcoin and Liquid builds.

Verification

• cargo +1.92.0 check --lib
• cargo +1.92.0 check --lib --features liquid

[EddieHouston]

@zloglevel Thank for your submission. The duplicated is_op_return() branch is clearly unreachable, and this restores the original REST distinction between op_return and other provably-unspendable scripts. (changed with e497577)

Could we avoid introducing a new call to the deprecated bitcoin::Script::is_provably_unspendable() and suppressing it with #[allow(deprecated)]?

The same compatibility issue was handled in mempool/electrs#138 (mempool/electrs#138), specifically in commit f8302d7c (mempool/electrs@f8302d7). It defines a small feature-specific IsProvablyUnspendable helper: the Bitcoin implementation preserves the existing ReturnOp | IllegalOp behavior, while the Liquid implementation delegates to elements::Script::is_provably_unspendable().

Using the same approach here would preserve the intended API behavior without depending on a method that rust-bitcoin plans to remove.

It would also be good to run the new unit test under both the default and liquid configurations rather than only cargo check.

[zloglevel]

@zloglevel Thank for your submission. The duplicated is_op_return() branch is clearly unreachable, and this restores the original REST distinction between op_return and other provably-unspendable scripts. (changed with e497577)

Could we avoid introducing a new call to the deprecated bitcoin::Script::is_provably_unspendable() and suppressing it with #[allow(deprecated)]?

The same compatibility issue was handled in mempool/electrs#138 (mempool/electrs#138), specifically in commit f8302d7c (mempool/electrs@f8302d7). It defines a small feature-specific IsProvablyUnspendable helper: the Bitcoin implementation preserves the existing ReturnOp | IllegalOp behavior, while the Liquid implementation delegates to elements::Script::is_provably_unspendable().

Using the same approach here would preserve the intended API behavior without depending on a method that rust-bitcoin plans to remove.

It would also be good to run the new unit test under both the default and liquid configurations rather than only cargo check.

Thanks for the review. @EddieHouston

I updated the implementation to follow the compatibility approach from mempool/electrs#138.

The REST classification now uses a feature-specific IsProvablyUnspendable helper instead of calling the deprecated bitcoin::Script::is_provably_unspendable().

The Bitcoin implementation preserves the existing ReturnOp | IllegalOp semantics, while the Liquid implementation delegates to elements::Script::is_provably_unspendable().

I also kept the regression test covering OP_RETURN, provably unspendable, and unknown scripts. Both Bitcoin and Liquid library builds pass with cargo check --lib. Running cargo test --lib locally on macOS is currently blocked by the repository's existing Linux-only electrumd build script, which fails because download_filename is unavailable on macOS.

[EddieHouston]

@zloglevel Thank for your submission. The duplicated is_op_return() branch is clearly unreachable, and this restores the original REST distinction between op_return and other provably-unspendable scripts. (changed with e497577)
Could we avoid introducing a new call to the deprecated bitcoin::Script::is_provably_unspendable() and suppressing it with #[allow(deprecated)]?
The same compatibility issue was handled in mempool/electrs#138 (mempool/electrs#138), specifically in commit f8302d7c (mempool/electrs@f8302d7). It defines a small feature-specific IsProvablyUnspendable helper: the Bitcoin implementation preserves the existing ReturnOp | IllegalOp behavior, while the Liquid implementation delegates to elements::Script::is_provably_unspendable().
Using the same approach here would preserve the intended API behavior without depending on a method that rust-bitcoin plans to remove.
It would also be good to run the new unit test under both the default and liquid configurations rather than only cargo check.

Thanks for the review. @EddieHouston

I updated the implementation to follow the compatibility approach from mempool/electrs#138.

The REST classification now uses a feature-specific IsProvablyUnspendable helper instead of calling the deprecated bitcoin::Script::is_provably_unspendable().

The Bitcoin implementation preserves the existing ReturnOp | IllegalOp semantics, while the Liquid implementation delegates to elements::Script::is_provably_unspendable().

I also kept the regression test covering OP_RETURN, provably unspendable, and unknown scripts. Both Bitcoin and Liquid library builds pass with cargo check --lib. Running cargo test --lib locally on macOS is currently blocked by the repository's existing Linux-only electrumd build script, which fails because download_filename is unavailable on macOS.

Thanks, we will take a look at it again.

For the macos issue... we plan to resolve this in the repo soon... in the short term you can do this (or add it to your AGENTS.md or CLAUDE.md):

When running tests in this repository, temporarily patch `electrumd` in `Cargo.toml`:

 [patch.crates-io.electrumd]
 git = "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/Randy808/electrumd"
 rev = "146792ffe892be4ef9b52510d1e58481a9cdd1eb"

After testing, restore the original `Cargo.toml` entry and undo any `Cargo.lock` churn.

Also see #224

[EddieHouston]

@zloglevel Thanks for the update. I reviewed the latest commit, 726e91c (726e91c).

The feature-specific IsProvablyUnspendable helper addresses the deprecation concern and preserves the intended behavior: Bitcoin retains the existing ReturnOp | IllegalOp classification, while Liquid delegates to elements::Script::is_provably_unspendable(). Keeping the dedicated op_return check first also preserves the REST API distinction.

I ran the focused regression test successfully under both configurations:

cargo test --lib test_script_type_unspendable_classification
cargo test --lib --features liquid test_script_type_unspendable_classification

Both passed using the temporary macOS electrumd patch described above. No further issues from my side. Thanks!

[EddieHouston]

lgtm. 726e91c