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

go/consensus/genesis: Add a public key blacklist #2959

Merged
merged 2 commits into from
May 29, 2020
Merged

Conversation

Yawning
Copy link
Contributor

@Yawning Yawning commented May 29, 2020

  • Basic changes to crypto/signature to support such a thing
  • Alter the genesis document format
  • Make the registry genesis checks blacklist aware
  • Figure out wtf to do for the staking ledger (Resolved: Ignore it till the address format changes)

@Yawning
Copy link
Contributor Author

Yawning commented May 29, 2020

This is mostly done, but I need to figure out how we want to handle genesis documents that contain staking ledger entries corresponding to blacklisted public keys.

As far as I know we want to allow this (a public key being invalid shouldn't obliterate the corresponding account state, just the ability to do transactions), but this might be a moot point based on how #2940 ends up being implemented.

go/consensus/genesis/genesis.go Show resolved Hide resolved
go/consensus/genesis/genesis.go Outdated Show resolved Hide resolved
@@ -85,3 +85,18 @@ func TestContext(t *testing.T) {
_, err = PrepareSignerMessage(unregCtx, []byte("message"))
require.NoError(err, "PrepareSignerMessage should work with unregisered context (bypassed)")
}

func TestBlacklist(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Testing whether you cannot submit transactions (and that transaction is not accepted by other nodes) with blacklisted key would be useful.

Copy link
Member

@kostko kostko May 29, 2020

Choose a reason for hiding this comment

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

You could do that by using show_tx (as long as the blacklist is part of the genesis document), you only would need to load the blacklist there as well (similar to how we set the chain domain separation context from the genesis doc to verify signatures).

@codecov
Copy link

codecov bot commented May 29, 2020

Codecov Report

Merging #2959 into master will decrease coverage by 0.13%.
The diff coverage is 26.41%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2959      +/-   ##
==========================================
- Coverage   68.38%   68.25%   -0.14%     
==========================================
  Files         361      361              
  Lines       35191    35216      +25     
==========================================
- Hits        24067    24035      -32     
- Misses       8018     8075      +57     
  Partials     3106     3106              
Impacted Files Coverage Δ
go/consensus/tendermint/tendermint.go 63.88% <0.00%> (-0.54%) ⬇️
go/genesis/api/sanity_check.go 10.34% <9.09%> (-0.77%) ⬇️
go/consensus/genesis/genesis.go 27.27% <14.28%> (-22.73%) ⬇️
go/registry/api/sanity_check.go 57.67% <40.00%> (-1.75%) ⬇️
go/common/crypto/signature/signature.go 55.43% <46.15%> (+0.16%) ⬆️
go/worker/compute/executor/committee/state.go 74.07% <0.00%> (-14.82%) ⬇️
...ensus/tendermint/apps/staking/proposing_rewards.go 77.77% <0.00%> (-7.41%) ⬇️
go/consensus/tendermint/abci/mux.go 64.51% <0.00%> (-4.38%) ⬇️
go/storage/mkvs/lookup.go 73.46% <0.00%> (-4.09%) ⬇️
go/common/sgx/common.go 68.68% <0.00%> (-4.05%) ⬇️
... and 26 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 7973a53...14e21be. Read the comment docs.

Yawning added 2 commits May 29, 2020 12:26
Add a method for checking if a public key is blacklisted, and for easily
blacklisting public keys on the fly.
@Yawning Yawning force-pushed the yawning/feature/2948 branch from fe9eec5 to 14e21be Compare May 29, 2020 12:26
@Yawning Yawning marked this pull request as ready for review May 29, 2020 12:29
@Yawning Yawning merged commit b37f24d into master May 29, 2020
@Yawning Yawning deleted the yawning/feature/2948 branch May 29, 2020 12:52
@kostko kostko linked an issue Jun 1, 2020 that may be closed by this pull request
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.

Support blacklisting public keys via a consensus parameter
3 participants