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

Enhancement: Detect when valid partkeys represent an offline or closed account #4030

Merged
merged 10 commits into from
Jul 19, 2022

Conversation

AlgoStephenAkiki
Copy link
Contributor

@AlgoStephenAkiki AlgoStephenAkiki commented May 23, 2022

Resolves #2060

Resolves all scenarios described below:

Scenario 1 (S1): Address is closed, but has a participation key that is still registered and valid given the current round number.
Scenario 2 (S2): Address is offline. Could imply no registration txn or an expired one.
Scenario 3 (S3): Address is online, with a valid participation key.

The breakdown of the scenarios by count is as follows:

S1: 7 addresses
S2: 43 addresses
S3: 2 addresses

The following actions are recommended for each scenario:

S1

Check that account is closed for the on-chain data. If account is closed, silently ignore since this account has been officially closed.

S2

Check that account is offline. If account is offline, return error indicating that no registration transaction has been issued or that previous registration transaction is expired.

S3

Recommend that error message indicates that user should regenerate participation key for the node.

@codecov
Copy link

codecov bot commented May 23, 2022

Codecov Report

Merging #4030 (9c6993a) into master (5e6c918) will increase coverage by 0.11%.
The diff coverage is 56.25%.

@@            Coverage Diff             @@
##           master    #4030      +/-   ##
==========================================
+ Coverage   54.46%   54.58%   +0.11%     
==========================================
  Files         391      391              
  Lines       48665    48674       +9     
==========================================
+ Hits        26506    26569      +63     
+ Misses      19930    19881      -49     
+ Partials     2229     2224       -5     
Impacted Files Coverage Δ
node/node.go 25.98% <56.25%> (+2.85%) ⬆️
catchup/service.go 68.88% <0.00%> (-0.75%) ⬇️
network/wsNetwork.go 64.98% <0.00%> (ø)
ledger/acctupdates.go 69.56% <0.00%> (+0.13%) ⬆️
catchup/peerSelector.go 100.00% <0.00%> (+1.04%) ⬆️
crypto/merkletrie/node.go 93.48% <0.00%> (+1.86%) ⬆️
agreement/proposalManager.go 98.03% <0.00%> (+1.96%) ⬆️
agreement/cryptoVerifier.go 69.71% <0.00%> (+2.11%) ⬆️
crypto/merkletrie/trie.go 68.61% <0.00%> (+2.18%) ⬆️
... and 3 more

@AlgoStephenAkiki AlgoStephenAkiki force-pushed the 2060-on-chain-warning-scenario-1 branch from abf8a9f to 8b16a78 Compare May 23, 2022 17:22
Copy link
Contributor

@winder winder left a comment

Choose a reason for hiding this comment

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

Looks good, just a couple small comments.

node/node.go Outdated Show resolved Hide resolved
node/node.go Outdated Show resolved Hide resolved
@AlgoStephenAkiki AlgoStephenAkiki requested a review from winder May 24, 2022 08:24
winder
winder previously approved these changes May 24, 2022
@winder winder changed the title Enhancement: Fixes Scenario 1 of on-chain warning Enhancement: Detect when valid partkeys represent an offline or closed account May 24, 2022
Resolves #2060

Puts in booleans to deduct account status from online account data and
uses that to upgrade log messages to info from warnings if necessary
@AlgoStephenAkiki
Copy link
Contributor Author

Updated PR to include all scenario warnings.

node/node.go Outdated Show resolved Hide resolved
@AlgoStephenAkiki AlgoStephenAkiki requested a review from winder June 8, 2022 14:31
node/node.go Outdated Show resolved Hide resolved
node/node.go Outdated Show resolved Hide resolved
@cce
Copy link
Contributor

cce commented Jun 17, 2022

Since there are no tests, was there manual testing to identify the messages work as desired for S1/S2/S3? Or maybe you could add a test if it's not too tricky?

@AlgoStephenAkiki AlgoStephenAkiki requested a review from cce June 24, 2022 15:40
@AlgoStephenAkiki
Copy link
Contributor Author

@cce Could you look at the updated PR?

node/node_test.go Outdated Show resolved Hide resolved
node/node.go Outdated Show resolved Hide resolved
Copy link
Contributor

@cce cce left a comment

Choose a reason for hiding this comment

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

Need to rename test, would be nice to consolidate duplication

@AlgoStephenAkiki AlgoStephenAkiki requested a review from cce July 8, 2022 13:34
@AlgoStephenAkiki AlgoStephenAkiki requested a review from winder July 11, 2022 17:06
@winder winder requested a review from algorandskiy July 15, 2022 14:22
@winder winder closed this Jul 19, 2022
@winder winder reopened this Jul 19, 2022
@winder winder merged commit 5519656 into master Jul 19, 2022
@winder winder deleted the 2060-on-chain-warning-scenario-1 branch July 19, 2022 16:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants