Skip to content

Remove useless filtring in events&actions sql - #134

Open
glyh wants to merge 4 commits into
mainfrom
lyh/remove-useless-filtering-event-actions-sql
Open

Remove useless filtring in events&actions sql#134
glyh wants to merge 4 commits into
mainfrom
lyh/remove-useless-filtering-event-actions-sql

Conversation

@glyh

@glyh glyh commented Nov 3, 2025

Copy link
Copy Markdown
Member

This is confirmed by running this on a online archive db:

 select COUNT(*) from blocks where id = parent_id;
 count 
-------
     0
(1 row)

pending_chain
WHERE 1=1
${
// If fromAsNum is not undefined, then we have also set toAsNum and can safely query the range

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I think these are auto format.

Comment thread .gitignore

# docker-compose postgres volumes
db/
/db/

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason for this diff?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

src/db will be ignored without this

FROM
blocks b
INNER JOIN pending_chain ON b.id = pending_chain.parent_id
AND pending_chain.id <> pending_chain.parent_id

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am not confident that this can never be true. I just don't know the Mina spec well enough. What is gained by removing this filter? Is the performance notably better?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No I don't think the perf would be much better. It's just this logic is very confusing here when I'm reading the code

@SanabriaRusso

Copy link
Copy Markdown
Collaborator

Thanks for digging into this and backing it with the COUNT(*) … WHERE id = parent_id = 0 check — the reasoning holds. I confirmed it against the integration fixture too: the genesis block (id=1, height=1) has parent_id = NULL, so the recursive walk already terminates on the NULL parent-join, and pending_chain.id <> pending_chain.parent_id can only ever matter for a self-parented block — of which there are none. So this is a genuine no-op on the returned events/actions rows. (No mina-explorer impact either — it doesn't use events/actions.)

Two small things before merge:

  1. Consistency: the identical guard still lives in getZkappsWithPendingEventsQuery in this same file (WHERE pc.id <> pc.parent_id AND pc.chain_status <> 'canonical'). If it's dead code in fullChainCTE, it's dead in both — worth removing it there too (or keeping both) so the file doesn't contradict itself on whether the guard is needed.

  2. This guard is effectively an infinite-recursion safety net — a self-parented non-canonical row would recurse forever — and it costs basically nothing to keep. Since we're now relying on the "no block has id = parent_id" invariant, could you drop a one-line SQL comment noting it? That way a future schema/hard-fork change that ever self-references a block surfaces in review instead of as a query hang.

Also, the branch is ~6 weeks behind main and shows BLOCKEDqueries.ts hasn't changed on main since your last sync, so it should rebase cleanly and just needs a refresh to re-run CI.

…nvariant

The reviewer flagged that this PR dropped the id <> parent_id guard from
fullChainCTE but left the identical one in getZkappsWithPendingEventsQuery,
so the file contradicted itself on whether the guard was needed. Removed
it there too, and documented the invariant both places rely on: no block
is its own parent (parent_id is a FK to blocks.id; a self-reference would
be a cycle), and the recursive walk terminates on genesis's NULL
parent_id regardless — so the guard never excludes a row.

Verified against the checked-in integration fixture: 39 blocks, 0 with
id = parent_id, exactly 1 with parent_id IS NULL. The comment points a
future schema/hard-fork change that ever self-references a block at the
spot that would loop, so it surfaces in review rather than as a query
hang.

Integration suite (25 tests) green; the modified recursive query executes
cleanly against the fixture without looping.

Addresses review feedback on #134.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.

4 participants