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

Cannot trust or retire a retired node #797

Merged
merged 2 commits into from
Feb 5, 2020

Conversation

jumaffre
Copy link
Contributor

@jumaffre jumaffre commented Feb 5, 2020

Follow up from #778

It should not be possible to trust a node that is already retired. Also, the ccf.nodes tables should not be writable from Lua - we really want to limit what members can change in that table (i.e. only trust and retire nodes, via C++ functions).

@jumaffre jumaffre requested a review from a team as a code owner February 5, 2020 08:06
@ghost
Copy link

ghost commented Feb 5, 2020

cannot_trust_retire_node@4562 aka 20200205.4 vs master ewma over 30 builds from 4183 to 4560
images

@codecov-io
Copy link

codecov-io commented Feb 5, 2020

Codecov Report

Merging #797 into master will decrease coverage by 3.02%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
- Coverage   77.38%   74.36%   -3.02%     
==========================================
  Files         150      125      -25     
  Lines       11578    10273    -1305     
==========================================
- Hits         8959     7639    -1320     
- Misses       2619     2634      +15
Flag Coverage Δ
#e2e_BFT ?
#e2e_CFT 63.23% <ø> (-6.13%) ⬇️
#unit_BFT ?
#unit_CFT 68.32% <ø> (+0.03%) ⬆️
Impacted Files Coverage Δ
src/consensus/pbft/pbftrequests.h 0% <0%> (-94.44%) ⬇️
src/node/notifier.h 20% <0%> (-80%) ⬇️
src/crypto/symmkey.h 50% <0%> (-47.06%) ⬇️
src/node/ledgersecrets.h 21.92% <0%> (-42.47%) ⬇️
src/node/nodestate.h 43.51% <0%> (-38.48%) ⬇️
src/host/notifyconnections.h 30.88% <0%> (-38.24%) ⬇️
src/node/seal.h 60.67% <0%> (-34.83%) ⬇️
src/node/genesisgen.h 58.33% <0%> (-32.41%) ⬇️
src/consensus/raft/raftconsensus.h 61.9% <0%> (-23.81%) ⬇️
src/node/history.h 36.53% <0%> (-20.49%) ⬇️
... and 50 more

@@ -32,7 +32,6 @@ namespace ccf
Tables::MEMBER_CERTS,
Copy link
Member

Choose a reason for hiding this comment

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

Members should have access to the nodes table via lua, same as they do to the members table, etc. If they want to change the way that the nodes are handled they should be able to propose and vote for that.

Copy link
Member

Choose a reason for hiding this comment

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

The join protocol is implemented in C++ though, and we do want to enforce a particular set of conditions and transitions for nodes to preserve security guarantees. What's the use case for opening this up completely?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Raised #806

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.

4 participants