Skip to content
This repository has been archived by the owner on Aug 19, 2024. It is now read-only.

fix incomplete pending block handling logic regarding API requests #1847

Merged
merged 2 commits into from
May 25, 2023

Conversation

aidan-kwon
Copy link
Member

Proposed changes

This PR contains two different fixes regarding "pending" block requests.

  1. panic because of the absence of pending block
    Panic was reported in Invalid memory address or nil pointer dereference #1841 regarding API servicing with "pending" block parameter. The panic can be triggered when a pending block has never been created yet but a user requests API with "pending" parameter.

  2. occasional API error with "invalid signature length" error message
    During the synchronization, a node uses a worker's snapshot to return a pending block. The block is created with the worker's current data in updateSnapshot() method. Since EN doesn't create a pending block correctly, however, the current.header doesn't have enough information including a miner's signature. Therefore, eth_getBlockByNumber and other API fail to recover a block proposer's address and return an error message. This PR set the previous block header for a pending block of EN/PN to support the API correctly.

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

Resolve #1842

@aidan-kwon aidan-kwon self-assigned this May 17, 2023
@aidan-kwon aidan-kwon changed the title add nil block handling logic for pending block API requests fix incomplete pending block handling logic regarding API requests May 17, 2023
work.Block = parent
work.header = parent.Header()
Copy link
Member

Choose a reason for hiding this comment

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

I didn't find any negative effect when setting the previous block header.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kjeom You mean that you can't find the current problem related to block header, right?

  1. Currently, an incomplete block header is used for makeCurrent and the method set the header to worker.current.header.

    klaytn/work/worker.go

    Lines 529 to 543 in 6833919

    header := &types.Header{
    ParentHash: parent.Hash(),
    Number: nextBlockNum,
    Extra: self.extra,
    Time: big.NewInt(tstamp),
    }
    if self.config.IsMagmaForkEnabled(nextBlockNum) {
    header.BaseFee = nextBaseFee
    }
    if err := self.engine.Prepare(self.chain, header); err != nil {
    logger.Error("Failed to prepare header for mining", "err", err)
    return
    }
    // Could potentially happen if starting to mine in an odd state.
    err = self.makeCurrent(parent, header)

  2. worker.snapshotBlock consists of worker.current.header and others

    klaytn/work/worker.go

    Lines 616 to 620 in 6833919

    self.snapshotBlock = types.NewBlock(
    self.current.header,
    self.current.txs,
    self.current.receipts,
    )

  3. worker.snapshotBlock can be returned as a pending block

    klaytn/work/worker.go

    Lines 220 to 225 in 6833919

    func (self *worker) pendingBlock() *types.Block {
    if atomic.LoadInt32(&self.mining) == 0 {
    // return a snapshot to avoid contention on currentMu mutex
    self.snapshotMu.RLock()
    defer self.snapshotMu.RUnlock()
    return self.snapshotBlock

  4. In the JSON marshaling logic, the pending block's header is used to calculate a block proposer address

    klaytn/api/api_ethereum.go

    Lines 1463 to 1474 in 6833919

    func (api *EthereumAPI) rpcMarshalHeader(head *types.Header) (map[string]interface{}, error) {
    var proposer common.Address
    var err error
    b := api.publicKlayAPI.b
    if head.Number.Sign() != 0 {
    proposer, err = b.Engine().Author(head)
    if err != nil {
    // miner is the field Klaytn should provide the correct value. It's not the field dummy value is allowed.
    logger.Error("Failed to fetch author during marshaling header", "err", err.Error())
    return nil, err
    }

Therefore, an incomplete block header assigned to worker.current.header can cause panic. Since EN/PN doesn't finish the pending block generation process, an alternative header is needed for worker.current.header

Copy link
Member

@kjeom kjeom May 18, 2023

Choose a reason for hiding this comment

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

@aidan-kwon Oh, sorry for confusing. I just noted what I have finished for review.
Anyway thanks for explaining it in detail.

I just wanted to inform this line to other reviewers to check if there's something that I didn't find

@aidan-kwon aidan-kwon merged commit 843a56b into klaytn:dev May 25, 2023
@aidan-kwon aidan-kwon deleted the 230517-ethPending branch May 25, 2023 03:03
@JayChoi1736 JayChoi1736 mentioned this pull request Jul 24, 2023
20 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"eth" namespace APIs can't handle "pending" block correctly.
3 participants