Skip to content

Add get_seq_commit_lane_proof RPC (gRPC + wRPC)#961

Open
biryukovmaxim wants to merge 9 commits into
kaspanet:toccatafrom
biryukovmaxim:covpp-reset2-rpc-lane-proof
Open

Add get_seq_commit_lane_proof RPC (gRPC + wRPC)#961
biryukovmaxim wants to merge 9 commits into
kaspanet:toccatafrom
biryukovmaxim:covpp-reset2-rpc-lane-proof

Conversation

@biryukovmaxim
Copy link
Copy Markdown
Collaborator

@biryukovmaxim biryukovmaxim commented Apr 22, 2026

Adds get_seq_commit_lane_proof(block_hash, lane_key) — returns the witness a client needs to verify a single KIP-21 active-lane against the seq_commit in the block header, without running a full node.

Response

  • smt_proofOwnedSmtProof wire bytes (inclusion or non-inclusion).
  • lane_tip, lane_blue_scoreSome iff the lane has an entry at this POV; None signals a non-inclusion proof.
  • payload_and_ctx_digest, parent_seq_commit — the remaining inputs to verify_smt_metadata so a client can chain lanes_root → seq_commit.

Client verification pattern: OwnedSmtProof::from_bytescompute_root::<SeqCommitActiveNode>verify_smt_metadata against header.accepted_id_merkle_root.

Changes

  • consensus-core: new ConsensusApi::get_seq_commit_lane_proof + SeqCommitLaneProof return type.
  • consensus: implementation on Consensus (ports the pattern from TestConsensus::seq_commit_lane_proof — pruning lock, canonicality check, SmtStores::prove_lane/get_lane, SmtMetadataStore::get). debug_assert!s that the returned proof re-verifies against the header.
  • Session wrapper async_get_seq_commit_lane_proof.
  • rpc-core: request/response types, serde round-trip tests.
  • gRPC: proto (hashes as bytes, empty laneTip = absent), conversions, ops, server factory, rpc-core mock.
  • wRPC: router + client wiring.
  • Service handler in rpc-service.
  • Integration test seq_commit_lane_proof_test (gRPC + wRPC): verifies an absence proof at a fresh block, asserts cross-transport equality, tests unknown-block error, and covers an existing lane by mining a second block and querying the coinbase-subnetwork lane (populated via the selected parent's coinbase acceptance).

@demisrael
Copy link
Copy Markdown
Contributor

Thanks -- the substance is solid: the consensus impl faithfully ports TestConsensus::seq_commit_lane_proof (test_consensus.rs:222-265) and adds the canonicality + depth checks the production path needs; the integration test verifies the proof end-to-end via OwnedSmtProof::from_bytes -> compute_root::<SeqCommitActiveNode> -> verify_smt_metadata, asserts cross-transport equality (gRPC vs. wRPC), and covers a populated lane through the coinbase-subnetwork lane after a second mined block. cargo fmt --check and cargo clippy --no-deps pass on the changed crates.

Two independent peer reviews ran (upstream review + architect peer review on the design surfaces); the issues consolidate into two HIGH concerns to fix before merge, three MEDIUMs, five LOW polish items, and one process note.

HIGH (must fix before merge)

H1 -- genesis-block panic via header.direct_parents()[0] (RPC-exposed -> DoS). consensus/src/consensus/mod.rs:1509 indexes an empty slice for genesis (header.rs:191-196 returns &[]). Genesis satisfies the function's documented contract -- it's on the selected-parent chain trivially (is_chain_ancestor_of(genesis, genesis) == true), and at startup it's also the pruning point so the depth check passes -- so an unauthenticated RPC client can crash the consensus thread with get_seq_commit_lane_proof(genesis_hash, _). RPC-exposed via service.rs:425-446.

Fix: check direct_parents().is_empty() before the index; return a specific error variant. The right variant is NOT BlockNotInSelectedChain (genesis IS on the chain) -- prefer a new GenesisHasNoSelectedParent(Hash) variant, or reuse MissingData(Hash) if it semantically fits. Add a negative test against genesis_hash so a future refactor of the canonicality check doesn't regress the guard (the current seq_commit_lane_proof_test mines a block before the proof call at rpc_tests.rs:848-857, so it never hits the genesis path).

H2 -- op number 152 collides with PR #991 (cross-PR coordination). ops.rs:144 on this PR assigns GetSeqCommitLaneProof = 152; ops.rs:146 on PR #991 assigns GetUtxosByAddressesV2 = 152. Both PRs are open. The op number is a wire-protocol commitment -- whichever lands first claims the slot, and the second must rebase to 153. Pre-merge action: lead decides merge order and the loser rebases. Worth amending ops.rs with a "RESERVED" comment line for any future in-flight op assignment so reviewers on the next PR catch this kind of collision at diff-review time.

MEDIUM

M1 -- unwrap() on store reads in the production path. consensus/src/consensus/mod.rs:1503, 1508, 1510 carries three .unwrap()s -- one on pruning_point_store.read().pruning_point() and two on headers_store.get_header(...). Most are post-validation invariants, but H1 (genesis) is a concrete case where "logically unreachable" assumptions are not always met. .expect("…why this can't fail") or proper error propagation is cheap and audit-friendly; the function signature is ConsensusResult<…> so ? propagation is natural.

M2 -- new RpcError variants don't survive the gRPC wire; gRPC clients always see General(string). rpc/core/src/error.rs:153-157 adds BlockNotInSelectedChain and BlockTooDeep, but RPCError over gRPC is just string message = 1; (rpc.proto:15-17). Encoding squashes the variant into a string (kaspad.rs:248-253); gRPC clients see RpcError::General(string). The integration test acknowledges this with a General(_) fallback (rpc_tests.rs:900-908).

Recommended resolution: keep the dedicated variants (they're useful for in-process / wRPC consumers and the service handler at service.rs:431-441 demuxes precisely so they survive the consensus->RPC boundary as identifiable types), but add a one-line doc-comment per new variant naming the wire visibility ("note: collapses to RpcError::General over the gRPC transport; preserved over wRPC and in-process"). Queue a separate cross-cutting follow-up to add int32 code = 2; to RPCError in the proto -- that change benefits every variant past, present, and future, and is too big for this PR.

M3 -- lane_tip / lane_blue_score paired-field invariant is enforced only by convention. Two related issues with the same fix:

(a) Wire schema: rpc.proto:1126-1140 -- bytes laneTip = 2; + uint64 laneBlueScore = 3; are non-optional. Encode side (message.rs:581-590) sets laneBlueScore = 0 when laneTip is empty; decode side (message.rs:1111-1120) discards the wire laneBlueScore when laneTip is empty. The "empty bytes = absent" convention is a comment-level invariant; non-Rust gRPC clients reading the schema have no way to know.

(b) Type discipline: consensus/core/src/api/mod.rs:90-95 and rpc/core/src/model/message.rs:3637-3643 carry lane_tip: Option<Hash> and lane_blue_score: Option<u64> as two separate Options. The doc-comment names the both-Some-or-both-None invariant; the type system doesn't enforce it. Today the impl honours it via consensus/src/consensus/mod.rs:1525 (.map(|v| (Some(*v.data()), Some(v.blue_score()))).unwrap_or((None, None))), but a downstream consumer or future internal caller can construct an invalid (Some, None) pair and the compiler accepts it.

One coherent fix addresses both: collapse to Option<LaneTip { hash, blue_score }> in consensus-core and rpc-core, AND promote the proto fields to proto3 optional. Result: invariant is enforced at three layers (consensus-core type -> rpc-core type -> proto schema). The conversion site between Rust and proto becomes the one place where the pair is unpacked, with the absence check explicit.

LOW (polish; not blockers)

L1 -- debug_assert! import smell. consensus/src/consensus/mod.rs:1533-1551 -- the let _ = BlueWorkType::default(); line silences an unused-import warning but reads as cargo-cult. Either feature-gate the import (#[cfg(...)] use …) or use #[allow(unused_imports)] on the use directly -- both name the rule.

L2 -- BlockTooDeep error path not exercised by any test. A unit test on the consensus impl with a mocked pruning_point that's chain-after the queried block would be cheaper than orchestrating a real prune. Currently the function has three error paths (BlockNotInSelectedChain, BlockTooDeep, MissingData-class from validate_block_exists) and only the third is tested.

L3 -- negative-test matches! arm carries unreachable patterns. testing/integration/src/rpc_tests.rs:900-908 -- per M2, the BlockNotInSelectedChain(_) arm is unreachable over gRPC, AND validate_block_exists returns HeaderNotFound (not BlockNotFound) for unknown hashes, so even if the variant survived the wire the first arm would not match. The test passes only via General(_). Tighten to RpcError::General(_) over gRPC (acknowledging the wire reality) or split the assertion per-transport.

L4 -- hash_from_bytes helper duplicates an existing pattern. rpc/grpc/core/src/convert/message.rs:1122-1126 -- the <[u8; 32]>::try_from(...) Hash-conversion pattern shows up elsewhere in the convert module. Worth either reusing an existing helper or, if none fits, lifting this one to a shared location. Also: the helper is not unit-tested for 31-byte / 33-byte / exact-32-byte inputs.

L5 -- RPC_API_REVISION not bumped. ops.rs:15 stays at 0 despite this PR adding op #152 (per H2, after rebase). PR #991 bumps revision 0 -> 1 for the same kind of additive change, creating policy drift. The comment at ops.rs:13-15 ("backwards-compatible changes") suggests the bump SHOULD happen on additive ops; bumping in this PR resolves the drift. The bump itself is informational (the only consumer interpolates it into an error-message string after a separate version-gate has already failed) so this is policy-alignment, not a runtime correctness item.

Process / surface notes

P1 -- PR description under-represents the consensus-core impact. The body says "consensus-core: new ConsensusApi::get_seq_commit_lane_proof + SeqCommitLaneProof return type". It does not mention:

Suggest extending the PR description with an "API surface extension" subsection so reviewers don't miss them and downstream consumers know what to update on bump.

Architect-level positions on design topics

(Recorded for the author's reference; none are blockers beyond the items already covered above.)

  • Wire-schema for optional hashes -- tighten now to proto3 optional; sets the precedent for every future "absent hash" RPC. Cost zero, downside zero. (Covered by M3.)
  • RpcError variant survival over gRPC -- keep variants in this PR + add wire-visibility doc-comment; queue the RPCError schema-tightening (int32 code = 2;) as a separate cross-cutting follow-up. (Covered by M2.)
  • RPC_API_REVISION bump -- bump 0 -> 1 in this PR. (Covered by L5.)
  • ConsensusApi boundary -- keep get_seq_commit_lane_proof at the ConsensusApi trait level. The method depends on consensus-private state (smt_stores, virtual_processor.is_smt_canonical, reachability_service, headers_store, pruning_point_store, pruning_lock); splitting into a SeqCommitWitnessApi trait would force every implementor to compose two traits. The unimplemented!() default for unoverriding impls is a known wart of this trait, not specific to this PR.
  • Dedicated error variants vs. GeneralOwned/General -- dedicated variants are correct here. The service handler at service.rs:431-441 demuxes consensus -> rpc-core errors precisely because the variants need to survive that boundary as identifiable types. The wire-survival concern is separate and should not drive the variant-vs-string decision.

Static evidence on HEAD

cwd: kaspanet/rusty-kaspa @ e39d4cf4bccf0edb9afe67837001aef64a83b5d4
cargo fmt --check -p kaspa-consensus -p kaspa-consensus-core -p kaspa-rpc-core -p kaspa-rpc-service -p kaspa-grpc-core -p kaspa-consensusmanager -p kaspa-testing-integration  -- clean
cargo clippy -p kaspa-consensus -p kaspa-rpc-core -p kaspa-rpc-service -p kaspa-grpc-core -p kaspa-consensusmanager --no-deps  -- clean

The integration test (seq_commit_lane_proof_test) was not executed locally as part of this review (#[tokio::test] requires a running simnet daemon and is gated by network/FD budget); test logic was verified by code-read end-to-end (cross-transport equality, both negative + inclusion paths). The PR author reports it passes via cargo test --release --package kaspa-testing-integration --lib -- rpc_tests::seq_commit_lane_proof_test.

The op-collision in H2 was verified by checking PR #991's HEAD (7372a8156b73f11eca05435370ca4c9fb12fa266) directly: rpc/core/src/api/ops.rs:146 on PR #991 = GetUtxosByAddressesV2 = 152, identical numeric assignment.

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.

2 participants