Participation Metrics #2677

winder · 2021-08-02T15:34:46Z

Summary

Draft PR - Currently have the public interface and plumbing to call functions from the agreement code.

This PR adds a new SQLite DB to store metadata and metrics for participation keys.

Test Plan

New unit tests TBD

agreement/abstractions.go

tsachiherman · 2021-08-02T15:46:16Z

agreement/abstractions.go

+	// RecordVote sets the LastVote field for the Account.
+	RecordVote(account basics.Address, round basics.Round) error
+
+	// RecordBlockProposal sets the LastBlockProposal field for the Account.
+	RecordBlockProposal(account basics.Address, round basics.Round) error


The vote and proposal aren't really that far apart from each other. In fact, tracking the proposal-vote is suffice to indicate a block proposals. I'd collapse these two into a single call, and add a bool indicating whether it's a proposal vote or not.

I changed this so there's a single Record(basics.Address, basics.Round, <participation type>) function.

One problem with this was that I had to import the node package in the agreement code in order to get this new participation type.

To avoid that, I moved the current keymanager implementation into the data/account directory for this new type. The current partkey db code is in data/account, so this isn't unprecedented. I'm trying to find a better way, so this is may change.

moving the simpleKeyManager to a different package makes it a non-testing code - which isn't really what we want.

I'm not sure that I followed the reasoning for that, as adding the account.ParticipationAction should not change any of the dependencies.

These were 2 different changes.

moving the simpleKeyManager to a different package makes it a non-testing code - which isn't really what we want.

I didn't realize that. So public packages that are only imported by _test.go files are excluded from the production binaries?

I'm not sure that I followed the reasoning for that, as adding the account.ParticipationAction should not change any of the dependencies.

Previously it was node.ParticipationAction defined in node/participation.go, so the agreement abstraction would have had to include node.

I'll continue to think about these two items as I implement the interfaces.

tsachiherman · 2021-08-02T15:48:36Z

node/participation.go

+func (db *participationDB) RecordVote(account basics.Address, round basics.Round) error {
+	return nil
+}


make sure this one is non-blocking.

node/participation.go

tsachiherman

looks like a promising start. I'd put all the new node files in node/keymanager/... since there will be more files around that.

tsachiherman · 2021-08-03T13:40:07Z

agreement/abstractions.go

+
+	// Record indicates that the given participation action has been taken.
+	// This function is also part of account.ParticipationRegistry.
+	Record(account basics.Address, round basics.Round, participationType account.ParticipationAction) error


I'm wondering if we really need to have this error return variable. I can't think of anything useful we can do with it. Can you ? i.e. having the the error between the participationRegistry and the AlgorandFullNode makes sense, since we want the node to log all the errors. However, providing that feedback to the agreement isn't very helpful since we don't have any useful thing we can do with that.
( i.e. just ignoring it implies that the interface is wrong.. )

I hadn't thought about it like that. I think that the ParticipationRegistry interface can return the error, and the agreement abstraction can remove it. In the shim function implemented by the FullNode I'll log a warning and the agreement code can ignore that case.

tsachiherman · 2021-08-03T13:46:23Z

agreement/pseudonode.go

@@ -386,6 +386,7 @@ func (t pseudonodeVotesTask) execute(verifier *AsyncVoteVerifier, quit chan stru

 	var totalWeight uint64
 	for _, result := range verifiedResults {
+		t.node.keys.Record(result.v.R.Sender, result.v.R.Round, account.Vote)


consider moving this to

case t.out <- messageEvent{T: voteVerified, Input: r.message, Err: makeSerErr(r.err)}:

as it would be more CPU efficient as well as more "correct" in the case where we failed to persist.

data/account/participationRegistry.go

node/node.go

util/db/initialize_test.go

tsachiherman

looks like promising progress.

…ry to data.

codecov-commenter · 2021-08-05T19:33:58Z

Codecov Report

Merging #2677 (8703a00) into feature/partkey (bfeba29) will increase coverage by 0.33%.
The diff coverage is 70.93%.

@@                 Coverage Diff                 @@
##           feature/partkey    #2677      +/-   ##
===================================================
+ Coverage            47.05%   47.38%   +0.33%     
===================================================
  Files                  349      351       +2     
  Lines                55862    56034     +172     
===================================================
+ Hits                 26284    26551     +267     
+ Misses               26623    26490     -133     
- Partials              2955     2993      +38

Impacted Files	Coverage Δ
agreement/abstractions.go	`50.00% <ø> (ø)`
agreement/agreementtest/keyManager.go	`100.00% <ø> (ø)`
config/config.go	`47.82% <ø> (ø)`
util/db/dbutil.go	`41.80% <ø> (+2.25%)`	⬆️
node/node.go	`26.63% <41.66%> (+22.84%)`	⬆️
data/account/participationRegistry.go	`68.03% <68.03%> (ø)`
data/account/participation.go	`36.45% <75.00%> (+3.50%)`	⬆️
util/db/initialize.go	`92.85% <92.85%> (ø)`
agreement/pseudonode.go	`72.26% <100.00%> (+0.23%)`	⬆️
catchup/peerSelector.go	`98.95% <0.00%> (-1.05%)`	⬇️
... and 13 more

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 bfeba29...8703a00. Read the comment docs.

tsachiherman · 2021-08-05T20:48:28Z

data/account/participation.go

+func (part Participation) ParticipationID() ParticipationID {
+	data := new(bytes.Buffer)
+
+	data.Write(part.Parent[:])
+	binary.Write(data, binary.LittleEndian, part.FirstValid)
+	binary.Write(data, binary.LittleEndian, part.LastValid)
+	binary.Write(data, binary.LittleEndian, part.KeyDilution)
+	if part.VRF != nil {
+		data.Write(part.VRF.PK[:])
+	}
+
+	// this too?
+	//part.Write(part.Voting.SubKeyPK[:])
+
+	return ParticipationID(crypto.Hash(data.Bytes()))
+}


The common way we calculate a hash of a structure is by msgp-ing it. Are you thinking of any particular reason we shouldn't do it here ?

My understanding is that the subkeys will change over time, so this was an attempt to make sure the same keyset wouldn't produce different ParticipationIDs

While what you just wrote is correct, it's not what I meant.
I was thinking of -

serializing := Participation{ FirstValid: part.FirstValid, ...

while omitting the changing parts ( i.e. Voting keys ).

then

return ParticipationID(crypto.Hash(serializing.Marshal(nil)))

tsachiherman · 2021-08-05T20:53:08Z

data/account/participationRegistry.go

+	})
+}
+
+func (db *participationDB) Record(account basics.Address, round basics.Round, participationAction ParticipationAction) error {


don't forget that you'd want to make this into a non-blocking : missing a database update because the disk access is slow, is fine. But you don't want to slow down the agreement due to slow disk performance.

Do you think running this in a goroutine is sufficient? It looks like the synchronization options have a non-zero chance of DB corruption.

@tsachiherman I decided to keep it synchronous at this point and name the agreement abstraction RecordAsync. The node layer makes it non-blocking.

I think that you'd rather do it here so that you can update the in-memory statistics without blocking, while allowing the persistence take longer.
i.e. Ideally, you would be able to respond to a http query without any blocking on the database.

… binary.

agreement/keyManager_test.go

data/account/participationRegistry.go

winder · 2021-08-06T14:28:45Z

data/account/participationRegistry.go

+	"github.com/algorand/go-algorand/util/db"
+)
+
+const maxBalLookback = 320


@tsachiherman I don't want to bring in a dependency on the consensus params here, so I'd like to remove this and simply record the registration round:
EffectiveFirst -> RegisteredAt
EffectiveLast -> DeregisteredAt

Also, I think I have an off-by-one error and EffectiveLast == EffectiveFirst when replacing a key. Do you happen to know if EffectiveLast should be -1 or EffectiveFirst should be +1?

The RegisteredAt is very different from EffectiveFirst, and EffectiveLast is very different from DeregisteredAt. While I don't mind storing these, there is no "notable" gains here.
Btw - I always used the term "register/unregister".. so "deregister" sounds a bit weird. I went looking, and found this - https://english.stackexchange.com/questions/25931/unregister-vs-deregister. so, it looks like you're correct : since you're denoting the action rather then the state, it should be Deregister and it would make it's state unregistered.

As for the other question, the effective round should be real round numbers where the node can use the voting key. so, having these equal is legit.

For the second question let me clarify. When I'm overriding key1 with key2, I'm not sure which round to use for key1.EffectiveLast and key2.EffectiveFirst.

Currently I have it so key1.EffectiveLast = key2.EffectiveFirst = currentRound + 320, I'm confident that at least one of those is wrong.

tsachiherman · 2021-08-06T17:45:57Z

data/account/participation.go

 	"fmt"
-


nit: leave this spacing line.

winder added the Team Carbon-11 label Aug 2, 2021

winder requested a review from tsachiherman August 2, 2021 15:34

winder self-assigned this Aug 2, 2021

tsachiherman reviewed Aug 2, 2021

View reviewed changes

agreement/abstractions.go Outdated Show resolved Hide resolved

tsachiherman reviewed Aug 2, 2021

View reviewed changes

node/participation.go Outdated Show resolved Hide resolved

tsachiherman reviewed Aug 2, 2021

View reviewed changes

node/participation.go Outdated Show resolved Hide resolved

tsachiherman reviewed Aug 2, 2021

View reviewed changes