Skip to content

Limit how many nodes a refit proof can carry - #929

Merged
mafintosh merged 4 commits into
mainfrom
fix/refit-node-cap
Nov 4, 2026
Merged

Limit how many nodes a refit proof can carry#929
mafintosh merged 4 commits into
mainfrom
fix/refit-node-cap

Conversation

@marcus-pousette-hp

@marcus-pousette-hp marcus-pousette-hp commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

Co-authored with AI

@marcus-pousette-hp
marcus-pousette-hp requested a review from a team November 4, 2026 14:30
@greptile-apps

greptile-apps Bot commented Nov 4, 2026 •
edited
Loading

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge.

Summary

The PR limits each node list in a refit proof to 128 entries and adds a rejection test.

  • Proof refitting rejects node lists longer than 128.

Reviews (3) · Last reviewed commit: "Skip the upgrade node check when there i..." · Reviewed by Greptile

mafintosh
mafintosh reviewed Nov 4, 2026
View reviewed changes
Comment thread lib/merkle-tree.js
function hasTooManyNodes(proof) {
if (proof.block && proof.block.nodes.length > MAX_NODES) return true
if (proof.hash && proof.hash.nodes.length > MAX_NODES) return true
return proof.upgrade.nodes.length > MAX_NODES || proof.upgrade.additionalNodes.length > MAX_NODES

@mafintosh mafintosh Nov 4, 2026

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.

Dont you need to guard this with if (upgrade)?

@marcus-pousette-hp marcus-pousette-hp Nov 4, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good point, added!

@mafintosh
mafintosh merged commit 8d0f719 into main Nov 4, 2026
6 checks passed
@mafintosh
mafintosh deleted the fix/refit-node-cap branch November 4, 2026 17:56
@andrewosh

andrewosh commented Nov 4, 2026

Copy link
Copy Markdown
Contributor

@greptileai explain why you missed #929 (comment)

@greptile-apps

greptile-apps Bot commented Nov 4, 2026

Copy link
Copy Markdown

I missed it because I focused on whether the new node-count limit bounded the proof, and didn’t trace the optional proof.upgrade case through hasTooManyNodes. In the original version, the helper could reach proof.upgrade.nodes when proof.upgrade was absent, causing a TypeError instead of handling that proof safely. The maintainer caught it, and the follow-up added the if (!proof.upgrade) return false guard.

So my “safe to merge” summary was too confident: I caught the intended limit and its test, but missed this missing-input-shape check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mafintosh mafintosh mafintosh left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@marcus-pousette-hp @andrewosh @mafintosh