Skip to content

Adjust for optional totalDifficulty in JSON-RPC BlockObject#3883

Merged
kdeme merged 2 commits intomasterfrom
rpc-total-diff-optional
Dec 22, 2025
Merged

Adjust for optional totalDifficulty in JSON-RPC BlockObject#3883
kdeme merged 2 commits intomasterfrom
rpc-total-diff-optional

Conversation

@kdeme
Copy link
Copy Markdown
Contributor

@kdeme kdeme commented Dec 18, 2025

Going for optionally totalDifficulty instead of removal.

See reasoning in discussion here: #3882 (comment)

Depends on status-im/nim-web3#233

defer:
txFrame.dispose()

txFrame.getScore(blockHash)
Copy link
Copy Markdown
Contributor Author

@kdeme kdeme Dec 18, 2025

Choose a reason for hiding this comment

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

I want to note that, ideally here getScore returns a None in case the block referenced is post-merge.

However from reading the code it would seem that is not the case (but it was probably the long term plan).

One could assume that getScore here would return failure if block post merge:

if error.error != KvtNotFound:

But this comment indicates that the score is still stored also for each post merge block:

# TODO it's slightly wrong to fail here and leave the block in the db,
# but this code is going away soon enough
return err(info & ": cannot get score")
score = parentScore + header.difficulty
# After EIP-3675, difficulty is set to 0 but we still save the score for
# each block to simplify totalDifficulty reporting
# TODO get rid of this and store a single value

I have not tested this yet but I would assume that it will still find a score.

return res

proc getTotalDifficulty*(chain: ForkedChainRef, blockHash: Hash32): Opt[UInt256] =
let txFrame = chain.txFrame(blockHash).txFrameBegin()
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.

Using txFrame here for the fork-aware view, even though this value is not needed for current forks (post merge forks). I assumed it was useful also when using it for uncle blocks (pre-merge). But after looking into the code it is not. I will adjust to the baseTxFrame version.

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.

@bhartnett I think it is not needed to do the txFrame version as you have added here: https://github.com/status-im/nimbus-eth1/blob/bal-devnet-0/execution_chain/rpc/debug.nim#L119-L122

As this value is not supposed to change anymore. (and ideally does not get set post merge, but ok)

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.

@kdeme Good point. In that case should it just always return zero?

@kdeme kdeme force-pushed the rpc-total-diff-optional branch from 8124197 to 1e1bc62 Compare December 22, 2025 10:42
@kdeme kdeme marked this pull request as ready for review December 22, 2025 10:43
@kdeme
Copy link
Copy Markdown
Contributor Author

kdeme commented Dec 22, 2025

I will go ahead and merge this version as it is an improvement:

  • same behaviour for TD for blocks that are available: This can be improved in the future for post-merge blocks. Either by changing the TD storage in the future, or by checking simply if it is a pre or post merge block number.
  • blocks that are not found -> return none instead of the latest TD

@kdeme kdeme merged commit bbf88b2 into master Dec 22, 2025
17 of 18 checks passed
@kdeme kdeme deleted the rpc-total-diff-optional branch December 22, 2025 13:53
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