Skip to content

Conversation

@mpskowron
Copy link
Contributor

@mpskowron mpskowron commented Jan 28, 2026

Overview

Currently, if a block gets overfilled, its fullness is reported as 0% instead of 100%. This happens because SyntheticCost::normalize() returns None when any dimension exceeds limits, and the code falls back to NormalizedCost::ZERO.

This incorrect reporting affects fee adjustment calculations - the algorithm would treat an overfilled block as empty, leading to lower fees when they should be higher due to congestion.

This PR clamps costs to limits before normalizing, ensuring overfilled blocks report maximum fullness.

⚠️ Toolkit Backwards Compatibility

When the toolkit replays historical blocks to build its local ledger state:

Blocks On-chain state Toolkit replay
Before upgrade Old logic (0% for overfull) New logic (100%)
After upgrade New logic (100%) New logic (100%)

If overfilled blocks exist in chain history, the toolkit's local state will diverge from on-chain for those blocks.

Questions:

  • Do overfilled blocks exist on any network?
  • If they do, should we add a version gate (use old logic before upgrade block)?
  • how was it resolved last time? (I see that it was change already on 5th of January)

🗹 TODO before merging

  • Ready
  • Resolve toolkit backwards compatibility approach

📌 Submission Checklist

  • Changes are backward-compatible (or flagged if breaking)
  • Pull request description explains why the change is needed
  • Self-reviewed the diff
  • I have included a change file, or skipped for this reason:
  • If the changes introduce a new feature, I have bumped the node minor version
  • Update documentation (if relevant)
  • Updated AGENTS.md if build commands, architecture, or workflows changed
  • No new todos introduced

🧪 Testing Evidence

Please describe any additional testing aside from CI:
Local load test sending over 20 shielded token mint transactions to fill the block (had to modify the code to allow block overfilling). The error message was displayed with block clamped correctly to the limits.

  • Additional tests are provided (if possible)

🔱 Fork Strategy

  • Node Runtime Update
  • Node Client Update
  • Other: See the Toolkit Backwards Compatibility point above
  • N/A

Links

Ticket: https://shielded.atlassian.net/browse/PM-20839

@mpskowron mpskowron self-assigned this Jan 28, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Jan 28, 2026

kics-logo

KICS version: v2.1.16

Category Results
CRITICAL CRITICAL 0
HIGH HIGH 0
MEDIUM MEDIUM 94
LOW LOW 12
INFO INFO 83
TRACE TRACE 0
TOTAL TOTAL 189
Metric Values
Files scanned placeholder 28
Files parsed placeholder 28
Files failed to scan placeholder 0
Total executed queries placeholder 73
Queries failed to execute placeholder 0
Execution time placeholder 6

@mpskowron mpskowron marked this pull request as ready for review January 28, 2026 17:11
@mpskowron mpskowron requested a review from a team as a code owner January 28, 2026 17:11
@mpskowron mpskowron requested a review from ozgb January 28, 2026 17:16
@ozgb ozgb enabled auto-merge January 29, 2026 09:11
@ozgb ozgb added this pull request to the merge queue Jan 29, 2026
@gilescope gilescope removed this pull request from the merge queue due to a manual request Jan 29, 2026
@gilescope gilescope added this pull request to the merge queue Jan 29, 2026
@gilescope gilescope removed this pull request from the merge queue due to a manual request Jan 29, 2026
Copy link
Contributor

@ozgb ozgb left a comment

Choose a reason for hiding this comment

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

Missed the concerns raised in the description r.e. the toolkit - we'll need to update it so that the logic matches

EDIT: I misunderstood - the issue was instead that chains with existing overfilled blocks would cause the toolkit to fail to sync. @mpskowron is checking this

@mpskowron
Copy link
Contributor Author

mpskowron commented Jan 29, 2026

Missed the concerns raised in the description r.e. the toolkit - we'll need to update it so that the logic matches

EDIT: I misunderstood - the issue was instead that chains with existing overfilled blocks would cause the toolkit to fail to sync. @mpskowron is checking this

@ozgb I've done the test, added details to the ticket: https://shielded.atlassian.net/browse/PM-20839?focusedCommentId=56688

@gilescope gilescope changed the title fix: overfilled blocks report max fullness instead of 0 fix: overfilled blocks should report max fullness instead of 0 Jan 30, 2026
@gilescope gilescope enabled auto-merge (squash) January 30, 2026 08:29
Copy link
Contributor

@ozgb ozgb left a comment

Choose a reason for hiding this comment

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

LGTM

@gilescope gilescope merged commit a043799 into main Jan 30, 2026
36 checks passed
@gilescope gilescope deleted the skowron/fix-block-fullness-reporting branch January 30, 2026 11:28
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