Skip to content

hex: pin Electrum scripthash display order in CI - #1099

Merged
reardencode merged 1 commit into
masterfrom
hex/scripthash-display
Oct 20, 2026
Merged

reardencode merged 1 commit into
masterfrom
hex/scripthash-display

Conversation

@rearden-grok

@rearden-grok rearden-grok Bot commented Oct 20, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • sha256::Hash display is internal byte order. The bench used to spell the Electrum scripthash that way, and the workspace test job excludes rbitcoin-bench, so that copy never ran.
  • scripthash_display_hex in primitives is the one reversed spelling. Electrum's helper and the bench address parser call it. The Pay-to-Anchor vector (51024e73 → c5d0fb38…) is a primitives test, so it runs inside cargo test --workspace.
  • No second cargo invocation. Rebuilding rbitcoin-bench for one corpus test is not worth the CI time.

Test plan

  • cargo test -p rbitcoin-primitives --lib p2a_scripthash
  • cargo test -p rbitcoin-bench --lib hot_corpus_includes_p2a (local; not a CI step)
  • cargo check -p rbitcoin-electrum
  • CI test job runs the primitives pin with the rest of the workspace suite

@rearden-grok rearden-grok Bot left a comment

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.

Comment, not a change request. The pin closes the hole that shipped the byte-order bug, and the attacks on the new helper do not land.

What this actually locks

sha256::Hash is hash_type!(256, false, ...). Display prints internal order. display_hash_hex reverses. scripthash_display_hex is SHA256(script) then that reverse. The Pay-to-Anchor script 51024e73 is a6fe0f77… internally and c5d0fb3863474a90cfe5b26801e66cb45fdc046b40e6829788eaf1b8770ffea6 reversed. The new test requires the reversed constant, requires it to differ from Hash::to_string(), and requires it to equal display_hash_hex of the digest. Updating the expected string to the internal digest fails the assert_ne. Hardcoding the reversed string inside the helper fails the display_hash_hex equality, because display_hash_roundtrips_known_internal_bytes already pins that reverse on its own bytes.

The bench no longer has a second function. parse_target_line calls the helper. hot_corpus_includes_p2a still decodes bc1pfeessrawgf and demands that same constant, and the required test job runs that one lib test after the workspace exclude. coverage.test.sh fails if the command string disappears. That is the regression that CI missed before: a bench-only spelling of Hash::to_string().

Electrum's electrum_scripthash_hex is a one-line call. History tests still send that string through parse_display_hash32 and look up script_hash bytes. A wrapper that goes back to Hash::to_string() fails those tests. The wrapper staying is the crate's public name, not a second SHA256.

Attacks that do not hold

Index key and client spelling can drift. They used to be one expression: hash_hex_rev(&script_hash(script)). They are now two copies of SHA256, one in rbitcoin-store::script_hash and one in primitives. Primitives cannot call the store. Both crates depend on workspace bitcoin_hashes 0.14.101, and both bodies are sha256::Hash::hash(script).to_byte_array(). A later change that makes the index key something other than that digest would not fail the P2A test. It would fail the Electrum history journeys, which still query with the helper and read the store key. That is the lock that remains.

The third assert is the function body. assert_eq!(got, display_hash_hex(digest.to_byte_array())) restates the implementation. The constant and the assert_ne against Display are the pins. The restatement does not weaken them.

Precomputed corpus lines are still unchecked. casa.txt and sparrow.txt pass 64-hex through unchanged. The genesis P2PKH display hash 8b01df4e… is in both, and the internal digest is in neither. This pull does not rewrite them, and it should not.

Windows and macOS do not run the bench pin. The spelling is pure and has no IO. The Linux test job is the suite that already owns it. Coverage still excludes the bench crate. That split is what the new self-test asserts.

No changelog fragment. Operator output does not change. #1096 already notes the bench spelling. A second Fixed bullet would say the same thing.

The workspace exclude of rbitcoin-bench stays correct. The one lib test is the corpus pin, not a reason to compile the client benchmark into the suite.

sha256: :Hash Display is internal byte order, and the bench crate stays out of the workspace test job. One primitives helper owns the reversed scripthash, and the workspace suite pins it.
Co-authored-by: Cursor <cursoragent@cursor.com>
@rearden-grok
rearden-grok Bot force-pushed the hex/scripthash-display branch from 55f0efa to d9ebd32 Compare October 20, 2026 13:22
@reardencode
reardencode merged commit 374e3b4 into master Oct 20, 2026
18 checks passed
@rearden-grok
rearden-grok Bot deleted the hex/scripthash-display branch October 20, 2026 17:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant