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

Validator: Store and record change to stake #2877

Merged
merged 1 commit into from
Jan 25, 2022
Merged

Conversation

Geod24
Copy link
Collaborator

@Geod24 Geod24 commented Jan 18, 2022

Currently, changes to stake are indirectly recorded and handshake may start
to return different result any time. With this change, we ensure that we
have a single point from which changes are performed, and tracked.

@omerfirmak : This was my foray into #2868 as well as vibe-d/vibe.d#2638 and vibe-d/vibe.d#2637 (with the underlying rationale that I wanted to break down vibe.web.rest : request so we could re-use it in an override of RestInterfaceClient.request).

If this doesn't make sense, feel free to close.

@codecov
Copy link

codecov bot commented Jan 18, 2022

Codecov Report

Merging #2877 (1bdc372) into v0.x.x (f027f34) will decrease coverage by 0.14%.
The diff coverage is 81.25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           v0.x.x    #2877      +/-   ##
==========================================
- Coverage   79.17%   79.03%   -0.15%     
==========================================
  Files         207      207              
  Lines       18168    18180      +12     
==========================================
- Hits        14385    14368      -17     
- Misses       3783     3812      +29     
Flag Coverage Δ
integration 21.26% <ø> (+0.21%) ⬆️
unittests 87.28% <81.25%> (-0.20%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
source/agora/node/Validator.d 92.57% <81.25%> (-1.30%) ⬇️
source/agora/consensus/BlockStorage.d 68.90% <0.00%> (-10.61%) ⬇️
source/agora/consensus/protocol/Nominator.d 90.75% <0.00%> (-0.95%) ⬇️
source/agora/consensus/pool/Transaction.d 94.13% <0.00%> (+0.32%) ⬆️
source/agora/node/FullNode.d 72.16% <0.00%> (+0.68%) ⬆️
agora/submodules/ocean/src/ocean/util/log/Logger.d 69.15% <0.00%> (+2.80%) ⬆️
source/agora/test/LocalTransactions.d 91.11% <0.00%> (+4.44%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f027f34...1bdc372. Read the comment docs.

Currently, changes to stake are indirectly recorded and handshake may start
to return different result any time. With this change, we ensure that we
have a single point from which changes are performed, and tracked.
@Geod24
Copy link
Collaborator Author

Geod24 commented Jan 25, 2022

Rebased

@Geod24 Geod24 merged commit 6a08827 into bosagora:v0.x.x Jan 25, 2022
@Geod24 Geod24 deleted the identity branch January 25, 2022 12:20
@omerfirmak
Copy link
Contributor

omerfirmak commented Jan 25, 2022

Well, the existing implementation was consistent IMO. A validator would always enroll with the stake that it presents in a handshake call. I didnt see the point but I held back reviewing this until I started working on #2868

@omerfirmak
Copy link
Contributor

I believe this introduced a bug

Some nodes are reporting missing peers although they are connected to all 32 nodes, verified thru node_info.

Jan 30 10:42:21 eu-005 agora[2436465]: 2022-01-30 10:42:19,603 Info [agora.network.Manager] - Doing periodic network discovery: 31 required peers requested, 6 missing, known 32

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.

3 participants