goal: Added consensus upgrade fields to node status api & goal output #4800

mciccarello · 2022-11-16T07:02:09Z

Summary

Test Plan

CLAassistant · 2022-11-16T07:02:13Z

All committers have signed the CLA.

mciccarello · 2022-11-16T07:06:42Z

Eric (or someone) can you let me know what else is expected? I'm unsure what's required for testing in particular. I can't really test for correct values if there's not an upgrade going on, for example.

Eric-Warehime

LGTM. For testing you can probably extend the existing NodeStatus test here by just adding the proper data--probably injecting a custom Consensus Protocol here

It looks like the tests are already complaining about some of the new fields being added that aren't expected.

Eric-Warehime · 2022-11-16T15:53:34Z

Also, the go-algorand repo requires you to tag your PR with one of New Feature, Enhancement, Bug-Fix, Not-Yet-Enabled, Skip-Release-Notes.

And I think the title has to be something like <string describing PR category>: <PR TITLE> so like algod: Added consensus upgrade fields to node status api & goal output

codecov · 2022-11-17T21:42:06Z

Codecov Report

Merging #4800 (ab22d88) into master (4f1ac12) will decrease coverage by 0.07%.
The diff coverage is 14.51%.

@@            Coverage Diff             @@
##           master    #4800      +/-   ##
==========================================
- Coverage   53.70%   53.62%   -0.08%     
==========================================
  Files         433      433              
  Lines       54050    54106      +56     
==========================================
- Hits        29028    29017      -11     
- Misses      22776    22840      +64     
- Partials     2246     2249       +3

Impacted Files	Coverage Δ
cmd/goal/node.go	`11.90% <0.00%> (-0.78%)`	⬇️
daemon/algod/api/server/v2/handlers.go	`0.82% <0.00%> (-0.03%)`	⬇️
node/node.go	`4.22% <0.00%> (-0.04%)`	⬇️
daemon/algod/api/server/v2/test/helpers.go	`74.83% <90.00%> (+<0.01%)`	⬆️
ledger/blockqueue.go	`85.63% <0.00%> (-2.88%)`	⬇️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
cmd/tealdbg/debugger.go	`72.69% <0.00%> (-0.81%)`	⬇️
catchup/service.go	`69.80% <0.00%> (-0.73%)`	⬇️
network/wsNetwork.go	`64.74% <0.00%> (-0.18%)`	⬇️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

mciccarello · 2022-11-17T23:02:33Z

Please approve

winder

I left a couple suggestions. Did you have a chance to review Eric's suggestion? There may be an existing tests you can hook into.

daemon/algod/api/server/v2/handlers.go

winder · 2022-11-22T17:26:21Z

daemon/algod/api/server/v2/handlers.go

@@ -781,6 +798,14 @@ func (v2 *Handlers) GetStatus(ctx echo.Context) error {
 		CatchpointAcquiredBlocks:    &stat.CatchpointCatchupAcquiredBlocks,
 	}

+	if upgradePropose != "" {


This also needs to check that the vote has not finished:

Suggested change

if upgradePropose != "" {

if upgradePropose != "" && latestBlkHdr.NextProtocolVoteBefore > stat.LastRound {

winder · 2022-11-22T17:31:10Z

cmd/goal/node.go

@@ -458,6 +459,34 @@ func makeStatusString(stat model.NodeStatusResponse) string {
 		if stat.StoppedAtUnsupportedRound {
 			statusString = statusString + "\n" + fmt.Sprintf(catchupStoppedOnUnsupported, stat.LastRound)
 		}
+
+		if stat.UpgradePropose != nil {


Same as below, no need to display this once the vote is complete.

Suggested change

if stat.UpgradePropose != nil {

if stat.UpgradePropose != nil && stat.LastVersion != stat.NextVersion {

Eric-Warehime · 2022-11-28T21:02:34Z

daemon/algod/api/server/v2/handlers.go

+	upgradePropose := string(latestBlkHdr.UpgradeVote.UpgradePropose)
+	if upgradePropose != "" && latestBlkHdr.NextProtocolVoteBefore > stat.LastRound {
+		votesToGo := uint64(latestBlkHdr.NextProtocolVoteBefore) - uint64(stat.LastRound)
+		consensus := config.Consensus[protocol.ConsensusCurrentVersion]
+		upgradeVoteRounds := consensus.UpgradeVoteRounds
+		upgradeThreshold := consensus.UpgradeThreshold
+		votes := uint64(upgradeVoteRounds) - votesToGo
+		votesYes := latestBlkHdr.UpgradeState.NextProtocolApprovals
+		votesNo := votes - votesYes
+		upgradeDelay := uint64(latestBlkHdr.UpgradeVote.UpgradeDelay)
+
+		response.UpgradePropose = &upgradePropose
+		response.UpgradeThreshold = &upgradeThreshold
+		response.UpgradeApprove = &latestBlkHdr.UpgradeApprove
+		response.UpgradeDelay = &upgradeDelay
+		response.UpgradeNo = &votesNo
+	}


Discussed offline, but I think it's useful to write down here for posterity...

I think it's important to put this logic in the node.Status function.
Reason being is that the Status call uses the ledger.BlockHdr call used above internally to fill in the output. So the following sequence of events becomes problematic.

Node.Status is called returning status info for the latest round, round X.

Either agreement or catchup advances the latest round in the ledger to X+1.

Ledger.BlockHdr is called for the latest round, X+1.

Now the data in latestBlkHdr potentially conflicts the data in stat.

daemon/algod/api/server/v2/handlers.go

mciccarello · 2022-12-03T03:30:36Z

@winder The values don't look right to me either. But the field names and their sources come directly from the ticket as far as I can tell:

Fields from the UpgradeVote blockheader struct:

UpgradePropose
UpgradeDelay
UpgradeApprove

Consensus value:

UpgradeThreshold

The no votes is a computed value, as per the ticket based on a formula in node-ui.

votes - votes yes where
votes = upgrade rounds - votes to go where
votes to go = Header.NextProtocolVoteBefore - Status.LastRound
and
votes yes = Header.ProtocolApprovals

...and it looks to me like that's what my code does, though perhaps I'm getting one or more of these fields from the wrong data structure?

What I'm really not sure of is how "Upgrade proposal" could be empty. Investigating further

winder · 2022-12-05T14:14:47Z

@winder The values don't look right to me either. But the field names and their sources come directly from the ticket as far as I can tell:

Fields from the UpgradeVote blockheader struct:
UpgradePropose
UpgradeDelay
UpgradeApprove
Consensus value:
UpgradeThreshold

The no votes is a computed value, as per the ticket based on a formula in node-ui.

votes - votes yes where votes = upgrade rounds - votes to go where votes to go = Header.NextProtocolVoteBefore - Status.LastRound and votes yes = Header.ProtocolApprovals

...and it looks to me like that's what my code does, though perhaps I'm getting one or more of these fields from the wrong data structure?

What I'm really not sure of is how "Upgrade proposal" could be empty. Investigating further

It looks like you have the correct values, but we should just figure out how to label them for the user to make sure what they represent is clear.

I made a diagram to help refresh my memory about how these formulas relate to each other.

It looks like your formula is slightly wrong, you need to get the proposal round first:
Votes = Header.NextProtocolVoteBefore - (Header.NextProtocolVoteBefore - Consensus.UpgradeThreshold)

For reference, I think this is pretty clear. But the goal command / API have more constraints.

What do you think about the following:

Consensus Upgrade State: Voting
Next Protocol: xxxxx
Votes: 1500
Yes Votes: 750
Votes Remaining: 8500
Yes Votes Required: 9000
Vote Window Close: xxxxx

Once scheduled

Consensus Upgrade State: Scheduled
...
The existing upgrade scheduled information here
...

mciccarello · 2022-12-06T08:19:29Z

It looks like your formula is slightly wrong, you need to get the proposal round first: Votes = Header.NextProtocolVoteBefore - (Header.NextProtocolVoteBefore - Consensus.UpgradeThreshold)

I don't see how that's possible. That's just ConsensusUpgradeThreshold!

algoanne · 2022-12-20T19:46:01Z

@mciccarello and I chatted a bit about this. Here is what I propose:

Consensus Upgrade State: Voting
Next Protocol: 44fa607d6051730f5264526bf3c108d51f0eadb6

Yes Votes: 750
No Votes: 218
Votes Remaining: 8389

Yes Votes Required: 8422
Vote Window Close round: 25229176

And I like what @winder suggests for the "once scheduled" note.

winder

Looks good!

Tested with mainnet:

During vote:

Time since last block: 0.0s
Sync Time: 35.3s
Last consensus protocol: https://github.com/algorandfoundation/specs/tree/433d8e9a7274b6fca703d91213e05c7e6a589e69
Next consensus protocol: https://github.com/algorandfoundation/specs/tree/433d8e9a7274b6fca703d91213e05c7e6a589e69
Round for next consensus protocol: 25231779
Next consensus protocol supported: true
Last Catchpoint: 
Consensus upgrate state: Voting
Yes votes: 2427
No votes: 174
Votes remaining: 7399
Yes votes required: 9000
Vote window close round: 25239177
Genesis ID: mainnet-v1.0
Genesis hash: wGHE2Pwdvd7S12BL5FaOP20EGYesN73ktiC1qzkkit8=

After vote:

Time since last block: 0.1s
Sync Time: 596.7s
Last consensus protocol: https://github.com/algorandfoundation/specs/tree/433d8e9a7274b6fca703d91213e05c7e6a589e69
Next consensus protocol: https://github.com/algorandfoundation/specs/tree/44fa607d6051730f5264526bf3c108d51f0eadb6
Round for next consensus protocol: 25379177
Next consensus protocol supported: true
Last Catchpoint: 
Consensus upgrade state: Scheduled
Genesis ID: mainnet-v1.0
Genesis hash: wGHE2Pwdvd7S12BL5FaOP20EGYesN73ktiC1qzkkit8=

Added consensus upgrade fields to node status api & goal output

a335ad1

mciccarello marked this pull request as draft November 16, 2022 07:05

mciccarello requested a review from Eric-Warehime November 16, 2022 07:05

mciccarello marked this pull request as ready for review November 16, 2022 07:06

mciccarello marked this pull request as draft November 16, 2022 07:07

Eric-Warehime reviewed Nov 16, 2022

View reviewed changes

slight code cleanup

f739ef8

winder changed the title ~~Added consensus upgrade fields to node status api & goal output~~ goal: Added consensus upgrade fields to node status api & goal output Nov 16, 2022

winder added the Enhancement label Nov 16, 2022

mciccarello added 2 commits November 16, 2022 23:17

made consensus goal output conditional

c67a052

don't return consensus upgrade fields unless consensus upgrade active

0960c15

mciccarello marked this pull request as ready for review November 17, 2022 23:01

mciccarello requested a review from winder November 17, 2022 23:02

winder suggested changes Nov 22, 2022

View reviewed changes

review cleanup

d0dfae6

Eric-Warehime suggested changes Nov 28, 2022

View reviewed changes

mciccarello added 2 commits November 28, 2022 20:35

moved consensus vote variable logic from status handler to node

5cc0b58

added unit test for node status during consensus upgrade

272ba2c

winder reviewed Nov 29, 2022

View reviewed changes

daemon/algod/api/server/v2/handlers.go Outdated Show resolved Hide resolved

mciccarello requested review from Eric-Warehime and winder November 29, 2022 18:09

mciccarello added 4 commits November 29, 2022 12:46

Merge branch 'master' into feature/consensus-upgrade

5014f84

hopefully fixed merge issue with generated code

7c10fa6

fixed up node status consensus upgrade test with new fields from merge

4095fee

fixed node status consensus upgrade test case caused by merge

01a41fa

reworked the api and output

db18cf8

mciccarello dismissed Eric-Warehime’s stale review via db18cf8 December 9, 2022 18:12

mciccarello added 2 commits December 9, 2022 13:23

fixed test

aa07daf

fixed output format

8c639e6

mciccarello and others added 14 commits December 21, 2022 22:30

update to anne's specs

8f9daea

test push

0f8ddc6

Merge branch 'master' into feature/consensus-upgrade

5ab84e4

Merge branch 'master' into feature/consensus-upgrade

3d9571e

fix conflict with tests

f213636

problem with generated code

018fe7c

another try to fix generated code

39ebc47

test

016f5a4

more test

f72720d

more and more

7573309

more test test test

9893b0c

sdf

b6345d9

final (hopefully) format

2a1920e

Merge branch 'master' into feature/consensus-upgrade

ab22d88

mciccarello requested a review from winder January 3, 2023 18:22

winder approved these changes Jan 4, 2023

View reviewed changes

winder merged commit a35f4b8 into algorand:master Jan 4, 2023

This was referenced Jan 23, 2023

go-algorand 3.14.0-beta Release PR #5043

Merged

go-algorand 3.14.1-beta Release PR #5055

Merged

This was referenced Jan 30, 2023

go-algorand 3.14.1-stable Release PR #5076

Merged

go-algorand 3.14.2-stable Release PR #5110

Merged

winder mentioned this pull request Mar 22, 2023

API: upgrade status "no" vote underflow #5227

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

goal: Added consensus upgrade fields to node status api & goal output #4800

goal: Added consensus upgrade fields to node status api & goal output #4800

mciccarello commented Nov 16, 2022

CLAassistant commented Nov 16, 2022 •

edited

Loading

mciccarello commented Nov 16, 2022

Eric-Warehime left a comment •

edited

Loading

Eric-Warehime commented Nov 16, 2022

codecov bot commented Nov 17, 2022 •

edited

Loading

mciccarello commented Nov 17, 2022

winder left a comment

winder Nov 22, 2022

mciccarello Nov 29, 2022

winder Nov 22, 2022

mciccarello Nov 29, 2022

Eric-Warehime Nov 28, 2022

mciccarello Nov 29, 2022

mciccarello commented Dec 3, 2022 •

edited

Loading

winder commented Dec 5, 2022

mciccarello commented Dec 6, 2022

algoanne commented Dec 20, 2022

winder left a comment

	if upgradePropose != "" {
	if upgradePropose != "" && latestBlkHdr.NextProtocolVoteBefore > stat.LastRound {

	if stat.UpgradePropose != nil {
	if stat.UpgradePropose != nil && stat.LastVersion != stat.NextVersion {

goal: Added consensus upgrade fields to node status api & goal output #4800

goal: Added consensus upgrade fields to node status api & goal output #4800

Conversation

mciccarello commented Nov 16, 2022

Summary

Test Plan

CLAassistant commented Nov 16, 2022 • edited Loading

mciccarello commented Nov 16, 2022

Eric-Warehime left a comment • edited Loading

Choose a reason for hiding this comment

Eric-Warehime commented Nov 16, 2022

codecov bot commented Nov 17, 2022 • edited Loading

Codecov Report

mciccarello commented Nov 17, 2022

winder left a comment

Choose a reason for hiding this comment

winder Nov 22, 2022

Choose a reason for hiding this comment

mciccarello Nov 29, 2022

Choose a reason for hiding this comment

winder Nov 22, 2022

Choose a reason for hiding this comment

mciccarello Nov 29, 2022

Choose a reason for hiding this comment

Eric-Warehime Nov 28, 2022

Choose a reason for hiding this comment

mciccarello Nov 29, 2022

Choose a reason for hiding this comment

mciccarello commented Dec 3, 2022 • edited Loading

winder commented Dec 5, 2022

mciccarello commented Dec 6, 2022

algoanne commented Dec 20, 2022

winder left a comment

Choose a reason for hiding this comment

CLAassistant commented Nov 16, 2022 •

edited

Loading

Eric-Warehime left a comment •

edited

Loading

codecov bot commented Nov 17, 2022 •

edited

Loading

mciccarello commented Dec 3, 2022 •

edited

Loading