q-56: Miri islands beyond primitives (scriptnum + compact) - #623
Hero-Gamer wants to merge 3 commits into
Conversation
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
This does not close Q-56.
Q-56 is cfg(miri) coverage of the FFI-free shipped helpers (scriptnum + pack integers) without pulling secp/store and without expanding nightly miri.yml past -p rbitcoin-primitives (Q-53). Putting those helpers in rbitcoin-primitives is the right seam: consensus and store already depend on that crate, so Miri can see the same functions confirm and archive run. Copying the algorithms into new modules, testing the copies, and leaving the originals in place is a dual path. Miri never executes the code the node uses.
What landed:
- New
scriptnum.rs/compact.rsin primitives, crate-rootpub use, no callers outside this crate. crates/rbitcoin-consensus/src/script/interpreter.rsstill hasscriptnum_encode/scriptnum_decode_width/scriptnum_is_minimal(eval still calls those).crates/rbitcoin-store/src/compact.rsstill has the CompactSize + ULEB128 implementation (andrbitcoin-mempool/src/packed.rshas a third CompactSize copy).- Docs claim a peel from god-files and delete the Open row. That is not what the diff does.
R-10 does not license this. Residual peels happen only when a higher row needs a seam, and you do not split the interpreter.rs opcode match. Extracting the four scriptnum helpers (and store pack-ints) is the Q-56 seam — but only if production calls the extract and the originals go away. A fork is the dual-path Protect rule / code-shape “one owner per algorithm.”
To actually close Q-56:
- Move, do not copy. Keep one implementation in primitives. Interpreter becomes a thin wrapper (
ConsensusErrorfromScriptNumError). Storecompact.rswrapsStoreErroraround the same pack-int fns and deletes its copies. Existing interpreter / store tests stay the pin (docs/code-shape.mdextract policy). - Switch callers in the same change. Until
scriptnum_decodein eval andwrite_compact_sizeon the archive path import primitives, this is unusedpub(CONTRIBUTING 11) plus a second algorithm. - Miri the shipped fns.
#[cfg(miri)]extra loops are the right pattern (always-on units + denser Miri). Point them at the functions production calls. Port the cases already pinned inscriptnum_minimal_encoding([0x00],[0x80]negative zero,+255high-bit pad) and CLTV/CSV width 5. Compact Miri should hit 5-byte / 9-byte CompactSize, ULEB128, and the truncated/overflow paths store already has incompact_and_uleb_error_paths—0..10000never leaves the 1-byte / low 3-byte CompactSize forms and never runs ULEB. - One CompactSize owner. If pack-ints move to primitives, mempool
packed.rsshould call that too. Signet’s streamingread_compact_sizecan stay a thin slice walker over the same decoder. - Close the roadmap only with the landing change. Restore the Q-56 Open row until (1)–(3) are done. Then move it to CHANGELOG describing what production now calls, and re-rank Q-67 (it is still listed as rank 7 with 6 missing).
Leave miri.yml / miri.sh as primitives-only. Do not --workspace miri.
The cfg(miri) test shape and keeping nightly primitives-only are the correct Q-53/Q-56 constraints. The missing piece is that Miri has to run the helpers confirm and store actually call.
| mod compact; | ||
| mod scriptnum; | ||
| pub use compact::{ | ||
| compact_size_len, read_compact_size, read_uleb128, uleb128_len, write_compact_size, | ||
| write_uleb128, CompactError, | ||
| }; | ||
| pub use scriptnum::{ | ||
| decode_scriptnum, decode_scriptnum_4, encode_scriptnum, is_minimal_scriptnum, ScriptNumError, | ||
| }; |
There was a problem hiding this comment.
These pub use names have no caller outside this crate. CONTRIBUTING 11: crate-root pub is the production graph only.
Q-56 is Miri on the FFI-free helpers the node already runs (scriptnum_* in interpreter.rs, CompactSize/ULEB in store/src/compact.rs), not a parallel copy. rbitcoin-consensus and rbitcoin-store already depend on this crate — the extract is switch those callers, then delete the originals. Until that happens this is unused pub plus a dual path (code-shape rule 6).
Nit: mod compact / mod scriptnum belong with the other modules at the top of this file, not after the tests module.
| pub fn encode_scriptnum(mut n: i64) -> Vec<u8> { | ||
| if n == 0 { | ||
| return vec![]; | ||
| } | ||
| let neg = n < 0; | ||
| if neg { | ||
| n = -n; | ||
| } | ||
| let mut out = Vec::new(); | ||
| while n > 0 { | ||
| out.push((n & 0xff) as u8); | ||
| n >>= 8; | ||
| } | ||
| if out.last().map(|b| b & 0x80 != 0).unwrap_or(false) { | ||
| out.push(if neg { 0x80 } else { 0x00 }); | ||
| } else if neg { | ||
| *out.last_mut().unwrap() |= 0x80; | ||
| } | ||
| out | ||
| } |
There was a problem hiding this comment.
This is the same algorithm as scriptnum_encode / scriptnum_decode_width / scriptnum_is_minimal in crates/rbitcoin-consensus/src/script/interpreter.rs (~1505–1594). Those fns are still there; eval still calls them. This is a fork, not an R-10 peel.
R-10 allows extracting these helpers (not the opcode match) as the Q-56 seam if production calls the extract. Green is: interpreter wraps ScriptNumError → ConsensusError and deletes the local copies. Existing scriptnum_minimal_encoding / CLTV-CSV width-5 tests stay the pin (docs/code-shape.md extract policy).
n = -n on i64::MIN overflows; the production copy has the same line. A real island would let Miri see that if anyone passed MIN. Do not leave two copies of that negate.
| fn minimality() { | ||
| assert!(is_minimal_scriptnum(&[])); | ||
| assert!(is_minimal_scriptnum(&[0x01])); | ||
| assert!(!is_minimal_scriptnum(&[0x01, 0x00])); | ||
| } | ||
| #[cfg(miri)] | ||
| #[test] | ||
| fn miri_roundtrip() { | ||
| for n in -1000..1000 { | ||
| let enc = encode_scriptnum(n as i64); | ||
| if enc.len() <= 4 { | ||
| assert_eq!(decode_scriptnum_4(&enc, true).unwrap(), n as i64); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The shipped pin is scriptnum_minimal_encoding in the interpreter tests: [], [0x00], [0x80] (negative zero), [0x01], [0x01, 0x00], [0xff, 0x00] (+255 high-bit pad), [0xff, 0x80] (−255). This island drops the two cases that actually catch non-minimal encoding.
CLTV/CSV decode at width 5 (scriptnum_decode_width(..., 5, ...)). decode_scriptnum_4 never sees that path. TESTING.md: tests drive the shipped function, not a reimplementation.
The #[cfg(miri)] extra loop is the right shape — point it at the functions eval calls, and include width 5 / non-minimal bytes, not only -1000..1000 roundtrips that already fit in 4 bytes.
| pub fn write_compact_size(out: &mut Vec<u8>, n: u64) { | ||
| if n < 253 { | ||
| out.push(n as u8); | ||
| } else if n <= u16::MAX as u64 { | ||
| out.push(253); | ||
| out.extend_from_slice(&(n as u16).to_le_bytes()); | ||
| } else if n <= u32::MAX as u64 { | ||
| out.push(254); | ||
| out.extend_from_slice(&(n as u32).to_le_bytes()); | ||
| } else { | ||
| out.push(255); | ||
| out.extend_from_slice(&n.to_le_bytes()); | ||
| } | ||
| } |
There was a problem hiding this comment.
Copy of crates/rbitcoin-store/src/compact.rs (and a third CompactSize lives as private fns in rbitcoin-mempool/src/packed.rs). Store still owns the production pack-int path.
If this extract is real: store wraps StoreError around these fns and deletes its copies; mempool calls the same owner (code-shape: one algorithm). write_uleb128 here uses .unwrap(); store uses .expect("10-byte stack holds any u64 uleb128") — keep that invariant message. Truncated ULEB maps to CompactError::Empty, which collapses two store error strings (empty vs truncated).
| #[test] | ||
| fn miri_compact() { | ||
| for n in 0..10000u64 { | ||
| let mut out = Vec::new(); | ||
| write_compact_size(&mut out, n); | ||
| let (dec, _) = read_compact_size(&out).unwrap(); | ||
| assert_eq!(dec, n); | ||
| } | ||
| } |
There was a problem hiding this comment.
0..10000 only hits 1-byte CompactSize and the start of the 3-byte form (253+). Q-56 is pack integers: 5-byte / 9-byte CompactSize and ULEB128 are the encodings that matter, and Miri never runs them here.
Store already pins truncated/overflow in compact_and_uleb_error_paths — those are the cases Miri is for (overflowing << on a 10-byte ULEB with extra high bits). Add a cfg(miri) ULEB loop and the width/error cases, on the shipped functions, not this fork.
| | 4 | **R-10** | Residual god-files | Peel **only** when a higher row needs a seam. Do not split `interpreter.rs` opcode `match` or io_uring machines. Named extracts: **Q-61** Completed. | | ||
| | 5 | **Q-54** | ast-grep named-cap rules | One rule per easy-to-delete cap from [`ibd-memory.md`](./ibd-memory.md): `pending_blocks` 128, `held_bodies` 320, `MAX_SERVE_BLOCKS` 16, `follow_live` vs `max_outbound`. Each has `lint/ast-grep/fixtures/{good,bad}/`. Today **four** structural rules, **zero** cap rules. | | ||
| | 6 | **Q-56** | Miri islands beyond primitives | `cfg(miri)` tests for FFI-free helpers (scriptnum, pack integers) that do not pull secp/store. Never workspace miri. Nightly `miri.yml` is still primitives-only (**Q-53**). | | ||
| | 7 | **Q-67** | `asked_blocks` clone on hold | `hold_body` clones `asked_blocks` before `held_bodies` insert so the read lock does not overlap the write (`HeldBodies::insert` already takes `&HashSet`). Bound is `MAX_SERVE_BLOCKS` × peers. Follow-up: pass the read guard with a documented lock order, or keep the clone as a named trade. Owner: `crates/rbitcoin-net/src/chain.rs`. | |
There was a problem hiding this comment.
Do not close Q-56 by deleting the Open row while confirm/store still run the old helpers. The close rule is: move the row into CHANGELOG in the same edit as the landing change (production callers + Miri on those fns).
Also re-rank: Q-67 is still listed as rank 7 with rank 6 gone.
| ### Added | ||
|
|
||
| - **Q-56 Miri islands beyond primitives:** `scriptnum` (encode/decode/is_minimal) + `CompactSize` + `ULEB128` peeled into `rbitcoin-primitives` (FFI-free, 0 deps). Adds `cfg(miri)` islands (1000-range scriptnum, 0..10000 compact) so `cargo +nightly miri test -p rbitcoin-primitives` works. Never `--workspace` miri. R-10 peel allowed. | ||
|
|
There was a problem hiding this comment.
This says the helpers were “peeled into rbitcoin-primitives.” On this branch fn scriptnum_encode is still in interpreter.rs and the full CompactSize/ULEB set is still in store/src/compact.rs.
Write this bullet in the same commit as the caller switch, and describe what production now calls — not a parallel island. Extra blank line below is accidental.
|
Sorry — Q-56 should not have been on Open either. Same leftover lint program as Q-54. Extra Miri islands of scriptnum/pack-ints either peel interpreter/store (R-10: a lower row cannot demand that seam) or copy the helpers (dual path). Nightly Miri stays primitives-only (Q-53); workspace miri is already Won't-fix. The Q-56 is moving to Won't-fix in #624. |
|
Reopened — sorry for the close. Q-56 stays. The copies in this PR are not enough, but moving the shipped helpers into Q-54 is still Won't-fix (#624). This row is not. |
|
rebase this PR and update |
|
Taken over: the copies are now a real peel (production callers, local copies deleted, Q-56 closed only with the landing change). Follow-up PR on |
|
Takeover PR: #639 |
2 participants
Closes Q-56 from docs/quality.md:33
Test: cargo test -p rbitcoin-primitives --lib (19 passed)
Miri dry: cargo +nightly miri test -p rbitcoin-primitives