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.

Sign up for GitHub

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

Jump to bottom

Beacon API: update GetStateRoot #8437

Merged
merged 9 commits into from
Feb 12, 2021
Merged

Beacon API: update GetStateRoot #8437

merged 9 commits into from
Feb 12, 2021

Conversation

terencechain
Copy link
Member

What type of PR is this?

Feature

What does this PR do? Why is it needed?

Address feedbacks from #8402

Which issues(s) does this PR fix?

N/A

Other notes for review

N/A

@terencechain terencechain requested a review from a team as a code owner February 12, 2021 00:46
@terencechain terencechain self-assigned this Feb 12, 2021
terencechain
terencechain commented Feb 12, 2021
View reviewed changes
beacon-chain/rpc/beaconv1/state.go
if !e {
return nil, status.Errorf(codes.NotFound, "No block exists")
}
if len(blks) != 1 {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I checked eth2api and I'm not sure what to do here. There could be multiple blocks exist in the same slot. In that case, which state root do we use?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ask this question on the APIs channel? https://discord.com/channels/595666850260713488/710831784991916072

@terencechain
Copy link
Member Author

cc @0xKiwi @rkapka

rkapka
rkapka requested changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

Requesting changes for the test where the same things are compared. A very minor thing is the incorrect order of expected/actual in other tests (it's annoying that it cannot be enforced...).

beacon-chain/rpc/beaconv1/state_test.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beaconv1/state.go
if !e {
return nil, status.Errorf(codes.NotFound, "No block exists")
}
if len(blks) != 1 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe ask this question on the APIs channel? https://discord.com/channels/595666850260713488/710831784991916072

beacon-chain/rpc/beaconv1/state_test.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beaconv1/state_test.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beaconv1/state_test.go Outdated Show resolved Hide resolved
beacon-chain/rpc/beaconv1/state_test.go Outdated Show resolved Hide resolved
terencechain and others added 5 commits February 12, 2021 07:08
@terencechain @rkapka
Update beacon-chain/rpc/beaconv1/state_test.go
ca6a887 
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
@terencechain @rkapka
Update beacon-chain/rpc/beaconv1/state_test.go
38a5d28 
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
@terencechain @rkapka
Update beacon-chain/rpc/beaconv1/state_test.go
18576a5 
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
@terencechain @rkapka
Update beacon-chain/rpc/beaconv1/state_test.go
3d24f84 
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
@terencechain @rkapka
Update beacon-chain/rpc/beaconv1/state_test.go
49e43f6 
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
@terencechain
Copy link
Member Author

Maybe ask this question on the APIs channel?

Let's just leave it as it is now. I like the current approach as simpler. Anything fancier to be more of a dos issue

rkapka
rkapka approved these changes Feb 12, 2021
View reviewed changes
Copy link
Contributor

@rkapka rkapka left a comment

Choose a reason for hiding this comment

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

Looks great, thanks for fixing my mess!

@terencechain
Copy link
Member Author

Looks great, thanks for fixing my mess!

Anytime. You already did great amount of work there, I just modified a few things :)

@prylabs-bulldozer prylabs-bulldozer bot merged commit 66471c2 into develop Feb 12, 2021
ahadda5 pushed a commit to ahadda5/prysm that referenced this pull request Feb 14, 2021
@terencechain @rkapka @ahadda5
Beacon API: update GetStateRoot (prysmaticlabs#8437)
9c5a864 
* Address various feedbacks

* Gaz

* More nil check

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Co-authored-by: Radosław Kapka <rkapka@wp.pl>
ahadda5 pushed a commit to ahadda5/prysm that referenced this pull request Feb 14, 2021
@terencechain @rkapka @ahadda5
Beacon API: update GetStateRoot (prysmaticlabs#8437)
5044ad5 
* Address various feedbacks

* Gaz

* More nil check

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Co-authored-by: Radosław Kapka <rkapka@wp.pl>
@terencechain terencechain deleted the get-state-root branch February 15, 2021 20:00
prylabs-bulldozer bot added a commit that referenced this pull request Feb 15, 2021
@ahadda5 @prestonvanloon
@prylabs-bulldozer @terencechain @rauljordan @nisdas @deepsource-autofix @rkapka
Mask credentials for logging (#8429)
f4a6b90 
* Added MaskCredentialsLogging to logutil, which masks the user info, path and query. It leaves the hostname and port untouched . Making it more secure during logging

* Added MaskCredentialsLogging to logutil, which masks the user info,path and query. It leaves the hostname and port untouched . Making it more secure during logging

* Added newline based on the PR checks

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Gazelle

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>

* added unit tests

* updated one test case

* added logutil_test.go unit test cases

* Refactor validator subnet subscriptions to be non-blocking (#8319)

* Use response.NextEpochDuties for aggregator subnet subscriptions (credit: @KaanKC PR #8204). Make committee subnet subscriptions method non-blocking call

* Fix test

* Fix test

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>

* Add Ability to Specify All Public Keys When Exiting Validators (#8399)

* add programmatic voluntary exit

* add exit all flag

* test

* lint

* add multiple exits test

* fix test

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* Increase Validation Queue (#8431)

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* Validator: add a DEBUG log to show batch attestation save duration (#8432)

* Add a debug log to show duration

* Autofix issues in 1 file

Resolved issues in validator/db/kv/attester_protection.go via DeepSource Autofix

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>

* Add Mutex and Block Profiling (#8435)

* Implement GetStateRoot in the beacon API (#8402)

* update ethereumapis dependency

* span

* initial implementation

* introduce stategen Service interface and MockService

* Include AddStateForSlot function in the mock service

* return states from mock

* add GenesisState to POWChain mock

* populate roots in helper state

* initialize Slot when creating helper state

* tests

* code refactor - extract helper functions

* gzl

* use SetSlot in tests

* handle SetSlot error

* use new testutil's NewBeaconState

* gzl

* go mod tidy

* rename Service to StateManager

* move regex check to helper

Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

* On Block Cleanup (#8438)

* Beacon API: update GetStateRoot  (#8437)

* Address various feedbacks

* Gaz

* More nil check

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* Update beacon-chain/rpc/beaconv1/state_test.go

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

Co-authored-by: Radosław Kapka <rkapka@wp.pl>

* qualifying my unix user ahaddad - no real changes to the files

* Update shared/logutil/logutil.go

* Update shared/logutil/logutil.go

* Update shared/logutil/logutil.go

* Update shared/logutil/logutil.go

* Update shared/logutil/logutil.go

Co-authored-by: Preston Van Loon <preston@prysmaticlabs.com>
Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>
Co-authored-by: terence tsao <terence@prysmaticlabs.com>
Co-authored-by: Raul Jordan <raul@prysmaticlabs.com>
Co-authored-by: Nishant Das <nishdas93@gmail.com>
Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Co-authored-by: Radosław Kapka <rkapka@wp.pl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rkapka rkapka rkapka approved these changes

@farazdagi farazdagi Awaiting requested review from farazdagi

@rauljordan rauljordan Awaiting requested review from rauljordan

Assignees

@terencechain terencechain

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@terencechain @rkapka