Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Default gas_limit is not followed in mev-builder configuration #13797

Closed
aimxhaisse opened this issue Mar 25, 2024 · 11 comments · Fixed by #13645
Closed

Default gas_limit is not followed in mev-builder configuration #13797

aimxhaisse opened this issue Mar 25, 2024 · 11 comments · Fixed by #13645
Assignees
Labels
Bug Something isn't working Builder PR or issue that supports builder related work Need-Info Need more information from author

Comments

@aimxhaisse
Copy link

Describe the bug

Currently the documentation around the fee-recipient configuration mentions that the gas_limit when not specified for block proposal, defaults to 30M:

gas_limit: A gas limit. Default limit is 30M gwei - 30000000.

(source)

This is not the case when using a mapping without an explicit gas_limit with a json-configuration like:

    "...": {
      "builder": {
        "enabled": true
      },
      "fee_recipient": "..."
    }

On the relayer side, the registration call will look as follows:

[{"message":{"fee_recipient":"...","gas_limit":"0","timestamp":"1709765619","pubkey":"...", ...}]

Where gas_limit should be 30M.

Has this worked before in a previous version?

I believe this has been around for a while, I've seen blocks > 8 months or so with the same behavior.

🔬 Minimal Reproduction

Configure a validator with --proposer-settings-file=/recipients/config.json without specifying a gas_limit, start the validator and check the registration call made to the relayer, the gas_limit will be 0.

Error

No response

Platform(s)

Linux (x86), Linux (ARM), Mac (Intel), Mac (Apple Silicon), Windows (x86), Windows (ARM)

What version of Prysm are you running? (Which release)

5.0.1

Anything else relevant (validator index / public key)?

No response

@aimxhaisse aimxhaisse added the Bug Something isn't working label Mar 25, 2024
@james-prysm james-prysm self-assigned this Mar 25, 2024
@james-prysm james-prysm added the Builder PR or issue that supports builder related work label Mar 25, 2024
@james-prysm
Copy link
Contributor

looking into this

@james-prysm
Copy link
Contributor

james-prysm commented Mar 25, 2024

does this consistently happen? also wanted to confirm the network is mainnet
Wondering if anything was changed in the config file DefaultBuilderGasLimit . there have been a lot of fixes with #13645 in our next release. I'm having some trouble reproducing just through unit tests and will need to dive deeper to find root cause.

will dig deeper on holesky

@james-prysm
Copy link
Contributor

Initial test on holesky with 1 key seemed to work correctly. this was against mev boost relay from flashbots

with the following configuration

{
  "proposer_config": {
    "pubkey": {
      "fee_recipient": "some address",
      "builder":{
        "enabled": true
      }
    }
  },
  "default_config": {
    "fee_recipient": "some address"
  }
}

@james-prysm james-prysm added the Need-Info Need more information from author label Mar 26, 2024
@aimxhaisse
Copy link
Author

I've been toying a bit more on this, and maybe you need to explicitly have enable-builder set to true in your validator configuration to reproduce, I've tried with and without and get different results, setting it to false will result in a gas_limit of 30000000.

To be clear, I use on the validator side:

  • flag --proposer-settings-file=<proposer-settings.json>, in this file, no mention of gas_limit
  • flag --suggested-fee-recipient is set to a dummy address I have control on in case
  • config file contains validators-external-signer-public-keys as I'm using a remote signer
  • config file contains enable-builder: true # this is important and changes the outcome

To reproduce I do the following:

  • I setup a dummy python HTTP server that logs everything, returns 200 upon GET status calls, and 500 upon registration, it's OK as we only want to log the content of the POST requests
  • I configure the beacon to target this dummy server (http-mev-relay in the config.yaml pointing to my dummy python server)

Then I start the beacon and watch registrations, and can see the gas_limit set to 0:

[{"message":{"fee_recipient":"0x4d4021c5142e92e4a3afeaa1d9f88e3540192a27","gas_limit":"0","timestamp":"1711394817","pubkey":"0xb0be530e2b1f9977127dc0a2779461859c4540efc56c5a670683c2f3d0e41b95655bda7701fb4885099ac342c61f1c63"},"signature":"0xa704dbcacff86191277dfa40cb3d5df4c386ed85a8d60c0be805fe97bd80648f2a2fe54fec088f615c85ad8adee156c6115af44196b98285a94a330e6b4ef85c36e688fb9a90bf790c96691aa19b2dd713751ab46b2834eb1ecf6f8a362a3759"} ...

Happy to dig more / try to narrow a bit more the issue if that helps.

@aimxhaisse
Copy link
Author

Also attaching an example of an Holesky config I use for --proposer-settings-file on the validator file, in case that helps.

recipients.json

@james-prysm
Copy link
Contributor

james-prysm commented Mar 26, 2024

This was helpful thanks, you should not be using suggested-fee-recipient + proposer-settings-file at the same time, though i do think it should allow it still. Will dig deeper on where exactly this is triggering, appreciate the collaboration here.

@james-prysm
Copy link
Contributor

I was able to reproduce the bug with --enable-builder set to true on v5.0.1

I just tested it on latest develop and this bug has been patched, so it should be fixed in v5.0.2

@aimxhaisse
Copy link
Author

Thanks a lot!

About the mixed usage of suggested-fee-recipient + proposer config, the idea is that we don't rely on it and have it as a safe-fallback in case we somehow screw up the proposer config, so we can at least have fees land on an address we can fiddle with. Not specifying one with a mishap in the config would mean it goes to 0x000... right?

Documentation on https://docs.prylabs.network/docs/execution-node/fee-recipient#configure-fee-recipient-via-jsonyaml-validator-client-only mentions both are usable together:

You can assign different wallet addresses to each of your validator public keys using JSON/YAML configuration. Fee recipient address assignments specified through JSON/YAML override those configured through the --suggested-fee-recipient flag.

Do you think this documentation should be updated if it's not recommended?

@james-prysm
Copy link
Contributor

Perhaps the wording can be more clear, the JSON/YAML files will override the suggested-fee-recipient flag in the current implementation, is the desired effect to do the opposite?

fee recipient address assignments specified through JSON/YAML override those configured through the --suggested-fee-recipient flag.

the fall back for suggested fee recipient would be for the beacon node.

i can reverse this effect and update the document in a future release if there is a desired effect to use the suggested fee recipient to override the json file, but may need to see if there are other implications to this.

it should be fine however to just use the defined default from the json/yaml file.

@aimxhaisse
Copy link
Author

Sorry the intent is the opposite: if for some reason a key is missing from the proposer config, use the suggested-fee-recipient, otherwise, use what's in the config. That way if you forget something in the config, the rewards will use the suggested-fee-recipient.

IMHO there is nothing to change :)

@james-prysm
Copy link
Contributor

the suggested fee recipient flag == default_config from the file, if there's something missed in the proposer configs for a missing key or something the default config kicks in. behind the scenes the suggested fee recipient just sets the default config internally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Builder PR or issue that supports builder related work Need-Info Need more information from author
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants