Register keys with ParticipationRegistry. #2808

winder · 2021-08-26T15:23:34Z

Summary

Call participationRegistry Register command in OnNewBlock.

Required adjusting how the ParticipationID is computed, it's now a hash of the keyreg fields + account address.

Test Plan

Existing unit tests.

codecov-commenter · 2021-08-26T17:20:43Z

Codecov Report

Merging #2808 (8265fae) into feature/partkey (5e16918) will increase coverage by 0.04%.
The diff coverage is 81.81%.

@@                 Coverage Diff                 @@
##           feature/partkey    #2808      +/-   ##
===================================================
+ Coverage            47.44%   47.48%   +0.04%     
===================================================
  Files                  352      352              
  Lines                56857    56868      +11     
===================================================
+ Hits                 26975    27005      +30     
+ Misses               26846    26832      -14     
+ Partials              3036     3031       -5

Impacted Files	Coverage Δ
logging/testingLogger.go	`0.00% <ø> (ø)`
node/node.go	`26.44% <0.00%> (-0.14%)`	⬇️
data/account/participationRegistry.go	`76.28% <90.00%> (-0.05%)`	⬇️
network/requestTracker.go	`71.12% <0.00%> (-0.44%)`	⬇️
ledger/acctupdates.go	`62.55% <0.00%> (+0.50%)`	⬆️
catchup/service.go	`70.12% <0.00%> (+0.77%)`	⬆️
catchup/peerSelector.go	`100.00% <0.00%> (+1.04%)`	⬆️
data/account/participation.go	`40.20% <0.00%> (+2.06%)`	⬆️
network/wsPeer.go	`74.65% <0.00%> (+2.50%)`	⬆️
ledger/roundlru.go	`96.22% <0.00%> (+5.66%)`	⬆️

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 5e16918...8265fae. Read the comment docs.

tsachiherman · 2021-08-30T15:00:57Z

node/node.go

+	// Look for keyreg events and notify the participationRegistry.
+	for _, tx := range block.Payset {
+		if tx.Txn.Type == protocol.KeyRegistrationTx {
+			id := account.MakeParticipationID(tx.Txn.Sender, tx.Txn.KeyregTxnFields)
+			err := node.participationRegistry.Register(id, block.BlockHeader.Round)
+			// If the key is not installed on this node, ErrParticipationIDNotFound is quickly returned.
+			if err != nil && err != account.ErrParticipationIDNotFound {
+				node.log.Error("Problem with participationRegistry.Register: %w", err)
+			}
+		}
+	}
+


I'm not sure that this is the best way to do that. Is it possible to conceptually defer this to the moment where we actually need the information and check on the ledger ?

So when GetKeys is called it fetches all keys here and checks if any should be registered?

I think that could work and be less error-prone but require more account lookups. I'll investigate.

I think that GetKeys would be called only on user-driven cases, whereas this would be called on each and every round.. so we should try and make it so that we push the "extra work" to be per-user case.

tsachiherman · 2021-08-30T15:03:04Z

data/account/participation.go

-	FirstValid  basics.Round      `codec:"fv"`
-	LastValid   basics.Round      `codec:"lv"`
-	KeyDilution uint64            `codec:"kd"`
+	transactions.KeyregTxnFields


I think that the previous implementation was fine - the identity of the participation key is not really related to the key registration transaction fields. ( it happened to have a similar set of fields, but it's a different "creature" ).

My thinking was that Idan's project is likely to change both of these, and maybe this would allow the change to happen in one place.

Interestingly, the existing ID doesn't include key dilution. I'm not sure if that's a bug, but including it here definitely gives more entropy to the ID.

This ParticipationID hash need to capture the "identity" of the file.. but that's about it. I don't think we need to worry too much about Idan's upcoming changes, as there would probably mean adding one or two more fields to the transactions.KeyregTxnFields which we can add to the ParticipationID, but don't have to.

winder · 2021-08-31T14:53:37Z

data/account/participationRegistry.go


 	// GetAll of the participation records.
-	GetAll() ([]ParticipationRecord, error)


Most of the changes in this file are related to simplifying the interface call. There isn't a DB error to pass along anymore, so no need for the error value. The caller can check for a zero value / empty list if they need to know that nothing was found.

tsachiherman · 2021-08-31T15:07:51Z

node/node.go

+		// This is usually a no-op, but the first time it will update the DB.
+		err := node.participationRegistry.Register(part.ParticipationID(), keysRound)
+		if err != nil {
+			node.log.Error("Failed to register participation key (%s) with participation registry.", part.ParticipationID())


Error -> Warnf.
Errors are meant for critical functional issues with the node. The node would continue to work just fine even if this call fails.

tsachiherman · 2021-08-31T15:10:04Z

node/node.go

@@ -1161,6 +1161,12 @@ func (node *AlgorandFullNode) VotingKeys(votingRound, keysRound basics.Round) []
 		}
 		participations = append(participations, part)
 		matchingAccountsKeys[part.Address()] = true
+
+		// This is usually a no-op, but the first time it will update the DB.
+		err := node.participationRegistry.Register(part.ParticipationID(), keysRound)


please benchmark the Register and make sure it won't take too long. ( if it takes less than 50ms, then it's probably just fine. Otherwise, we need to consider moving it to a go-routine )

Looks very fast to me. When you say ms are you think microseconds or milliseconds? Either way we're still under 50 if there's only 1 account registered:

BenchmarkKeyRegistration1/KeyInsert_1 BenchmarkKeyRegistration1/KeyInsert_1-8 202903 6154 ns/op BenchmarkKeyRegistration1/KeyRegistered_1 BenchmarkKeyRegistration1/KeyRegistered_1-8 26833 44209 ns/op BenchmarkKeyRegistration1/NoOp_1 BenchmarkKeyRegistration1/NoOp_1-8 194818 5980 ns/op BenchmarkKeyRegistration5 BenchmarkKeyRegistration5/KeyInsert_5 BenchmarkKeyRegistration5/KeyInsert_5-8 41196 30087 ns/op BenchmarkKeyRegistration5/KeyRegistered_5 BenchmarkKeyRegistration5/KeyRegistered_5-8 5782 221874 ns/op BenchmarkKeyRegistration5/NoOp_5 BenchmarkKeyRegistration5/NoOp_5-8 39292 29898 ns/op BenchmarkKeyRegistration10 BenchmarkKeyRegistration10/KeyInsert_10 BenchmarkKeyRegistration10/KeyInsert_10-8 19551 59042 ns/op BenchmarkKeyRegistration10/KeyRegistered_10 BenchmarkKeyRegistration10/KeyRegistered_10-8 2823 448233 ns/op BenchmarkKeyRegistration10/NoOp_10 BenchmarkKeyRegistration10/NoOp_10-8 19406 60828 ns/op BenchmarkKeyRegistration50 BenchmarkKeyRegistration50/KeyInsert_50 BenchmarkKeyRegistration50/KeyInsert_50-8 3784 394140 ns/op BenchmarkKeyRegistration50/KeyRegistered_50 BenchmarkKeyRegistration50/KeyRegistered_50-8 394 3207132 ns/op BenchmarkKeyRegistration50/NoOp_50 BenchmarkKeyRegistration50/NoOp_50-8 2625 427918 ns/op

milliseconds. and these looks good. ( I just wanted to make sure )

Do you think it's worth making this asynchronous? This design means that we'll frequently re-check for keys to register so it no longer needs to update the DB in real-time.

The VotingKeys is called by the pseudonode once every round so that it could use the keys for voting on that round. By slowing this down, we're holding off the agreement from making the proposal message ( which is a time critical). This could be really bad if it takes too long; however, if we're taking about 50Kns, than it's probably wouldn't make a difference. Just a side note that if we take any lock in there - that lock might take considerably more than 50Kns, so we need to be careful around that.

There is a lock taken for most of the record function. There is probably a clever solution with multiple mutexes that would avoid a DB access lock in the agreement path, I'll need to think about that more.

Created #2878 to track further optimizations to limit mutexes to very short read/copy events, and use a channel to optimize Record/Register.

data/account/participationRegistry.go

Register keys with ParticipationRegistry OnNewBlock.

c9e19ff

winder self-assigned this Aug 26, 2021

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

Fix node test.

ecaf450

winder mentioned this pull request Aug 26, 2021

Add ParticipationRegistry Insert/Delete usage in node. #2810

Merged

winder marked this pull request as ready for review August 26, 2021 18:03

winder requested a review from tsachiherman August 26, 2021 18:13

tsachiherman reviewed Aug 30, 2021

View reviewed changes

winder added 3 commits August 31, 2021 10:45

Simplify key registration, cleanup ParticipationRegistry interface.

a89833a

Revert participationIDData change.

aedceb2

Add partitiontest back

d0bf74e

winder commented Aug 31, 2021

View reviewed changes

tsachiherman reviewed Aug 31, 2021

View reviewed changes

data/account/participationRegistry.go Outdated Show resolved Hide resolved

winder added 6 commits August 31, 2021 11:19

Improve DB in inconsistent state tests.

5972f4a

Fix node test.

8bd7c83

Fix node test and update benchmark.

f4dad58

Move the Register early exit up.

f61267a

Downgrade an error to a warning.

8265fae

Fix Register lock.

5a4b2fb

winder changed the title ~~Register keys with ParticipationRegistry OnNewBlock.~~ Register keys with ParticipationRegistry. Sep 13, 2021

winder merged commit 0aec085 into algorand:feature/partkey Sep 13, 2021

winder deleted the will/participationregistry-keyreg branch September 13, 2021 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Register keys with ParticipationRegistry. #2808

Register keys with ParticipationRegistry. #2808

winder commented Aug 26, 2021

codecov-commenter commented Aug 26, 2021 •

edited

Loading

tsachiherman Aug 30, 2021

winder Aug 30, 2021

tsachiherman Aug 30, 2021

tsachiherman Aug 30, 2021

winder Aug 30, 2021

tsachiherman Aug 30, 2021

winder Aug 31, 2021 •

edited

Loading

tsachiherman Aug 31, 2021

tsachiherman Aug 31, 2021

winder Aug 31, 2021

tsachiherman Aug 31, 2021

winder Sep 1, 2021

tsachiherman Sep 1, 2021

winder Sep 8, 2021

winder Sep 13, 2021 •

edited

Loading


		// GetAll of the participation records.
		GetAll() ([]ParticipationRecord, error)

Register keys with ParticipationRegistry. #2808

Register keys with ParticipationRegistry. #2808

Conversation

winder commented Aug 26, 2021

Summary

Test Plan

codecov-commenter commented Aug 26, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winder Aug 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

winder Sep 13, 2021 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Aug 26, 2021 •

edited

Loading

winder Aug 31, 2021 •

edited

Loading

winder Sep 13, 2021 •

edited

Loading