Skip to content

Conversation

@caparrosm
Copy link

No description provided.

@caparrosm caparrosm requested a review from a team as a code owner November 18, 2025 19:28
midnight-transient-crypto_v6 = { git = "https://github.com/midnightntwrk/midnight-ledger", package = "midnight-transient-crypto", tag = "ledger-6.1.0-alpha.5" }
midnight-zswap_v6 = { git = "https://github.com/midnightntwrk/midnight-ledger", package = "midnight-zswap", tag = "ledger-6.1.0-alpha.5" }
nix = { version = "0.30" }
once_cell = { version = "1.19" }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is once_cell really required? I thought most of the functionality was merged into std now.
(E.g. rust-lang/rust#105587 )

{"timestamp":"2025-11-17T23:05:51.417863+00:00","level":"ERROR","target":"spo_indexer","file":"spo-indexer/src/main.rs","line":31,"message":"process exited with ERROR","kvs":{"backtrace":"disabled backtrace","error":"cannot make rpc call: sidechain_getEpochCommittee"}}
```

**Root Cause**: The `sidechain_getEpochCommittee` RPC method does not exist in the Midnight Preview network API (likely removed or renamed between alpha.2 and alpha.5).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sidechain_getEpochCommittee does still exist. https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.preview.midnight.network#/rpc rpc / methods() call returns it in the list.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possibly not shown on the polkadot.js UI due to a bug in that but it's there if you call it.

@hseeberger hseeberger changed the title integrate spo indexer feat: integrate spo indexer Nov 19, 2025
@@ -0,0 +1,84 @@
CREATE TABLE epochs (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a single sql file called "002_spo.sql"; no further history yet.

Copy link
Contributor

@cosmir17 cosmir17 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @caparrosm, thanks for the good work on the SPO integration! The full integration approach with path dependencies is excellent, and I can see you've followed several midnight-indexer patterns correctly.

I can't approve this PR as I defer to Giles and Heiko for decisions, but I wanted to provide these review comments to help get this ready for them. The core integration work seems solid.

genesis_protocol_version: 16000
reconnect_max_delay: "10s" # 10ms, 100ms, 1s, 10s
reconnect_max_attempts: 30 # Roughly 5m
blockfrost_id: "previewukkFxumNW31cXmsBtKI1JTnbxvcVCbCj"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this sensitive information?
if so, please let us know 🙏

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sensitive information as anybody could reuse and spend your blockfrost tokens on your behalf. If you are using a free plan it would prevent you from executing more calls if a malicious party reuses this and reaches your threshold. On other plans, this could have a financial impact.
Secrets should be managed at deployment.

@@ -0,0 +1,163 @@
# SPO API
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not about this readme.md file.

midnight-indexer/justfile's generate-indexer-api-schema command can also generate a graphql schema file for your component and we can include the generated file in this pr as well?

@@ -0,0 +1,49 @@
// This file is part of midnight-indexer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not about this file.

automated tests are needed.

Unit tests :

  • Unit tests following midnight-indexer conventions (inline #[cfg(test)] modules)
  • test functions across:
    • Helper functions (rpc.rs)
    • Config deserialization (config.rs)
      ...

Integration tests (can be follow-up PR):

  • main branch example, indexer-tests native_e2e.rs, e2e.rs

@@ -0,0 +1,88 @@
// This file is part of midnight-indexer.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems, sigterm codes are missing in spo-indexer codes (spo-indexer/src/main.rs and spo-indexer/src/application.rs).

they are needed in k8s envs.

telemetry:
tracing:
enabled: false
service_name: "chain-indexer"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this seems wrong.
you want service_name: "spo-indexer" instead?

loop {
ticker.tick().await;
if let Err(e) = refresh_stake_snapshots(&client_bg, &storage_bg, &st_cfg).await {
eprintln!("stake refresh failed: {e:?}");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see :

println!("latest epoch reached");
println!("processing epoch {}", epoch.epoch_no);
println!("\tcommittee size: {}", committee.len());
eprintln!("stake refresh failed: {e:?}");
eprintln!("stake refresh for {pid} failed: {err:?}");

the main branch's production code uses log::{debug, error, info, warn}; or context(...) instead..?

continue;
}

let epoch = cur_epoch.unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see :

let epoch = cur_epoch.unwrap();  // <- Could panic if None
let raw_spo = val_to_registration.get(&spo_sk).unwrap();  // <- Could panic if missing
produced_blocks: *produced.unwrap() as u64,  // <- Could panic if None
+ if leftover > index.try_into().unwrap() {  // <- Could panic on conversion
let mut hasher = Blake2bVar::new(28).unwrap();  // <- Could panic (unlikely)
hasher.finalize_variable(&mut buffer).unwrap();  // <- Could panic (unlikely)

indexer main branch code only uses unwrap for intended very few fatal error cases with comment. most of fatal cases, we use .expect(...)

recently, this has become famous : https://blog.cloudflare.com/18-november-2025-outage/#memory-preallocation

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

cosmir17 added a commit that referenced this pull request Jan 9, 2026
- Replace println!/eprintln! with log::{debug, error, info, warn}
- Replace .unwrap() with .expect() with meaningful messages
- Add SIGTERM handling to spo-indexer (graceful shutdown)
- Fix telemetry service_name: "chain-indexer" → "spo-indexer"
- Remove Blockfrost ID from config.yaml (use env var instead)
- Consolidate SPO SQL migrations into 003_spo.sql
- Remove unused imports in spo-indexer
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.

6 participants