Skip to content

Conversation

@garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Sep 22, 2022

PR description

PR to enable debugging block proposals by re-adding an updated version of the deprecated
engine_preparePayload endpoint

be sure to use the current or a recent parent blockhash or you will DoS bonsai db

All fields in the parameter are optional, including the parameter itself

example usage:

curl --location --request POST 'http://localhost:8551' \
--header 'Content-Type: application/json' \
--header 'Authorization: Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE2NTkwNDEzMzZ9.4PDdSaG9hFOFR4Th7rEgaKKECsXfz6IPckFRcfSP13o' \
--data-raw '{
    "jsonrpc":"2.0",
    "method":"engine_preparePayload_debug",
    "params":
    [
        {
            "parentHash": "0xd4e56740f876aef8c010b86a40d5f56745a118d0906a34e69aec8c0db1cb8fa3",
            "feeRecipient": "0x0000000000000000000000000000000000000000",
            "timestamp": "0x0",
            "prevRandao": "0x0",
            "withdrawals": []
        }
    ],
    "id":1
}'

OR, this usage will use fee recipient 0x00..00, current timestamp, current chain head, "deadbeef" prevrandao, and empty withdrawals:

curl --location --request POST 'http://localhost:8551' \
--header 'Content-Type: application/json' \
--header 'Authorization: Bearer eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpYXQiOjE2NTkwNDEzMzZ9.4PDdSaG9hFOFR4Th7rEgaKKECsXfz6IPckFRcfSP13o' \
--data-raw '{
    "jsonrpc":"2.0",
    "method":"engine_preparePayload_debug",
    "params":
    [],
    "id":1
}'

Fixed Issue(s)

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@garyschulte garyschulte force-pushed the feature/debug_preparePayload branch from 5daa0ad to d2a46f4 Compare September 22, 2022 21:41
@jflo
Copy link
Contributor

jflo commented Sep 27, 2022

Since this is a debug method, would it be simpler to use if it always built on the chainhead? It would be less timing/juggling when it is being used.

@garyschulte
Copy link
Contributor Author

Since this is a debug method, would it be simpler to use if it always built on the chainhead? It would be less timing/juggling when it is being used.

yes, yes it would 👍 . I think we can do the same thing we do with timestamp and default it to the current head if it is otherwise not specified

@garyschulte garyschulte force-pushed the feature/debug_preparePayload branch from a08359a to 097264e Compare October 5, 2022 22:33
@garyschulte garyschulte force-pushed the feature/debug_preparePayload branch from 0cb6ee6 to 097e121 Compare October 28, 2022 23:40
@gfukushima
Copy link
Contributor

Hi Gary, can this be closed since it's been merged as part of another PR?

@garyschulte garyschulte force-pushed the feature/debug_preparePayload branch from 097e121 to 35c9fb9 Compare February 23, 2023 19:05
@garyschulte
Copy link
Contributor Author

Hi Gary, can this be closed since it's been merged as part of another PR?

this was merged in a speculative RC candidate, and isn't in main yet.

@garyschulte garyschulte marked this pull request as ready for review February 23, 2023 19:07
@garyschulte garyschulte force-pushed the feature/debug_preparePayload branch from bd81e68 to 0166200 Compare February 23, 2023 19:20
@non-fungible-nelson
Copy link
Contributor

can we close or re-status this @garyschulte ?

@garyschulte
Copy link
Contributor Author

garyschulte commented Apr 13, 2023

IMO we should rebase and merge as it is useful for debugging proposals. @fab-10 was using a different mechanism to test proposals IIRC, so would be helpful to get his opinion

@fab-10
Copy link
Contributor

fab-10 commented Apr 18, 2023

IMO we should rebase and merge as it is useful for debugging proposals. @fab-10 was using a different mechanism to test proposals IIRC, so would be helpful to get his opinion

It make sense to have this endpoint as well, it is an easier and fast method to trigger a payload creation

fab-10
fab-10 previously requested changes Apr 18, 2023
@garyschulte garyschulte force-pushed the feature/debug_preparePayload branch from 0166200 to 154f2fb Compare April 19, 2023 03:15
@garyschulte garyschulte enabled auto-merge (squash) April 19, 2023 03:16
ENGINE_GET_PAYLOAD_BODIES_BY_RANGE_V1("engine_getPayloadBodiesByRangeV1"),
ENGINE_EXCHANGE_CAPABILITIES("engine_exchangeCapabilities"),

ENGINE_PREPARE_PAYLOAD_DEBUG("engine_preparePayload_debug"),
Copy link
Contributor

@gfukushima gfukushima Apr 19, 2023

Choose a reason for hiding this comment

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

Here is a tricky question, shouldn't this method be in the debug set? Another reason to move that is, the way we built the engine_exchange_capabilities mean Besu might report this method. CL is probably going to ignore it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DEBUG is present even if the ENGINE api is not, so I think this needs to live within engine api. Good point about the capabilities though 🤔 I will check to see if this causes problems for CLs

Copy link
Contributor

Choose a reason for hiding this comment

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

What about a ENGINE_DEBUG set? So it is separated from ENGINE and can be enabled only if used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I filtered the debug prepare payload from the capabilities list in the same way the capabilities list filters itself 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just saw the ENGINE_DEBUG set suggestion. I think we could add something like that if we have more than one engine debug endpoint in the future 👍 . Rule of three applies in this case IMO

@garyschulte garyschulte force-pushed the feature/debug_preparePayload branch from 154f2fb to c845900 Compare April 19, 2023 17:27
@garyschulte garyschulte disabled auto-merge April 19, 2023 17:41
@garyschulte garyschulte enabled auto-merge (squash) April 19, 2023 18:02
@garyschulte garyschulte dismissed fab-10’s stale review April 19, 2023 23:31

implemented suggested changes

Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

LGTM

@garyschulte garyschulte merged commit de4aa7b into hyperledger:main Apr 19, 2023
@garyschulte garyschulte deleted the feature/debug_preparePayload branch April 20, 2023 00:30
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
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.

6 participants