P2P orphanage: park is not reject, fetch parents, 1p1c, compact extra

[rearden-grok]

Goal

Child-before-parent on P2P actually completes: parked orphans are not logged as rejects, INV already-have includes the orphanage, we GETDATA missing parents, compact reconstruction uses orphan/extra bodies, known-invalid parents do not park forever, and a below-min-relay parent plus paying child admits as 1p1c.

Why

txrelay: reject orphaned was a successful park mislabeled as a hard reject (INFO + debug). INV ignored the orphanage so we re-GETDATA'd parked children. We never asked the announcer for unique missing parents, so orphans only promoted if the parent was independently INV'd or landed in a block.

Core has cluster mempool; this is not a unique-advantage play. 1p1c here is Core-class package admit from the orphanage + reconsiderable min-relay rejects, without waiting on BIP331 (Q-48).

Plan (how-we-plan slices)

Each slice is Red test → Green → still-green related tests.

1. Logs — Orphaned is park (debug txrelay: park), not INFO reject. Duplicate stays silent.
2. INV already-have — try_contains / try_contains_wtxid include the orphanage.
3. Known-parent OOB vout — MissingPrevout, not orphan-forever (Q-58).
4. Parent GETDATA — after park, GetData unique missing parent txids (60s TTL, 16/park cap).
5. Compact extra — BIP152 short-id fill clones orphanage bodies, then extra-compact (RBF replacements + min-relay rejects).
6. Recent-invalid — InputsDuplicate / script / finality rejects are remembered; children of those txids do not park.
7. 1p1c — min-relay-rejected parent body + paying child admits as a package (package feerate vs min-relay).
8. Observability — getmempoolinfo.orphanage.{size,bytes}.

Out of scope

• Durable orphans
• BIP331 wire package relay (Q-48)
• Full Core TxRequestTracker (delays, multi-peer retry)
• Feerate-based orphan eviction (no fee without parent values)

Test

cargo test -p rbitcoin-mempool --lib -- accept::
cargo test -p rbitcoin-net --lib parked_orphan
cargo test -p rbitcoin-rpc --lib blockchain_empty_store

[rearden-grok]

Verdict: request changes (posted as a comment review because the App token authored this PR and GitHub refuses REQUEST_CHANGES on its own PR).

Reviewed all 13 commits; ran the branch's tests plus three throwaway probes against fae74c6d (probes not committed anywhere). CI is green, the log/INV/GETDATA/compact-extra slices look right, but the 1p1c slice does not work on the path the PR is about (P2P via MempoolHub) and the skip_min_relay flag opens a min-relay bypass. Details below; inline comments point at the exact lines.

Blocking

1. Hub-level 1p1c never admits — the P2P path is not exercised by any test. MempoolHub::accept_with_utxo → admit_staged already parks the child (park_orphan under write) before admit_1p1c runs. admit_1p1c then calls MempoolHub::accept_package, which preps all members under the read lock before committing any; the child's prepare_admit returns Orphaned (first from the orphanage.contains(&txid) early return, and even without that because the parent is not in the graph yet). accept_package errors, res.ok()? is None, and the child is left parked with the parent unadmitted.

Probe (hub, regtest UTXO from pad_empty_from, set_min_relay_sat_kvb(50_000)): parent alone → Policy("min relay fee") ✓; then hub.accept_tx(&child) → Err(Orphaned(..)), orphan_count()==1, neither tx live. The only passing 1p1c test (one_parent_one_child_admits_below_minrelay_parent) drives ActiveMempool::accept_tx, which P2P/RPC/Esplora never call. Needs a red hub test first, then a fix that commits the parent before prepping the child (sequential staged admit), or unparks the child and evaluates the package under one write lock with the trade named.

2. skip_min_relay is process-global mutable state and leaks. (a) Inside ActiveMempool::accept_package, promote_orphans_of(parent) runs while the flag is still true, so every parked orphan of that parent is admitted at floor 0. Probe: parent with two outputs (min-relay reject), sibling A spending vout 0 with a 1 sat fee (parks), child B spending vout 1 with a real fee → after B's 1p1c, A is live. (b) In the hub, set_skip_min_relay(true) / (false) are separate write-lock acquisitions around accept_package, so any concurrent accept_tx from another peer during that window is admitted below min relay. Fix: pass the floor (or skip_min_relay: bool) through AdmitSpec / prepare_admit / accept_tx_inner as a parameter; drop the field and setter.

Should fix in this PR

3. try_one_parent_package counts confirmed inputs as missing parents. It mirrors park_orphan's graph-only scan, so a child that spends the low-fee parent and one confirmed coin has missing.len()==2 and is never eligible. Probe: Err(Orphaned). The same over-approximation feeds orphan_missing_parents → take_parent_getdata, which filters only via try_contains (mempool/orphanage), so we GETDATA confirmed txids from the peer and burn the 16-slot cap on them. prepare_admit already computed the exact missing_parents set — carry it (e.g. in the Orphaned variant or into Orphanage::insert) instead of recomputing graph-only.

4. recent_invalid remembers tip-dependent rejects and is keyed by txid. NotFinal / NonBip68Final / ImmatureCoinbase are true only at the current tip; Core clears its recent-rejects filter on every tip change (hashRecentRejectsChainTip). Here they persist until the 4096 cap wipes everything, so children of a parent that becomes final next block get MissingPrevout instead of parking, and 1p1c refuses the parent. Either exclude those three variants or clear recent_invalid in remove_for_block_with_utxo. Separately, for Script(_) Core only inserts the txid when the tx has no witness (wtxid==txid) because a peer can send a witness-malleated invalid copy; inserting txid unconditionally lets one peer poison a valid txid so honest children never park. Gate on !tx.input.iter().any(|i| !i.witness.is_empty()) (or key by wtxid).

5. Write lock on every hub reject. accept_with_utxo now takes lock_write() for note_accept_failure on all failures including Duplicate and Orphaned (the common ones), which is a write-lock acquisition on the hot reject path with compute_txid inside. Match the recordable variants first and skip the lock otherwise.

Minor

• note_conflict_and_parent: .unwrap_or(true) when the creator's body is missing maps a broken invariant to MissingPrevout. AGENTS: missing promised fact → Durable/Corrupt, not a fallback (prepare_admit already uses Durable("parent body missing") for the same case).
• The recent_invalid branch returns MissingPrevout(tx.input[0].previous_output) regardless of which input is missing (the existing non-park branch has the same defect); report the actual outpoint.
• note_extra does retain(|t| t.compute_txid() != txid) — up to 100 double-SHA256 per RBF replacement / min-relay reject. Store the txid alongside the body (the orphanage already caches wtxid).
• orphan_stats bytes uses wu / 4; vsize is ceil(wu/4) (policy::get_virtual_size). The live bytes above has the same rounding, so not blocking, but both are off.
• No test for PARENT_GETDATA_TTL / MAX_PARENTS_PER_PARK (take_parent_getdata dedupe, TTL expiry, cap).
• CodeQL alert 199 (nonce = 1u64 in compact_fill_uses_parked_orphan) fires only in a test: per AGENTS, ask the operator to dismiss; do not shuffle the literal.

Looks good

• tx_accept_log / TxAcceptLog split and the INFO→debug park log.
• try_contains / try_contains_wtxid including the orphanage; checked the other callers (announce gate, rebroadcast_unbroadcast) and none can see an orphan txid there.
• Known-parent OOB vout → MissingPrevout and the Q-58 doc update.
• Compact fill from orphanage + extra_compact, and note_extra on RBF replacement (matches Core vExtraTxnForCompact).
• Orphanage::by_wtxid kept in sync on all three removal paths.

[rearden-grok]

Re-review of 655dcfcc (fixup on top of the 13 reviewed commits). Verdict: one more round — the two blockers from round 1 are fixed properly, but the fix introduced a user-visible RPC regression and left some dead/misleading pieces.

Round-1 items — resolved

• Hub 1p1c: accept_with_utxo no longer parks before trying 1p1c; admit_1p1c admits the parent at min_relay: Some(0), then the child at the real floor, and rolls the parent back if the child fails. hub_one_parent_one_child_admits drives MempoolHub::accept_tx. ✓
• skip_min_relay: gone; floor is an AdmitSpec / prepare_admit parameter. Sibling orphans are promoted at the real floor (one_parent_one_child_does_not_skip_minrelay_for_siblings). ✓
• Chain-aware missing set: Orphaned { missing } carries prepare_admit's set into park_orphan / GETDATA / try_one_parent_package; confirmed extra input covered. ✓
• recent_invalid no longer records tip-dependent variants; Script keyed by txid only for witness-less txs. ✓
• Durable("parent body missing"), actual missing outpoint in MissingPrevout, (Txid, Transaction) in extra-compact, write lock gated by records_accept_failure, take_parent_getdata_at TTL/cap test. ✓

New in this round

1. testmempoolaccept regression (behavior change, untested). prepare_admit now ignores park_orphans (let _ = park_orphans;) and returns Orphaned { .. } unconditionally on missing inputs. MempoolHub::test_accept (park_orphans: false) therefore returns Orphaned instead of MissingPrevout, and accept_reject_reason has no arm for "orphaned <txid>", so the RPC reject-reason falls through to the raw string orphaned <txid> where master returned bad-txns-inputs-missingorspent. Same for the sendrawtransaction-style error path via accept_reject_reason. Either keep park_orphans=false → MissingPrevout in prepare_admit (and have the hub's 1p1c path pass true), or map Orphaned in accept_reject_reason and pin it with an RPC test. Whichever you pick, the existing prepare_admit_without_park_does_not_orphan / hub dry-run tests currently accept either variant, so they did not catch this — tighten one of them.

2. let _ = park_orphans; is dead code. If the parameter is genuinely unused after (1), remove it from prepare_admit (the hub still needs AdmitSpec.park_orphans for admit_staged, that's fine). AGENTS: do not silence unused warnings — delete the code.

3. recent_invalid.clear() on tip connect is on a path the node never calls. It lives in ActiveMempool::remove_for_block_with_utxo, but MempoolHub::remove_for_block / evict_live_txids go through remove_live_txids + erase_orphans_for_block directly. With NotFinal/NonBip68Final/ImmatureCoinbase no longer recorded, what remains (InputsDuplicate, Coinbase, witness-less Script) is permanently invalid, so a tip-change clear is arguably unnecessary — but then drop the line and the review-reply claim rather than leave a clear that only fires in crate tests. If you want the Core semantics, put it in remove_live_txids.

4. publish_admitted does not unindex replaced txs. accept_with_utxo and accept_package both do for old in &r.replaced { self.unindex_txid(old) }; publish_admitted skips it, so a child that RBF-replaces a live tx during 1p1c leaves the replaced txid in sh_index / relay maps / unbroadcast. Add the loop (or route both call sites through one shared post-commit helper — accept_with_utxo's success arm is now a near-duplicate of publish_admitted).

Minor

• Hub 1p1c: between the parent's commit_after_script (write lock #1) and the child's commit (write lock #2) the parent is live in the graph while the child's scripts verify off-lock. A concurrent accept_tx of another spender of the parent that lands in that window, followed by a child failure, hits remove_txid(parent) with a live descendant. Narrow, and accept_package's rollback has the same shape but under one lock. Worth a comment naming the window, or commit both under one write lock and accept the on-lock script cost as a named trade.
• Hub admit_1p1c passes fee_delta: 0 for the parent (ignores prioritisetransaction); the crate path does the same. Nit, but self.fee_delta(&parent_txid) is one line.
• If the parent fails its own admit_staged inside admit_1p1c (e.g. invalid scripts), the failure is discarded (.ok()?) and not recorded, so every re-INV of the child re-verifies the parent. Bounded by extra_compact, but records_accept_failure + note_accept_failure on that error would close it.
• CodeQL alert 199 is still the test-only nonce — operator dismiss.

Coverage job was still pending when I looked; everything else green.

[rearden-grok]

Re-review of fd7324f3 against the PR's eight goals, with a pass for tightness.

Verdict: goals met; one cleanup round before merge. Nothing below is a behavior bug in the P2P path — the earlier blockers stay fixed and the round-2 items are all resolved (testmempoolaccept pinned to bad-txns-inputs-missingorspent including for an already-parked tx, let _ = park_orphans gone, dead recent_invalid.clear() gone, publish_admitted unindexes replaced and is now the single post-commit path for accept_with_utxo, parent fee_delta honored, parent-stage failures recorded, race window named). What is left is duplicated machinery and one silent semantic drift.

Goals

#	Goal	State
1	Park is debug, not INFO reject	✓ tx_accept_log
2	INV already-have includes orphanage	✓ try_contains{,_wtxid}
3	Known-parent OOB vout → MissingPrevout	✓
4	GETDATA unique missing parents (TTL, cap)	✓ exact chain-aware set, tested
5	Compact fill from orphanage + extra	✓
6	Recent-invalid parents do not park	✓ permanent variants only, txid only for witness-less
7	1p1c on the hub	✓ hub_one_parent_one_child_admits
8	getmempoolinfo.orphanage	✓

Tightness (please fix)

1. Two 1p1c implementations. ActiveMempool::admit_1p1c_pair (crate) and MempoolHub::admit_1p1c (hub) are parallel implementations of the same sequence. The hub never calls ActiveMempool::accept_tx; the only callers of the crate-level accept_tx are crate tests and crate-level reorg_reaccept (which the hub also does not use). So admit_1p1c_pair is production code whose only consumer is its own tests — the dual path AGENTS says not to add. Keep try_one_parent_package / package_pays_min_relay in the crate (that is the shared policy), delete admit_1p1c_pair from ActiveMempool::accept_tx, and re-target the three crate tests (one_parent_one_child_*) at try_one_parent_package directly (sibling → None for the 1-sat child; confirmed extra input → Some(parent)). Admission-sequence coverage already lives in the hub test.

2. AdmitSpec.park_orphans no longer parks, and accept_package silently changed. Removing the park arm from admit_staged turned park_orphans into "return Orphaned instead of MissingPrevout"; parking is now the caller's job (accept_with_utxo, staged_reorg_admit). MempoolHub::accept_package still passes park_orphans: true but no longer parks an orphan member (master did, via the removed arm). Not obviously wrong — Core's package accept does not park either — but it is an unflagged behavior change on the Esplora POST /txs/package path with no test. Pick one: (a) rename the field to what it now means (e.g. orphan_as_error / report_orphans) and pin accept_package's no-park in a test, or (b) park explicitly in accept_package if that was the intended behavior. Either way the name and the behavior should agree.

3. accept_tx_inner is accept_tx_with(tx, utxos, tip, 0, true, None) minus note_accept_failure. Fold it; promote_orphans_of and crate accept_package call the one function.

4. records_accept_failure / note_accept_failure restate the same match. Two match arms lists to keep in sync, and the hub repeats the if records { lock_write; note } block twice. One shape: fn accept_failure_record(tx, e) -> Option<FailureRecord> (enum Invalid(Txid) | Extra), note_accept_failure applies it, hub does if let Some(rec) = ... { lock_write().apply(rec) } from one helper.

5. orphan_missing_parents is redundant. The Orphaned { missing } error in on_tx's Park arm already carries the exact set; queue_orphan_parent_getdata re-takes a read lock to fetch it again. Pass missing from the error and delete the hub API.

Minor

• admit_1p1c: the two rollback arms are identical (remove_txid + unindex_txid, the latter on a parent that was never indexed) — one closure/helper; stages / lock_us are collected but never metered, so the 1p1c admit is invisible to meter_accept_stages; expire_stale runs twice per 1p1c (once per publish_admitted).
• prepare_admit missing-inputs block scans tx.input twice with near-identical predicates and an unreachable unwrap_or(tx.input[0]...); one first_missing_outpoint(&missing_parents) helper covers both the recent_invalid and the non-park return.
• park_orphan already-parked arm: missing_of(..).cloned().unwrap_or(missing) — the unwrap_or is unreachable after contains.
• package_pays_min_relay recomputes parent.compute_txid() per child input; hoist. It also requires every parent input to be confirmed (unconfirmed-grandparent 1p1c is out) — fine, but say so in the doc comment since Core's 1p1c has the same limit.
• Debug txrelay: park takes a read lock for orphan_count() on every park; fine at these rates, just noting it is a lock in the log line.
• CodeQL 199 remains the test-only nonce — operator dismiss.

All required checks green except coverage, still running when I posted.

[rearden-grok]

Round 4, b5b55001 (refactor(mempool): one 1p1c path; report_orphans; shared failure record).

Verdict: approve once required checks are green. Posted before test / windows / macos / coverage finished at the operator's request; fmt, clippy, deny, ast-grep were already green. Nothing below is blocking.

Round-3 items — resolved

• One 1p1c path. admit_1p1c_pair deleted; ActiveMempool::accept_tx just parks on Orphaned. try_one_parent_package / package_pays_min_relay stay in the crate as the shared policy; the three crate tests now drive try_one_parent_package directly; admission sequence is covered by hub_one_parent_one_child_admits. ✓
• report_orphans replaces park_orphans in AdmitSpec, prepare_admit, accept_tx_with; doc on prepare_admit says what it does; accept_package_does_not_park_orphan_member pins the no-park behavior. ✓
• accept_tx_inner folded into accept_tx_with(.., 0, true, None). ✓
• AcceptFailureRecord — one arm list (accept_failure_record), one applier (apply_accept_failure), hub note_if_accept_failure used at both sites. ✓
• orphan_missing_parents removed; on_tx passes the error's missing set. ✓
• rollback_1p1c_parent (no stray unindex_txid), 1p1c stages metered, expire_stale once per accept at the two callers, first_missing_outpoint replaces the doubled scan, park_orphan unwrap_or gone, pid hoisted, parent-inputs-confirmed limit documented. ✓

Non-blocking nits

1. Sibling test lost its original contract. one_parent_one_child_does_not_skip_minrelay_for_siblings now asserts "a 1-sat sibling is not itself a 1p1c package" — that is package-feerate policy, not the min-relay-leak regression it was written for. The real contract (a parked below-min-relay sibling stays out when the parent is admitted via 1p1c, because promote_orphans_staged runs at the real floor) is now only true by construction and has no test on the hub. Cheapest fix: in hub_one_parent_one_child_admits, park a 1-sat sibling before the paying child, then assert the sibling is not live afterwards; and rename the crate test to what it asserts.
2. take_parent_getdata(&[Txid]) forces missing.iter().copied().collect::<Vec<_>>() at the one call site; take &BTreeSet<Txid> (or impl IntoIterator<Item = Txid>) and drop the alloc.
3. on_tx matches e twice: tx_accept_log classifies to Park, then the arm re-matches Orphaned { missing }. TxAcceptLog::Park(&BTreeSet<Txid>) would carry it in one step.
4. On the 1p1c success return in accept_with_utxo, the child's first (orphaned) prep stages are no longer metered — admit_1p1c meters its own, so only the initial UTXO lookups are lost. Fine to leave.
5. first_missing_outpoint(..).expect("missing_parents is built from tx.input") — the invariant is local and trivially true; just noting it is a new expect in the accept path.
6. CodeQL 199 (test-only BIP152 nonce) — still awaiting operator dismissal.

Good work getting this to one path each for park, 1p1c, and failure recording.