Repository navigation
Land verified Lightning stack - #1700
Conversation
The contract cited a false P2P-05 commit authority, dead transitions_* proof paths, and closed trackers as owners, and it lacked the required ownership map. Readers followed stale evidence for SYNC-01 through SYNC-07. Point every clause at its live owner and smallest real proof, state the unproven seed_live_from_utxo hold and the missing process-level settlement scenario explicitly, and add the seven-row ownership table plus the settlement-closure row.
ApplyScratch repackaged facts apply_block_admitted already held and serialized raw transactions before the Propose return and coinbase verification, so the wire bytes existed on paths that never committed. Hold txids, the same-block spent set, and the two UTXO capacity locals directly, and serialize raw transactions only in the commit tail when capture_rawtx asks for them. A new regression applies an external spend plus a same-block spend with the flag off and on and requires identical UTXO membership, txids, tip, and commit identity, with undo restoring the same state.
The first form of the test proved undo only through post-disconnect membership. Compare the persisted undo records byte for byte so a capture-flag-dependent durable record cannot pass silently.
ordered checked package structure and submit_package_with_times repeated the same check, so local submission paid two ordered passes over the same transactions. Snapshot restore calls only submit_package_with_times, so that site stays the single authoritative structural gate. No result is cached in another facts type.
block_by_hash only delegated to record_for_hash, two names for one lookup. block_hash_at_height repeated the in-view branches, next_applied_block_hash wrapped one checked successor computation that two other surfaces inlined (one with saturating_add, which made a u32::MAX record its own successor), and handlers repeated confirmation arithmetic the render owner already had. Fold the lookup into block_by_hash, route the height read through the in-view form, give every successor computation the same checked form, and delegate confirmation depth to render::confirmations. REST, Core RPC, and Esplora keep their distinct serializers and error precedence.
NodeState::blocks handed out the raw RwLock<BlockLog> and NodeState::shutdown duplicated the chainstate shutdown handle. Both had canonical owners already: BlockLogReader for reads and Chainstate::shutdown_handle for the atomic. Migrate every caller, preserving each load and store ordering, and keep the pointer-identity assertion on the test-only raw_handle. The generic composition visitors stay: they preserve concrete KvStore types across backend erasure and have live Fjall, redb, and RocksDB consumers.
Pending.fallback kept terminal reconstruction state alive after failure, and compact follow-ups published fetch ownership through two separate marks that the enqueue could race. A failed send could leave an orphan mark suppressing recovery from another peer. Absence of the pending entry now means terminal, releasing filled transactions and retained bytes at once. One enqueue_owned_body_fetch holds the exact-source peer-table guard across the scheduler lock, the nonblocking send, and ownership publication under the 16-entry deferred cap; on failure it publishes nothing. The header-carried ownership flag and its second publisher are deleted.
Keep replayable side headers in the authoritative checkpoint so restart can seed stale-tip relay from the restored block tree. Preserve the active tip and reject corrupt or heavier side sections. Write header artifact version 2 and retain the explicitly approved version 1 reader. Reuse matching restored nodes during journal replay. Checkpoint and journal tests pass on disposable stores; no operator data was opened, converted, or reset.
A peer's block notfound reply left the requested body assigned to that peer until expiry. Release only its exact owned request and let the existing scheduler select another source without a busy retry loop. Keep unavailable-body evidence bounded and expiring. Retain ownership while resolving deferred requests so stale snapshots cannot reinstall a declined or disconnected owner.
Use the captured applied ancestry for locator-based header replies. Keep header admission, best-header selection, and direct hash lookups unchanged. This explicitly approved policy delays early header relay. The same corrected process fixture fails Core convergence with the old serving policy and passes all 99 checks with this change. The 102-test correction union and strict lint also pass.
LND received blocks but could not observe unconfirmed incoming payments. Capture admitted transaction ownership within the commit writer only when rawtx is enabled, then publish exact bytes through the ordered observer before its admission sequence event. Retained payloads survive same-batch expiry without deep copies. Rejected and duplicate submissions publish no admission event.
LND sends the legacy allowhighfees argument when it broadcasts a wallet transaction. Core accepts that shape, but the amount-only parser rejected it before admission. Keep legacy sendrawtransaction semantics separate from strict testmempoolaccept amount parsing.
Reject rename path escape and open checkpoint sources nonblocking with regular-file validation. Preserve operator data and exercise the live storage helpers.
Use explicit error matches and remove an unused test import without changing checkpoint assertions.
Run the pinned unmodified bcli consumer against bitcoin-rs and Core with bounded ownership and scoped private loader paths. Record the observed version-gate refusal separately from the getblockfrompeer fallback gap. Keep downstream cells unexercised and preserve the distinct LND, overlap, and research dispositions. The frozen runner passed syntax, ShellCheck, and the real comparison; no backend version, wallet behavior, or RPC was changed.
Identify the script guide and route Lightning readers to the existing assessment and qualification documents without duplicating their claims.
Replace host-local paths (/home/alpha, /opt) and job IDs with portable placeholders in the invocation examples; keep run-2/bg_543 as historical evidence prose. Remove Feynman/Tufte Anchors line and grounding clauses. Verdicts and upstream blockers unchanged.
…add Core comparator path Matrix LND block/tx detection keeps passed but no longer presents unconfirmed-via-rawtx as clean-commit behavior: same-millisecond BTWL insert, dirty zmq.rs build order, and child commit 4aeb37e are cited, with missing topic capture and Core comparison stated as limits. Validation tooling reproduction gains explicit --bitcoind-bin path.
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary: Lightning Stack Documentation and ZMQ Probe Changes
This PR adds Lightning integration documentation (docs/lightning-integration.md, docs/lightning-interop-matrix.md), updates docs/validation-tooling.md, and delivers functional changes to the ZMQ notification path — specifically adding mempool-admission rawtx publishing via MempoolSequenceObserver.
Critical Finding
crates/rpc/src/zmq.rs — rawtx encode result ignored (lines 557-559): The new on_mutation rawtx path calls tx.consensus_encode(&mut bytes) and discards the Result. While encoding to Vec<u8> cannot produce an I/O error in practice, if consensus_encode returns an error for any reason, the partially-written bytes buffer is forwarded to publish_rawtx, transmitting corrupt/truncated transaction bytes to all ZMQ subscribers. This is a correctness defect in the new code path. I've left a code suggestion inline.
Lower-Priority Observations
-
crates/node/src/chain_effects.rs—block_log()narrowed fromany(test, feature = "test-seam")to#[cfg(test)]: This is safe in this PR because notest-seamconsumer callsblock_log()directly — benchmarks and integration tests use the always-publicblock_log_reader()instead. Confirmed via thesync_pipeline.rsbenchmark. No action needed unless futuretest-seamcallers are added. -
crates/rpc/src/zmq.rs—admitted_unconfirmed_rawtx_is_exact_and_not_repeatedtest counter assertion: Theraw[2] == 0_u32.to_le_bytes()assertion (line 1116) implicitly relies on theRawTxcounter being zero at point of the first admission, which is correct because the warm-up loop publishes on theSequencetopic (counter index 4), notRawTx(counter index 3). A comment would prevent future fragility. I've left a note inline.
Documentation
The Lightning documentation is well-structured and accurately records upstream blockers with precise code references. The interoperability matrix correctly marks LND cells as passed with clearly stated provenance limits (dirty source tree, .outline runner, no fresh Core comparison), and correctly classifies CLN's version-gate failure. No inaccuracies found in the documented evidence or status vocabulary.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Publish the accepted transaction through the consensus-bytes helper and keep the per-topic counter explanation. The remote branch already hardens the interop source check.
Merge origin/main (46de452, #1684 Tor/I2P overlay + #1676/#1677 scans) into the Lightning lane. Listener (crates/p2p/src/listener.rs, aggregate_budget_tests.rs): main's #1685 addressing wins — PeerRemote/PeerEndpoint throughout, no ListenerRole or rpc-public-bind restored. The compact-outcome path keeps the branch's enqueue-owned-body-fetch design (Option return, single linearizable enqueue site) because the merged tree carries the branch's 3-arg send_headers/InboundHeaders and enqueue-only BlockSync API; main's bool/record variant has no matching API left post-merge. Tests updated to PeerEndpoint with the is_none assertion. All other auto-merged regions keep both intents: branch Lightning/ZMQ/admission work plus main's overlay, scan-bound, and RPC compat changes.
Merge origin/main (fa187f9: #1676 ranged/multipath descriptor scans, #1677 compact scan bound) into the Lightning lane. Auto-merged with no conflicts. Keeps both intents: main's #1677 COMPACT_RECONSTRUCTION_MAX_AHEAD scan bound in BlockSync alongside the branch-owned enqueue-owned-body-fetch protocol; main's PeerRemote/PeerEndpoint addressing untouched. Branch Lightning/ZMQ/admission work plus main's scantxoutset RPC surface (handlers/chain, handlers/util, registry, e2e) combined.
Main compact-sync tests called record_owned_body_fetch, which the branch replaced with enqueue_owned_body_fetch. The NotFoundFixture fields also kept SocketAddr after current_source and test_addr moved to PeerRemote. - Add the missing SocketAddr import. - Store owner and alternate as PeerRemote. - Mark the owned fetch through enqueue_owned_body_fetch.
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Draft subsystem stack split from PR #1687.
Base: f024fc3
Tip: 4e1f8e1
Full repository gate: native-gate-20261027T232832Z-1404447 (scripts/ci-pr.sh all, exit 0).
Scope: Lightning docs and probe.
This is a verified stack tip. Do not rebase or reparent it
without owner approval.
No better-than-Core claim. No mainnet qualification.
See also: #1687
The encoding change appears safe, but the repository's required synchronization-impact explanation must be supplied before merging.
Findings
Summary
This stack adds Lightning integration docs and an external qualification runner, alongside changes to checkpoint recovery, block requests, RPC reads, and transaction notifications.
Reviews (3) · Last reviewed commit: "Use consensus bytes for ZMQ rawtx public..." · Reviewed by Greptile
Sync impact: YES.
block_not_foundchanges how the node releases and retries block requests. Affected clauses: SYNC-04 (recovery closure) and SYNC-06 (work drivers). Smallest proof:crates/p2p/src/sync/tests.rsnotfound-only and historical-notfound regressions.