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

feat: implement expected withdrawals API #6514

Draft
wants to merge 4 commits into
base: unstable
Choose a base branch
from

Conversation

HiroyukiNaito
Copy link
Contributor

@HiroyukiNaito HiroyukiNaito commented Mar 6, 2024

Motivation

Latest beacon API spec requires expected withdrawals API

Description

Implement the following route:

https://ethereum.github.io/beacon-APIs/#/Builder/getNextWithdrawals

Closes #5696

Steps to test or reproduce

curl -X 'GET' \
  'http://localhost:9596/eth/v1/builder/states/head/expected_withdrawals?proposal_slot=1' \
  -H 'accept: application/json'

Progress

  • Creating routes packages/api/src/beacon/routes/builder.ts
  • Defining types packages/api/src/beacon/routes/builder.ts
  • Implementing route and namaspace
  • packages/api/src/beacon/client/builder.ts
  • packages/api/src/beacon/client/index.ts
  • packages/api/src/beacon/index.ts
  • packages/api/src/beacon/routes/index.ts
  • packages/api/src/beacon/server/builder.ts
  • packages/api/src/beacon/server/index.ts
  • packages/beacon-node/src/api/impl/api.ts
  • Implementing function
  • packages/beacon-node/src/api/impl/builder/index.ts
  • Implementing unit test

Components

  1. get state from state_id
    async getState(stateId: string | number, format?: ResponseFormat) {
  2. get expectedWithdrawals array from getExpectedWithdrawls(state from 1.)
  3. filter proposal_slot form the 2. data

Currently Lodestar doesn't support finalized in the apis. You can discard this field for now. See #5693 for more.

@HiroyukiNaito HiroyukiNaito requested a review from a team as a code owner March 6, 2024 10:46
@HiroyukiNaito
Copy link
Contributor Author

HiroyukiNaito commented Mar 6, 2024

This is a tentative implement.
Please, do not hesitate to point out if I am in a wrong direction because I'm a new born baby in this field.

@HiroyukiNaito HiroyukiNaito changed the title feat: Implement expected withdrawals API feat: implement expected withdrawals API Mar 6, 2024
@ensi321
Copy link
Contributor

ensi321 commented Mar 7, 2024

Thanks for your contribution🙂

This is a tentative implement.
Please, do not hesitate to point out if I am in a wrong direction because I'm a new born baby in this field.

Just trying to understand the scope of this PR. Is it only adding the route definition of getNextWithdrawals, or is it going to implement the its functionality as well but you are still working on it?

If latter, it would be great if you can mark it as PR draft and reach out to the maintainers on Discord if you need any assistance. If former, I am happy to review it once the CI checks are passed😀

@HiroyukiNaito
Copy link
Contributor Author

HiroyukiNaito commented Mar 7, 2024

Thanks for your contribution🙂

This is a tentative implement.
Please, do not hesitate to point out if I am in a wrong direction because I'm a new born baby in this field.

Just trying to understand the scope of this PR. Is it only adding the route definition of getNextWithdrawals, or is it going to implement the its functionality as well but you are still working on it?

If latter, it would be great if you can mark it as PR draft and reach out to the maintainers on Discord if you need any assistance. If former, I am happy to review it once the CI checks are passed😀

That would be appreciated! So far, surveying how to implement the API. If both adding Routes and Types are completed, I would like to ask you to review before the implementation of the function and test.

@HiroyukiNaito
Copy link
Contributor Author

@ensi321
Could you please review my code both routes and types when you have a moment before implementing the function?
I'm wondering it is right implement.

Copy link

codecov bot commented Mar 8, 2024

Codecov Report

Merging #6514 (7359cad) into unstable (918924e) will decrease coverage by 0.08%.
Report is 17 commits behind head on unstable.
The diff coverage is 70.23%.

Additional details and impacted files
@@             Coverage Diff              @@
##           unstable    #6514      +/-   ##
============================================
- Coverage     61.55%   61.47%   -0.08%     
============================================
  Files           556      558       +2     
  Lines         58586    58736     +150     
  Branches       1847     1848       +1     
============================================
+ Hits          36060    36110      +50     
- Misses        22486    22586     +100     
  Partials         40       40              

@HiroyukiNaito
Copy link
Contributor Author

I misunderstood the concept. I will recreate it.

Copy link
Contributor

@ensi321 ensi321 left a comment

Choose a reason for hiding this comment

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

Good work! Left some comments regarding the route definition

packages/api/src/builder/routes.ts Outdated Show resolved Hide resolved
packages/api/src/builder/routes.ts Outdated Show resolved Hide resolved
packages/api/src/builder/routes.ts Outdated Show resolved Hide resolved
packages/api/test/unit/builder/testData.ts Outdated Show resolved Hide resolved
packages/api/src/builder/routes.ts Outdated Show resolved Hide resolved
packages/api/src/builder/routes.ts Outdated Show resolved Hide resolved
@ensi321
Copy link
Contributor

ensi321 commented Mar 11, 2024

Please convert this PR into PR draft while working on the implementation and convert it back when it's ready for another round of review. You can read more about it here

Maintainers will get notifications every time you commit unless you make this a draft 😅

@HiroyukiNaito HiroyukiNaito marked this pull request as draft March 11, 2024 07:33
@HiroyukiNaito
Copy link
Contributor Author

@ensi321 Great! Thank you for the tips. According to these hints, I will program it again.

@HiroyukiNaito
Copy link
Contributor Author

HiroyukiNaito commented Mar 13, 2024

@ensi321

I implemented the interface for getting the withdrawal data.
However, curl -X 'GET' 'http://localhost:9596/eth/v1/builder/states/head/expected_withdrawals' -H 'accept: application/json' still returns {"message":"Route GET:/eth/v1/builder/states/head/expected_withdrawals not found","error":"Not Found","statusCode":404}.
It looks failed to route the api. Anything needs to add for routing the API?

@ensi321
Copy link
Contributor

ensi321 commented Mar 14, 2024

@ensi321

I implemented the interface for getting the withdrawal data. However, curl -X 'GET' 'http://localhost:9596/eth/v1/builder/states/head/expected_withdrawals' -H 'accept: application/json' still returns {"message":"Route GET:/eth/v1/builder/states/head/expected_withdrawals not found","error":"Not Found","statusCode":404}. It looks failed to route the api. Anything needs to add for routing the API?

You will also need to add a http server for your newly created builder api.
See

import {ChainForkConfig} from "@lodestar/config";

debug: () => debug.getRoutes(config, api.debug),

Lodestar should start serving this route afterwards. Also you will need to define a http client too which is similar to http server. See packages/api/src/beacon/client/debug.ts

@HiroyukiNaito
Copy link
Contributor Author

@ensi321 Thank you so much for taking the time to review the code, especially with your busy schedule.
I could get fake response now before implementating the function.
If you have any correction before that, please do not hesitate to point out.

 curl -X 'GET'   'http://localhost:9596/eth/v1/builder/states/head/expected_withdrawals'   -H 'accept: application/json'| python3 -m json.tool
{
    "execution_optimistic": false,
    "data": [
        {
            "index": 1,
            "validator_index": 1,
            "address": "0xAbcF8e0d4e9587369b2301D0790347320302cc09",
            "amount": 1
        }
    ]
}

@HiroyukiNaito
Copy link
Contributor Author

Hi, I almost done (exception handlings are not yet) , but, Development network ./lodestar dev --genesisValidators 8 --reset --rest.namespace "*" always returns http://localhost:9596/eth/v1/builder/states/head/expected_withdrawals empty array. How do I get getExpectedWithdrawals method value on this environment?
Do I need to implement on my mainnet environment for conforming the logic?

@ensi321
Copy link
Contributor

ensi321 commented Mar 18, 2024

Do I need to implement on my mainnet environment for conforming the logic?

I recommend testing this kind of features by running it as a beacon node on mainnet or other public testnet like holesky. I admit it might be hard for contributors who have limited hardwares.

Hi, I almost done (exception handlings are not yet) , but, Development network ./lodestar dev --genesisValidators 8 --reset --rest.namespace "*" always returns http://localhost:9596/eth/v1/builder/states/head/expected_withdrawals empty array. How do I get getExpectedWithdrawals method value on this environment?

This is expected. The reason why getExpectedWithdrawals() is returning empty list is because

  1. The validators don't have eth1 withdrawal credentials so this condition fails
    if (!hasEth1WithdrawalCredential(validator.withdrawalCredentials)) {
  2. The validators are not qualified for full or partial withdrawals. The latter is because their balance is 32ETH. See this and this

The current dev command does a poor job to initiate validators that meet these two requirements.

If running a beacon node on mainnet/testnet is too much for you, and wish to test this locally with dev command, I recommend to modify the codebase as follow such that validators can meet the two requirements above:

  1. Change this line
    const {state} = nodeUtils.initDevState(config, validatorCount, {genesisTime, eth1BlockHash});

    to const {state} = nodeUtils.initDevState(config, validatorCount, {genesisTime, eth1BlockHash, withEth1Credentials: true});
  2. Change this line to amount: MAX_EFFECTIVE_BALANCE + 1,

Lastly run ./lodestar dev --genesisValidators 8 --reset --rest.namespace "*" --params.ALTAIR_FORK_EPOCH 0 --params.BELLATRIX_FORK_EPOCH 0 --params.CAPELLA_FORK_EPOCH 0 --terminal-total-difficulty-override 0 to activate cappella fork.

I ran this locally and the api returns getExpectedWithdrawals error - Do not know how to serialize a BigInt TypeError: Do not know how to serialize a BigInt.

@HiroyukiNaito
Copy link
Contributor Author

@ensi321 Thanks! I will resume this soon.

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.

Implement expected withdrawals API
2 participants