PR #491 follow-up: deferred review items

[spreston8]

PR #491 follow-up: deferred review items

Tracks items deferred from a multi-pass review of #491 (feat(merge): bitmask-OR mergeable channels for registry concurrency).

Resolved in shipped commits (not listed below):

• a6f4884b — rholang_cli regression fix; assert_eq! → Result propagation through EventLogIndex::combine, compute_merged_state, resolve_conflicts, merge, dag_merger::conflicts_fn; BitmaskOr OR-fold dispatch via fold_multi_value; try_get_number_with_rnd migration with deletion of the panicking variant; 28 new test cases.
• 14f89956 — Registry.rho conflict-analysis comment refreshed for BitmaskOr semantics + dual-typing block (was item Protobuf -> CapnProto #20); docs/rholang/12-vaults-and-tokens.md patch-history phrasing rewritten (was item Port interpreter (and supporting code) to Rust #22) and Mergeable-Tagged Channels section added with footgun documentation; metaweta inline review comments addressed.

The bitmap monotonicity claim is verified — Registry.rho's interior bitmap writes are strictly bit-add (val | (1 << bit)) with no clearing path, so the BitmaskOr end & !prev diff representation is sound.

This issue groups remaining items by category. None are merge-blocking.

---

Code-level cleanup

1. unforgeable_name_rng is duplicated between casper and rholang

The PR's stated goal was "single source of truth" for tag identity, but the underlying primitive is now duplicated:

• casper/src/rust/util/rholang/tools.rs:10 — Tools::unforgeable_name_rng (still used by 5 production sites: runtime.rs:884, 1008, 1072, block_api.rs:1445, plus indirect via system_deploy_util.rs)
• rholang/src/rust/interpreter/merging/mergeable_tags.rs:47 — new identical copy

Bodies are byte-identical. Both compute Blake2b512Random::create_from_bytes(&DeployDataProto{deployer, timestamp, ..Default::default()}.encode_to_vec()). Won't drift today because both are stable, but a future change to one and not the other would silently break tag derivation for one consumer set.

Fix: consolidate. Either move the rholang copy out and have casper's Tools import it, or move both into a shared crate (crypto?) since the function is purely cryptographic.

2. is_mergeable_channel allocates Vec<Par> per call on every channel touch

File: rholang/src/rust/interpreter/reduce.rs:782-797

fn is_mergeable_channel(&self, chan: &Par) -> Option<MergeType> {
    let tuple_elms: Vec<Par> = chan
        .exprs
        .iter()
        .flat_map(|y| match &y.expr_instance {
            Some(ExprInstance::ETupleBody(etuple)) => etuple.ps.clone(),
            _ => ETuple::default().ps,
        })
        .collect();

    let result = tuple_elms
        .first()
        .and_then(|head| self.mergeable_tags.get(head).copied());
    ...
}

Hot path — runs on every channel write/consume during deploy execution. The Vec<Par> allocation plus etuple.ps.clone() (clones the entire tuple body) is unnecessary; only the head element matters.

Fix: read head as Option<&Par> without owning the Vec:

fn is_mergeable_channel(&self, chan: &Par) -> Option<MergeType> {
    let head = chan.exprs.iter().find_map(|y| match &y.expr_instance {
        Some(ExprInstance::ETupleBody(etuple)) => etuple.ps.first(),
        _ => None,
    })?;
    self.mergeable_tags.get(head).copied()
    // diagnostic trace block adapts to use Option<&Par> for head
}

Avoids the allocation and the per-call Par::clone on tuple elements.

3. concurrent_registry_inserts_should_not_conflict test added in #[ignore] state

File: casper/tests/util/rholang/runtime_manager_test.rs:2113-2120

/// Exercises the conflict-detection path for two independent contracts both
/// calling insertArbitrary. ... the test's `rejected.is_empty()` assertion
/// encodes an obsolete premise and needs to be rewritten once the
/// rejected-deploy recovery mechanism lands.
#[tokio::test(flavor = "multi_thread", worker_threads = 4)]
#[ignore = "assertion contradicts multi-parent DAG design; awaits rewrite"]
async fn concurrent_registry_inserts_should_not_conflict() { ... }

Concerns:

1. 470-line ignored test in the codebase that no one runs and that drifts out of date with each refactor.
2. The reason given ("rejected-deploy recovery mechanism lands") refers to PR fix(merge): close merge-stale-diff bug class #488's work, which is already shipped in rust/staging.
3. Reads bridge.rho (not bridge-v2.rho) — different fixture from the active test bridge_contract_concurrent_merge.

Fix: ask the author. Likely outcomes:

• Delete the ignored test and rely on bridge_contract_concurrent_merge.rs for coverage (cleanest).
• Rewrite the assertion to match post-fix(merge): close merge-stale-diff bug class #488 semantics (test recovery via buffer rather than zero-rejection).
• Wrap with explicit stub-naming convention so it doesn't masquerade as a regular test.

4. Multiple BitmaskOr tags would race for the same URI

File: rholang/src/rust/interpreter/rho_runtime.rs:1097-1119

for (tag_par, merge_type) in mergeable_tags.iter() {
    if let MergeType::BitmaskOr = merge_type {
        ...
        urn_map.insert(
            "rho:system:bitmaskMergeableTag".to_string(),
            tag_par.clone(),
        );
    }
}

The URI binding loop iterates a HashMap<Par, MergeType>. Currently default_mergeable_tags() has exactly one BitmaskOr entry, so iteration order doesn't matter. But the loop pattern matches every BitmaskOr-typed entry, and they'd all map to the same URI key. With N>1 BitmaskOr entries, the last-iterated wins — non-deterministic across runs because of HashMap iteration order.

Latent issue. Today nobody adds a second BitmaskOr tag. But a future PR that does (e.g., one BitmaskOr tag for registry, another for some other concurrent-bitmap structure) would silently expose the non-determinism: different validators might bind different Pars to the same URI based on hash-seed randomness, and merge dispatch would diverge.

Fix: make the URI binding key per-tag (e.g., "rho:system:bitmaskMergeableTag:{tag_index}"), OR enforce one-tag-per-URI at runtime by making urn_map.insert for the bitmask URI return an error on second insert.

5. mergeable_tags: HashMap<Par, MergeType> should be a BTreeMap (defense-in-depth)

The current consumer is_mergeable_channel only does .get(head) lookups (HashMap is fine), and create_rho_env iterates only to build the urn_map (single-entry today). No current site iterates the map to produce committed state.

But the type signature Arc<HashMap<Par, MergeType>> is exposed broadly: passed into runtime constructors, threaded through Reduce, accessible via mergeable_tags field. A future consumer that iterates the map to produce, say, a hash chain or a serialized list, would inherit non-determinism.

Fix: convert to Arc<BTreeMap<Par, MergeType>> everywhere. Lookup cost is O(log N) instead of O(1), but N is currently 2 — irrelevant. Future consumers can iterate without thinking.

6. Stale "Mirrors / redefined" doc comment on MergeType

File: rspace++/src/rspace/merger/merging_logic.rs:12-14

/// Merge strategy for a mergeable channel. Mirrors
/// `rholang::interpreter::merging::merge_type::MergeType` but is redefined
/// here to avoid `rspace++` taking a dependency on `rholang`.

There is no rholang::interpreter::merging::merge_type::MergeType in the codebase — rholang imports MergeType from rspace++ directly. The comment describes a design that was either discarded or never landed.

Fix: delete the second sentence. Replace with a one-liner about the enum's semantics:

/// Merge strategy for a mergeable channel. `IntegerAdd` combines diffs by
/// wrapping addition (vault balances, gas accumulators); `BitmaskOr` combines
/// by bitwise OR through `u64` (registry interior-node bitmaps).

7. pub → pub(crate) field encapsulation (5 sites)

File:line	Field	Suggested visibility
rholang/src/rust/interpreter/reduce.rs:118	pub merge_chs	pub(crate)
rholang/src/rust/interpreter/reduce.rs:119	pub mergeable_tags	pub(crate)
rholang/src/rust/interpreter/rho_runtime.rs:264	pub merge_chs	pub(crate)
casper/src/rust/util/rholang/runtime_manager.rs:68	pub mergeable_tags	pub(crate)
casper/src/rust/rholang/types/eval_collector.rs:10	pub mergeable_channels	check call sites

Verified no external consumer accesses these directly; pub(crate) would tighten the API surface.

The exception is EvaluateResult.mergeable (interpreter.rs:22) — genuinely part of the public API contract; keep as pub.

8. Committed TODO in bridge-v2.rho

File: casper/tests/resources/bridge-v2.rho

// TODO: change order of args to match unlock eth transfer style

PR #491 added a 340-line test fixture that includes a TODO. Committed source — the TODO will rot.

Fix: apply the change, file a tracking issue and reference its number, or delete the TODO.

9. bridge-v2.rho exists in 3 unsynchronized copies

1. system-integration/integration-tests/resources/bridge-v2.rho
2. f1r3node/casper/tests/resources/bridge-v2.rho
3. system-integration/docs/bridge-v2.rho

Identical content; no automatic sync. Future fixes to one will diverge from the others.

Fix: add a CI drift check that fails when copies disagree, OR consolidate to a single source of truth.

10. Arc::new(Genesis::default_mergeable_tags()) boilerplate

PR #491 added 12+ test sites that build Arc::new(Genesis::default_mergeable_tags()) inline.

Fix: add a helper:

impl Genesis {
    pub fn default_mergeable_tags_arc() -> Arc<HashMap<Par, MergeType>> {
        Arc::new(Self::default_mergeable_tags())
    }
}

11. BITMASK_OR_TAG_PK API uses misleading "private key" naming

File: rholang/src/rust/interpreter/merging/mergeable_tags.rs:37-38

The "private key" isn't used for signing — only seeds the RNG. The pubkey + timestamp gives a deterministic seed; the priv key just satisfies the unforgeable_name_rng signature.

Fix: consider renaming the API to drop the "private key" framing — a simpler tag_name_from_seed(&[u8]) would make the no-cryptographic-use clearer.

12. First rng.next() discarded in tag_name

File: rholang/src/rust/interpreter/merging/mergeable_tags.rs:56-66

fn tag_name(deployer_pk_hex: &str, timestamp: i64) -> Par {
    let pubkey = pub_key_from_hex(deployer_pk_hex);
    let mut rng = unforgeable_name_rng(&pubkey, timestamp);
    rng.next();                              // ← discarded
    let unforgeable_byte = rng.next();
    ...
}

The first rng.next() output is thrown away. This matches the original derivation pattern where the constants were originally located, and the pattern is load-bearing — but undocumented.

Fix: add a code comment explaining why the first rng.next() is skipped.

13. Per-call diagnostic-trace cost when enabled

File: rholang/src/rust/interpreter/reduce.rs:799-833

When RUST_LOG=f1r3fly.merge.tag_check=trace is enabled, every channel miss does:

• One encode_to_vec() on the head Par (proto encoding — non-trivial)
• N encode_to_vec() on each registered tag Par (currently N=2)
• N+1 hex-string formats

Fix: memoize the registered tag hex strings at runtime startup. Per-call cost drops to one head encoding + N string lookups.

14. URI binding INFO log fires per runtime construction

File: rholang/src/rust/interpreter/rho_runtime.rs:1107-1117

Logged at INFO level — one-shot startup diagnostic per runtime construction. Reasonable, but worth confirming it doesn't fire per-deploy or per-block in any test or replay path.

15. Pre-existing intra-branch wrapping vs cross-branch checked_add asymmetry

Files: casper/src/rust/merging/conflict_set_merger.rs:540-558

Stage	IntegerAdd	BitmaskOr
Intra-branch combine	wrapping_add (silent)	OR
Cross-branch apply	checked_add rejects on overflow / negative	OR

For IntegerAdd, an intra-branch overflow can produce a wrapped value that the cross-branch step accepts. Pre-existing behavior; PR #491 inherits via the new dispatch.

Verified safe under current gas-limit assumptions. Worth a code comment so future readers don't try to "fix" the inconsistency.

16. Pre-existing wrapping_add debug-mode behavior change

OLD code at conflict_set_merger.rs and event_log_index.rs used *existing += value. In release the same as wrapping_add; in debug Rust panics on overflow. PR #491's new wrapping_add always wraps regardless of build profile.

Validators run release, so consensus is unchanged. But debug-mode tests lose the implicit overflow check.

Fix: doc the silent-overflow contract on combine_mergeable_value so test authors know to use checked_add if they want the debug-mode guard.

---

Style nits

17. Patch-history phrasing in bridge_contract_concurrent_merge.rs extended comments

File: casper/tests/multi_node/bridge_contract_concurrent_merge.rs

Two adjacent comment blocks anchor on patch-history.

Lines 13-20 (top-of-file commentary):

"Note on bridge findOrCreate regression: the bridge-v2.rho fixture in this branch also includes a SystemVault.findOrCreate(bridgeVaultAddr) call at bridge init time. That fix is required for the integration-level test_multi_block_state_evolution test to pass..."

Lines 246-258 (end-of-test commentary):

"The bridge findOrCreate fix (bridge-v2.rho calls findOrCreate on its own vault address at init) is exercised at integration level... A code-level equivalent ... was attempted but the in-process TestNode's per-deploy data lookups don't expose deployId data the way live gRPC getDataAtName does..."

After the PR merges, "this branch" no longer exists as a distinct artifact. "The fix" describes a state that's now just "the code." A future reader doesn't care that there was a fix.

Fix: rewrite the top block in invariant terms ("Bridge fixture invariant: the bridge-v2.rho contract calls findOrCreate at init..."); drop the tail block.

18. Brittle tokio::time::sleep(1ms) between deploy constructions

File: casper/tests/multi_node/bridge_contract_concurrent_merge.rs:89, 101, 115, 130

Pattern repeated 4× to ensure each deploy gets a distinct millisecond-resolution timestamp, avoiding signature collision when two deploys signed by the same key would otherwise be byte-identical.

Fix: use distinct keys for each deploy, OR pass an explicit time_stamp override that monotonically increments. Eliminates the sleep dependency.

19. Misleading variable name bridge3_deploy

File: casper/tests/multi_node/bridge_contract_concurrent_merge.rs:129

The variable is named bridge3_deploy but the source is "Nil".to_string() — it's not a bridge contract, just a sibling-trigger.

Fix: rename to sibling3_trigger or nil_sibling_deploy.

---

Docs cleanup

20. Patch-history phrasing in docs/casper/README.md

"BitmaskOr was added to handle a class of failure where two registry inserts from sibling blocks both touched the same TreeHashMap interior node. Without bitmask merging, one of the inserts would be rejected at multi-parent merge..."

Frames the section around what got fixed. After 12 months, "was added" stops being interesting.

Fix:

"BitmaskOr exists for a specific class of channel: two writes to the same channel can each set distinct bits in a bitmap and combine without conflict. The motivating case is the registry's TreeHashMap interior nodes, where two inserts at different keys race on the parent's child-bitmap update."

21. Cross-repo "investigation report" reference is fragile

File: docs/testing-tracing.md

"A planned improvement is to surface orphan continuations directly in deploy results — see the 'How to make this easier next time' section of the bridge-deposit-orphan investigation report (in the system-integration repo, docs/)."

Two problems:

1. Cross-repo prose pointer that could rot.
2. "Tracked separately" pattern.

Fix: drop the forward-reference, OR replace with a real tracking-issue link.

22. Python correlation snippet uniqueness assumption

File: docs/testing-tracing.md

The orphan-detection Python snippet uses random_state as a per-produce identity. Under deterministic execution, two produces with the same seed could produce identical random_state values — they'd be aliased in the set, and one orphan could mask another.

Fix: add a one-line comment in the snippet noting that random_state is unique per-produce in normal Rholang execution but ambiguity could arise in pathological cases (deterministic-replay testing with identical inputs).

---

Test scenarios

Cumulative test coverage gaps from sections 1-9

These are above and beyond the 28 tests the author shipped in a6f4884b (commutativity proptests, multi-value OR-fold, monotonicity, mismatch error propagation, TreeHashMap delete invariants, depth-1 stress, registry inserts).

Critical / high-value

• CLI smoke test: cargo run --bin rholang-cli -- <Registry.rho> should construct a runtime without panic. The regression is fixed but no test prevents recurrence.
• URI binding presence assertion: for the CLI and production runtime construction paths, assert urn_map.contains_key("rho:system:bitmaskMergeableTag") after construction.
• Same-binary cross-validator parity: two validators running the same binary, same DAG state, registry-bitmap-modifying scenario; assert byte-identical body.deploys, body.rejected_deploys, post-state hash.
• Tag-identity stability lock: regression test that non_negative_mergeable_tag_name() and bitmask_or_mergeable_tag_name() produce the exact byte sequences the rest of the system expects (locks the magic constants so future changes break visibly).

Section 1 / 2

• Wrapping-add overflow regression: confirm no single-deploy diff can reach i64::MAX / max_deploys_per_block. If gas limits don't enforce this, file a separate consensus-safety issue.
• try_get_number_with_rnd returns None for Map values: unit test that constructs a ListParWithRandom containing a Rholang Map and asserts the function returns None without panicking.

Section 3

• is_mergeable_channel tuple-arity edge cases: zero-element tuple, single-element tuple matching a tag, single-element tuple NOT matching, two-element tuple where head matches, head-doesn't-match-but-second-element-does (must NOT match — only first element checked).
• merge_chs lifecycle: assert that two consecutive evaluate() calls produce independent EvaluateResult.mergeable maps (no bleed-through).
• URI-binding consistency: test that the Par returned by urn_map.get("rho:system:bitmaskMergeableTag") is byte-identical to mergeable_tags::bitmask_or_mergeable_tag_name() and to the key under which MergeType::BitmaskOr is registered in default_mergeable_tags().

Section 5

• Leaf-Map fall-through test: two concurrent inserts that share a leaf path with the same key. Assert that exactly one is rejected (leaf conflict preserved).
• Mixed interior+leaf merge test: deploy A inserts at key K1, deploy B inserts at key K2, where K1 and K2 share a common prefix at depth d but differ at depth d+1. Assert that the interior-node bit-set at depth d is mergeable (both deploys land) and the leaves at depth d+1 are different and both land.

Section 6

• Bridge unit test for the orphan-send pattern: deploy a bridge contract that DOESN'T call findOrCreate and assert that a transfer hangs (verifies the bug exists), then deploy with findOrCreate and assert success. Documents the requirement.
• CI grep check: assert that no production-resource Rholang contract has _deposit references without a corresponding findOrCreate call.

---

Summary

Category	Count
Code-level cleanup	16 (#1-16)
Style nits in tests	3 (#17-19)
Docs cleanup	3 (#20-22)
Test scenarios	~12

None merge-blocking. Items #1-5 are the highest-value follow-ups (real maintenance / correctness / future-proofing); the rest are incremental polish.

---

Co-Authored-By: Claude noreply@anthropic.com

[linear]

F1R-153

[spreston8]

This issue has been migrated to F1R3FLY-io/f1r3node-rust#46 as part of the deprecation of this repository. Please continue tracking the work there.