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.

Sign up for GitHub

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

Jump to bottom

read_ledger script does not correctly handle primary retirement #6814

Open
achamayou opened this issue Feb 7, 2025 · 2 comments · May be fixed by #6849
Open

read_ledger script does not correctly handle primary retirement #6814

achamayou opened this issue Feb 7, 2025 · 2 comments · May be fixed by #6849
Assignees
@achamayou
Milestone
6.0.0-rc0

Comments

@achamayou
Copy link
Member

The script currently rejects any signatures done by a node after having seen the transaction in which it retires, but that node can continue to sign, as per #5973, and:

https://github.com/microsoft/CCF/blob/main/tla/consensus/ccfraft.tla#L760
https://github.com/microsoft/CCF/blob/main/tla/consensus/ccfraft.tla#L876

One further commit event in the same term, by the same node, is allowed (there can also be a view change, in which case this is not hit).

The behaviour of read_ledger/ledger.py needs to align with that of the implementation and the spec.
@achamayou achamayou self-assigned this Feb 7, 2025
@achamayou achamayou added this to the 6.0.0-rc0 milestone Feb 13, 2025
@achamayou
Copy link
Member Author

The current logic observes the KV writes to membership_status and retired_committed, and rejects signatures from a Node that has reached (Retired, true).

But a node can, and will continue to issue signatures and run for elections until retired_committed itself is committed. Since commit information is not materialised in the ledger, there is no way for read_ledger.py to enforce that. I mistakenly thought that it was possible to use the term for this, but it's not, because a node can run for, and win elections in (Retired, true), which is necessary for liveness.

So as far as I can tell, the only implementable check is to check that the Node is in node_activity_status at all, with any state other than Pending. I don't think we can implement a more precise check, but I would love to be wrong.

@eddyashton
Copy link
Member

The current logic observes the KV writes to membership_status and retired_committed, and rejects signatures from a Node that has reached (Retired, true).

But a node can, and will continue to issue signatures and run for elections until retired_committed itself is committed. Since commit information is not materialised in the ledger, there is no way for read_ledger.py to enforce that. I mistakenly thought that it was possible to use the term for this, but it's not, because a node can run for, and win elections in (Retired, true), which is necessary for liveness.

So as far as I can tell, the only implementable check is to check that the Node is in node_activity_status at all, with any state other than Pending. I don't think we can implement a more precise check, but I would love to be wrong.

I think going to the weaker check is fine. It's nice-but-not-critical for read_ledger.py to do some validity/consistency checks, but more important that it read all valid ledgers. Any enforcement checks we're not 100% sure of should probably be removed from read_ledger.py, and maybe be options to a separate check_ledger.py.

@achamayou achamayou linked a pull request Feb 21, 2025 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@achamayou achamayou

Labels
None yet
Projects
None yet
Milestone
6.0.0-rc0
Development

Successfully merging a pull request may close this issue.

Correct node membership condition achamayou/CCF
2 participants
@achamayou @eddyashton